Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Fix dispatch issues for actions pre windowReady
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bbondy
bbondy committed Sep 19, 2017
1 parent f375e93 commit fe6366c
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 68 deletions.
1 change: 1 addition & 0 deletions app/browser/windows.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ const api = {
win.__alreadyPinnedSites = new Immutable.Set()
updatePinnedTabs(win)
win.__ready = true
win.emit('window-renderer-ready')
}
})
},
Expand Down
20 changes: 10 additions & 10 deletions app/common/actions/extensionActions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -49,7 +49,7 @@ const extensionActions = {
* @param {string} extensionId - the extension id
*/
extensionUninstalled: function (extensionId) {
AppDispatcher.dispatch({
appDispatcher.dispatch({
actionType: ExtensionConstants.EXTENSION_UNINSTALLED,
extensionId
})
Expand All @@ -61,7 +61,7 @@ const extensionActions = {
* @param {string} extensionId - the extension id
*/
extensionEnabled: function (extensionId) {
AppDispatcher.dispatch({
appDispatcher.dispatch({
actionType: ExtensionConstants.EXTENSION_ENABLED,
extensionId
})
Expand All @@ -73,7 +73,7 @@ const extensionActions = {
* @param {string} extensionId - the extension id
*/
extensionDisabled: function (extensionId) {
AppDispatcher.dispatch({
appDispatcher.dispatch({
actionType: ExtensionConstants.EXTENSION_DISABLED,
extensionId
})
Expand All @@ -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,
Expand All @@ -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
})
Expand All @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions js/actions/syncActions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
}
Expand Down
79 changes: 51 additions & 28 deletions js/dispatcher/appDispatcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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

Expand All @@ -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()) {
Expand Down Expand Up @@ -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) => {
Expand Down
10 changes: 5 additions & 5 deletions js/state/contentSettings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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)
})
Expand All @@ -374,5 +374,5 @@ module.exports.init = () => {
updateContentSettings(AppStore.getState(), appConfig, incognito)
)

AppDispatcher.register(doAction)
appDispatcher.register(doAction)
}
6 changes: 3 additions & 3 deletions js/state/privacy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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)
}
27 changes: 14 additions & 13 deletions js/stores/appStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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(),
Expand All @@ -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())
})
}

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -890,6 +891,6 @@ const handleAppAction = (action) => {
emitChanges()
}

appStore.dispatchToken = AppDispatcher.register(handleAppAction)
appStore.dispatchToken = appDispatcher.register(handleAppAction)

module.exports = appStore

0 comments on commit fe6366c

Please sign in to comment.