Change sites from List to Map (#5531)

* Change sites from List to Map

fix #4879

Auditors: @bbondy, @bsclifton, @cezaraugusto

Test Plan:
Performance:
1. Import bulk bookmarks (4000+) from other browsers (Don't merge into toolbar)
2. The process should finish instantly
3. Delete Import from XXX folder
4. The process should finish instantly

Migration:
1. Make sure session-store-1 contains the sites data before 0.12.10
2. Lauch Brave and Close
3. sites of session-store-1 should change from [] tp {}

* Address @bsclifton's feedback

* Apply changes to aboutNewTabState

* - Added migration for newtab data (and tested manually several times)
- Updated tests to be using/expecting a map (not a list)
- Updated siteUtil to return empty map where appropriate (instead of empty array)
- Removed unused method in siteUtil
- Extra error handling in siteUtil.updateSiteFavicon
- Added back some of the missing tests for updateSiteFavicon
- Update newtab > getUnpinned() to use slice (since it's a map)

Auditors: @darkdh, @cezaraugusto

* removed tags (not needed)

app/common/state/aboutNewTabState.js

@@ -10,14 +10,6 @@ const excludeSiteDetail = (siteDetail) => {
   return !siteUtil.isBookmark(siteDetail) && !siteUtil.isHistoryEntry(siteDetail)
 }
 
-const removeDuplicateSites = (sites) => {
-  // Filter out duplicate entries by location
-  return sites.filter((element, index, list) => {
-    if (!element) return false
-    return index === list.findIndex((site) => site && site.get('location') === element.get('location'))
-  })
-}
-
 const aboutNewTabState = {
   mergeDetails: (state, props) => {
     state = makeImmutable(state)
@@ -45,9 +37,7 @@ const aboutNewTabState = {
     }
 
     // Keep track of the last 18 visited sites
-    let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.List()
-    sites = sites.unshift(siteDetail)
-    sites = removeDuplicateSites(sites)
+    let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.Map()
     sites = sites.take(18)
     // TODO(cezaraugusto): Sort should respect unshift and don't prioritize bookmarks
     // |

app/importer.js

@@ -23,6 +23,7 @@ const locale = require('./locale')
 var isMergeFavorites = false
 var isImportingBookmarks = false
 var hasBookmarks
+var importedSites
 
 exports.init = () => {
   importer.initialize()
@@ -171,6 +172,7 @@ importer.on('add-bookmarks', (e, bookmarks, topLevelFolder) => {
       sites.push(site)
     }
   }
+  importedSites = Immutable.fromJS(sites)
   appActions.addSite(Immutable.fromJS(sites))
 })
 
@@ -187,7 +189,7 @@ importer.on('add-favicons', (e, detail) => {
       }
     }
   })
-  let sites = AppStore.getState().get('sites')
+  let sites = importedSites
   sites = sites.map((site) => {
     if ((site.get('favicon') === undefined && site.get('location') !== undefined &&
       faviconMap[site.get('location')] !== undefined) ||

app/renderer/components/bookmarksToolbar.js

@@ -319,27 +319,37 @@ class BookmarksToolbar extends ImmutableComponent {
 
     // Loop through until we fill up the entire bookmark toolbar width
     let i
-    for (i = 0; i < noParentItems.size; i++) {
+    let noParentItemsList = noParentItems.toList()
+    for (i = 0; i < noParentItemsList.size; i++) {
       let iconWidth = props.showFavicon ? iconSize : 0
       // font-awesome file icons are 3px smaller
-      if (props.showFavicon && !noParentItems.getIn([i, 'folderId']) && !noParentItems.getIn([i, 'favicon'])) {
+      if (props.showFavicon && !noParentItemsList.getIn([i, 'folderId']) && !noParentItemsList.getIn([i, 'favicon'])) {
         iconWidth -= 3
       }
-      const chevronWidth = props.showFavicon && noParentItems.getIn([i, 'folderId']) ? this.chevronWidth : 0
+      const chevronWidth = props.showFavicon && noParentItemsList.getIn([i, 'folderId']) ? this.chevronWidth : 0
       if (props.showFavicon && props.showOnlyFavicon) {
         widthAccountedFor += this.padding + iconWidth + chevronWidth
       } else {
-        const text = noParentItems.getIn([i, 'customTitle']) || noParentItems.getIn([i, 'title']) || noParentItems.getIn([i, 'location'])
+        const text = noParentItemsList.getIn([i, 'customTitle']) || noParentItemsList.getIn([i, 'title']) || noParentItemsList.getIn([i, 'location'])
         widthAccountedFor += Math.min(calculateTextWidth(text, `${this.fontSize} ${this.fontFamily}`) + this.padding + iconWidth + chevronWidth, this.maxWidth)
       }
       widthAccountedFor += margin
       if (widthAccountedFor >= props.windowWidth - overflowButtonWidth) {
         break
       }
     }
-    this.bookmarksForToolbar = noParentItems.take(i)
+    const bookmarkSort = (x, y) => {
+      if (x.get('order') < y.get('order')) {
+        return -1
+      } else if (x.get('order') > y.get('order')) {
+        return 1
+      } else {
+        return 0
+      }
+    }
+    this.bookmarksForToolbar = noParentItems.take(i).sort(bookmarkSort)
     // Show at most 100 items in the overflow menu
-    this.overflowBookmarkItems = noParentItems.skip(i).take(100)
+    this.overflowBookmarkItems = noParentItems.skip(i).take(100).sort(bookmarkSort)
   }
   componentWillMount () {
     this.updateBookmarkData(this.props)

app/sessionStore.js

@@ -432,6 +432,28 @@ module.exports.loadAppState = () => {
         }
       }
 
+      // sites refactoring migration
+      if (Array.isArray(data.sites) && data.sites.length) {
+        let sites = {}
+        data.sites.forEach((site) => {
+          let key = siteUtil.getSiteKey(Immutable.fromJS(site))
+          sites[key] = site
+        })
+        data.sites = sites
+      }
+      if (data.about && data.about.newtab && data.about.newtab.sites) {
+        if (Array.isArray(data.about.newtab.sites) && data.about.newtab.sites.length) {
+          let sites = {}
+          data.about.newtab.sites.forEach((site) => {
+            if (site) {
+              let key = siteUtil.getSiteKey(Immutable.fromJS(site))
+              sites[key] = site
+            }
+          })
+          data.about.newtab.sites = sites
+        }
+      }
+
       // version information (shown on about:brave)
       const os = require('os')
       const versionInformation = [
@@ -466,7 +488,7 @@ module.exports.loadAppState = () => {
 module.exports.defaultAppState = () => {
   return {
     firstRunTimestamp: new Date().getTime(),
-    sites: [],
+    sites: {},
     tabs: [],
     extensions: {},
     visits: [],

docs/state.md

@@ -34,7 +34,8 @@ AppStore
       }
     }
   },
-  sites: [{
+  sites: {
+  [siteKey]: { // folder: folderId; bookmark/history: location + partitionNumber + parentFolderId
     location: string,
     title: string,
     customTitle: string, // User provided title for bookmark; overrides title
@@ -46,7 +47,7 @@ AppStore
     partitionNumber: number, // Optionally specifies a specific session
     folderId: number, // Set for bookmark folders only
     parentFolderId: number // Set for bookmarks and bookmark folders only
-  }],
+  }},
   downloads: [{
     [downloadId]: {
       startTime: number, // datetime.getTime()

js/about/newtab.js

@@ -105,7 +105,7 @@ class NewTabPage extends React.Component {
 
     const getUnpinned = () => {
       const firstSite = unpinnedTopSites.first()
-      unpinnedTopSites = unpinnedTopSites.shift()
+      unpinnedTopSites = unpinnedTopSites.slice(1)
       return firstSite
     }