Reduce redundant siteSort to sort once and used everywhere

fix #7240 Auditors: @bbondy, @bsclifton Test Plan: 1. Import bunch of bookmarks 2. Open folder on bookmark toolbar shouldn't affect performance
brave · Apr 5, 2017 · 463524a · 463524a
1 parent e97495d
commit 463524a
Show file tree

Hide file tree

Showing 6 changed files with 17 additions and 11 deletions.
diff --git a/app/browser/bookmarksExporter.js b/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.toList().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>`)

diff --git a/app/browser/menu.js b/app/browser/menu.js
@@ -24,7 +24,7 @@ const {fileUrl} = require('../../js/lib/appUrlUtil')
 const menuUtil = require('../common/lib/menuUtil')
 const getSetting = require('../../js/settings').getSetting
 const locale = require('../locale')
-const {isSiteBookmarked, siteSort} = require('../../js/state/siteUtil')
+const {isSiteBookmarked} = require('../../js/state/siteUtil')
 const isDarwin = process.platform === 'darwin'
 const isLinux = process.platform === 'linux'
 const aboutUrl = 'https://brave.com/'
@@ -367,7 +367,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)

diff --git a/app/renderer/components/bookmarksToolbar.js b/app/renderer/components/bookmarksToolbar.js
@@ -329,7 +329,7 @@ class BookmarksToolbar extends ImmutableComponent {
     contextMenus.onShowBookmarkFolderMenu(this.bookmarks, bookmark, this.activeFrame, e)
   }
   updateBookmarkData (props) {
-    this.bookmarks = siteUtil.getBookmarks(props.sites).toList().sort(siteUtil.siteSort)
+    this.bookmarks = siteUtil.getBookmarks(props.sites)
 
     const noParentItems = this.bookmarks
       .filter((bookmark) => !bookmark.get('parentFolderId'))
@@ -374,9 +374,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)

diff --git a/js/components/frame.js b/js/components/frame.js
@@ -113,8 +113,8 @@ class Frame extends ImmutableComponent {
       if (!Immutable.is(prevProps.bookmarks, this.props.bookmarks) ||
           !Immutable.is(prevProps.bookmarkFolders, this.props.bookmarkFolders)) {
         this.webview.send(messages.BOOKMARKS_UPDATED, {
-          bookmarks: this.props.bookmarks.toList().sort(siteUtil.siteSort).toJS(),
-          bookmarkFolders: this.props.bookmarkFolders.toList().sort(siteUtil.siteSort).toJS()
+          bookmarks: this.props.bookmarks.toJS(),
+          bookmarkFolders: this.props.bookmarkFolders.toJS()
         })
       }
     } else if (location === 'about:history' && this.props.history) {

diff --git a/js/components/window.js b/js/components/window.js
@@ -13,7 +13,6 @@ const Main = require('./main')
 const SiteTags = require('../constants/siteTags')
 const cx = require('../lib/classSet')
 const {getPlatformStyles} = require('../../app/common/lib/platformUtil')
-const {siteSort} = require('../state/siteUtil')
 
 class Window extends React.Component {
   constructor (props) {
@@ -116,7 +115,7 @@ class Window extends React.Component {
             frame.get('pinnedLocation') === site.get('location') &&
             (frame.get('partitionNumber') || 0) === (site.get('partitionNumber') || 0))
       })
-    sitesToAdd.toList().sort(siteSort).forEach((site) => {
+    sitesToAdd.forEach((site) => {
       windowActions.newFrame({
         location: site.get('location'),
         partitionNumber: site.get('partitionNumber'),

diff --git a/js/stores/appStore.js b/js/stores/appStore.js
@@ -244,7 +244,10 @@ class AppStore extends EventEmitter {
 
   emitChanges (emitFullState) {
     if (lastEmittedState) {
-      const d = diff(lastEmittedState, appState)
+      // diff and patch will make map out of order so toList() is to maintain
+      // sites map order
+      const diffAppState = appState.set('sites', appState.get('sites').toList())
+      const d = diff(lastEmittedState, diffAppState)
       if (!d.isEmpty()) {
         BrowserWindow.getAllWindows().forEach((wnd) =>
           wnd.webContents.send(messages.APP_STATE_CHANGE, { stateDiff: d.toJS() }))
@@ -456,6 +459,7 @@ const handleAppAction = (action) => {
               siteUtil.removeSite(sites, siteDetail, tag))
             break
         }
+        appState = appState.set('sites', appState.get('sites').sort(siteUtil.siteSort))
       })
       appState = aboutNewTabState.setSites(appState)
       appState = aboutHistoryState.setHistory(appState)
@@ -478,6 +482,7 @@ const handleAppAction = (action) => {
         appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'),
           action.siteDetail, action.destinationDetail, false, false, true))
       }
+      appState = appState.set('sites', appState.get('sites').sort(siteUtil.siteSort))
       // If there was an item added then clear out the old history entries
       if (oldSiteSize !== appState.get('sites').size) {
         filterOutNonRecents()
@@ -488,6 +493,7 @@ const handleAppAction = (action) => {
     case appConstants.APP_REMOVE_SITE:
       const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite
       appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag, true, removeSiteSyncCallback))
+      appState = appState.set('sites', appState.get('sites').sort(siteUtil.siteSort))
       appState = aboutNewTabState.setSites(appState, action)
       appState = aboutHistoryState.setHistory(appState, action)
       break
@@ -496,6 +502,7 @@ const handleAppAction = (action) => {
         appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'),
           action.sourceDetail, action.destinationDetail, action.prepend,
           action.destinationIsParent, false, syncActions.updateSite))
+        appState = appState.set('sites', appState.get('sites').sort(siteUtil.siteSort))
         break
       }
     case appConstants.APP_CLEAR_HISTORY: