Prevent firing updatePinnedTabs repeatedly for the same state/window …

…combination Fix brave#11861
petemill · Nov 9, 2017 · 9e1716d · 9e1716d
1 parent 8d8e0fb
commit 9e1716d
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 14 deletions.
diff --git a/app/browser/windows.js b/app/browser/windows.js
@@ -4,6 +4,7 @@
 
 const {app, BrowserWindow, ipcMain} = require('electron')
 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')
@@ -19,6 +20,7 @@ const pinnedSitesState = require('../common/state/pinnedSitesState')
 
 // TODO(bridiver) - set window uuid
 let currentWindows = {}
+const windowPinnedTabStateMemoize = {}
 
 const getWindowState = (win) => {
   if (win.isFullScreen()) {
@@ -33,6 +35,7 @@ const getWindowState = (win) => {
 }
 
 const cleanupWindow = (windowId) => {
+  windowPinnedTabStateMemoize[windowId] = undefined
   delete currentWindows[windowId]
 }
 
@@ -72,19 +75,26 @@ const siteMatchesTab = (site, tab) => {
   return matchesLocation && matchesPartition
 }
 
-const updatePinnedTabs = (win) => {
+const updatePinnedTabs = (win, appState) => {
   if (win.webContents.browserWindowOptions.disposition === 'new-popup') {
     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[windowId] === 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[windowId] = 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
@@ -239,9 +249,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)
         }
       }
     })
@@ -305,7 +316,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
@@ -84,7 +84,8 @@ describe('window API unit tests', function () {
         browserWindowOptions: {
           disposition: ''
         }
-      }
+      },
+      isDestroyed: () => false
     }
 
     before(function () {
@@ -103,20 +104,18 @@ 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)
       })
     })