Skip to content

Commit

Permalink
Merge pull request brave#8894 from brave/fix/url-bar-is-bookmarked-cache
Browse files Browse the repository at this point in the history
Add locationSiteKeyCache and use for siteUtil.isBookmarked
  • Loading branch information
bbondy authored May 19, 2017
2 parents 9a4e067 + 980a855 commit 5fc6f05
Show file tree
Hide file tree
Showing 14 changed files with 524 additions and 196 deletions.
4 changes: 2 additions & 2 deletions app/browser/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {isSiteBookmarked, siteSort} = require('../../js/state/siteUtil')
const {isLocationBookmarked, siteSort} = require('../../js/state/siteUtil')
const tabState = require('../../app/common/state/tabState')
const isDarwin = process.platform === 'darwin'
const isLinux = process.platform === 'linux'
Expand Down Expand Up @@ -351,7 +351,7 @@ const createHistorySubmenu = () => {
}

const isCurrentLocationBookmarked = () => {
return isSiteBookmarked(appStore.getState().get('sites'), Immutable.fromJS({location: currentLocation}))
return isLocationBookmarked(appStore.getState(), currentLocation)
}

const createBookmarksSubmenu = () => {
Expand Down
48 changes: 30 additions & 18 deletions app/browser/reducers/sitesReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

const appConstants = require('../../../js/constants/appConstants')
const filtering = require('../../filtering')
const siteCache = require('../../../js/state/siteCache')
const siteTags = require('../../../js/constants/siteTags')
const siteUtil = require('../../../js/state/siteUtil')
const syncActions = require('../../../js/actions/syncActions')
Expand All @@ -14,13 +15,26 @@ const Immutable = require('immutable')
const settings = require('../../../js/constants/settings')
const {getSetting} = require('../../../js/settings')
const writeActions = require('../../../js/constants/sync/proto').actions
const tabState = require('../../common/state/tabState')

const syncEnabled = () => {
return getSetting(settings.SYNC_ENABLED) === true
}

const updateActiveTabBookmarked = (state) => {
const tab = tabState.getActiveTab(state)
if (!tab) {
return state
}
const bookmarked = siteUtil.isLocationBookmarked(state, tab.get('url'))
return tabState.updateTabValue(state, tab.set('bookmarked', bookmarked))
}

const sitesReducer = (state, action, immutableAction) => {
switch (action.actionType) {
case appConstants.APP_SET_STATE:
state = siteCache.loadLocationSiteKeysCache(state)
break
case appConstants.APP_ON_CLEAR_BROWSING_DATA:
if (immutableAction.getIn(['clearDataDetail', 'browserHistory'])) {
state = state.set('sites', siteUtil.clearHistory(state.get('sites')))
Expand All @@ -30,39 +44,41 @@ const sitesReducer = (state, action, immutableAction) => {
case appConstants.APP_ADD_SITE:
if (Immutable.List.isList(action.siteDetail)) {
action.siteDetail.forEach((s) => {
state = state.set('sites', siteUtil.addSite(state.get('sites'), s, action.tag, undefined, action.skipSync))
state = siteUtil.addSite(state, s, action.tag, undefined, action.skipSync)
})
} else {
let sites = state.get('sites')
if (!action.siteDetail.get('folderId') && siteUtil.isFolder(action.siteDetail)) {
action.siteDetail = action.siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
}
state = state.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag, action.originalSiteDetail, action.skipSync))
state = siteUtil.addSite(state, action.siteDetail, action.tag, action.originalSiteDetail, action.skipSync)
}
if (action.destinationDetail) {
const sourceKey = siteUtil.getSiteKey(action.siteDetail)
const destinationKey = siteUtil.getSiteKey(action.destinationDetail)

if (sourceKey != null) {
state = state.set('sites', siteUtil.moveSite(state.get('sites'),
sourceKey, destinationKey, false, false, true))
state = siteUtil.moveSite(state,
sourceKey, destinationKey, false, false, true)
}
}
if (syncEnabled()) {
state = syncUtil.updateSiteCache(state, action.destinationDetail || action.siteDetail)
}
state = updateActiveTabBookmarked(state)
break
case appConstants.APP_REMOVE_SITE:
const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite
state = state.set('sites', siteUtil.removeSite(state.get('sites'), action.siteDetail, action.tag, true, removeSiteSyncCallback))
state = siteUtil.removeSite(state, action.siteDetail, action.tag, true, removeSiteSyncCallback)
if (syncEnabled()) {
state = syncUtil.updateSiteCache(state, action.siteDetail)
}
state = updateActiveTabBookmarked(state)
break
case appConstants.APP_MOVE_SITE:
state = state.set('sites', siteUtil.moveSite(state.get('sites'),
state = siteUtil.moveSite(state,
action.sourceKey, action.destinationKey, action.prepend,
action.destinationIsParent, false))
action.destinationIsParent, false)
if (syncEnabled()) {
const destinationDetail = state.getIn(['sites', action.destinationKey])
state = syncUtil.updateSiteCache(state, destinationDetail)
Expand All @@ -85,19 +101,15 @@ const sitesReducer = (state, action, immutableAction) => {
const siteData = syncUtil.getSiteDataFromRecord(record, state, action.records)
const tag = siteData.tag
let siteDetail = siteData.siteDetail
const sites = state.get('sites')
switch (record.action) {
case writeActions.CREATE:
state = state.set('sites',
siteUtil.addSite(sites, siteDetail, tag, undefined, true))
state = siteUtil.addSite(state, siteDetail, tag, undefined, true)
break
case writeActions.UPDATE:
state = state.set('sites',
siteUtil.addSite(sites, siteDetail, tag, siteData.existingObjectData, true))
state = siteUtil.addSite(state, siteDetail, tag, siteData.existingObjectData, true)
break
case writeActions.DELETE:
state = state.set('sites',
siteUtil.removeSite(sites, siteDetail, tag))
state = siteUtil.removeSite(state, siteDetail, tag)
break
}
state = syncUtil.updateSiteCache(state, siteDetail)
Expand All @@ -115,9 +127,9 @@ const sitesReducer = (state, action, immutableAction) => {
const sites = state.get('sites')
const siteDetail = siteUtil.getDetailFromTab(tab, siteTags.PINNED, sites)
if (pinned) {
state = state.set('sites', siteUtil.addSite(sites, siteDetail, siteTags.PINNED))
state = siteUtil.addSite(state, siteDetail, siteTags.PINNED)
} else {
state = state.set('sites', siteUtil.removeSite(sites, siteDetail, siteTags.PINNED))
state = siteUtil.removeSite(state, siteDetail, siteTags.PINNED)
}
if (syncEnabled()) {
state = syncUtil.updateSiteCache(state, siteDetail)
Expand All @@ -127,8 +139,8 @@ const sitesReducer = (state, action, immutableAction) => {
case appConstants.APP_CREATE_TAB_REQUESTED: {
const createProperties = immutableAction.get('createProperties')
if (createProperties.get('pinned')) {
state = state.set('sites', siteUtil.addSite(state.get('sites'),
siteUtil.getDetailFromCreateProperties(createProperties), siteTags.PINNED))
state = siteUtil.addSite(state,
siteUtil.getDetailFromCreateProperties(createProperties), siteTags.PINNED)
}
break
}
Expand Down
6 changes: 5 additions & 1 deletion app/common/state/tabState.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const frameState = require('./frameState')
const windowState = require('./windowState')
// this file should eventually replace frameStateUtil
const frameStateUtil = require('../../../js/state/frameStateUtil')
const {isLocationBookmarked} = require('../../../js/state/siteUtil')

const validateId = function (propName, id) {
assert.ok(id, `${propName} cannot be null`)
Expand Down Expand Up @@ -283,7 +284,10 @@ const tabState = {
return state
}

tabValue = tabValue.set('frame', makeImmutable(action.get('frame')))
const frameLocation = action.getIn(['frame', 'location'])
const frameBookmarked = isLocationBookmarked(state, frameLocation)
const frameValue = action.get('frame').set('bookmarked', frameBookmarked)
tabValue = tabValue.set('frame', makeImmutable(frameValue))
return tabState.updateTabValue(state, tabValue)
},

Expand Down
7 changes: 3 additions & 4 deletions app/renderer/components/navigation/navigationBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,7 @@ class NavigationBar extends React.Component {

get bookmarked () {
return this.props.activeFrameKey !== undefined &&
siteUtil.isSiteBookmarked(this.props.sites, Immutable.fromJS({
location: this.props.location,
partitionNumber: this.props.partitionNumber
}))
this.props.bookmarked
}

componentDidMount () {
Expand All @@ -126,6 +123,7 @@ class NavigationBar extends React.Component {
const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map()
const activeFrameKey = activeFrame.get('key')
const activeTabId = activeFrame.get('tabId') || tabState.TAB_ID_NONE
const activeTab = tabState.getByTabId(state, activeTabId)

const activeTabShowingMessageBox = tabState.isShowingMessageBox(state, activeTabId)
const bookmarkDetail = currentWindow.get('bookmarkDetail')
Expand Down Expand Up @@ -155,6 +153,7 @@ class NavigationBar extends React.Component {
props.activeFrameKey = activeFrameKey
props.location = location
props.title = title
props.bookmarked = activeTab && activeTab.get('bookmarked')
props.partitionNumber = activeFrame.get('partitionNumber') || 0
props.isSecure = activeFrame.getIn(['security', 'isSecure'])
props.loading = loading
Expand Down
1 change: 1 addition & 0 deletions app/sessionStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,7 @@ module.exports.defaultAppState = () => {
lastFetchTimestamp: 0,
objectsById: {}
},
locationSiteKeysCache: undefined,
sites: getTopSiteMap(),
tabs: [],
windows: [],
Expand Down
3 changes: 3 additions & 0 deletions docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,9 @@ AppStore
title: string
} // folder: folderId; bookmark/history: location + partitionNumber + parentFolderId
},
locationSiteKeyCache: {
[location]: Array.<string> // location -> site keys
},
siteSettings: {
[hostPattern]: {
adControl: string, // (showBraveAds | blockAds | allowAdsAndTracking)
Expand Down
1 change: 1 addition & 0 deletions js/about/newtab.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class NewTabPage extends React.Component {
}).size > 0
}
isBookmarked (siteProps) {
// XXX: Fixme, not passing state in!
return siteUtil.isSiteBookmarked(this.topSites, siteProps)
}
get gridLayout () {
Expand Down
15 changes: 12 additions & 3 deletions js/lib/urlutil.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,10 +306,19 @@ const UrlUtil = {
* @return {string}
*/
getLocationIfPDF: function (url) {
if (url && url.startsWith(`chrome-extension://${pdfjsExtensionId}/`)) {
return url.replace(`chrome-extension://${pdfjsExtensionId}/`, '')
if (!url || url.indexOf(`chrome-extension://${pdfjsExtensionId}/`) === -1) {
return url
}
return url

if (url.indexOf('content/web/viewer.html?file=') !== -1) {
const querystring = require('querystring')
const parsedUrl = urlParse(url)
const query = querystring.parse(parsedUrl.query)
if (query && query.file) {
return query.file
}
}
return url.replace(`chrome-extension://${pdfjsExtensionId}/`, '')
},

/**
Expand Down
90 changes: 90 additions & 0 deletions js/state/siteCache.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/* This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

'use strict'
const Immutable = require('immutable')
const siteUtil = require('./siteUtil')
const UrlUtil = require('../lib/urlutil')

const createLocationSiteKeysCache = (state) => {
state = state.set('locationSiteKeysCache', new Immutable.Map())
state.get('sites').forEach((site, siteKey) => {
const location = siteUtil.getLocationFromSiteKey(siteKey)
if (!location) {
return
}
state = addLocationSiteKey(state, location, siteKey)
})
return state
}

module.exports.loadLocationSiteKeysCache = (state) => {
const cache = state.get('locationSiteKeysCache')
if (cache) {
return state
}
return createLocationSiteKeysCache(state)
}

/**
* Given a location, get matching appState siteKeys based on cache.
* Loads cache from appState if it hasn't been loaded yet.
* @param state Application state
* @param location {string}
* @return {Immutable.List<string>|null} siteKeys including this location.
*/
module.exports.getLocationSiteKeys = (state, location) => {
const normalLocation = UrlUtil.getLocationIfPDF(location)
return state.getIn(['locationSiteKeysCache', normalLocation])
}

/**
* Given a location, add appState siteKey to cached siteKeys list.
* Returns new state.
* @param state Application state
* @param location {string}
* @param siteKey {string}
*/
const addLocationSiteKey = (state, location, siteKey) => {
if (!siteKey || !location) {
return state
}
const normalLocation = UrlUtil.getLocationIfPDF(location)
const cacheKey = ['locationSiteKeysCache', normalLocation]
const siteKeys = state.getIn(cacheKey)
if (!siteKeys) {
return state.setIn(cacheKey, new Immutable.List([siteKey]))
} else {
if (siteKeys.includes(siteKey)) {
return state
}
return state.setIn(cacheKey, siteKeys.push(siteKey))
}
}
module.exports.addLocationSiteKey = addLocationSiteKey

/**
* Given a location, remove matching appState siteKeys in cache.
* Loads cache from appState if it hasn't been loaded yet.
* @param state Application state
* @param location {string}
* @param siteKey {string}
*/
const removeLocationSiteKey = (state, location, siteKey) => {
if (!siteKey || !location) {
return state
}
const normalLocation = UrlUtil.getLocationIfPDF(location)
const cacheKey = ['locationSiteKeysCache', normalLocation]
let siteKeys = state.getIn(cacheKey)
if (!siteKeys) {
return state
}
siteKeys = siteKeys.filter(key => key !== siteKey)
if (siteKeys.size > 0) {
return state.setIn(cacheKey, siteKeys)
} else {
return state.deleteIn(cacheKey)
}
}
module.exports.removeLocationSiteKey = removeLocationSiteKey
Loading

0 comments on commit 5fc6f05

Please sign in to comment.