Remove ref dependency from Frame #8336

NejcZdovc · 2017-04-15T07:08:50Z

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

Resolves #8322

Auditors: @bsclifton @bridiver

Test Plan 1

go to https://clifton.io and click on a inside link
check if back is working

Test Plan 2

go to https://clifton.io and click on a inside link
click on back
check if forward is working

Test Plan 3

go to https://clifton.io and click on a inside link (3 different links)
check if long press back is working

Test Plan 4

go to https://clifton.io and click on a inside link (3 different links)
click on back 3 times
check if long press forward is working

bbondy · 2017-04-17T15:30:19Z

app/browser/tabs.js

+  goBack: (state, action) => {
+    action = makeImmutable(action)
+    const tab = api.getWebContents(action.get('tabId'))
+    tab.goBack()


please wrap this with if (tab && !tab.isDestroyed()) { and all below as well.

bbondy · 2017-04-17T15:35:05Z

app/common/state/historyState.js

+  }
+
+  if (BrowserWindow.getFocusedWindow()) {
+    BrowserWindow.getFocusedWindow().webContents.send(messages.SHOW_CONTEXT_MENU, {


Our dispatcher now has the ability to send messages to a particular window. So it's more preferable to do an app action with a window Id specified. See for example newWebContentsAdded.

actually *State.js files specifically should not use any of these apis. They should manipulate state only and avoid other dependencies to make unit testing easier. The logic you have here looks like it probably belongs in a reducer

bridiver · 2017-04-17T15:38:42Z

app/renderer/components/navigation/navigator.js

@@ -66,7 +65,7 @@ class Navigator extends ImmutableComponent {
        })
      }
    } else {
-      navAction.call(this.activeFrame)
+      navAction.call(this, this.props.activeTab.get('tabId'))


can use this.props.tabId to make the prop a primitive type?

we don't have this in yet. I didn't yet implement redux here, so we have what we have for now, but when I will add redux to it, we will have primitives.

This PR only removes refs from Frame.

ok, no problem

bridiver · 2017-04-17T16:57:42Z

app/common/state/historyState.js

+        icon: history.entries[index].icon,
+        click: function (e) {
+          if (eventUtil.isForSecondaryAction(e)) {
+            appActions.createTabRequested({


this is also going to be a problem because you can't serialize the method call in the windowAction

bbondy · 2017-04-18T00:25:04Z

js/actions/windowActions.js

@@ -638,15 +638,15 @@ const windowActions = {
   * @param {Object} frameToSkip - Properties of the frame to keep audio
   */
  muteAllAudioExcept: function (frameToSkip) {
-    let framePropsList = windowStore.getState().get('frames')
+    /* let framePropsList = windowStore.getState().get('frames')


if this isn't needed anymore can it just be removed?

this is still necessary, but I need to refactor it, because if you require windowStore in appAction (you are requiring it, because I am are calling windowActions in appStore) you are getting errors. I will move this logic from windowActions into reducer, so that we don't need store here.

I removed it in #8381

@bsclifton

Resolves brave#8322 Auditors: @bsclifton @bridiver Test Plan: - tests are green

bridiver

Looks great! I'd really like to make the changes in tabs.js and change the params in contextMenuState.js. The other comments are just nits that shouldn't block merging

bridiver · 2017-04-19T15:03:49Z

app/browser/tabs.js

+  },
+
+  getWindowId: () => {
+    if (BrowserWindow.getFocusedWindow()) {


I think this should probably be a getActiveWindowId method on windows.js and return windowState.WINDOW_ID_NONE instead of -1 just for consistency

bridiver · 2017-04-19T15:04:53Z

app/browser/tabs.js

+          icon: null
+        }
+
+        if (url.startsWith('chrome-extension://')) {


this should use the appUrlUtil about page methods

not sure which function should be used

isTargetAboutUrl

bridiver · 2017-04-19T15:08:47Z

app/common/state/contextMenuState.js

+}
+
+const contextMenuState = {
+  setContextMenu: (state, action) => {


the helpers are meant to be more general purpose so this should probably take detail as a parameter instead of action

bridiver · 2017-04-19T15:12:47Z

app/common/state/contextMenuState.js

+    return state
+  },
+
+  onLongBacHistory: (state, action) => {


I think these two methods probably make more sense in the reducer. The state helpers are mostly just getters/setters for the state. I'm not adamantly opposed to leaving them here, but if they stay the params should be tabId, partitionNumber and history instead of action

bridiver · 2017-04-19T15:13:51Z

js/actions/appActions.js

+   * Go back in a history for a given tab
+   * @param {number} tabId - Tab id used for an action
+   */
+  onNavigateBack: function (tabId) {


just a nit, but it would be more consistent to use onGoBack, onGoForward, etc...

bridiver · 2017-04-19T15:19:11Z

app/common/state/contextMenuState.js

+    return state
+  },
+
+  onLongBacHistory: (state, action) => {


BacHistory -> BackHistory

bridiver · 2017-04-19T17:40:07Z

app/browser/tabs.js

+          }
+
+          if (!entry.icon) {
+            entry.icon = getDefaultFaviconUrl(url)


does this work correctly? It looks like we normally get it using window which isn't available here

you are correct we are not receiving anything here, that's why I added if typeof undefined

Code updated

bridiver · 2017-04-19T17:42:05Z

js/lib/urlutil.js

@@ -353,7 +353,7 @@ const UrlUtil = {
   * @return {string} url The base favicon URL
   */
  getDefaultFaviconUrl: function (url) {
-    if (UrlUtil.isURL(url)) {
+    if (typeof window !== 'undefined' && UrlUtil.isURL(url)) {


I think we can fix this by using urlParse instead of window.URL

bridiver

lgtm if all the tests are passing

bsclifton · 2017-04-19T18:20:16Z

++ nice cleanup! 😄

NejcZdovc self-assigned this Apr 15, 2017

NejcZdovc force-pushed the hotfix/#8322-refs branch 3 times, most recently from e74bd83 to 2f512b0 Compare April 16, 2017 10:30

bbondy reviewed Apr 17, 2017

View reviewed changes

bridiver reviewed Apr 17, 2017

View reviewed changes

NejcZdovc added the PR/work-in-progress ⚒ label Apr 17, 2017

NejcZdovc force-pushed the hotfix/#8322-refs branch 4 times, most recently from 35145b9 to bc5e2aa Compare April 17, 2017 16:42

bridiver reviewed Apr 17, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#8322-refs branch 3 times, most recently from 7fabdb4 to 5e35810 Compare April 17, 2017 22:49

bbondy reviewed Apr 18, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#8322-refs branch 6 times, most recently from 5876329 to f81695d Compare April 19, 2017 05:18

NejcZdovc requested review from bridiver, bbondy and bsclifton April 19, 2017 05:46

NejcZdovc added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Apr 19, 2017

NejcZdovc added this to the 0.15.1 milestone Apr 19, 2017

Remove ref dependency from Frame

24b5036

Resolves brave#8322 Auditors: @bsclifton @bridiver Test Plan: - tests are green

NejcZdovc force-pushed the hotfix/#8322-refs branch from f81695d to 24b5036 Compare April 19, 2017 08:09

bridiver suggested changes Apr 19, 2017

View reviewed changes

bridiver reviewed Apr 19, 2017

View reviewed changes

luixxiul added the QA/test-plan-required label Apr 19, 2017

NejcZdovc force-pushed the hotfix/#8322-refs branch 3 times, most recently from 38168e2 to c5e6255 Compare April 19, 2017 17:00

bridiver reviewed Apr 19, 2017

View reviewed changes

Review fixes

81aa4d2

NejcZdovc force-pushed the hotfix/#8322-refs branch from c5e6255 to 81aa4d2 Compare April 19, 2017 17:45

bridiver approved these changes Apr 19, 2017

View reviewed changes

NejcZdovc merged commit 22dc2b2 into brave:master Apr 19, 2017

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

This was referenced Apr 27, 2017

Redux refactoring tracking issue #8528

Closed

Back/Forward bindings change wrong tab; causes empty new tab #8640

Closed

NejcZdovc added QA/test-plan-specified and removed QA/test-plan-required labels May 7, 2017

NejcZdovc mentioned this pull request May 7, 2017

Remove ref dependency from Frame #8322

Closed

luixxiul mentioned this pull request May 20, 2017

Tab title often isn't updated until the cursor is moved over it or tab focus is changed #7765

Closed

NejcZdovc added the refactoring/redux-component label May 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ref dependency from Frame #8336

Remove ref dependency from Frame #8336

NejcZdovc commented Apr 15, 2017 •

edited

Loading

bbondy Apr 17, 2017

NejcZdovc Apr 17, 2017

bbondy Apr 17, 2017

bridiver Apr 17, 2017

bridiver Apr 17, 2017

NejcZdovc Apr 17, 2017 •

edited

Loading

NejcZdovc Apr 17, 2017

bridiver Apr 17, 2017

bridiver Apr 17, 2017

bbondy Apr 18, 2017

NejcZdovc Apr 18, 2017

NejcZdovc Apr 18, 2017

bridiver left a comment

bridiver Apr 19, 2017

NejcZdovc Apr 19, 2017

bridiver Apr 19, 2017

NejcZdovc Apr 19, 2017

bridiver Apr 19, 2017

NejcZdovc Apr 19, 2017

bridiver Apr 19, 2017

NejcZdovc Apr 19, 2017

bridiver Apr 19, 2017

NejcZdovc Apr 19, 2017

bridiver Apr 19, 2017

bridiver Apr 19, 2017

bridiver Apr 19, 2017

NejcZdovc Apr 19, 2017

NejcZdovc Apr 19, 2017

bridiver Apr 19, 2017

NejcZdovc Apr 19, 2017

bridiver left a comment

bsclifton commented Apr 19, 2017

Remove ref dependency from Frame #8336

Remove ref dependency from Frame #8336

Conversation

NejcZdovc commented Apr 15, 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

NejcZdovc Apr 17, 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

bridiver left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

bsclifton commented Apr 19, 2017

NejcZdovc commented Apr 15, 2017 •

edited

Loading

NejcZdovc Apr 17, 2017 •

edited

Loading