From fcb4fc57eaa884998896fa795e0bd328585f3e3f Mon Sep 17 00:00:00 2001 From: yan Date: Sat, 18 Feb 2017 01:33:08 +0000 Subject: [PATCH 1/2] call a single appAction when receiving sync site records fix https://github.com/brave/browser-laptop/issues/7308 fix https://github.com/brave/browser-laptop/issues/7293 fix https://github.com/brave/browser-laptop/issues/7307 Auditors: @ayumi @alexwykoff Test Plan: 1. in about:preferences#sync, choose the 'i have an existing sync code' option. enter in the code words that Alex posted in #testers (starts with 'forsaken') 2. wait for 6000+ bookmarks to appear in the bookmarks toolbar. the browser should not freeze or use excessive memory during the sync. for me it takes about 5 seconds. 3. open about:bookmarks. you should see a bunch of folders with bookmarks, including subfolders. --- app/browser/menu.js | 7 +++ docs/appActions.md | 12 ++++ js/actions/appActions.js | 12 ++++ js/constants/appConstants.js | 1 + js/state/syncUtil.js | 112 ++++++++++++++++++++++------------- js/stores/appStore.js | 28 +++++++++ 6 files changed, 132 insertions(+), 40 deletions(-) diff --git a/app/browser/menu.js b/app/browser/menu.js index f44a127232e..aeafde0ec0d 100644 --- a/app/browser/menu.js +++ b/app/browser/menu.js @@ -620,6 +620,13 @@ const doAction = (action) => { } }) break + case appConstants.APP_APPLY_SITE_RECORDS: + if (action.records.find((record) => record.objectData === 'bookmark')) { + appDispatcher.waitFor([appStore.dispatchToken], () => { + createMenu() + }) + } + break case appConstants.APP_ADD_SITE: if (action.tag === siteTags.BOOKMARK || action.tag === siteTags.BOOKMARK_FOLDER) { appDispatcher.waitFor([appStore.dispatchToken], () => { diff --git a/docs/appActions.md b/docs/appActions.md index 6b164a97054..7599db0cac1 100644 --- a/docs/appActions.md +++ b/docs/appActions.md @@ -712,6 +712,18 @@ Dispatches a message when sync init data needs to be saved +### applySiteRecords(records) + +Dispatches a message to apply a batch of site records from Brave Sync +TODO: Refactor this to merge it into addSite/removeSite + +**Parameters** + +**records**: `Array.<Object>`, Dispatches a message to apply a batch of site records from Brave Sync +TODO: Refactor this to merge it into addSite/removeSite + + + ### resetSyncData() Dispatches a message to delete sync data. diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 2e84c9b2704..9845c1caf3d 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -871,6 +871,18 @@ const appActions = { }) }, + /** + * Dispatches a message to apply a batch of site records from Brave Sync + * TODO: Refactor this to merge it into addSite/removeSite + * @param {Array.} records + */ + applySiteRecords: function (records) { + AppDispatcher.dispatch({ + actionType: appConstants.APP_APPLY_SITE_RECORDS, + records + }) + }, + /** * Dispatches a message to delete sync data. */ diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 3ec69fd7751..3f084308082 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -20,6 +20,7 @@ const appConstants = { APP_SET_STATE: _, APP_REMOVE_SITE: _, APP_MOVE_SITE: _, + APP_APPLY_SITE_RECORDS: _, APP_MERGE_DOWNLOAD_DETAIL: _, // Sets an individual download detail APP_CLEAR_COMPLETED_DOWNLOADS: _, // Removes all completed downloads APP_ADD_PASSWORD: _, /** @param {Object} passwordDetail */ diff --git a/js/state/syncUtil.js b/js/state/syncUtil.js index 505870ffe47..e6fc29f9e7d 100644 --- a/js/state/syncUtil.js +++ b/js/state/syncUtil.js @@ -5,6 +5,7 @@ const Immutable = require('immutable') const writeActions = require('../constants/sync/proto').actions +const siteTags = require('../constants/siteTags') const siteUtil = require('./siteUtil') const CATEGORY_MAP = { @@ -51,20 +52,27 @@ const pickFields = (object, fields) => { }, {}) } +// Cache of bookmark folder object IDs mapped to folder IDs +let folderIdMap = new Immutable.Map() + /** - * Apply a bookmark or historySite SyncRecord to the browser data store. + * Converts sync records into a form that can be consumed by AppStore. * @param {Object} record - * @param {Immutable.Map} siteDetail + * @param {Immutable.Map} appState */ -const applySiteRecord = (record) => { - const appActions = require('../actions/appActions') - const siteTags = require('../constants/siteTags') +module.exports.getSiteDataFromRecord = (record, appState) => { const objectId = new Immutable.List(record.objectId) const category = CATEGORY_MAP[record.objectData].categoryName - const existingObject = module.exports.getObjectById(objectId, category) - const existingObjectData = existingObject && existingObject[1] - let tag + let existingObjectData + + if (record.action !== writeActions.CREATE) { + // XXX: Is this necessary for CREATEs also? + const existingObject = module.exports.getObjectById(objectId, category, + appState) + existingObjectData = existingObject && existingObject[1] + } + let tag let siteProps = Object.assign( {}, existingObjectData && existingObjectData.toJS(), @@ -87,22 +95,11 @@ const applySiteRecord = (record) => { const parentFolderObjectId = siteProps.parentFolderObjectId if (parentFolderObjectId && parentFolderObjectId.length > 0) { siteProps.parentFolderId = - getFolderIdByObjectId(new Immutable.List(parentFolderObjectId)) + getFolderIdByObjectId(new Immutable.List(parentFolderObjectId), appState) } } const siteDetail = new Immutable.Map(pickFields(siteProps, SITE_FIELDS)) - - switch (record.action) { - case writeActions.CREATE: - appActions.addSite(siteDetail, tag, undefined, undefined, true) - break - case writeActions.UPDATE: - appActions.addSite(siteDetail, tag, existingObjectData, null, true) - break - case writeActions.DELETE: - appActions.removeSite(siteDetail, tag, true) - break - } + return {siteDetail, tag, existingObjectData} } const applySiteSettingRecord = (record) => { @@ -163,11 +160,20 @@ const applySiteSettingRecord = (record) => { } } +const applyNonBatchedRecords = (records) => { + if (!records.length) { return } + setImmediate(() => { + const record = records.shift() + applySyncRecord(record) + applyNonBatchedRecords(records) + }) +} + /** * Given a SyncRecord, apply it to the browser data store. * @param {Object} record */ -module.exports.applySyncRecord = (record) => { +const applySyncRecord = (record) => { if (!record || !record.objectData) { console.log(`Warning: Can't apply empty record: ${record}`) return @@ -175,7 +181,8 @@ module.exports.applySyncRecord = (record) => { switch (record.objectData) { case 'bookmark': case 'historySite': - applySiteRecord(record) + // these are handled in batches now + console.log(`Warning: Skipping unexpected site record: ${record}`) break case 'siteSetting': applySiteSettingRecord(record) @@ -194,11 +201,21 @@ module.exports.applySyncRecord = (record) => { */ module.exports.applySyncRecords = (records) => { if (!records || records.length === 0) { return } - setImmediate(() => { - const record = records.shift() - module.exports.applySyncRecord(record) - module.exports.applySyncRecords(records) + const siteRecords = [] + const otherRecords = [] + records.forEach((record) => { + if (record && ['bookmark', 'historySite'].includes(record.objectData)) { + siteRecords.push(record) + } else { + otherRecords.push(record) + } }) + applyNonBatchedRecords(otherRecords) + if (siteRecords.length) { + setImmediate(() => { + require('../actions/appActions').applySiteRecords(new Immutable.List(siteRecords)) + }) + } } /** @@ -212,7 +229,7 @@ module.exports.getExistingObject = (categoryName, syncRecord) => { const AppStore = require('../stores/appStore') const appState = AppStore.getState() const objectId = new Immutable.List(syncRecord.objectId) - const existingObject = module.exports.getObjectById(objectId, categoryName) + const existingObject = module.exports.getObjectById(objectId, categoryName, appState) if (!existingObject) { return null } const existingObjectData = existingObject[1].toJS() @@ -220,7 +237,7 @@ module.exports.getExistingObject = (categoryName, syncRecord) => { switch (categoryName) { case 'BOOKMARKS': case 'HISTORY_SITES': - item = module.exports.createSiteData(existingObjectData) + item = module.exports.createSiteData(existingObjectData, appState) break case 'PREFERENCES': const hostPattern = existingObject[0] @@ -245,15 +262,18 @@ module.exports.getExistingObject = (categoryName, syncRecord) => { * Given an objectId and category, return the matching browser object. * @param {Immutable.List} objectId * @param {string} category + * @param {Immutable.Map=} appState * @returns {Array} [, ] array is AppStore searchKeyPath e.g. ['sites', 10] for use with updateIn */ -module.exports.getObjectById = (objectId, category) => { +module.exports.getObjectById = (objectId, category, appState) => { if (!(objectId instanceof Immutable.List)) { throw new Error('objectId must be an Immutable.List') } - const AppStore = require('../stores/appStore') - const appState = AppStore.getState() + if (!appState) { + const AppStore = require('../stores/appStore') + appState = AppStore.getState() + } switch (category) { case 'BOOKMARKS': return appState.get('sites').findEntry((site, index) => { @@ -280,12 +300,18 @@ module.exports.getObjectById = (objectId, category) => { /** * Given an bookmark folder objectId, find the folder and return its folderId. * @param {Immutable.List} objectId + * @param {Immutable.Map=} appState * @returns {number|undefined} */ -const getFolderIdByObjectId = (objectId) => { - const entry = module.exports.getObjectById(objectId, 'BOOKMARKS') +const getFolderIdByObjectId = (objectId, appState) => { + if (folderIdMap.has(objectId)) { + return folderIdMap.get(objectId) + } + const entry = module.exports.getObjectById(objectId, 'BOOKMARKS', appState) if (!entry) { return undefined } - return entry[1].get('folderId') + const folderId = entry[1].get('folderId') + folderIdMap = folderIdMap.set(objectId, folderId) + return folderId } /** @@ -332,12 +358,16 @@ module.exports.newObjectId = (objectPath) => { /** * Given a bookmark folder's folderId, get or set its object ID. * @param {number} folderId + * @param {Immutable.Map} appState * @returns {Array.} */ -module.exports.findOrCreateFolderObjectId = (folderId) => { +const findOrCreateFolderObjectId = (folderId, appState) => { if (typeof folderId !== 'number') { return undefined } - const AppStore = require('../stores/appStore') - const folder = AppStore.getState().getIn(['sites', folderId.toString()]) + if (!appState) { + const AppStore = require('../stores/appStore') + appState = AppStore.getState() + } + const folder = appState.getIn(['sites', folderId.toString()]) if (!folder) { return undefined } const objectId = folder.get('objectId') if (objectId) { @@ -350,9 +380,10 @@ module.exports.findOrCreateFolderObjectId = (folderId) => { /** * Converts a site object into input for sendSyncRecords * @param {Object} site + * @param {Immutable.Map} appState * @returns {{name: string, value: object, objectId: Array.}} */ -module.exports.createSiteData = (site) => { +module.exports.createSiteData = (site, appState) => { const siteData = { location: '', title: '', @@ -372,7 +403,8 @@ module.exports.createSiteData = (site) => { } if (module.exports.isSyncable('bookmark', Immutable.fromJS(site))) { const objectId = site.objectId || module.exports.newObjectId(['sites', siteKey]) - const parentFolderObjectId = site.parentFolderObjectId || (site.parentFolderId && module.exports.findOrCreateFolderObjectId(site.parentFolderId)) + const parentFolderObjectId = site.parentFolderObjectId || + (site.parentFolderId && findOrCreateFolderObjectId(site.parentFolderId, appState)) return { name: 'bookmark', objectId, diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 213ef357538..6ca4211b343 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -9,7 +9,9 @@ const ExtensionConstants = require('../../app/common/constants/extensionConstant const AppDispatcher = require('../dispatcher/appDispatcher') const appConfig = require('../constants/appConfig') const settings = require('../constants/settings') +const writeActions = require('../constants/sync/proto').actions const siteUtil = require('../state/siteUtil') +const syncUtil = require('../state/syncUtil') const siteSettings = require('../state/siteSettings') const appUrlUtil = require('../lib/appUrlUtil') const electron = require('electron') @@ -485,6 +487,32 @@ const handleAppAction = (action) => { case appConstants.APP_DATA_URL_COPIED: nativeImage.copyDataURL(action.dataURL, action.html, action.text) break + case appConstants.APP_APPLY_SITE_RECORDS: + action.records.forEach((record) => { + const siteData = syncUtil.getSiteDataFromRecord(record, appState) + const tag = siteData.tag + let siteDetail = siteData.siteDetail + const sites = appState.get('sites') + if (record.action !== writeActions.DELETE && + !siteDetail.get('folderId') && siteUtil.isFolder(siteDetail)) { + siteDetail = siteDetail.set('folderId', siteUtil.getNextFolderId(sites)) + } + switch (record.action) { + case writeActions.CREATE: + appState = appState.set('sites', + siteUtil.addSite(sites, siteDetail, tag)) + break + case writeActions.UPDATE: + appState = appState.set('sites', + siteUtil.addSite(sites, siteDetail, tag, siteData.existingObjectData)) + break + case writeActions.DELETE: + appState = appState.set('sites', + siteUtil.removeSite(sites, siteDetail, tag)) + break + } + }) + break case appConstants.APP_ADD_SITE: const oldSiteSize = appState.get('sites').size const addSiteSyncCallback = action.skipSync ? undefined : syncActions.updateSite From fa180226c62807105b1e6f78f9454c4908b299cc Mon Sep 17 00:00:00 2001 From: yan Date: Fri, 24 Feb 2017 22:35:44 +0000 Subject: [PATCH 2/2] Update newtab and history pages after sync records are applied --- js/state/syncUtil.js | 1 - js/stores/appStore.js | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/js/state/syncUtil.js b/js/state/syncUtil.js index e6fc29f9e7d..da19b7e8c8c 100644 --- a/js/state/syncUtil.js +++ b/js/state/syncUtil.js @@ -66,7 +66,6 @@ module.exports.getSiteDataFromRecord = (record, appState) => { let existingObjectData if (record.action !== writeActions.CREATE) { - // XXX: Is this necessary for CREATEs also? const existingObject = module.exports.getObjectById(objectId, category, appState) existingObjectData = existingObject && existingObject[1] diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 6ca4211b343..eed7fc28cc1 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -512,6 +512,8 @@ const handleAppAction = (action) => { break } }) + appState = aboutNewTabState.setSites(appState) + appState = aboutHistoryState.setHistory(appState) break case appConstants.APP_ADD_SITE: const oldSiteSize = appState.get('sites').size