Merge pull request #9279 from brave/fix/update-site-favicon-speed

In siteUtil.updateSiteFavicon use location cache
brave · Jun 7, 2017 · 15d8d36 · 15d8d36
1 parent 9ee3e6c
commit 15d8d36
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 63 deletions.
diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
@@ -3,7 +3,6 @@
 
 'use strict'
 const Immutable = require('immutable')
-const normalizeUrl = require('normalize-url')
 const siteCache = require('../../app/common/state/siteCache')
 const siteTags = require('../constants/siteTags')
 const settings = require('../constants/settings')
@@ -587,64 +586,26 @@ module.exports.getDetailFromCreateProperties = function (createProperties, tag)
 }
 
 /**
- * Update the favicon URL for all entries in the sites list
+ * Update the favicon URL for all entries in the state sites
  * which match a given location. Currently, there should only be
  * one match, but this will handle multiple.
  *
- * @param sites The application state's Immutable sites list
+ * @param state The application state
  * @param location URL for the entry needing an update
  * @param favicon favicon URL
  */
-module.exports.updateSiteFavicon = function (sites, location, favicon) {
-  sites = makeImmutable(sites)
-
+module.exports.updateSiteFavicon = function (state, location, favicon) {
   if (UrlUtil.isNotURL(location)) {
-    return sites
+    return state
   }
-  if (!Immutable.Map.isMap(sites)) {
-    return sites
+  const siteKeys = siteCache.getLocationSiteKeys(state, location)
+  if (!siteKeys || siteKeys.length === 0) {
+    return state
   }
-
-  const matchingIndices = []
-
-  sites.filter((site, index) => {
-    if (!site || typeof site.get !== 'function') {
-      return false
-    }
-    if (isBookmarkFolder(site.get('tags'))) {
-      return false
-    }
-    if (UrlUtil.isNotURL(site.get('location'))) {
-      return false
-    }
-    if (normURL(site.get('location')) === normURL(location)) {
-      matchingIndices.push(index)
-      return true
-    }
-    return false
-  })
-
-  if (!matchingIndices.length) return sites
-
-  let updatedSites = sites
-  matchingIndices.forEach((index) => {
-    updatedSites = updatedSites.setIn([index, 'favicon'], favicon)
+  siteKeys.forEach((siteKey) => {
+    state = state.setIn(['sites', siteKey, 'favicon'], favicon)
   })
-
-  return updatedSites
-}
-
-/**
- * Normalizes a URL for comparison, with special handling for magnet links
- */
-function normURL (url) {
-  const lowerURL = url.toLowerCase()
-  if (lowerURL.startsWith('magnet:?')) return lowerURL
-  try {
-    return normalizeUrl(url)
-  } catch (e) {
-    return url
-  }
+  return state
 }
 
 /**

diff --git a/js/stores/appStore.js b/js/stores/appStore.js
@@ -801,7 +801,7 @@ const handleAppAction = (action) => {
       appState = appState.set('defaultBrowserCheckComplete', {})
       break
     case windowConstants.WINDOW_SET_FAVICON:
-      appState = appState.set('sites', siteUtil.updateSiteFavicon(appState.get('sites'), action.frameProps.get('location'), action.favicon))
+      appState = siteUtil.updateSiteFavicon(appState, action.frameProps.get('location'), action.favicon)
       appState = aboutNewTabState.setSites(appState, action)
       break
     case appConstants.APP_RENDER_URL_TO_PDF:

diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js
@@ -1051,37 +1051,44 @@ describe('siteUtil', function () {
 
   describe('updateSiteFavicon', function () {
     it('updates the favicon for all matching entries (normalizing the URL)', function () {
+      const folderDetail = Immutable.fromJS({
+        folderId: 1,
+        tags: [siteTags.BOOKMARK_FOLDER]
+      })
       const siteDetail1 = Immutable.fromJS({
         tags: [siteTags.BOOKMARK],
         location: testUrl1,
         title: 'bookmarked site',
-        lastAccessedTime: 123
+        lastAccessedTime: 123,
+        parentFolderId: 1
       })
       const siteDetail2 = Immutable.fromJS({
         tags: [],
-        location: 'https://www.brave.com',
+        location: testUrl1,
         title: 'history entry',
         lastAccessedTime: 456
       })
-      let state = siteUtil.addSite(emptyState, siteDetail1, siteTags.BOOKMARK)
+      let state = siteUtil.addSite(emptyState, folderDetail, siteTags.BOOKMARK_FOLDER)
+      state = siteUtil.addSite(state, siteDetail1, siteTags.BOOKMARK)
       state = siteUtil.addSite(state, siteDetail2)
-      const processedSites = siteUtil.updateSiteFavicon(state.get('sites'), testUrl1, testFavicon1)
+      const processedState = siteUtil.updateSiteFavicon(state, testUrl1, testFavicon1)
       const updatedSiteDetail1 = siteDetail1.set('favicon', testFavicon1)
       const updatedSiteDetail2 = siteDetail2.set('favicon', testFavicon1)
-      let expectedState = siteUtil.addSite(emptyState, updatedSiteDetail1, siteTags.BOOKMARK)
+      let expectedState = siteUtil.addSite(emptyState, folderDetail, siteTags.BOOKMARK_FOLDER)
+      expectedState = siteUtil.addSite(expectedState, updatedSiteDetail1, siteTags.BOOKMARK)
       expectedState = siteUtil.addSite(expectedState, updatedSiteDetail2)
 
-      assert.deepEqual(processedSites.toJS(), expectedState.get('sites').toJS())
+      assert.deepEqual(processedState.get('sites').toJS(), expectedState.get('sites').toJS())
     })
     it('returns the object unchanged if location is not a URL', function () {
-      const sites = siteUtil.addSite(emptyState, bookmarkMinFields, siteTags.BOOKMARK)
-      const processedSites = siteUtil.updateSiteFavicon(sites, 'not-a-url', 'https://brave.com/favicon.ico')
-      assert.deepEqual(processedSites, sites)
+      const state = siteUtil.addSite(emptyState, bookmarkMinFields, siteTags.BOOKMARK)
+      const processedState = siteUtil.updateSiteFavicon(state, 'not-a-url', 'https://brave.com/favicon.ico')
+      assert.deepEqual(processedState.get('sites'), state.get('sites'))
     })
     it('returns the object unchanged if it is not an Immutable.Map', function () {
       const emptyLegacySites = Immutable.fromJS([])
-      const processedSites = siteUtil.updateSiteFavicon(emptyLegacySites, testUrl1, 'https://brave.com/favicon.ico')
-      assert.deepEqual(processedSites, emptyLegacySites)
+      const processedState = siteUtil.updateSiteFavicon(emptyLegacySites, testUrl1, 'https://brave.com/favicon.ico')
+      assert.deepEqual(processedState.get('sites'), emptyLegacySites.get('sites'))
     })
     it('works even if null/undefined entries are present', function () {
       const stateWithInvalidEntries = Immutable.fromJS({
@@ -1091,10 +1098,10 @@ describe('siteUtil', function () {
         }
       })
       const state = siteUtil.addSite(stateWithInvalidEntries, bookmarkMinFields, siteTags.BOOKMARK)
-      const processedSites = siteUtil.updateSiteFavicon(state.get('sites'), testUrl1, 'https://brave.com/favicon.ico')
+      const processedState = siteUtil.updateSiteFavicon(state, testUrl1, 'https://brave.com/favicon.ico')
       const updatedSiteDetail = bookmarkMinFields.set('favicon', 'https://brave.com/favicon.ico')
       const expectedState = siteUtil.addSite(stateWithInvalidEntries, updatedSiteDetail, siteTags.BOOKMARK)
-      assert.deepEqual(processedSites.toJS(), expectedState.get('sites').toJS())
+      assert.deepEqual(processedState.get('sites').toJS(), expectedState.get('sites').toJS())
     })
   })