Removes non-primitive types from UrlBar #9757

NejcZdovc · 2017-06-28T16:29:25Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Resolves #9756

Auditors: @bridiver @bbondy

Test Plan:

general test of url bar
focus on suggestions

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bsclifton

Comments left; otherwise, looks great 😄

bsclifton · 2017-07-03T21:24:53Z

test/unit/app/common/lib/suggestionTest.js

+      }
+    ])
+
+    it('suggestion list is empty', function () {


bsclifton · 2017-07-03T21:28:11Z

app/renderer/components/navigation/urlBar.js

              (this.props.urlbarLocationSuffix && this.props.autocompleteEnabled))) {
            // Hack to make alt enter open a new tab for url bar suggestions when hitting enter on them.
-            const isDarwin = process.platform === 'darwin'
            if (e.altKey) {
              if (isDarwin) {


the isDarwin that is inside platformUtil is a little different than the one which was removed; it's actually a function that needs to be called (as opposed to the one that was removed above (which only checks process.platform). This is because platformUtil is safe to use in the renderer (which doesn't have access to node (ex: process.platform). Instead, it checks navigator.platform

isDarwin = () => { return process.platform === 'darwin' || navigator.platform === 'MacIntel' }

it first checks process.platform === 'darwin' same as we did in the removed code. Isn't then this the same or can navigator.platform === 'MacIntel' lead into wrong determination of the OS?

@NejcZdovc the check in platformUtil has been tested well in both the browser and renderer process. Typically, if you're in the renderer, process.platform will return undefined- hence the need to also check navigator.platform (which IS set in the renderer)

Using this utility method is encouraged over duplicating the code (especially since it has tests 😄 )

bsclifton · 2017-07-03T21:29:14Z

app/renderer/components/navigation/urlBar.js

@@ -125,11 +118,10 @@ class UrlBar extends React.Component {
          const isLocationUrl = isUrl(location)
          if (!isLocationUrl && e.ctrlKey) {
            appActions.loadURLRequested(this.props.activeTabId, `www.${location}.com`)
-          } else if (this.shouldRenderUrlBarSuggestions &&
-              ((typeof this.activeIndex === 'number' && this.activeIndex >= 0) ||
+          } else if (this.props.showrUrlBarSuggestions &&


misspelling here; s/showrUrlBarSuggestions/showUrlBarSuggestions/g

bsclifton · 2017-07-03T22:30:03Z

app/renderer/components/navigation/navigationBar.js

@@ -144,7 +139,8 @@ class NavigationBar extends React.Component {
    props.showHomeButton = !props.titleMode && getSetting(settings.SHOW_HOME_BUTTON)

    // used in other functions
-    props.navbar = navbar // TODO(nejc) remove, primitives only
+    props.isFocused = navbar.getIn(['urlbar', 'focused'])


should this have a default (in case null, etc)? ex:
props.isFocused = navbar.getIn(['urlbar', 'focused'], false)

We only use it in if statement, so we don't need it, but I added it anyway 😃

NejcZdovc · 2017-07-05T08:32:24Z

@bsclifton can we merged this one?

bridiver · 2017-07-05T15:41:33Z

app/renderer/components/navigation/urlBar.js

@@ -428,6 +398,16 @@ class UrlBar extends React.Component {
    contextMenus.onUrlBarContextMenu(e)
  }

+  onStop () {
+    ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_STOP)


I don't like duplicating this logic here. Can we create windowActions.onStop and have both methods call that instead?

So just move the whole content of this function into another window action?

yea, that will have the added benefit of calling one windowAction instead of 2. You can refactor WINDOW_SET_URL_BAR_SELECTED and WINDOW_SET_URL_BAR_ACTIVE handlers in urlBarReducer into methods so you can call them from WINDOW_ON_STOP. In a follow-up we can get rid of SHORTCUT_ACTIVE_FRAME_STOP and replace it with a WINDOW_ON_STOP handler in the browser process

sendToFocusedWindow also needs to die eventually :)

bridiver · 2017-07-05T18:23:48Z

app/renderer/components/navigation/navigationBar.js

@@ -83,17 +83,7 @@ class NavigationBar extends React.Component {

  onStop () {
    ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_STOP)


I think we should put this ipc message in there as well. When we get rid of it, we'll just switch over to the browser process handling WINDOW_ON_STOP

problem is that ipc.emit is not working in urlBarReducer

as pre our discussion, I added TODO's, so that we remove this shortcut.

@bsclifton

Resolves brave#9756 Auditors: @bsclifton @bbondy Test Plan:

bsclifton · 2017-07-06T04:56:22Z

++

NejcZdovc added perf refactoring/redux-component labels Jun 28, 2017

NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 28, 2017

NejcZdovc self-assigned this Jun 28, 2017

NejcZdovc requested review from bridiver and bbondy June 28, 2017 16:29

NejcZdovc force-pushed the refactor/#9756-urlbar branch 2 times, most recently from d5beac9 to 486c202 Compare June 30, 2017 11:35

NejcZdovc mentioned this pull request Jun 30, 2017

Only primitives in NavigationBar #9791

Closed

bsclifton suggested changes Jul 3, 2017

View reviewed changes

NejcZdovc requested a review from bsclifton July 4, 2017 09:14

NejcZdovc force-pushed the refactor/#9756-urlbar branch 2 times, most recently from e66733c to 47f4af6 Compare July 4, 2017 20:35

NejcZdovc mentioned this pull request Jul 5, 2017

Only primitives in UrlBar #9756

Closed

bridiver reviewed Jul 5, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9756-urlbar branch from 47f4af6 to c33d345 Compare July 5, 2017 18:17

NejcZdovc requested a review from bridiver July 5, 2017 18:18

bridiver reviewed Jul 5, 2017

View reviewed changes

Removes non-primitive types from UrlBar

7588a3c

Resolves brave#9756 Auditors: @bsclifton @bbondy Test Plan:

bridiver approved these changes Jul 5, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9756-urlbar branch from c33d345 to 7588a3c Compare July 5, 2017 18:44

NejcZdovc merged commit 0562bf4 into brave:master Jul 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes non-primitive types from UrlBar #9757

Removes non-primitive types from UrlBar #9757

NejcZdovc commented Jun 28, 2017 •

edited

Loading

bsclifton left a comment

bsclifton Jul 3, 2017

bsclifton Jul 3, 2017

NejcZdovc Jul 4, 2017 •

edited

Loading

bsclifton Jul 4, 2017

bsclifton Jul 4, 2017

NejcZdovc Jul 4, 2017

bsclifton Jul 3, 2017

NejcZdovc Jul 4, 2017

bsclifton Jul 3, 2017

NejcZdovc Jul 4, 2017

NejcZdovc commented Jul 5, 2017

bridiver Jul 5, 2017

NejcZdovc Jul 5, 2017

bridiver Jul 5, 2017

bridiver Jul 5, 2017 •

edited

Loading

NejcZdovc Jul 5, 2017

bridiver Jul 5, 2017

NejcZdovc Jul 5, 2017

NejcZdovc Jul 5, 2017

bsclifton commented Jul 6, 2017

		@@ -83,17 +83,7 @@ class NavigationBar extends React.Component {

		onStop () {
		ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_STOP)

Removes non-primitive types from UrlBar #9757

Removes non-primitive types from UrlBar #9757

Conversation

NejcZdovc commented Jun 28, 2017 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Jul 6, 2017

NejcZdovc commented Jun 28, 2017 •

edited

Loading

NejcZdovc Jul 4, 2017 •

edited

Loading

bridiver Jul 5, 2017 •

edited

Loading