Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Reduce redundant siteSort to sort once and used everywhere
Browse files Browse the repository at this point in the history
fix #7240

Auditors: @bbondy, @bsclifton

Test Plan:
1. Import bunch of bookmarks
2. Open bookmarks of menu shouldn't affect performance
3. Open about:bookmarks shouldn't affect performance
  • Loading branch information
darkdh committed May 31, 2017
1 parent 4ce9744 commit 33f3622
Show file tree
Hide file tree
Showing 8 changed files with 22 additions and 16 deletions.
2 changes: 1 addition & 1 deletion app/browser/bookmarksExporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.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>`)
Expand Down
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 {isLocationBookmarked, siteSort} = require('../../js/state/siteUtil')
const {isLocationBookmarked} = 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 @@ -381,7 +381,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)
Expand Down
3 changes: 3 additions & 0 deletions app/browser/reducers/sitesReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ const sitesReducer = (state, action, immutableAction) => {
sourceKey, destinationKey, false, false, true)
}
}
state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
if (syncEnabled()) {
state = syncUtil.updateSiteCache(state, action.destinationDetail || action.siteDetail)
}
Expand All @@ -77,6 +78,7 @@ const sitesReducer = (state, action, immutableAction) => {
case appConstants.APP_REMOVE_SITE:
const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite
state = siteUtil.removeSite(state, action.siteDetail, action.tag, true, removeSiteSyncCallback)
state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
if (syncEnabled()) {
state = syncUtil.updateSiteCache(state, action.siteDetail)
}
Expand All @@ -86,6 +88,7 @@ const sitesReducer = (state, action, immutableAction) => {
state = siteUtil.moveSite(state,
action.sourceKey, action.destinationKey, action.prepend,
action.destinationIsParent, false)
state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
if (syncEnabled()) {
const destinationDetail = state.getIn(['sites', action.destinationKey])
state = syncUtil.updateSiteCache(state, destinationDetail)
Expand Down
9 changes: 4 additions & 5 deletions app/browser/tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ const settings = require('../../js/constants/settings')
const {getBaseUrl, aboutUrls} = require('../../js/lib/appUrlUtil')
const siteSettings = require('../../js/state/siteSettings')
const messages = require('../../js/constants/messages')
const siteUtil = require('../../js/state/siteUtil')
const aboutHistoryState = require('../common/state/aboutHistoryState')
const appStore = require('../../js/stores/appStore')
const appConfig = require('../../js/constants/appConfig')
Expand Down Expand Up @@ -135,8 +134,8 @@ ipcMain.on(messages.ABOUT_COMPONENT_INITIALIZED, (e) => {
})

const getBookmarksData = function (state) {
let bookmarkSites = new Immutable.Map()
let bookmarkFolderSites = new Immutable.Map()
let bookmarkSites = new Immutable.OrderedMap()
let bookmarkFolderSites = new Immutable.OrderedMap()
state.get('sites').forEach((site, siteKey) => {
const tags = site.get('tags')
if (tags.includes(siteTags.BOOKMARK)) {
Expand All @@ -146,8 +145,8 @@ const getBookmarksData = function (state) {
bookmarkFolderSites = bookmarkFolderSites.set(siteKey, site)
}
})
const bookmarks = bookmarkSites.toList().sort(siteUtil.siteSort).toJS()
const bookmarkFolders = bookmarkFolderSites.toList().sort(siteUtil.siteSort).toJS()
const bookmarks = bookmarkSites.toList().toJS()
const bookmarkFolders = bookmarkFolderSites.toList().toJS()
return {bookmarks, bookmarkFolders}
}

Expand Down
3 changes: 1 addition & 2 deletions app/browser/windows.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const LocalShortcuts = require('../localShortcuts')
const {getPinnedSiteProps} = require('../common/lib/windowsUtil')
const {makeImmutable} = require('../common/state/immutableUtil')
const {getPinnedTabsByWindowId} = require('../common/state/tabState')
const {siteSort} = require('../../js/state/siteUtil')
const messages = require('../../js/constants/messages')
const settings = require('../../js/constants/settings')
const siteTags = require('../../js/constants/siteTags')
Expand Down Expand Up @@ -84,7 +83,7 @@ const updatePinnedTabs = (win) => {
const sitesToAdd = pinnedSites.filter((site) =>
!win.__alreadyPinnedSites.find((pinned) => pinned.equals(site)))

sitesToAdd.sort(siteSort).forEach((site) => {
sitesToAdd.forEach((site) => {
win.__alreadyPinnedSites = win.__alreadyPinnedSites.add(site)
appActions.createTabRequested({
url: site.get('location'),
Expand Down
6 changes: 5 additions & 1 deletion app/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const privacy = require('../js/state/privacy')
const async = require('async')
const settings = require('../js/constants/settings')
const BookmarksExporter = require('./browser/bookmarksExporter')
const siteUtil = require('../js/state/siteUtil')

app.commandLine.appendSwitch('enable-features', 'BlockSmallPluginContent,PreferHtmlOverPlugins')

Expand Down Expand Up @@ -321,7 +322,10 @@ app.on('ready', () => {
// For tests we always want to load default app state
const loadedPerWindowState = initialState.perWindowState
delete initialState.perWindowState
appActions.setState(Immutable.fromJS(initialState))
// Retore map order after load
let state = Immutable.fromJS(initialState)
state = state.set('sites', state.get('sites').sort(siteUtil.siteSort))
appActions.setState(state)
setImmediate(() => perWindowStateLoaded(loadedPerWindowState))
})

Expand Down
7 changes: 4 additions & 3 deletions app/renderer/components/bookmarks/bookmarksToolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ class BookmarksToolbar extends ImmutableComponent {
contextMenus.onShowBookmarkFolderMenu(this.bookmarks, bookmark, this.activeFrame, e)
}
updateBookmarkData (props) {
this.bookmarks = siteUtil.getBookmarks(props.sites).toList().sort(siteUtil.siteSort)
// TODO(darkdh): Remove siteSort when we have #9030 landed
this.bookmarks = siteUtil.getBookmarks(props.sites).sort(siteUtil.siteSort)

const noParentItems = this.bookmarks
.filter((bookmark) => !bookmark.get('parentFolderId'))
Expand Down Expand Up @@ -153,9 +154,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)
Expand Down
4 changes: 2 additions & 2 deletions test/unit/app/browser/reducers/sitesReducerTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ describe('sitesReducerTest', function () {
moveAction.prepend = true
newState = sitesReducer(Immutable.fromJS(newState), moveAction).toJS()
assert.equal(Object.keys(newState.sites).length, 3)
assert.equal(Object.values(newState.sites)[2].location, url)
assert.equal(Object.values(newState.sites)[2].order, 1)
assert.equal(Object.values(newState.sites)[1].location, url)
assert.equal(Object.values(newState.sites)[1].order, 1)
})
})
})

0 comments on commit 33f3622

Please sign in to comment.