Reduce redundant siteSort to sort once and used everywhere

fix #7240

Auditors: @bbondy, @bsclifton

Test Plan:
1. Import bunch of bookmarks
2. Open bookmarks of menu shouldn't affect performance
3. Open about:bookmarks shouldn't affect performance

app/browser/bookmarksExporter.js

@@ -52,7 +52,7 @@ function createBookmarkArray (sites, parentFolderId, first = true, depth = 1) {
 
   if (first) payload.push(`${indentFirst}<DL><p>`)
 
-  filteredBookmarks.toList().sort(siteUtil.siteSort).forEach((site) => {
+  filteredBookmarks.forEach((site) => {
     if (site.get('tags').includes(siteTags.BOOKMARK) && site.get('location')) {
       title = site.get('customTitle') || site.get('title') || site.get('location')
       payload.push(`${indentNext}<DT><A HREF="${site.get('location')}">${title}</A>`)

app/browser/menu.js

@@ -26,7 +26,7 @@ const menuUtil = require('../common/lib/menuUtil')
 const {getByTabId} = require('../common/state/tabState')
 const getSetting = require('../../js/settings').getSetting
 const locale = require('../locale')
-const {isLocationBookmarked, siteSort} = require('../../js/state/siteUtil')
+const {isLocationBookmarked} = require('../../js/state/siteUtil')
 const tabState = require('../../app/common/state/tabState')
 const isDarwin = process.platform === 'darwin'
 const isLinux = process.platform === 'linux'
@@ -381,7 +381,7 @@ const createBookmarksSubmenu = () => {
     CommonMenu.exportBookmarksMenuItem()
   ]
 
-  const bookmarks = menuUtil.createBookmarkTemplateItems(appStore.getState().get('sites').toList().sort(siteSort))
+  const bookmarks = menuUtil.createBookmarkTemplateItems(appStore.getState().get('sites'))
   if (bookmarks.length > 0) {
     submenu.push(CommonMenu.separatorMenuItem)
     submenu = submenu.concat(bookmarks)

app/browser/reducers/sitesReducer.js

@@ -69,6 +69,7 @@ const sitesReducer = (state, action, immutableAction) => {
             sourceKey, destinationKey, false, false, true)
         }
       }
+      state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
       if (syncEnabled()) {
         state = syncUtil.updateSiteCache(state, action.destinationDetail || action.siteDetail)
       }
@@ -77,6 +78,7 @@ const sitesReducer = (state, action, immutableAction) => {
     case appConstants.APP_REMOVE_SITE:
       const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite
       state = siteUtil.removeSite(state, action.siteDetail, action.tag, true, removeSiteSyncCallback)
+      state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
       if (syncEnabled()) {
         state = syncUtil.updateSiteCache(state, action.siteDetail)
       }
@@ -86,6 +88,7 @@ const sitesReducer = (state, action, immutableAction) => {
       state = siteUtil.moveSite(state,
         action.sourceKey, action.destinationKey, action.prepend,
         action.destinationIsParent, false)
+      state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
       if (syncEnabled()) {
         const destinationDetail = state.getIn(['sites', action.destinationKey])
         state = syncUtil.updateSiteCache(state, destinationDetail)

app/browser/tabs.js

@@ -18,7 +18,6 @@ const settings = require('../../js/constants/settings')
 const {getBaseUrl, aboutUrls} = require('../../js/lib/appUrlUtil')
 const siteSettings = require('../../js/state/siteSettings')
 const messages = require('../../js/constants/messages')
-const siteUtil = require('../../js/state/siteUtil')
 const aboutHistoryState = require('../common/state/aboutHistoryState')
 const appStore = require('../../js/stores/appStore')
 const appConfig = require('../../js/constants/appConfig')
@@ -135,8 +134,8 @@ ipcMain.on(messages.ABOUT_COMPONENT_INITIALIZED, (e) => {
 })
 
 const getBookmarksData = function (state) {
-  let bookmarkSites = new Immutable.Map()
-  let bookmarkFolderSites = new Immutable.Map()
+  let bookmarkSites = new Immutable.OrderedMap()
+  let bookmarkFolderSites = new Immutable.OrderedMap()
   state.get('sites').forEach((site, siteKey) => {
     const tags = site.get('tags')
     if (tags.includes(siteTags.BOOKMARK)) {
@@ -146,8 +145,8 @@ const getBookmarksData = function (state) {
       bookmarkFolderSites = bookmarkFolderSites.set(siteKey, site)
     }
   })
-  const bookmarks = bookmarkSites.toList().sort(siteUtil.siteSort).toJS()
-  const bookmarkFolders = bookmarkFolderSites.toList().sort(siteUtil.siteSort).toJS()
+  const bookmarks = bookmarkSites.toList().toJS()
+  const bookmarkFolders = bookmarkFolderSites.toList().toJS()
   return {bookmarks, bookmarkFolders}
 }
 

app/browser/windows.js

@@ -12,7 +12,6 @@ const LocalShortcuts = require('../localShortcuts')
 const {getPinnedSiteProps} = require('../common/lib/windowsUtil')
 const {makeImmutable} = require('../common/state/immutableUtil')
 const {getPinnedTabsByWindowId} = require('../common/state/tabState')
-const {siteSort} = require('../../js/state/siteUtil')
 const messages = require('../../js/constants/messages')
 const settings = require('../../js/constants/settings')
 const siteTags = require('../../js/constants/siteTags')
@@ -84,7 +83,7 @@ const updatePinnedTabs = (win) => {
   const sitesToAdd = pinnedSites.filter((site) =>
     !win.__alreadyPinnedSites.find((pinned) => pinned.equals(site)))
 
-  sitesToAdd.sort(siteSort).forEach((site) => {
+  sitesToAdd.forEach((site) => {
     win.__alreadyPinnedSites = win.__alreadyPinnedSites.add(site)
     appActions.createTabRequested({
       url: site.get('location'),

app/index.js

@@ -76,6 +76,7 @@ const privacy = require('../js/state/privacy')
 const async = require('async')
 const settings = require('../js/constants/settings')
 const BookmarksExporter = require('./browser/bookmarksExporter')
+const siteUtil = require('../js/state/siteUtil')
 
 app.commandLine.appendSwitch('enable-features', 'BlockSmallPluginContent,PreferHtmlOverPlugins')
 
@@ -321,7 +322,10 @@ app.on('ready', () => {
     // For tests we always want to load default app state
     const loadedPerWindowState = initialState.perWindowState
     delete initialState.perWindowState
-    appActions.setState(Immutable.fromJS(initialState))
+    // Retore map order after load
+    let state = Immutable.fromJS(initialState)
+    state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
+    appActions.setState(state)
     setImmediate(() => perWindowStateLoaded(loadedPerWindowState))
   })
 

app/renderer/components/bookmarks/bookmarksToolbar.js

@@ -108,7 +108,8 @@ class BookmarksToolbar extends ImmutableComponent {
     contextMenus.onShowBookmarkFolderMenu(this.bookmarks, bookmark, this.activeFrame, e)
   }
   updateBookmarkData (props) {
-    this.bookmarks = siteUtil.getBookmarks(props.sites).toList().sort(siteUtil.siteSort)
+    // TODO(darkdh): Remove siteSort when we have #9030 landed
+    this.bookmarks = siteUtil.getBookmarks(props.sites).sort(siteUtil.siteSort)
 
     const noParentItems = this.bookmarks
       .filter((bookmark) => !bookmark.get('parentFolderId'))
@@ -153,9 +154,9 @@ class BookmarksToolbar extends ImmutableComponent {
         break
       }
     }
-    this.bookmarksForToolbar = noParentItems.take(i).sort(siteUtil.siteSort)
+    this.bookmarksForToolbar = noParentItems.take(i)
     // Show at most 100 items in the overflow menu
-    this.overflowBookmarkItems = noParentItems.skip(i).take(100).sort(siteUtil.siteSort)
+    this.overflowBookmarkItems = noParentItems.skip(i).take(100)
   }
   componentWillMount () {
     this.updateBookmarkData(this.props)

test/unit/app/browser/reducers/sitesReducerTest.js

@@ -139,6 +139,9 @@ describe('sitesReducerTest', function () {
       newState = sitesReducer(newState, addAction)
       assert.equal(Object.keys(newState.get('sites').toJS()).length, 3)
 
+      // sites are sorted after #8075 landed
+      // sites[0] will be order 0, sites[1] will be order 1...etc
+
       // Move the site to the 2nd position
       newState = sitesReducer(newState, moveAction).toJS()
       assert.equal(Object.keys(newState.sites).length, 3)
@@ -149,8 +152,8 @@ describe('sitesReducerTest', function () {
       moveAction.prepend = true
       newState = sitesReducer(Immutable.fromJS(newState), moveAction).toJS()
       assert.equal(Object.keys(newState.sites).length, 3)
-      assert.equal(Object.values(newState.sites)[2].location, url)
-      assert.equal(Object.values(newState.sites)[2].order, 1)
+      assert.equal(Object.values(newState.sites)[1].location, url)
+      assert.equal(Object.values(newState.sites)[1].order, 1)
     })
   })
 })