diff --git a/app/browser/windows.js b/app/browser/windows.js index 5b4aedf12d9..75ab434b77e 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -5,6 +5,7 @@ const electron = require('electron') const Immutable = require('immutable') const appActions = require('../../js/actions/appActions') +const appStore = require('../../js/stores/appStore') const appUrlUtil = require('../../js/lib/appUrlUtil') const {getLocationIfPDF} = require('../../js/lib/urlutil') const debounce = require('../../js/lib/debounce') @@ -29,6 +30,7 @@ const {app, BrowserWindow, ipcMain} = electron // TODO(bridiver) - set window uuid let currentWindows = {} +const windowPinnedTabStateMemoize = new WeakMap() const getWindowState = (win) => { if (win.isFullScreen()) { @@ -83,7 +85,7 @@ const siteMatchesTab = (site, tab) => { return matchesLocation && matchesPartition } -const updatePinnedTabs = (win) => { +const updatePinnedTabs = (win, appState) => { // don't continue if window won't need pinned tabs updated if ( !win || @@ -92,15 +94,22 @@ const updatePinnedTabs = (win) => { ) { return } - const appStore = require('../../js/stores/appStore') - const state = appStore.getState() const windowId = win.id - const pinnedSites = pinnedSitesState.getSites(state) - let pinnedWindowTabs = getPinnedTabsByWindowId(state, windowId) + const statePinnedSites = pinnedSitesState.getSites(appState) + // no need to continue if we've already processed this state for this window + if (windowPinnedTabStateMemoize.get(win) === statePinnedSites) { + return + } + // cache that this state has been updated for this window, + // so we do not repeat the operation until + // this specific part of the state has changed + // See + windowPinnedTabStateMemoize.set(win, statePinnedSites) + let pinnedWindowTabs = getPinnedTabsByWindowId(appState, windowId) // sites are instructions of what should be pinned // tabs are sites our window already has pinned // for each site which should be pinned, find if it's already pinned - for (const site of pinnedSites.values()) { + for (const site of statePinnedSites.values()) { const existingPinnedTabIdx = pinnedWindowTabs.findIndex(tab => siteMatchesTab(site, tab)) if (existingPinnedTabIdx !== -1) { // if it's already pinned we don't need to consider the tab in further searches @@ -276,9 +285,10 @@ const api = { pinnedTabsChanged: () => { setImmediate(() => { + const state = appStore.getState() for (let windowId in currentWindows) { if (currentWindows[windowId].__ready) { - updatePinnedTabs(currentWindows[windowId]) + updatePinnedTabs(currentWindows[windowId], state) } } }) @@ -342,7 +352,8 @@ const api = { setImmediate(() => { const win = currentWindows[windowId] if (win && !win.isDestroyed()) { - updatePinnedTabs(win) + const state = appStore.getState() + updatePinnedTabs(win, state) win.__ready = true win.emit(messages.WINDOW_RENDERER_READY) } diff --git a/test/unit/app/browser/windowsTest.js b/test/unit/app/browser/windowsTest.js index 4f9d5da2dbf..d48b4765148 100644 --- a/test/unit/app/browser/windowsTest.js +++ b/test/unit/app/browser/windowsTest.js @@ -123,6 +123,7 @@ describe('window API unit tests', function () { describe('privateMethods', function () { let updatePinnedTabs let createTabRequestedSpy, tabCloseRequestedSpy + const win = { id: 1, webContents: { @@ -149,22 +150,31 @@ describe('window API unit tests', function () { describe('updatePinnedTabs', function () { it('takes no action if pinnedSites list matches tab state', function () { - updatePinnedTabs(win) + updatePinnedTabs(win, defaultState) assert.equal(createTabRequestedSpy.calledOnce, false) assert.equal(tabCloseRequestedSpy.calledOnce, false) }) it('calls `appActions.createTabRequested` for pinnedSites not in tab state', function () { - appStore.getState = () => Immutable.fromJS(createTabState) - updatePinnedTabs(win) + updatePinnedTabs(win, createTabState) assert.equal(createTabRequestedSpy.calledOnce, true) }) it('calls `appActions.tabCloseRequested` for items in tab state but not in pinnedSites', function () { - appStore.getState = () => Immutable.fromJS(tabCloseState) - updatePinnedTabs(win) + updatePinnedTabs(win, tabCloseState) assert.equal(tabCloseRequestedSpy.calledOnce, true) }) + + it('does not create duplicate pinnedSites whilst waiting for tabs to be created', function () { + // use a unique state for this test since updatePinnedTabs memoizes itself based on its input + const createTabStateClone = Immutable.fromJS(createTabState.toJS()) + // should ask for a new tab + updatePinnedTabs(win, createTabStateClone) + assert.equal(createTabRequestedSpy.callCount, 1) + // should not ask for a new tab + updatePinnedTabs(win, createTabStateClone) + assert.equal(createTabRequestedSpy.callCount, 1) + }) }) describe('createWindow', function () {