Bug 1316348 - Make eraseEverything notify for removals within the top-level bookmarks folders to ensure correct updates on the UI. r=mak

MozReview-Commit-ID: 2bt24qqOd4S

--HG--
extra : rebase_source : b6a5ee35e535655ecc249953de532645f40ddc8d

toolkit/components/places/Bookmarks.jsm

@@ -1154,7 +1154,8 @@ function notify(observers, notification, args = [], information = {}) {
       continue;
     }
 
-    if (information.isDescendantRemoval && observer.skipDescendantsOnItemRemoval) {
+    if (information.isDescendantRemoval && observer.skipDescendantsOnItemRemoval &&
+        !(PlacesUtils.bookmarks.userContentRoots.includes(information.parentGuid))) {
       continue;
     }
 
@@ -2234,7 +2235,10 @@ async function(db, folderGuids, options) {
                                          source ],
                                        // Notify observers that this item is being
                                        // removed as a descendent.
-                                       { isDescendantRemoval: true });
+                                       {
+                                         isDescendantRemoval: true,
+                                         parentGuid: item.parentGuid
+                                       });
 
     let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
     if (isUntagging) {

toolkit/components/places/tests/bookmarks/head_bookmarks.js

@@ -19,3 +19,37 @@ Cu.import("resource://gre/modules/Services.jsm");
 }
 
 // Put any other stuff relative to this test folder below.
+
+function expectNotifications(skipDescendants) {
+  let notifications = [];
+  let observer = new Proxy(NavBookmarkObserver, {
+    get(target, name) {
+      if (name == "skipDescendantsOnItemRemoval") {
+        return skipDescendants;
+      }
+
+      if (name == "check") {
+        PlacesUtils.bookmarks.removeObserver(observer);
+        return expectedNotifications =>
+          Assert.deepEqual(notifications, expectedNotifications);
+      }
+
+      if (name.startsWith("onItem")) {
+        return (...origArgs) => {
+          let args = Array.from(origArgs, arg => {
+            if (arg && arg instanceof Ci.nsIURI)
+              return new URL(arg.spec);
+            return arg;
+          });
+          notifications.push({ name, arguments: { guid: args[5] }});
+        }
+      }
+
+      if (name in target)
+        return target[name];
+      return undefined;
+    }
+  });
+  PlacesUtils.bookmarks.addObserver(observer);
+  return observer;
+}

toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js

@@ -132,3 +132,50 @@ add_task(async function test_eraseEverything_reparented() {
                          /no item found for the given GUID/);
   }
 });
+
+add_task(async function test_notifications() {
+  let bms = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: [{
+      title: "test",
+      url: "http://example.com",
+    }, {
+      title: "folder",
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      children: [{
+        title: "test2",
+        url: "http://example.com/2",
+      }]
+    }]
+  });
+
+  let skipDescendantsObserver = expectNotifications(true);
+  let receiveAllObserver = expectNotifications(false);
+
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  let expectedNotifications = [{
+    name: "onItemRemoved",
+    arguments: {
+      guid: bms[1].guid,
+    },
+  }, {
+    name: "onItemRemoved",
+    arguments: {
+      guid: bms[0].guid,
+    },
+  }];
+
+  // If we're skipping descendents, we'll only be notified of the folder.
+  skipDescendantsObserver.check(expectedNotifications);
+
+  // Note: Items of folders get notified first.
+  expectedNotifications.unshift({
+    name: "onItemRemoved",
+    arguments: {
+      guid: bms[2].guid,
+    },
+  });
+
+  receiveAllObserver.check(expectedNotifications);
+});

toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js

@@ -4,30 +4,12 @@
 const UNVISITED_BOOKMARK_BONUS = 140;
 
 function promiseFrecencyChanged(expectedURI, expectedFrecency) {
-  return new Promise(resolve => {
-    let obs = new NavHistoryObserver();
-    obs.onFrecencyChanged = (uri, newFrecency) => {
+  return PlacesTestUtils.waitForNotification("onFrecencyChanged",
+    (uri, newFrecency) => {
       Assert.equal(uri.spec, expectedURI, "onFrecencyChanged is triggered for the correct uri.");
       Assert.equal(newFrecency, expectedFrecency, "onFrecencyChanged has the expected frecency");
-      PlacesUtils.history.removeObserver(obs)
-      resolve();
-    };
-
-    PlacesUtils.history.addObserver(obs);
-  });
-}
-
-function promiseManyFrecenciesChanged() {
-  return new Promise(resolve => {
-    let obs = new NavHistoryObserver();
-    obs.onManyFrecenciesChanged = () => {
-      Assert.ok(true, "onManyFrecenciesChanged is triggered.");
-      PlacesUtils.history.removeObserver(obs)
-      resolve();
-    };
-
-    PlacesUtils.history.addObserver(obs);
-  });
+      return true;
+    }, "history");
 }
 
 add_task(async function setup() {
@@ -185,7 +167,7 @@ add_task(async function remove_folder() {
   Assert.equal(bm2.title, "a folder");
   Assert.ok(!("url" in bm2));
 
-  // No promiseManyFrecenciesChanged in this test as the folder doesn't have
+  // No wait for onManyFrecenciesChanged in this test as the folder doesn't have
   // any children that would need updating.
 });
 
@@ -198,7 +180,10 @@ add_task(async function test_contents_removed() {
                                                  url: "http://example.com/",
                                                  title: "" });
 
-  let manyFrencenciesPromise = promiseManyFrecenciesChanged();
+  let skipDescendantsObserver = expectNotifications(true);
+  let receiveAllObserver = expectNotifications(false);
+  let manyFrencenciesPromise =
+    PlacesTestUtils.waitForNotification("onManyFrecenciesChanged", () => true, "history");
   await PlacesUtils.bookmarks.remove(folder1);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder1.guid)), null);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null);
@@ -206,6 +191,28 @@ add_task(async function test_contents_removed() {
   // We should get an onManyFrecenciesChanged notification with the removal of
   // a folder with children.
   await manyFrencenciesPromise;
+
+  let expectedNotifications = [{
+    name: "onItemRemoved",
+    arguments: {
+      guid: folder1.guid,
+    },
+  }];
+
+  // If we're skipping descendents, we'll only be notified of the folder.
+  skipDescendantsObserver.check(expectedNotifications);
+
+  // Note: Items of folders get notified first.
+  expectedNotifications.unshift({
+    name: "onItemRemoved",
+    arguments: {
+      guid: bm1.guid
+    },
+  });
+
+  // If we don't skip descendents, we'll be notified of the folder and the
+  // bookmark.
+  receiveAllObserver.check(expectedNotifications);
 });
 
 
@@ -221,7 +228,8 @@ add_task(async function test_nested_contents_removed() {
                                                  url: "http://example.com/",
                                                  title: "" });
 
-  let manyFrencenciesPromise = promiseManyFrecenciesChanged();
+  let manyFrencenciesPromise =
+    PlacesTestUtils.waitForNotification("onManyFrecenciesChanged", () => true, "history");
   await PlacesUtils.bookmarks.remove(folder1);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder1.guid)), null);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder2.guid)), null);