From e828c16b2861fd21e00a5251c90fdd7a8c3f0f66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EF=BD=81=EF=BD=99=EF=BD=95=EF=BD=8D=EF=BD=89=C2=A0=20?= =?UTF-8?q?=EF=BD=99=EF=BD=95?= Date: Tue, 6 Jun 2017 01:15:38 +0000 Subject: [PATCH] In siteUtil.updateSiteFavicon use location cache Fix #9276 Auditors: @bsclifton @bbondy Test Plan: 1. Enable bookmarks bar favicons via Preferences > General > "Bookmarks Bar" => "Text and Favicons". 2. Create a bookmark folder. 3. Navigate to https://archive.org 4. Bookmark page. 5. Navigate to https://wikipedia.org 6. Bookmark page into the folder. 7. Examine bookmarks toolbar; there should be favicons for archive and wikipedia. 8. Open the Bookmarks Manager and confirm there are favicons. --- js/state/siteUtil.js | 59 ++++++--------------------------- js/stores/appStore.js | 2 +- test/unit/state/siteUtilTest.js | 33 ++++++++++-------- 3 files changed, 31 insertions(+), 63 deletions(-) diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 7237b9af77c..595df5a97b7 100644 --- 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 index 5d6f25a5df5..c2f945729d0 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -805,7 +805,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 index 3d9faa45ecd..b8b157716b6 100644 --- 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()) }) })