Prevent firing updatePinnedTabs repeatedly for the same state/window …

…combination Use WeakMap so that we lose the reference to an old state when we lose the reference to an old window Fix brave#11861
petemill · Dec 23, 2017 · 67024e3 · 67024e3
1 parent cc39571
commit 67024e3
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 13 deletions.
diff --git 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
@@ -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 () {