-
Notifications
You must be signed in to change notification settings - Fork 973
Conversation
Resolves brave#6770 Auditors: @alexwykoff @bsclifton @mrose17 Test Plan: - described in brave#6770
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me... please commit when all reviews complete!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I ran the unit tests, fixed one that I had broke (whoops!). Great job- merging 😄
fn(ses, partition, module.exports.isPrivate(partition)) | ||
}) | ||
ses.on('register-navigator-handler', (e, protocol, location) => { | ||
appActions.navigatorHandler(ses.partition, protocol, location, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should try to keep the declarative naming conventions here so maybe navigatorHandlerRegistered
and navigatorHandlerUnregistered
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call 👍 Since this is already merged, please feel free to do as a follow up, @NejcZdovc 😄
@@ -232,6 +232,11 @@ const doAction = (action) => { | |||
setTimeout(networkConnected, 1 * msecs.second) | |||
break | |||
|
|||
case appConstants.APP_NAVIGATOR_HANDLER_UPDATE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to the above comment, the app constant name should match the action name
Auditors: @bridiver @bsclifton Test Plan:
Auditors: @bridiver @bsclifton Test Plan:
git rebase -i
to squash commits (if needed).Resolves #6770
Auditors:
@alexwykoff @bsclifton @mrose17
Test plan