Converts Main into redux component #9678
Conversation
NejcZdovc
force-pushed
the
redux/main
branch
5 times, most recently
from
b36cd12
to
56f864c
Compare
app/renderer/components/main/main.js
Outdated
| }) | ||
|
|
||
| // TODO can we remove this? | ||
| ipc.on(messages.BLOCKED_PAGE, (e, blockType, details) => { |
There was a problem hiding this comment.
@cezaraugusto You commented this block, so can we remove it? (in this PR 2648803#diff-303f0b6a297506f2cc18bf7b4cb370c5R428)
@diracdeltas Do we need this ipc call at all, now that is commented out?
There was a problem hiding this comment.
it was listening to a message but taking no action when fired iirc. I can't recall why I didn't remove but given it is doing nothing seems like a leftover so I'm fine removing.
There was a problem hiding this comment.
Ok, thank you. Let's wait for @diracdeltas conifmation as well
There was a problem hiding this comment.
not sure what it would do if it blocked the whole page. We also have to be very careful about what we do inside onBeforeRequest because it blocks resource loading. Created #9702 to make it more efficient, but we should be saving the data in appState and reading the blocked resources from there
There was a problem hiding this comment.
So right now we have only comment in this call. We need to determinate what to do with it. Currently we are not doing anything. Now if you don't have any objections I will remove it otherwise we need to add some logic into this function. In this state it's waste of space 😃
There was a problem hiding this comment.
I'm fine with removing it
There was a problem hiding this comment.
Thank you for your inputs, function removed
NejcZdovc
force-pushed
the
redux/main
branch
2 times, most recently
from
ca08f47
to
0b91019
Compare
NejcZdovc
force-pushed
the
redux/main
branch
2 times, most recently
from
0b8d9f9
to
525e772
Compare
|
some actions are being changed to |
js/actions/windowActions.js
Outdated
| setFocusedFrame: function (frameProps) { | ||
| if (frameProps) { | ||
| setFocusedFrame: function (location, tabId) { | ||
| if (location) { |
There was a problem hiding this comment.
I think this logic should be in the reducer. setFocusedFrame should only need tabId
There was a problem hiding this comment.
I refactored menu.js so that now uses correct implementation of the reducer and have access to the sate. But still we can't remove location completely, because eventStore don't have access to the store and I don't feel comfortable refactoring it in this PR.
There was a problem hiding this comment.
I am talking about this file https://github.com/brave/browser-laptop/pull/9678/files#diff-bc67a4599e41618040dc83b94144397aR99
NejcZdovc
force-pushed
the
redux/main
branch
2 times, most recently
from
a59c4ba
to
68b7040
Compare
|
@bridiver I changed all frameKeys into tabIds. |
NejcZdovc
force-pushed
the
redux/main
branch
2 times, most recently
from
b74d997
to
a661112
Compare
Resolves brave#9452 Auditors: @bbondy @bridiver @bsclifton Test Plan:
…f platformUtil When #9678 was merged, unfortunately menu.js was not updated to call the function. Instead of updating to call the function, I updated all usage to resolve the value at the top of each file (preventing multiple unneeded calls) Auditors: @NejcZdovc
Submitter Checklist:
git rebase -ito squash commits (if needed).Resolves #9452
Auditors: @bbondy @bridiver @bsclifton
Test Plan:
Reviewer Checklist:
Tests