Fix dispatch issues for actions pre windowReady

Partially solves #10866 A second issue remains for #10866 where there's a race condition on the tab index that is used. This can exist without that though. Browser process now registers the process right away and callbacks are registered from the window store. It also waits until the window is ready before dispatching now.
brave · Sep 12, 2017 · 96722c3 · 96722c3
1 parent 78a0757
commit 96722c3
Show file tree

Hide file tree

Showing 10 changed files with 96 additions and 68 deletions.
diff --git a/app/browser/windows.js b/app/browser/windows.js
@@ -320,6 +320,7 @@ const api = {
         win.__alreadyPinnedSites = new Immutable.Set()
         updatePinnedTabs(win)
         win.__ready = true
+        win.emit('window-renderer-ready')
       }
     })
   },

diff --git a/app/common/actions/extensionActions.js b/app/common/actions/extensionActions.js
@@ -3,7 +3,7 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 'use strict'
-const AppDispatcher = require('../../../js/dispatcher/appDispatcher')
+const appDispatcher = require('../../../js/dispatcher/appDispatcher')
 const ExtensionConstants = require('../constants/extensionConstants')
 
 const extensionActions = {
@@ -14,15 +14,15 @@ const extensionActions = {
    * @param {object} browserAction - browser action details
    */
   browserActionRegistered: function (extensionId, browserAction) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.BROWSER_ACTION_REGISTERED,
       extensionId,
       browserAction
     })
   },
 
   browserActionUpdated: function (extensionId, browserAction, tabId) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.BROWSER_ACTION_UPDATED,
       extensionId,
       browserAction,
@@ -36,7 +36,7 @@ const extensionActions = {
    * @param {string} extensionId - the extension id
    */
   extensionInstalled: function (extensionId, installInfo) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.EXTENSION_INSTALLED,
       extensionId,
       installInfo
@@ -49,7 +49,7 @@ const extensionActions = {
    * @param {string} extensionId - the extension id
    */
   extensionUninstalled: function (extensionId) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.EXTENSION_UNINSTALLED,
       extensionId
     })
@@ -61,7 +61,7 @@ const extensionActions = {
    * @param {string} extensionId - the extension id
    */
   extensionEnabled: function (extensionId) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.EXTENSION_ENABLED,
       extensionId
     })
@@ -73,7 +73,7 @@ const extensionActions = {
    * @param {string} extensionId - the extension id
    */
   extensionDisabled: function (extensionId) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.EXTENSION_DISABLED,
       extensionId
     })
@@ -88,7 +88,7 @@ const extensionActions = {
    * @param {string} icon - 16x16 extension icon
    */
   contextMenuCreated: function (extensionId, menuItemId, properties, icon) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.CONTEXT_MENU_CREATED,
       extensionId,
       menuItemId,
@@ -103,7 +103,7 @@ const extensionActions = {
    * @param {string} extensionId - the extension id
    */
   contextMenuAllRemoved: function (extensionId) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.CONTEXT_MENU_ALL_REMOVED,
       extensionId
     })
@@ -117,7 +117,7 @@ const extensionActions = {
    * @param {object} info - the arg of onclick callback
    */
   contextMenuClicked: function (extensionId, tabId, info) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: ExtensionConstants.CONTEXT_MENU_CLICKED,
       extensionId,
       tabId,

diff --git a/js/actions/syncActions.js b/js/actions/syncActions.js
@@ -3,25 +3,25 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 'use strict'
-const AppDispatcher = require('../dispatcher/appDispatcher')
+const appDispatcher = require('../dispatcher/appDispatcher')
 const syncConstants = require('../constants/syncConstants')
 
 const syncActions = {
   removeSite: function (item) {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: syncConstants.SYNC_REMOVE_SITE,
       item
     })
   },
 
   clearHistory: function () {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: syncConstants.SYNC_CLEAR_HISTORY
     })
   },
 
   clearSiteSettings: function () {
-    AppDispatcher.dispatch({
+    appDispatcher.dispatch({
       actionType: syncConstants.SYNC_CLEAR_SITE_SETTINGS
     })
   }

diff --git a/js/dispatcher/appDispatcher.js b/js/dispatcher/appDispatcher.js
@@ -49,6 +49,16 @@ class AppDispatcher {
     if (process.type === 'renderer') {
       ipc.send('app-dispatcher-register')
     }
+    return this.registerLocalCallback(callback)
+  }
+
+  /**
+   * Same as above, but registers the specified callback
+   * locally only.  This is used by the windowStore since
+   * the store process is registered as soon as the window
+   * is created.
+   */
+  registerLocalCallback (callback) {
     this.callbacks.push(callback)
     return this.callbacks.length - 1 // index
   }
@@ -131,33 +141,8 @@ class AppDispatcher {
   shutdown () {
     appDispatcher.dispatch = (payload) => {}
   }
-}
-
-const appDispatcher = new AppDispatcher()
-
-const doneDispatching = () => {
-  if (dispatchCargo.idle()) {
-    appDispatcher.dispatching = false
-  }
-}
 
-const dispatchCargo = async.cargo((task, callback) => {
-  for (let i = 0; i < task.length; i++) {
-    appDispatcher.dispatchInternal(task[i], () => {})
-  }
-  callback()
-  doneDispatching()
-}, 200)
-
-const ipcCargo = async.cargo((tasks, callback) => {
-  ipc.send(messages.DISPATCH_ACTION, Serializer.serialize(tasks))
-  callback()
-}, 200)
-
-if (processType === 'browser') {
-  ipc.on('app-dispatcher-register', (event) => {
-    const registrant = event.sender
-    const hostWebContents = event.sender.hostWebContents || event.sender
+  registerWindow (registrant, hostWebContents) {
     const win = BrowserWindow.fromWebContents(hostWebContents)
     const windowId = win.id
 
@@ -168,6 +153,15 @@ if (processType === 'browser') {
       callback()
     }, 20)
 
+    // If the window isn't ready yet then wait until it is ready before delivering
+    // messages to it.
+    if (!win.__ready) {
+      registrantCargo.pause()
+      win.on('window-renderer-ready', () => {
+        registrantCargo.resume()
+      })
+    }
+
     const callback = function (payload) {
       try {
         if (registrant.isDestroyed()) {
@@ -196,13 +190,42 @@ if (processType === 'browser') {
         appDispatcher.unregister(callback)
       }
     }
-    event.sender.on('crashed', () => {
+    registrant.on('crashed', () => {
       appDispatcher.unregister(callback)
     })
-    event.sender.on('destroyed', () => {
+    registrant.on('destroyed', () => {
       appDispatcher.unregister(callback)
     })
     appDispatcher.register(callback)
+  }
+}
+
+const appDispatcher = new AppDispatcher()
+
+const doneDispatching = () => {
+  if (dispatchCargo.idle()) {
+    appDispatcher.dispatching = false
+  }
+}
+
+const dispatchCargo = async.cargo((task, callback) => {
+  for (let i = 0; i < task.length; i++) {
+    appDispatcher.dispatchInternal(task[i], () => {})
+  }
+  callback()
+  doneDispatching()
+}, 200)
+
+const ipcCargo = async.cargo((tasks, callback) => {
+  ipc.send(messages.DISPATCH_ACTION, Serializer.serialize(tasks))
+  callback()
+}, 200)
+
+if (processType === 'browser') {
+  ipc.on('app-dispatcher-register', (event) => {
+    const registrant = event.sender
+    const hostWebContents = event.sender.hostWebContents || event.sender
+    appDispatcher.registerWindow(registrant, hostWebContents)
   })
 
   const dispatchEventPayload = (event, payload) => {

diff --git a/js/state/contentSettings.js b/js/state/contentSettings.js
@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-const AppDispatcher = require('../dispatcher/appDispatcher')
+const appDispatcher = require('../dispatcher/appDispatcher')
 const AppStore = require('../stores/appStore')
 const appConstants = require('../constants/appConstants')
 const appConfig = require('../constants/appConfig')
@@ -342,21 +342,21 @@ const doAction = (action) => {
     case appConstants.APP_REMOVE_SITE_SETTING:
     case appConstants.APP_CHANGE_SITE_SETTING:
     case appConstants.APP_ADD_NOSCRIPT_EXCEPTIONS:
-      AppDispatcher.waitFor([AppStore.dispatchToken], () => {
+      appDispatcher.waitFor([AppStore.dispatchToken], () => {
         userPrefsUpdateTrigger(action.temporary)
         contentSettingsUpdateTrigger(action.temporary)
       })
       break
     case appConstants.APP_CHANGE_SETTING:
     case appConstants.APP_SET_RESOURCE_ENABLED:
-      AppDispatcher.waitFor([AppStore.dispatchToken], () => {
+      appDispatcher.waitFor([AppStore.dispatchToken], () => {
         userPrefsUpdateTrigger()
         contentSettingsUpdateTrigger()
       })
       break
     case appConstants.APP_ALLOW_FLASH_ONCE:
     case appConstants.APP_ALLOW_FLASH_ALWAYS:
-      AppDispatcher.waitFor([AppStore.dispatchToken], () => {
+      appDispatcher.waitFor([AppStore.dispatchToken], () => {
         userPrefsUpdateTrigger(action.isPrivate)
         contentSettingsUpdateTrigger(action.isPrivate)
       })
@@ -374,5 +374,5 @@ module.exports.init = () => {
     updateContentSettings(AppStore.getState(), appConfig, incognito)
   )
 
-  AppDispatcher.register(doAction)
+  appDispatcher.register(doAction)
 }
diff --git a/js/state/privacy.js b/js/state/privacy.js
@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-const AppDispatcher = require('../dispatcher/appDispatcher')
+const appDispatcher = require('../dispatcher/appDispatcher')
 const AppStore = require('../stores/appStore')
 const appConstants = require('../constants/appConstants')
 const {passwordManagers} = require('../constants/passwordManagers')
@@ -24,13 +24,13 @@ let updateTrigger
 // Register callback to handle all updates
 const doAction = (action) => {
   if (action.actionType === appConstants.APP_CHANGE_SETTING) {
-    AppDispatcher.waitFor([AppStore.dispatchToken], () => {
+    appDispatcher.waitFor([AppStore.dispatchToken], () => {
       updateTrigger()
     })
   }
 }
 
 module.exports.init = () => {
   updateTrigger = registerUserPrefs(() => getPrivacySettings())
-  AppDispatcher.register(doAction)
+  appDispatcher.register(doAction)
 }
diff --git a/js/stores/appStore.js b/js/stores/appStore.js
@@ -6,7 +6,7 @@
 const appConstants = require('../constants/appConstants')
 const windowConstants = require('../constants/windowConstants')
 const ExtensionConstants = require('../../app/common/constants/extensionConstants')
-const AppDispatcher = require('../dispatcher/appDispatcher')
+const appDispatcher = require('../dispatcher/appDispatcher')
 const appConfig = require('../constants/appConfig')
 const settings = require('../constants/settings')
 const siteUtil = require('../state/siteUtil')
@@ -199,9 +199,9 @@ const createWindow = (action) => {
   const toolbarUserInterfaceScale = getSetting(settings.TOOLBAR_UI_SCALE)
 
   setImmediate(() => {
-    let mainWindow = new BrowserWindow(Object.assign(windowProps, browserOpts, {disposition: frameOpts.disposition}))
+    const win = new BrowserWindow(Object.assign(windowProps, browserOpts, {disposition: frameOpts.disposition}))
     let restoredImmutableWindowState = action.restoredState
-    initWindowCacheState(mainWindow.id, restoredImmutableWindowState)
+    initWindowCacheState(win.id, restoredImmutableWindowState)
 
     // initialize frames state
     let frames = Immutable.List()
@@ -230,21 +230,22 @@ const createWindow = (action) => {
     }
 
     if (immutableWindowState.getIn(['ui', 'isMaximized'])) {
-      mainWindow.maximize()
+      win.maximize()
     }
 
     if (immutableWindowState.getIn(['ui', 'isFullScreen'])) {
-      mainWindow.setFullScreen(true)
+      win.setFullScreen(true)
     }
 
-    mainWindow.webContents.on('did-finish-load', (e) => {
+    appDispatcher.registerWindow(win, win.webContents)
+    win.webContents.on('did-finish-load', (e) => {
       lastEmittedState = appState
-      mainWindow.webContents.setZoomLevel(zoomLevel[toolbarUserInterfaceScale] || 0.0)
+      win.webContents.setZoomLevel(zoomLevel[toolbarUserInterfaceScale] || 0.0)
 
       const mem = muon.shared_memory.create({
         windowValue: {
           disposition: frameOpts.disposition,
-          id: mainWindow.id
+          id: win.id
         },
         appState: appState.toJS(),
         frames: frames.toJS(),
@@ -256,11 +257,11 @@ const createWindow = (action) => {
       }
     })
 
-    mainWindow.on('ready-to-show', () => {
-      mainWindow.show()
+    win.on('ready-to-show', () => {
+      win.show()
     })
 
-    mainWindow.loadURL(appUrlUtil.getBraveExtIndexHTML())
+    win.loadURL(appUrlUtil.getBraveExtIndexHTML())
   })
 }
 
@@ -436,7 +437,7 @@ const handleAppAction = (action) => {
       appState = require('../../app/sync').init(appState, action, appStore)
       break
     case appConstants.APP_SHUTTING_DOWN:
-      AppDispatcher.shutdown()
+      appDispatcher.shutdown()
       app.quit()
       break
     case appConstants.APP_NEW_WINDOW:
@@ -890,6 +891,6 @@ const handleAppAction = (action) => {
   emitChanges()
 }
 
-appStore.dispatchToken = AppDispatcher.register(handleAppAction)
+appStore.dispatchToken = appDispatcher.register(handleAppAction)
 
 module.exports = appStore