Bug 1316348 - Make eraseEverything notify for removals within the top-level bookmarks folders to ensure correct updates on the UI. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 23 Aug 2017 15:54:36 +0100
changeset 651316 44aeb39f76677b6541ee3ed587e68a49e578e6d8
parent 651207 b911a4c97fde5d8bdeebfd5d0266ee9f7b9e59b2
child 727669 0058336256807859c742c3fe4e5d447781bc306f
push id75679
push userbmo:standard8@mozilla.com
push dateWed, 23 Aug 2017 14:54:57 +0000
reviewersmak
bugs1316348
milestone57.0a1
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
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/head_bookmarks.js
toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -1149,17 +1149,18 @@ var Bookmarks = Object.freeze({
  *        based on the observer's preferences.
  */
 function notify(observers, notification, args = [], information = {}) {
   for (let observer of observers) {
     if (information.isTagging && observer.skipTags) {
       continue;
     }
 
-    if (information.isDescendantRemoval && observer.skipDescendantsOnItemRemoval) {
+    if (information.isDescendantRemoval && observer.skipDescendantsOnItemRemoval &&
+        !(PlacesUtils.bookmarks.userContentRoots.includes(information.parentGuid))) {
       continue;
     }
 
     try {
       observer[notification](...args);
     } catch (ex) {}
   }
 }
@@ -2229,17 +2230,20 @@ async function(db, folderGuids, options)
   for (let item of itemsRemoved.reverse()) {
     let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null;
     notify(observers, "onItemRemoved", [ item._id, item._parentId,
                                          item.index, item.type, uri,
                                          item.guid, item.parentGuid,
                                          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) {
       for (let entry of (await fetchBookmarksByURL(item, true))) {
         notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
                                              PlacesUtils.toPRTime(entry.lastModified),
                                              entry.type, entry._parentId,
                                              entry.guid, entry.parentGuid,
--- a/toolkit/components/places/tests/bookmarks/head_bookmarks.js
+++ b/toolkit/components/places/tests/bookmarks/head_bookmarks.js
@@ -14,8 +14,42 @@ Cu.import("resource://gre/modules/Servic
 {
   /* import-globals-from ../head_common.js */
   let commonFile = do_get_file("../head_common.js", false);
   let uri = Services.io.newFileURI(commonFile);
   Services.scriptloader.loadSubScript(uri.spec, this);
 }
 
 // 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;
+}
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
@@ -127,8 +127,55 @@ add_task(async function test_eraseEveryt
   await PlacesUtils.bookmarks.eraseEverything();
 
   // All the above items should no longer be in the GUIDHelper cache.
   for (let guid of [folder1.guid, bookmark1.guid, folder2.guid]) {
     await Assert.rejects(PlacesUtils.promiseItemId(guid),
                          /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);
+});
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -1,38 +1,20 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 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() {
   Services.prefs.setIntPref("places.frecency.unvisitedBookmarkBonus", UNVISITED_BOOKMARK_BONUS);
 });
 
 add_task(async function invalid_input_throws() {
   Assert.throws(() => PlacesUtils.bookmarks.remove(),
@@ -180,53 +162,79 @@ add_task(async function remove_folder() 
   Assert.deepEqual(bm1, bm2);
   Assert.equal(bm2.parentGuid, PlacesUtils.bookmarks.unfiledGuid);
   Assert.equal(bm2.index, 0);
   Assert.deepEqual(bm2.dateAdded, bm2.lastModified);
   Assert.equal(bm2.type, PlacesUtils.bookmarks.TYPE_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.
 });
 
 add_task(async function test_contents_removed() {
   let folder1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                      type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                      title: "a folder" });
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid,
                                                  type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                  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);
 
   // 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);
 });
 
 
 add_task(async function test_nested_contents_removed() {
   let folder1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                      type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                      title: "a folder" });
   let folder2 = await PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid,
                                                      type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                      title: "a folder" });
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: folder2.guid,
                                                  type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                  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);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null);
 
   // We should get an onManyFrecenciesChanged notification with the removal of
   // a folder with children.
   await manyFrencenciesPromise;