Incremently update the History/Recently closed menu

Improves performance of tab close and undo tab close by avoiding iterating over all bookmarks
and avoiding recreating the system app menu.

Fix #4848

Auditors: @bsclifton @bbondy

Note this should be tested on Linux, MacOS and Windows to confirm tab closing is okay.

Test plan:
1. Open the History menu and note Recently closed is not there.
2. Go to https://archive.org
3. Open a new tab and go to https://wikipedia.org
4. Close the tab
5. Examine the menu History > Recently closed; it should have Wikipedia
6. Close the archive.org tab
7. Examine the Recently closed menu; it should have the Internet archive
8. Use the menu item "Reopen last closed tab" or use the shortcut
9. Examine the Recently closed menu; it should be hidden now.

app/browser/menu.js

@@ -22,6 +22,7 @@ const dialog = electron.dialog
 const app = electron.app
 const BrowserWindow = electron.BrowserWindow
 const {fileUrl} = require('../../js/lib/appUrlUtil')
+const {isValidClosedFrame} = require('../../js/state/frameStateUtil')
 const menuUtil = require('../common/lib/menuUtil')
 const {getByTabId} = require('../common/state/tabState')
 const getSetting = require('../../js/settings').getSetting
@@ -33,7 +34,7 @@ const isLinux = process.platform === 'linux'
 
 let appMenu = null
 // TODO(bridiver) - these should be handled in the appStore
-let closedFrames = {}
+let closedFrames = new Immutable.OrderedMap()
 let lastClosedUrl = null
 let currentLocation = null
 
@@ -335,21 +336,32 @@ const createHistorySubmenu = () => {
       }
     }
   ]
-  submenu = submenu.concat(menuUtil.createRecentlyClosedTemplateItems(Immutable.fromJS(Object.keys(closedFrames).map(key => closedFrames[key]))))
+  const recentlyClosedItems = menuUtil.createRecentlyClosedTemplateItems(closedFrames)
+  submenu = submenu.concat(recentlyClosedItems)
 
   submenu.push(
-    // TODO: recently visited
-    // CommonMenu.separatorMenuItem,
-    // {
-    //   label: locale.translation('recentlyVisited'),
-    //   enabled: false
-    // },
     CommonMenu.separatorMenuItem,
-    CommonMenu.historyMenuItem())
+    CommonMenu.historyMenuItem()
+  )
 
   return submenu
 }
 
+const updateRecentlyClosedMenuItems = () => {
+  // Update electron menu (Mac / Linux)
+  menuUtil.updateRecentlyClosedMenuItems(appMenu, closedFrames)
+  Menu.setApplicationMenu(appMenu)
+
+  // Update in-memory menu template (Windows)
+  const oldTemplate = appStore.getState().getIn(['menu', 'template'])
+  const historySubmenuKey = oldTemplate.findKey(value =>
+    value.get('label') === locale.translation('history')
+  )
+  const newSubmenu = Immutable.fromJS(createHistorySubmenu())
+  const newTemplate = oldTemplate.set(historySubmenuKey, newSubmenu)
+  appActions.setMenubarTemplate(newTemplate)
+}
+
 const isCurrentLocationBookmarked = () => {
   return isLocationBookmarked(appStore.getState(), currentLocation)
 }
@@ -618,27 +630,31 @@ const doAction = (action) => {
       break
     case windowConstants.WINDOW_UNDO_CLOSED_FRAME:
       appDispatcher.waitFor([appStore.dispatchToken], () => {
-        delete closedFrames[lastClosedUrl]
-        createMenu()
+        if (!lastClosedUrl) {
+          return
+        }
+        closedFrames = closedFrames.delete(lastClosedUrl)
+        const nextLastFrame = closedFrames.last()
+        lastClosedUrl = nextLastFrame ? nextLastFrame.get('location') : null
+        updateRecentlyClosedMenuItems()
       })
       break
     case windowConstants.WINDOW_CLEAR_CLOSED_FRAMES:
       appDispatcher.waitFor([appStore.dispatchToken], () => {
-        closedFrames = {}
+        closedFrames = new Immutable.OrderedMap()
         lastClosedUrl = null
-        createMenu()
+        updateRecentlyClosedMenuItems()
       })
       break
     case appConstants.APP_TAB_CLOSE_REQUESTED:
       appDispatcher.waitFor([appStore.dispatchToken], () => {
         action = makeImmutable(action)
         const tab = getByTabId(appStore.getState(), action.get('tabId'))
-        if (tab && !tab.get('incognito') && tab.get('url') !== 'about:newtab') {
-          if (tab.get('frame')) {
-            lastClosedUrl = tab.get('url')
-            closedFrames[tab.get('url')] = tab.get('frame')
-            createMenu()
-          }
+        const frame = tab && tab.get('frame')
+        if (tab && !tab.get('incognito') && frame && isValidClosedFrame(frame)) {
+          lastClosedUrl = tab.get('url')
+          closedFrames = closedFrames.set(tab.get('url'), tab.get('frame'))
+          updateRecentlyClosedMenuItems()
         }
       })
       break

app/common/lib/menuUtil.js

@@ -3,13 +3,15 @@
 
 'use strict'
 
+const MenuItem = require('electron').MenuItem
 const {makeImmutable} = require('../../common/state/immutableUtil')
 const CommonMenu = require('../../common/commonMenu')
 const siteTags = require('../../../js/constants/siteTags')
 const eventUtil = require('../../../js/lib/eventUtil')
 const siteUtil = require('../../../js/state/siteUtil')
 const locale = require('../../locale')
 const appActions = require('../../../js/actions/appActions')
+const config = require('../../../js/constants/config')
 const {separatorMenuItem} = require('../../common/commonMenu')
 const windowActions = require('../../../js/actions/windowActions')
 
@@ -122,42 +124,154 @@ module.exports.createBookmarkTemplateItems = (sites) => {
 }
 
 /**
- * Create "recently closed" history entries for the "History" menu
+ * @param {string} key within closedFrames, i.e. a URL
+ * @return {string}
  */
-module.exports.createRecentlyClosedTemplateItems = (lastClosedFrames) => {
-  const payload = []
+const getRecentlyClosedMenuId = function (key) {
+  return `recentlyClosedFrame|${key}`
+}
+module.exports.getRecentlyClosedMenuId = getRecentlyClosedMenuId
 
-  lastClosedFrames = makeImmutable(lastClosedFrames)
+/**
+ * @param {string} menuId
+ * @return {string} key within closedFrames, i.e. a URL
+ */
+const getRecentlyClosedMenuKey = function (menuId) {
+  if (typeof menuId !== 'string' || menuId.indexOf('recentlyClosedFrame|') === -1) {
+    return undefined
+  }
+  return menuId.split('|')[1]
+}
 
-  if (lastClosedFrames && lastClosedFrames.size > 0) {
-    payload.push(
-      CommonMenu.separatorMenuItem,
-      {
-        label: locale.translation('recentlyClosed'),
-        enabled: false
+const recentlyClosedClickHandler = (frame) => {
+  return (item, focusedWindow, e) => {
+    const location = frame.get('location')
+    if (eventUtil.isForSecondaryAction(e)) {
+      appActions.createTabRequested({
+        url: location,
+        windowId: focusedWindow.id,
+        active: !!e.shiftKey
       })
+    } else {
+      appActions.loadURLInActiveTabRequested(focusedWindow.id, location)
+    }
+  }
+}
 
-    const lastTen = ((lastClosedFrames.size < 10) ? lastClosedFrames : lastClosedFrames.slice(-10)).reverse()
-    lastTen.forEach((closedFrame) => {
-      payload.push({
-        label: closedFrame.get('title') || closedFrame.get('location'),
-        click: (item, focusedWindow, e) => {
-          if (eventUtil.isForSecondaryAction(e)) {
-            appActions.createTabRequested({
-              url: closedFrame.get('location'),
-              windowId: focusedWindow.id,
-              active: !!e.shiftKey
-            })
-          } else {
-            appActions.loadURLInActiveTabRequested(focusedWindow.id, closedFrame.get('location'))
-          }
-        }
-      })
+const getFrameMenuLabel = (frame) => {
+  return frame.get('title') || frame.get('location')
+}
+
+const recentlyClosedTemplate = (key, frame) => {
+  return {
+    id: getRecentlyClosedMenuId(key),
+    click: recentlyClosedClickHandler(frame),
+    label: getFrameMenuLabel(frame)
+  }
+}
+
+module.exports.recentlyClosedHeadingTemplates = () => {
+  return [
+    {
+      id: 'recentlyClosedSeparator',
+      type: 'separator'
+    },
+    {
+      id: 'recentlyClosedHeading',
+      label: locale.translation('recentlyClosed'),
+      enabled: false
+    }
+  ]
+}
+
+/**
+ * Create "recently closed" history entries for the "History" menu.
+ * Labels and visibility change dynamically in updateRecentlyClosedMenuItems.
+ * @param {Immutable.OrderedMap} closedFrames
+ */
+module.exports.createRecentlyClosedTemplateItems = (closedFrames) => {
+  let payload = module.exports.recentlyClosedHeadingTemplates()
+  if (!closedFrames || !closedFrames.size) {
+    return payload.map((item) => {
+      item.visible = false
+      return item
     })
   }
+  let n = 0
+  closedFrames.reverse().forEach((frame) => {
+    payload.push(recentlyClosedTemplate(n, frame))
+    n = n + 1
+    if (n >= config.menu.maxClosedFrames) {
+      return false
+    }
+  })
   return payload
 }
 
+/**
+ * Update display of History menu "Recently closed" menu items by
+ * inserting MenuItems or hiding existing MenuItems.
+ * @param {electron.Menu} appMenu
+ * @param {Immutable.OrderedMap} closedFrames
+ */
+module.exports.updateRecentlyClosedMenuItems = (appMenu, closedFrames) => {
+  const headingVisible = closedFrames.size > 0
+  const maxMenuItems = config.menu.maxClosedFrames
+  const historyLabel = locale.translation('history')
+  const historyMenu = module.exports.getMenuItem(appMenu, historyLabel).submenu
+  let insertPosition = 0
+
+  const historyMenuIndicesByOrder = {}
+  for (let i = 0; i < historyMenu.items.length; i++) {
+    const item = historyMenu.items[i]
+    // New items go after "Recently closed"
+    if (!insertPosition && item.id === 'recentlyClosedHeading') {
+      insertPosition = i + 1
+      item.visible = headingVisible
+      continue
+    } else if (item.id === 'recentlyClosedSeparator') {
+      item.visible = headingVisible
+      continue
+    }
+
+    // Find existing items
+    const key = getRecentlyClosedMenuKey(item.id)
+    if (typeof key !== 'string') {
+      continue
+    }
+    // Undo close tab removes closed frames.
+    if (!closedFrames.get(key)) {
+      item.visible = false
+      continue
+    }
+    historyMenuIndicesByOrder[key] = i
+  }
+
+  let visibleItems = 0
+  closedFrames.reverse().forEach((frame, url) => {
+    const menuIndex = historyMenuIndicesByOrder[url]
+    if (visibleItems >= maxMenuItems) {
+      if (menuIndex) {
+        historyMenu.items[menuIndex].visible = false
+      }
+      return
+    }
+    if (menuIndex) {
+      historyMenu.items[menuIndex].visible = true
+      visibleItems += 1
+    } else {
+      const template = recentlyClosedTemplate(url, frame)
+      const item = new MenuItem(template)
+      // XXX: Can't set this with MenuItem constructor
+      item.id = template.id
+      historyMenu.insert(insertPosition, item)
+      visibleItems += 1
+      insertPosition = insertPosition + 1
+    }
+  })
+  return appMenu
+}
+
 const isItemValid = (currentItem, previousItem) => {
   if (previousItem && previousItem === CommonMenu.separatorMenuItem) {
     if (currentItem === CommonMenu.separatorMenuItem) {

app/renderer/reducers/frameReducer.js

@@ -106,7 +106,7 @@ const frameReducer = (state, action, immutableAction) => {
     case windowConstants.WINDOW_CLOSE_FRAMES:
       let closedFrames = new Immutable.List()
       action.framePropsList.forEach((frameProps) => {
-        if (!frameProps.get('isPrivate') && frameProps.get('location') !== 'about:newtab') {
+        if (frameStateUtil.isValidClosedFrame(frameProps)) {
           closedFrames = closedFrames.push(frameProps)
           if (closedFrames.size > config.maxClosedFrames) {
             closedFrames = closedFrames.shift()

js/constants/config.js

@@ -20,6 +20,10 @@ module.exports = {
   },
   fingerprintingInfoUrl: 'https://github.com/brave/browser-laptop/wiki/Fingerprinting-Protection-Mode',
   maxClosedFrames: 100,
+  menu: {
+    // History -> Recently closed frame list
+    maxClosedFrames: 10
+  },
   thumbnail: {
     width: 160,
     height: 100

js/state/frameStateUtil.js

@@ -377,7 +377,7 @@ function removeFrame (state, frameProps, framePropsIndex) {
   let closedFrames = state.get('closedFrames')
   const newFrames = frames.splice(framePropsIndex, 1)
 
-  if (!frameProps.get('isPrivate') && frameProps.get('location') !== 'about:newtab') {
+  if (isValidClosedFrame(frameProps)) {
     frameProps = frameProps.set('isFullScreen', false)
     closedFrames = closedFrames.push(frameProps)
     if (frameProps.get('thumbnailBlob')) {
@@ -484,6 +484,11 @@ const updateFramesInternalIndex = (state, fromIndex) => {
   return state.set('framesInternal', framesInternal)
 }
 
+const isValidClosedFrame = (frame) => {
+  const location = frame.get('location')
+  return !frame.get('isPrivate') && location.indexOf('about:newtab') !== 0 && location.indexOf('about:blank') !== 0
+}
+
 module.exports = {
   deleteTabInternalIndex,
   deleteFrameInternalIndex,
@@ -530,5 +535,6 @@ module.exports = {
   activeFrameStatePath,
   getLastCommittedURL,
   onFindBarHide,
-  getTotalBlocks
+  getTotalBlocks,
+  isValidClosedFrame
 }