Bug 1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine. r=markh draft
authorNicolas Ouellet-Payeur <nicolaso@google.com>
Fri, 11 Aug 2017 00:19:01 +0000
changeset 648639 8c7941478306cadf8aec59807b8ecb7a10d1203b
parent 641104 933a04a91ce3bd44b230937083a835cb60637084
child 726893 d752eac929b6f8723d51dde07c19dd9f97ee2ba8
push id74832
push userbmo:markh@mozilla.com
push dateFri, 18 Aug 2017 04:14:25 +0000
reviewersmarkh
bugs1353217
milestone57.0a1
Bug 1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine. r=markh MozReview-Commit-ID: 4F7KF5ZkNuX
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
toolkit/components/places/BookmarkJSONUtils.jsm
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -967,23 +967,28 @@ BookmarksTracker.prototype = {
         }
         break;
       case "bookmarks-restore-begin":
         this._log.debug("Ignoring changes from importing bookmarks.");
         break;
       case "bookmarks-restore-success":
         this._log.debug("Tracking all items on successful import.");
 
-        this._log.debug("Restore succeeded: wiping server and other clients.");
-        Async.promiseSpinningly((async () => {
-          await this.engine.service.resetClient([this.name]);
-          await this.engine.service.wipeServer([this.name]);
-          await this.engine.service.clientsEngine.sendCommand("wipeEngine", [this.name],
-                                                              null, { reason: "bookmark-restore" });
-        })());
+        if (data == "json") {
+          this._log.debug("Restore succeeded: wiping server and other clients.");
+          Async.promiseSpinningly((async () => {
+            await this.engine.service.resetClient([this.name]);
+            await this.engine.service.wipeServer([this.name]);
+            await this.engine.service.clientsEngine.sendCommand("wipeEngine", [this.name],
+                                                                null, { reason: "bookmark-restore" });
+          })());
+        } else {
+          // "html", "html-initial", or "json-append"
+          this._log.debug("Import succeeded.");
+        }
         break;
       case "bookmarks-restore-failed":
         this._log.debug("Tracking all items on failed import.");
         break;
     }
   },
 
   QueryInterface: XPCOMUtils.generateQI([
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -1,12 +1,13 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
+Cu.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
 Cu.import("resource://gre/modules/BookmarkJSONUtils.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
@@ -190,17 +191,32 @@ add_task(async function test_processInco
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     await PlacesSyncUtils.bookmarks.reset();
     await promiseStopServer(server);
   }
 });
 
 add_task(async function test_restorePromptsReupload() {
-  _("Ensure that restoring from a backup will reupload all records.");
+  await test_restoreOrImport(true);
+});
+
+add_task(async function test_importPromptsReupload() {
+  await test_restoreOrImport(false);
+});
+
+// Test a JSON restore or HTML import. Use JSON if `aReplace` is `true`, or
+// HTML otherwise.
+async function test_restoreOrImport(aReplace) {
+  let verb = aReplace ? "restore" : "import";
+  let verbing = aReplace ? "restoring" : "importing";
+  let bookmarkUtils = aReplace ? BookmarkJSONUtils : BookmarkHTMLUtils;
+
+  _(`Ensure that ${verbing} from a backup will reupload all records.`);
+
   let engine = new BookmarksEngine(Service);
   await engine.initialize();
   let store  = engine._store;
   let server = serverForFoo(engine);
   await SyncTestingInfrastructure(server);
 
   let collection = server.user("foo").collection("bookmarks");
 
@@ -215,35 +231,34 @@ add_task(async function test_restoreProm
 
     let fxuri = Utils.makeURI("http://getfirefox.com/");
     let tburi = Utils.makeURI("http://getthunderbird.com/");
 
     _("Create a single record.");
     let bmk1_id = PlacesUtils.bookmarks.insertBookmark(
       folder1_id, fxuri, PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!");
     let bmk1_guid = await store.GUIDForId(bmk1_id);
-    _("Get Firefox!: " + bmk1_id + ", " + bmk1_guid);
-
+    _(`Get Firefox!: ${bmk1_id}, ${bmk1_guid}`);
 
     let dirSvc = Cc["@mozilla.org/file/directory_service;1"]
       .getService(Ci.nsIProperties);
 
     let backupFile = dirSvc.get("TmpD", Ci.nsILocalFile);
 
     _("Make a backup.");
     backupFile.append("t_b_e_" + Date.now() + ".json");
 
-    _("Backing up to file " + backupFile.path);
-    await BookmarkJSONUtils.exportToFile(backupFile.path);
+    _(`Backing up to file ${backupFile.path}`);
+    await bookmarkUtils.exportToFile(backupFile.path);
 
     _("Create a different record and sync.");
     let bmk2_id = PlacesUtils.bookmarks.insertBookmark(
       folder1_id, tburi, PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!");
     let bmk2_guid = await store.GUIDForId(bmk2_id);
-    _("Get Thunderbird!: " + bmk2_id + ", " + bmk2_guid);
+    _(`Get Thunderbird!: ${bmk2_id}, ${bmk2_guid}`);
 
     PlacesUtils.bookmarks.removeItem(bmk1_id);
 
     let error;
     try {
       await sync_engine_and_validate_telem(engine, false);
     } catch (ex) {
       error = ex;
@@ -254,83 +269,138 @@ add_task(async function test_restoreProm
     _("Verify that there's only one bookmark on the server, and it's Thunderbird.");
     // Of course, there's also the Bookmarks Toolbar and Bookmarks Menu...
     let wbos = collection.keys(function(id) {
       return ["menu", "toolbar", "mobile", "unfiled", folder1_guid].indexOf(id) == -1;
     });
     do_check_eq(wbos.length, 1);
     do_check_eq(wbos[0], bmk2_guid);
 
-    _("Now restore from a backup.");
-    await BookmarkJSONUtils.importFromFile(backupFile, true);
+    _(`Now ${verb} from a backup.`);
+    await bookmarkUtils.importFromFile(backupFile, aReplace);
+
+    let bookmarksCollection = server.user("foo").collection("bookmarks");
+    if (aReplace) {
+      _("Verify that we wiped the server.");
+      do_check_true(!bookmarksCollection);
+    } else {
+      _("Verify that we didn't wipe the server.");
+      do_check_true(!!bookmarksCollection);
+    }
 
     _("Ensure we have the bookmarks we expect locally.");
     let guids = await fetchAllSyncIds();
     _("GUIDs: " + JSON.stringify([...guids]));
-    let found = false;
+    let bookmarkGuids = new Map();
     let count = 0;
-    let newFX;
     for (let guid of guids) {
       count++;
       let id = await store.idForGUID(guid, true);
       // Only one bookmark, so _all_ should be Firefox!
       if (PlacesUtils.bookmarks.getItemType(id) == PlacesUtils.bookmarks.TYPE_BOOKMARK) {
         let uri = PlacesUtils.bookmarks.getBookmarkURI(id);
-        _("Found URI " + uri.spec + " for GUID " + guid);
-        do_check_eq(uri.spec, fxuri.spec);
-        newFX = guid;   // Save the new GUID after restore.
-        found = true;   // Only runs if the above check passes.
+        _(`Found URI ${uri.spec} for GUID ${guid}`);
+        bookmarkGuids.set(uri.spec, guid);
       }
     }
-    _("We found it: " + found);
-    do_check_true(found);
+    do_check_true(bookmarkGuids.has(fxuri.spec));
+    if (!aReplace) {
+      do_check_true(bookmarkGuids.has(tburi.spec));
+    }
 
     _("Have the correct number of IDs locally, too.");
-    do_check_eq(count, ["menu", "toolbar", "mobile", "unfiled", folder1_id, bmk1_id].length);
+    let expectedResults = ["menu", "toolbar", "mobile", "unfiled", folder1_id,
+                           bmk1_id];
+    if (!aReplace) {
+      expectedResults.push("toolbar", folder1_id, bmk2_id);
+    }
+    do_check_eq(count, expectedResults.length);
 
     _("Sync again. This'll wipe bookmarks from the server.");
     try {
       await sync_engine_and_validate_telem(engine, false);
     } catch (ex) {
       error = ex;
       _("Got error: " + Log.exceptionStr(ex));
     }
     do_check_true(!error);
 
-    _("Verify that there's only one bookmark on the server, and it's Firefox.");
+    _("Verify that there's the right bookmarks on the server.");
     // Of course, there's also the Bookmarks Toolbar and Bookmarks Menu...
     let payloads     = server.user("foo").collection("bookmarks").payloads();
     let bookmarkWBOs = payloads.filter(function(wbo) {
                          return wbo.type == "bookmark";
                        });
+
     let folderWBOs   = payloads.filter(function(wbo) {
                          return ((wbo.type == "folder") &&
                                  (wbo.id != "menu") &&
                                  (wbo.id != "toolbar") &&
                                  (wbo.id != "unfiled") &&
-                                 (wbo.id != "mobile"));
+                                 (wbo.id != "mobile") &&
+                                 (wbo.parentid != "menu"));
                        });
 
-    do_check_eq(bookmarkWBOs.length, 1);
-    do_check_eq(bookmarkWBOs[0].id, newFX);
-    do_check_eq(bookmarkWBOs[0].bmkUri, fxuri.spec);
-    do_check_eq(bookmarkWBOs[0].title, "Get Firefox!");
+    let expectedFX = {
+      id: bookmarkGuids.get(fxuri.spec),
+      bmkUri: fxuri.spec,
+      title: "Get Firefox!"
+    };
+    let expectedTB = {
+      id: bookmarkGuids.get(tburi.spec),
+      bmkUri: tburi.spec,
+      title: "Get Thunderbird!"
+    };
+
+    let expectedBookmarks;
+    if (aReplace) {
+      expectedBookmarks = [expectedFX];
+    } else {
+      expectedBookmarks = [expectedTB, expectedFX];
+    }
+
+    doCheckWBOs(bookmarkWBOs, expectedBookmarks);
 
     _("Our old friend Folder 1 is still in play.");
-    do_check_eq(folderWBOs.length, 1);
-    do_check_eq(folderWBOs[0].title, "Folder 1");
+    let expectedFolder1 = { title: "Folder 1" };
+
+    let expectedFolders;
+    if (aReplace) {
+      expectedFolders = [expectedFolder1];
+    } else {
+      expectedFolders = [expectedFolder1, expectedFolder1];
+    }
+
+    doCheckWBOs(folderWBOs, expectedFolders);
 
   } finally {
     await store.wipe();
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     await PlacesSyncUtils.bookmarks.reset();
     await promiseStopServer(server);
   }
-});
+}
+
+function doCheckWBOs(WBOs, expected) {
+  do_check_eq(WBOs.length, expected.length);
+  for (let i = 0; i < expected.length; i++) {
+    let lhs = WBOs[i];
+    let rhs = expected[i];
+    if ("id" in rhs) {
+      do_check_eq(lhs.id, rhs.id);
+    }
+    if ("bmkUri" in rhs) {
+      do_check_eq(lhs.bmkUri, rhs.bmkUri);
+    }
+    if ("title" in rhs) {
+      do_check_eq(lhs.title, rhs.title);
+    }
+  }
+}
 
 function FakeRecord(constructor, r) {
   constructor.call(this, "bookmarks", r.id);
   for (let x in r) {
     this[x] = r[x];
   }
   // Borrow the constructor's conversion functions.
   this.toSyncBookmark = constructor.prototype.toSyncBookmark;
--- a/toolkit/components/places/BookmarkJSONUtils.jsm
+++ b/toolkit/components/places/BookmarkJSONUtils.jsm
@@ -52,25 +52,25 @@ this.BookmarkJSONUtils = Object.freeze({
    *        Boolean if true, replace existing bookmarks, else merge.
    *
    * @return {Promise}
    * @resolves When the new bookmarks have been created.
    * @rejects JavaScript exception.
    */
   importFromURL: function BJU_importFromURL(aSpec, aReplace) {
     return (async function() {
-      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN);
+      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN, aReplace);
       try {
         let importer = new BookmarkImporter(aReplace);
         await importer.importFromURL(aSpec);
 
-        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_SUCCESS);
+        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_SUCCESS, aReplace);
       } catch (ex) {
         Cu.reportError("Failed to restore bookmarks from " + aSpec + ": " + ex);
-        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED);
+        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED, aReplace);
       }
     })();
   },
 
   /**
    * Restores bookmarks and tags from a JSON file.
    * @note any item annotated with "places/excludeFromBackup" won't be removed
    *       before executing the restore.
@@ -89,31 +89,31 @@ this.BookmarkJSONUtils = Object.freeze({
     if (aFilePath instanceof Ci.nsIFile) {
       Deprecated.warning("Passing an nsIFile to BookmarksJSONUtils.importFromFile " +
                          "is deprecated. Please use an OS.File path string instead.",
                          "https://developer.mozilla.org/docs/JavaScript_OS.File");
       aFilePath = aFilePath.path;
     }
 
     return (async function() {
-      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN);
+      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN, aReplace);
       try {
         if (!(await OS.File.exists(aFilePath)))
           throw new Error("Cannot restore from nonexisting json file");
 
         let importer = new BookmarkImporter(aReplace);
         if (aFilePath.endsWith("jsonlz4")) {
           await importer.importFromCompressedFile(aFilePath);
         } else {
           await importer.importFromURL(OS.Path.toFileURI(aFilePath));
         }
-        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_SUCCESS);
+        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_SUCCESS, aReplace);
       } catch (ex) {
         Cu.reportError("Failed to restore bookmarks from " + aFilePath + ": " + ex);
-        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED);
+        notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED, aReplace);
         throw ex;
       }
     })();
   },
 
   /**
    * Serializes bookmarks using JSON, and writes to the supplied file path.
    *
@@ -318,18 +318,18 @@ BookmarkImporter.prototype = {
       let url = await fixupQuery(searchBookmark.url, folderIdToGuidMap);
       if (url != searchBookmark.url) {
         await PlacesUtils.bookmarks.update({ guid, url, source: this._source });
       }
     }
   },
 };
 
-function notifyObservers(topic) {
-  Services.obs.notifyObservers(null, topic, "json");
+function notifyObservers(topic, replace) {
+  Services.obs.notifyObservers(null, topic, replace ? "json" : "json-append");
 }
 
 /**
  * Replaces imported folder ids with their local counterparts in a place: URI.
  *
  * @param   {nsIURI} aQueryURI
  *          A place: URI with folder ids.
  * @param   {Object} aFolderIdMap