Fix bookmark importer and import sequence

app/importer.js

@@ -32,6 +32,7 @@ const locale = require('./locale')
 const tabMessageBox = require('./browser/tabMessageBox')
 const {makeImmutable} = require('./common/state/immutableUtil')
 const bookmarkFoldersUtil = require('./common/lib/bookmarkFoldersUtil')
+const FunctionBuffer = require('../js/lib/functionBuffer')
 
 let isImportingBookmarks = false
 let hasBookmarks
@@ -92,7 +93,7 @@ importer.on('add-history-page', (e, history) => {
 importer.on('add-homepage', (e, detail) => {
 })
 
-const getParentFolderId = (path, pathMap, folders, topLevelFolderId, nextFolderIdObject) => {
+const getParentFolderId = (path, pathMap, addFolderFunction, topLevelFolderId, nextFolderIdObject) => {
   const pathLen = path.length
   if (!pathLen) {
     return topLevelFolderId
@@ -103,10 +104,10 @@ const getParentFolderId = (path, pathMap, folders, topLevelFolderId, nextFolderI
   if (parentFolderId === undefined) {
     parentFolderId = nextFolderIdObject.id++
     pathMap[parentFolder] = parentFolderId
-    folders.push({
+    addFolderFunction({
       title: parentFolder,
       folderId: parentFolderId,
-      parentFolderId: getParentFolderId(path, pathMap, folders, topLevelFolderId, nextFolderIdObject)
+      parentFolderId: getParentFolderId(path, pathMap, addFolderFunction, topLevelFolderId, nextFolderIdObject)
     })
   }
   return parentFolderId
@@ -121,36 +122,47 @@ importer.on('add-bookmarks', (e, importedBookmarks, topLevelFolder) => {
   let folders = []
   let bookmarks = []
   let topLevelFolderId = nextFolderIdObject.id++
+  const functionBuffer = new FunctionBuffer((args) => makeImmutable(args), this)
+  const bufferedAddFolder = (folder) => {
+    functionBuffer.buffer(appActions.addBookmarkFolder, folder)
+    folders.push(folder)
+  }
 
-  folders.push({
+  const importTopLevelFolder = {
     title: bookmarkFoldersUtil.getNextFolderName(bookmarkFolders, topLevelFolder),
     folderId: topLevelFolderId,
     parentFolderId: 0
-  })
+  }
+  bufferedAddFolder(importTopLevelFolder)
 
   for (let i = 0; i < importedBookmarks.length; ++i) {
-    let path = importedBookmarks[i].path
-    let parentFolderId = getParentFolderId(path, pathMap, folders, topLevelFolderId, nextFolderIdObject)
-    if (importedBookmarks[i].is_folder) {
+    const importedBookmark = importedBookmarks[i]
+    const path = importedBookmark.path
+    const title = importedBookmark.title
+    const parentFolderId = getParentFolderId(path, pathMap, bufferedAddFolder, topLevelFolderId, nextFolderIdObject)
+    if (importedBookmark.is_folder) {
       const folderId = nextFolderIdObject.id++
-      pathMap[importedBookmarks[i].title] = folderId
-      folders.push({
-        title: importedBookmarks[i].title,
-        folderId: folderId,
-        parentFolderId: parentFolderId
-      })
+      pathMap[title] = folderId
+      const folder = {
+        title,
+        folderId,
+        parentFolderId
+      }
+      functionBuffer.buffer(appActions.addBookmarkFolder, folder)
+      folders.push(folder)
     } else {
-      bookmarks.push({
-        title: importedBookmarks[i].title,
-        location: importedBookmarks[i].url,
-        parentFolderId: parentFolderId
-      })
+      const location = importedBookmark.url
+      const bookmark = {
+        title,
+        location,
+        parentFolderId
+      }
+      functionBuffer.buffer(appActions.addBookmark, bookmark)
+      bookmarks.push(bookmark)
     }
   }
-
-  bookmarkList = bookmarks
-  appActions.addBookmarkFolder(makeImmutable(folders))
-  appActions.addBookmark(makeImmutable(bookmarks))
+  functionBuffer.flush()
+  bookmarkList = makeImmutable(bookmarks)
 })
 
 importer.on('add-favicons', (e, detail) => {

js/lib/functionBuffer.js

@@ -0,0 +1,42 @@
+/* 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/. */
+
+/**
+ * Perform a sequence of function calls, grouping together contiguous function
+ * calls for the same function different args.
+ * See syncUtil.applySyncRecords() for a usage example.
+ */
+module.exports = class FunctionBuffer {
+  /**
+   * @param {function=} prepareArguments Before calling a function, run this function to prepare the buffered arguments.
+   */
+  constructor (prepareArguments) {
+    this.argumentsBuffer = []
+    this.previousFunction = () => {}
+    this.prepareArguments = prepareArguments || ((args) => args)
+  }
+
+  /**
+   * @param {function} fn Function to call. NOTE: this supports only calling with the first argument.
+    * @param {any} arg Arg to buffer
+   */
+  buffer (fn, arg) {
+    if (fn !== this.previousFunction) {
+      this.flush()
+      this.previousFunction = fn
+    }
+    this.argumentsBuffer.push(arg)
+  }
+
+  /**
+   * Call previousFunction with buffered arguments and empty buffer.
+   */
+  flush () {
+    if (!this.argumentsBuffer.length) { return }
+    const preparedArgs = this.prepareArguments(this.argumentsBuffer)
+    const returnValue = this.previousFunction(preparedArgs)
+    this.argumentsBuffer = []
+    return returnValue
+  }
+}

js/state/syncUtil.js

@@ -9,6 +9,7 @@ const crypto = require('crypto')
 const writeActions = require('../constants/sync/proto').actions
 const {getSetting} = require('../settings')
 const {isDataUrl} = require('../lib/urlutil')
+const FunctionBuffer = require('../lib/functionBuffer')
 const bookmarkUtil = require('../../app/common/lib/bookmarkUtil')
 const bookmarkFoldersState = require('../../app/common/state/bookmarkFoldersState')
 const bookmarkFoldersUtil = require('../../app/common/lib/bookmarkFoldersUtil')
@@ -282,21 +283,7 @@ module.exports.applySyncRecords = (records) => {
    * bookmark sites), then when the apply action changes (e.g. to folders) we
    * first flush the buffer (e.g. apply the first sequence of sites).
    */
-  let buffer = []
-  let previousAppAction = () => {}
-  const flushBufferedAppActions = () => {
-    if (!buffer.length) { return }
-    previousAppAction(new Immutable.List(buffer))
-    buffer = []
-  }
-  const bufferAppAction = (appAction, item) => {
-    if (previousAppAction !== appAction) {
-      flushBufferedAppActions()
-      previousAppAction = appAction
-    }
-    buffer.push(item)
-  }
-
+  const functionBuffer = new FunctionBuffer((args) => new Immutable.List(args))
   records.forEach((record) => {
     if (!record) {
       return true
@@ -305,32 +292,32 @@ module.exports.applySyncRecords = (records) => {
       const siteData = module.exports.getSiteDataFromRecord(record, appState, records).set('skipSync', true)
       if (shouldAddRecord(record)) {
         if (record.bookmark.isFolder) {
-          bufferAppAction(appActions.addBookmarkFolder, siteData)
+          functionBuffer.buffer(appActions.addBookmarkFolder, siteData)
         } else {
-          bufferAppAction(appActions.addBookmark, siteData)
+          functionBuffer.buffer(appActions.addBookmark, siteData)
         }
       } else if (shouldRemoveRecord(record)) {
         if (record.bookmark.isFolder) {
           const folderKey = bookmarkFoldersUtil.getKey(siteData)
-          bufferAppAction(appActions.removeBookmarkFolder, folderKey)
+          functionBuffer.buffer(appActions.removeBookmarkFolder, folderKey)
         } else {
           const siteKey = bookmarkUtil.getKey(siteData)
-          bufferAppAction(appActions.removeBookmark, siteKey)
+          functionBuffer.buffer(appActions.removeBookmark, siteKey)
         }
       }
     } else if (record.objectData === 'historySite') {
       const siteData = module.exports.getSiteDataFromRecord(record, appState, records)
       if (shouldAddRecord(record)) {
-        bufferAppAction(appActions.addHistorySite, siteData)
+        functionBuffer.buffer(appActions.addHistorySite, siteData)
       } else if (shouldRemoveRecord(record)) {
         const siteKey = historyUtil.getKey(siteData)
-        bufferAppAction(appActions.removeHistorySite, siteKey)
+        functionBuffer.buffer(appActions.removeHistorySite, siteKey)
       }
     } else {
       otherRecords.push(record)
     }
   })
-  flushBufferedAppActions()
+  functionBuffer.flush()
   applyNonBatchedRecords(otherRecords)
 }