Fix bookmark importer and import sequence (#10272)

* Fix bookmark importer by making bookmarkList immutable
* Fix importer to import in order

app/common/state/bookmarksState.js

@@ -88,7 +88,8 @@ const bookmarksState = {
     }
 
     const key = bookmarkUtil.getKey(bookmarkDetail)
-    let dataItem = historyState.getSite(state, key)
+    const historyKey = key.slice(0, -2)
+    let dataItem = historyState.getSite(state, historyKey)
 
     if (dataItem.isEmpty()) {
       // check if we have data in tabs

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) => {

app/renderer/about/bookmarks/bookmarksList.js

@@ -20,7 +20,6 @@ const appActions = require('../../../../js/actions/appActions')
 
 // Utils
 const dndData = require('../../../../js/dndData')
-const formatUtil = require('../../../common/lib/formatUtil')
 
 class BookmarksList extends ImmutableComponent {
   onDoubleClick (entry, e) {
@@ -106,22 +105,17 @@ class BookmarksList extends ImmutableComponent {
       <SortableTable
         ref='bookmarkTable'
         headings={[
-          <BookmarkTitleHeader heading='title' selectedFolderId={this.props.selectedFolderId} />,
-          <span data-l10n-id='lastVisited' />
+          <BookmarkTitleHeader heading='title' selectedFolderId={this.props.selectedFolderId} />
         ]}
         defaultHeading='Title'
         rows={this.props.bookmarks.map((entry) => [
           {
             cell: <BookmarkTitleCell siteDetail={entry} />,
             value: entry.get('title') || entry.get('location')
-          },
-          {
-            html: formatUtil.toLocaleString(entry.get('lastAccessedTime'), ''),
-            value: entry.get('lastAccessedTime') || 0
           }
         ])}
         rowObjects={this.props.bookmarks}
-        columnClassNames={['title', 'date']}
+        columnClassNames={['title']}
         addHoverClass
         multiSelect
         onDoubleClick={this.onDoubleClick}

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)
 }
 

test/about/bookmarksManagerTest.js

@@ -9,7 +9,6 @@ const Immutable = require('immutable')
 
 describe('about:bookmarks', function () {
   const folderId = Math.floor(Math.random() * (100 - 1 + 1)) + 1
-  const lastVisit = 1476140184441
   const browseableSiteUrl = 'page1.html'
   const browseableSiteTitle = 'Page 1'
 
@@ -30,15 +29,13 @@ describe('about:bookmarks', function () {
         title: 'Page with Favicon',
         favicon: favicon,
         type: siteTags.BOOKMARK,
-        parentFolderId: 0,
-        lastAccessedTime: lastVisit
+        parentFolderId: 0
       },
       {
         location: siteWithoutFavicon,
         title: 'Page without Favicon',
         type: siteTags.BOOKMARK,
-        parentFolderId: 0,
-        lastAccessedTime: lastVisit
+        parentFolderId: 0
       }
     ])
     yield client
@@ -52,43 +49,36 @@ describe('about:bookmarks', function () {
       {
         location: 'https://brave.com',
         title: 'Brave',
-        parentFolderId: 0,
-        lastAccessedTime: lastVisit
+        parentFolderId: 0
       },
       {
         location: 'https://brave.com/test',
         title: 'customTest',
-        parentFolderId: 0,
-        lastAccessedTime: lastVisit
+        parentFolderId: 0
       },
       {
         location: 'https://www.youtube.com',
-        parentFolderId: 0,
-        lastAccessedTime: lastVisit
+        parentFolderId: 0
       },
       {
         location: 'https://www.facebook.com',
         title: 'facebook',
-        parentFolderId: 0,
-        lastAccessedTime: lastVisit
+        parentFolderId: 0
       },
       {
         location: 'https://duckduckgo.com',
         title: 'duckduckgo',
-        parentFolderId: folderId,
-        lastAccessedTime: lastVisit
+        parentFolderId: folderId
       },
       {
         location: 'https://google.com',
         title: 'Google',
-        parentFolderId: folderId,
-        lastAccessedTime: lastVisit
+        parentFolderId: folderId
       },
       {
         location: 'https://bing.com',
         title: 'Bing',
-        parentFolderId: folderId,
-        lastAccessedTime: lastVisit
+        parentFolderId: folderId
       }
     ])
 
@@ -112,8 +102,7 @@ describe('about:bookmarks', function () {
         location: site,
         title: browseableSiteTitle,
         type: siteTags.BOOKMARK,
-        parentFolderId: 0,
-        lastAccessedTime: lastVisit
+        parentFolderId: 0
       })
       .tabByIndex(0)
       .loadUrl(aboutBookmarksUrl)