Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them, r=mak

MozReview-Commit-ID: 8pKlBmDVX5X

--HG--
extra : rebase_source : 88a07f15e920d570576d0220f541f156f6da86f9

browser/components/migration/AutoMigrate.jsm

@@ -30,10 +30,11 @@ const kNotificationId = "abouthome-automigration-undo";
 
 Cu.import("resource:///modules/MigrationUtils.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
-Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
+                                  "resource://gre/modules/PlacesUtils.jsm");
 
 const AutoMigrate = {
   get resourceTypesToUse() {
@@ -390,6 +391,53 @@ const AutoMigrate = {
   UNDO_REMOVED_REASON_OFFER_EXPIRED: 4,
   UNDO_REMOVED_REASON_OFFER_REJECTED: 5,
 
+  _removeUnchangedBookmarks: Task.async(function* (bookmarks) {
+    if (!bookmarks.length) {
+      return;
+    }
+
+    let guidToLMMap = new Map(bookmarks.map(b => [b.guid, b.lastModified]));
+    let bookmarksFromDB = [];
+    let bmPromises = Array.from(guidToLMMap.keys()).map(guid => {
+      // Ignore bookmarks where the promise doesn't resolve (ie that are missing)
+      // Also check that the bookmark fetch returns isn't null before adding it.
+      return PlacesUtils.bookmarks.fetch(guid).then(bm => bm && bookmarksFromDB.push(bm), () => {});
+    });
+    // We can't use the result of Promise.all because that would include nulls
+    // for bookmarks that no longer exist (which we're catching above).
+    yield Promise.all(bmPromises);
+    let unchangedBookmarks = bookmarksFromDB.filter(bm => {
+      return bm.lastModified.getTime() == guidToLMMap.get(bm.guid).getTime();
+    });
+
+    // We need to remove items with no ancestors first, followed by their
+    // parents, etc. In order to do this, find out how many ancestors each item
+    // has that also appear in our list of things to remove, and sort the items
+    // by those numbers. This ensures that children are always removed before
+    // their parents.
+    function determineAncestorCount(bm) {
+      if (bm._ancestorCount) {
+        return bm._ancestorCount;
+      }
+      let myCount = 0;
+      let parentBM = unchangedBookmarks.find(item => item.guid == bm.parentGuid);
+      if (parentBM) {
+        myCount = determineAncestorCount(parentBM) + 1;
+      }
+      bm._ancestorCount = myCount;
+      return myCount;
+    }
+    unchangedBookmarks.forEach(determineAncestorCount);
+    unchangedBookmarks.sort((a, b) => b._ancestorCount - a._ancestorCount);
+    for (let {guid} of unchangedBookmarks) {
+      yield PlacesUtils.bookmarks.remove(guid, {preventRemovalOfNonEmptyFolders: true}).catch(err => {
+        if (err && err.message != "Cannot remove a non-empty folder.") {
+          Cu.reportError(err);
+        }
+      });
+    }
+  }),
+
   QueryInterface: XPCOMUtils.generateQI(
     [Ci.nsIObserver, Ci.nsINavBookmarkObserver, Ci.nsISupportsWeakReference]
   ),

browser/components/migration/MigrationUtils.jsm

@@ -38,6 +38,9 @@ var gProfileStartup = null;
 var gMigrationBundle = null;
 var gPreviousDefaultBrowserKey = "";
 
+let gKeepUndoData = false;
+let gUndoData = null;
+
 XPCOMUtils.defineLazyGetter(this, "gAvailableMigratorKeys", function() {
   if (AppConstants.platform == "win") {
     return [
@@ -948,7 +951,20 @@ this.MigrationUtils = Object.freeze({
 
   insertBookmarkWrapper(bookmark) {
     this._importQuantities.bookmarks++;
-    return PlacesUtils.bookmarks.insert(bookmark);
+    let insertionPromise = PlacesUtils.bookmarks.insert(bookmark);
+    if (!gKeepUndoData) {
+      return insertionPromise;
+    }
+    // If we keep undo data, add a promise handler that stores the undo data once
+    // the bookmark has been inserted in the DB, and then returns the bookmark.
+    let {parentGuid} = bookmark;
+    return insertionPromise.then(bm => {
+      let {guid, lastModified, type} = bm;
+      gUndoData.get("bookmarks").push({
+        parentGuid, guid, lastModified, type
+      });
+      return bm;
+    });
   },
 
   insertVisitsWrapper(places, options) {
@@ -961,6 +977,43 @@ this.MigrationUtils = Object.freeze({
     return LoginHelper.maybeImportLogin(login);
   },
 
+  initializeUndoData() {
+    gKeepUndoData = true;
+    gUndoData = new Map([["bookmarks", []], ["visits", new Map()], ["logins", []]]);
+  },
+
+  _postProcessUndoData: Task.async(function*(state) {
+    if (!state) {
+      return state;
+    }
+    let bookmarkFolders = state.get("bookmarks").filter(b => b.type == PlacesUtils.bookmarks.TYPE_FOLDER);
+
+    let bookmarkFolderData = [];
+    let bmPromises = bookmarkFolders.map(({guid}) => {
+      // Ignore bookmarks where the promise doesn't resolve (ie that are missing)
+      // Also check that the bookmark fetch returns isn't null before adding it.
+      return PlacesUtils.bookmarks.fetch(guid).then(bm => bm && bookmarkFolderData.push(bm), () => {});
+    });
+
+    yield Promise.all(bmPromises);
+    let folderLMMap = new Map(bookmarkFolderData.map(b => [b.guid, b.lastModified]));
+    for (let bookmark of bookmarkFolders) {
+      let lastModified = folderLMMap.get(bookmark.guid);
+      // If the bookmark was deleted, the map will be returning null, so check:
+      if (lastModified) {
+        bookmark.lastModified = lastModified;
+      }
+    }
+    return state;
+  }),
+
+  stopAndRetrieveUndoData() {
+    let undoData = gUndoData;
+    gUndoData = null;
+    gKeepUndoData = false;
+    return this._postProcessUndoData(undoData);
+  },
+
   /**
    * Cleans up references to migrators and nsIProfileInstance instances.
    */

browser/components/migration/tests/unit/test_automigration.js

@@ -261,17 +261,17 @@ add_task(function* checkUndoDisablingByBookmarksAndPasswords() {
   Services.prefs.setCharPref("browser.migrate.automigrate.finished", endTime);
   AutoMigrate.maybeInitUndoObserver();
 
-  ok(AutoMigrate.canUndo(), "Should be able to undo.");
+  Assert.ok(AutoMigrate.canUndo(), "Should be able to undo.");
 
   // Insert a login and check that that disabled undo.
   let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
   login.init("www.mozilla.org", "http://www.mozilla.org", null, "user", "pass", "userEl", "passEl");
   Services.logins.addLogin(login);
 
-  ok(!AutoMigrate.canUndo(), "Should no longer be able to undo.");
+  Assert.ok(!AutoMigrate.canUndo(), "Should no longer be able to undo.");
   Services.prefs.setCharPref("browser.migrate.automigrate.started", startTime);
   Services.prefs.setCharPref("browser.migrate.automigrate.finished", endTime);
-  ok(AutoMigrate.canUndo(), "Should be able to undo.");
+  Assert.ok(AutoMigrate.canUndo(), "Should be able to undo.");
   AutoMigrate.maybeInitUndoObserver();
 
   // Insert a bookmark and check that that disabled undo.
@@ -280,7 +280,7 @@ add_task(function* checkUndoDisablingByBookmarksAndPasswords() {
     url: "http://www.example.org/",
     title: "Some example bookmark",
   });
-  ok(!AutoMigrate.canUndo(), "Should no longer be able to undo.");
+  Assert.ok(!AutoMigrate.canUndo(), "Should no longer be able to undo.");
 
   try {
     Services.logins.removeAllLogins();
@@ -288,3 +288,99 @@ add_task(function* checkUndoDisablingByBookmarksAndPasswords() {
   yield PlacesUtils.bookmarks.eraseEverything();
 });
 
+add_task(function* checkUndoBookmarksState() {
+  MigrationUtils.initializeUndoData();
+  const {TYPE_FOLDER, TYPE_BOOKMARK} = PlacesUtils.bookmarks;
+  let title = "Some example bookmark";
+  let url = "http://www.example.com";
+  let parentGuid = PlacesUtils.bookmarks.toolbarGuid;
+  let {guid, lastModified} = yield MigrationUtils.insertBookmarkWrapper({
+    title, url, parentGuid
+  });
+  Assert.deepEqual((yield MigrationUtils.stopAndRetrieveUndoData()).get("bookmarks"),
+      [{lastModified, parentGuid, guid, type: TYPE_BOOKMARK}]);
+
+  MigrationUtils.initializeUndoData();
+  ({guid, lastModified} = yield MigrationUtils.insertBookmarkWrapper({
+    title, parentGuid, type: TYPE_FOLDER
+  }));
+  let folder = {guid, lastModified, parentGuid, type: TYPE_FOLDER};
+  let folderGuid = folder.guid;
+  ({guid, lastModified} = yield MigrationUtils.insertBookmarkWrapper({
+    title, url, parentGuid: folderGuid
+  }));
+  let kid1 = {guid, lastModified, parentGuid: folderGuid, type: TYPE_BOOKMARK};
+  ({guid, lastModified} = yield MigrationUtils.insertBookmarkWrapper({
+    title, url, parentGuid: folderGuid
+  }));
+  let kid2 = {guid, lastModified, parentGuid: folderGuid, type: TYPE_BOOKMARK};
+
+  let bookmarksUndo = (yield MigrationUtils.stopAndRetrieveUndoData()).get("bookmarks");
+  Assert.equal(bookmarksUndo.length, 3);
+  // We expect that the last modified time from first kid #1 and then kid #2
+  // has been propagated to the folder:
+  folder.lastModified = kid2.lastModified;
+  // Not just using deepEqual on the entire array (which should work) because
+  // the failure messages get truncated by xpcshell which is unhelpful.
+  Assert.deepEqual(bookmarksUndo[0], folder);
+  Assert.deepEqual(bookmarksUndo[1], kid1);
+  Assert.deepEqual(bookmarksUndo[2], kid2);
+  yield PlacesUtils.bookmarks.eraseEverything();
+});
+
+add_task(function* testBookmarkRemovalByUndo() {
+  const {TYPE_FOLDER} = PlacesUtils.bookmarks;
+  MigrationUtils.initializeUndoData();
+  let title = "Some example bookmark";
+  let url = "http://www.mymagicaluniqueurl.com";
+  let parentGuid = PlacesUtils.bookmarks.toolbarGuid;
+  let {guid} = yield MigrationUtils.insertBookmarkWrapper({
+    title: "Some folder", parentGuid, type: TYPE_FOLDER
+  });
+  let folderGuid = guid;
+  let itemsToRemove = [];
+  ({guid} = yield MigrationUtils.insertBookmarkWrapper({
+    title: "Inner folder", parentGuid: folderGuid, type: TYPE_FOLDER
+  }));
+  let innerFolderGuid = guid;
+  itemsToRemove.push(innerFolderGuid);
+
+  ({guid} = yield MigrationUtils.insertBookmarkWrapper({
+    title: "Inner inner folder", parentGuid: innerFolderGuid, type: TYPE_FOLDER
+  }));
+  itemsToRemove.push(guid);
+
+  ({guid} = yield MigrationUtils.insertBookmarkWrapper({
+    title: "Inner nested item", url: "http://inner-nested-example.com", parentGuid: guid
+  }));
+  itemsToRemove.push(guid);
+
+  ({guid} = yield MigrationUtils.insertBookmarkWrapper({
+    title, url, parentGuid: folderGuid
+  }));
+  itemsToRemove.push(guid);
+
+  for (let toBeRemovedGuid of itemsToRemove) {
+    let dbResultForGuid = yield PlacesUtils.bookmarks.fetch(toBeRemovedGuid);
+    Assert.ok(dbResultForGuid, "Should be able to find items that will be removed.");
+  }
+  let bookmarkUndoState = (yield MigrationUtils.stopAndRetrieveUndoData()).get("bookmarks");
+  // Now insert a separate item into this folder, not related to the migration.
+  let newItem = yield PlacesUtils.bookmarks.insert(
+    {title: "Not imported", parentGuid: folderGuid, url: "http://www.example.com"}
+  );
+
+  yield AutoMigrate._removeUnchangedBookmarks(bookmarkUndoState);
+  Assert.ok(true, "Successfully removed imported items.");
+
+  let itemFromDB = yield PlacesUtils.bookmarks.fetch(newItem.guid);
+  Assert.ok(itemFromDB, "Item we inserted outside of migration is still there.");
+  itemFromDB = yield PlacesUtils.bookmarks.fetch(folderGuid);
+  Assert.ok(itemFromDB, "Folder we inserted in migration is still there because of new kids.");
+  for (let removedGuid of itemsToRemove) {
+    let dbResultForGuid = yield PlacesUtils.bookmarks.fetch(removedGuid);
+    let dbgStr = dbResultForGuid && dbResultForGuid.title;
+    Assert.equal(null, dbResultForGuid, "Should not be able to find items that should have been removed, but found " + dbgStr);
+  }
+  yield PlacesUtils.bookmarks.eraseEverything();
+});