call a single appAction when receiving sync site records

fix #7308
fix #7293
fix #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

@@ -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], () => {

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.

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.<Object>} records
+   */
+  applySiteRecords: function (records) {
+    AppDispatcher.dispatch({
+      actionType: appConstants.APP_APPLY_SITE_RECORDS,
+      records
+    })
+  },
+
   /**
    * Dispatches a message to delete sync data.
    */

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 */

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,19 +160,29 @@ 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
   }
   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,15 +229,15 @@ 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()
   let item
   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>, <Immutable.Map>] 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.<number>}
  */
-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.<number>}}
  */
-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,

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