Moves navigation logic from main.js to new component #8205

NejcZdovc · 2017-04-10T19:12:32Z

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).

Resovles #8189

Auditor

Test plan:

check if navigation buttons are working correctly (back and forward)
check if long press is working on navigation buttons
check if lion icon is behaving correctly (try disabling shield, check if block numbers from bravery is the same as is on a lion icon)
check if refresh and home buttons are working correctly

NejcZdovc · 2017-04-11T07:42:38Z

@bridiver as per your comment https://github.com/brave/browser-laptop/pull/8149/files#r110748326 I changed util into state and change braveShieldsDisabled to braveShieldsEnabled. Can you review it again please?

NejcZdovc · 2017-04-11T08:51:41Z

@bridiver in d759270 I also changed entry param, so that we only operate with activeFrame, we don't need state for this function

NejcZdovc · 2017-04-12T10:03:38Z

@bsclifton @bridiver can we please merge this one?

bridiver

great work! Just need to address the perf problem with the props and a few minor naming issues

bridiver · 2017-04-12T15:17:02Z

app/common/state/shieldState.js

+const urlParse = require('../urlParse')
+
+function braveShieldsEnabled (frame) {
+  const activeRequestedLocation = frameStateUtil.activeRequestedLocation(frame)


does it still make sense to name this activeRequestedLocation now that we're passing in a frame? I think we should try to bring the naming conventions in line with muon/chromium to make things a little easier to follow which would mean this method would be getLastCommittedURL. The error pages are transient entries (or will be anyway) so they will never be a committed url.

as a side node, committed does not mean that the url has completely loaded, it just means that a load was started and could have been blocked or redirected. Before load-start a url would be considered provisional

bridiver · 2017-04-12T15:17:02Z

test/unit/app/renderer/components/navigation/navigatorTest.js

  describe('when user has history going forwards and backwards', function () {
    let wrapper

    before(function () {
      wrapper = shallow(
-        <Main windowState={windowState} appState={appState} />
+        <Navigator
+          windowState={windowState}


passing in the entire window and app state as props will cause the Navigator component to re-render every time anything changes in either one of these stores. We need to make these props much more fine grained so they only update when they actually need to

bridiver · 2017-04-12T15:17:02Z

app/renderer/components/navigation/navigator.js

+
+  onNav (e, navCheckProp, navType, navAction) {
+    const activeFrame = frameStateUtil.getActiveFrame(this.props.windowState)
+    const activeTabId = tabUtil.activeTabId(this.props.windowState)


can't we use props.activeTab here?

bridiver · 2017-04-12T15:17:02Z

app/renderer/components/navigation/navigator.js

+        browserAction={browserAction}
+        extensionId={id}
+        tabId={activeTabId}
+        popupWindowSrc={this.props.windowState.getIn(['popupWindowDetail', 'src'])} />


Can we pass in popupWindowSrc as a prop? See my general note below about passing in props for anything that uses this.props.windowState or this.props.appState. When we transition to ReduxComponent we will just move the props from the parent to mergeProps

bridiver · 2017-04-12T15:17:02Z

app/renderer/components/navigation/navigator.js

+
+  get extensionButtons () {
+    const activeTabId = tabUtil.activeTabId(this.props.windowState)
+    const enabledExtensions = extensionState.getEnabledExtensions(this.props.appState)


comments about this.props.windowState also apply to anything using this.props.appState

bridiver · 2017-04-12T15:17:02Z

app/renderer/components/navigation/navigator.js

+          </div>
+          <NavigationBar
+            navbar={activeFrame && activeFrame.get('navbar')}
+            sites={this.props.appState.get('sites')}


this is where prop lifting gets really annoying. All of these props need to be passed in to the Navigator component so they can be passed along to NavigationBar. It's very convenient to use this.props.appState instead, but unfortunately it is not good for performance

bridiver · 2017-04-12T15:17:02Z

js/state/siteUtil.js

@@ -747,3 +749,7 @@ module.exports.setObjectId = (item) => {
  const crypto = require('crypto')
  return item.set('objectId', new Immutable.List(crypto.randomBytes(16)))
 }
+
+module.exports.enableNoScript = (settings, state) => {


siteUtil.js is actually for appState.sites and site settings are in appState.siteSettings. The names are similar, but they are two very distinct things. Maybe a new siteSettingsState in app/common/state?

Also this method sounds like it will enable noScript, but it's actually checking the state of noScript so maybe noScriptEnabled or isNoScriptEnabled? Can we swap also swap the state and settings parameters to be consistent with other state helpers and maybe move this to siteSettingsState?

bridiver · 2017-04-12T15:24:37Z

after discussion with @NejcZdovc all of this stuff was going to be re-rendered anyway as part of Main.js so this doesn't really create any new performance problems.

NejcZdovc · 2017-04-12T15:44:19Z

yes, this PR only moves existing logic from main.js to the new component. I didn't introduce any new logic or optimization. All problems that you mentioned were already there. With this PR I wanted to prepare a good base for the redux implementation into the navigator section.


        Moves navigation logic from main.js to new component

@bridiver

Resovles #8189 Auditor: @bridiver

bridiver · 2017-04-12T16:56:46Z

app/common/state/shieldState.js

+  }
+
+  const parsedUrl = urlParse(lastCommittedURL)
+  return !(parsedUrl.protocol !== 'https:' && parsedUrl.protocol !== 'http:')


merging now, but just a nit that this would be easier to read in the positive form parsedUrl === 'https' or better yet use urlutil.isHttpOrHttps

bridiver · 2017-04-12T16:57:00Z

++

bbondy · 2017-05-08T02:41:38Z

windowActions.newFrame doesn't exist, seems like the code removed was the proper appActions.createTabRequested({ url }), probably a rebase problem. I'll fix.

bbondy · 2017-05-08T02:47:55Z

fixed here #8750

NejcZdovc self-assigned this Apr 10, 2017

NejcZdovc requested a review from bridiver Apr 10, 2017

NejcZdovc added feature/navbar PR/needs-QA-attention ☕ labels Apr 10, 2017

NejcZdovc force-pushed the NejcZdovc:hotfix/#8149-navigator branch 2 times, most recently from 67c3f1e to dccc4a0 Apr 11, 2017

NejcZdovc requested a review from bsclifton Apr 12, 2017

bridiver suggested changes Apr 12, 2017

View changes

NejcZdovc force-pushed the NejcZdovc:hotfix/#8149-navigator branch 2 times, most recently from 494df0a to 276bef9 Apr 12, 2017

Moves navigation logic from main.js to new component

Unverified

This user has not uploaded their public key yet.

GPG key ID: 584931E990AC6064 Learn about signing commits

Loading status checks…

1cdaaf3

Resovles #8189 Auditor: @bridiver

NejcZdovc force-pushed the NejcZdovc:hotfix/#8149-navigator branch from 276bef9 to 1cdaaf3 Apr 12, 2017

bridiver reviewed Apr 12, 2017

View changes

bridiver merged commit b1a6c61 into brave:master Apr 12, 2017
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

bridiver added this to the 0.14.3 milestone Apr 12, 2017

NejcZdovc added the QA/test-plan-specified label May 17, 2017

NejcZdovc mentioned this pull request May 20, 2017

Move navigation logic into new component #8189

Closed

luixxiul removed the PR/needs-QA-attention ☕ label May 20, 2017

brave / browser-laptop Archived

Moves navigation logic from main.js to new component #8205

Moves navigation logic from main.js to new component #8205

NejcZdovc commented Apr 10, 2017 •

edited

NejcZdovc commented Apr 11, 2017 •

edited

NejcZdovc commented Apr 11, 2017

NejcZdovc commented Apr 12, 2017

bridiver left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bridiver commented Apr 12, 2017

NejcZdovc commented Apr 12, 2017

This comment has been minimized.

bridiver commented Apr 12, 2017

This comment has been minimized.

bbondy commented on `app/renderer/components/navigation/navigator.js` in `1cdaaf3` May 8, 2017 •

edited

This comment has been minimized.

bbondy replied May 8, 2017

brave / browser-laptop Archived

Join GitHub today

Moves navigation logic from main.js to new component #8205

Moves navigation logic from main.js to new component #8205

Conversation

NejcZdovc commented Apr 10, 2017 • edited

Auditor

NejcZdovc commented Apr 11, 2017 • edited

NejcZdovc commented Apr 11, 2017

NejcZdovc commented Apr 12, 2017

bridiver left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bridiver commented Apr 12, 2017

NejcZdovc commented Apr 12, 2017

This comment has been minimized.

bridiver commented Apr 12, 2017

This comment has been minimized.

bbondy commented on app/renderer/components/navigation/navigator.js in 1cdaaf3 May 8, 2017 • edited

This comment has been minimized.

bbondy replied May 8, 2017

NejcZdovc commented Apr 10, 2017 •

edited

NejcZdovc commented Apr 11, 2017 •

edited

bbondy commented on `app/renderer/components/navigation/navigator.js` in `1cdaaf3` May 8, 2017 •

edited