Bug 1372936 - onManyFrecenciesChanged doesn't get notified for the new Bookmarks eraseEverything and remove APIs. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 14 Jun 2017 16:07:45 +0100
changeset 594147 8bceaabd67475cedaebfc02ad3b33d4066b3a0fd
parent 594056 ad3f1138ce6f199408ad58d65c7476636e924909
child 633345 b9e5794763ce7272b5dc4344121120a1352d8337
push id63945
push userbmo:standard8@mozilla.com
push dateWed, 14 Jun 2017 16:16:33 +0000
reviewersmak
bugs1372936
milestone56.0a1
Bug 1372936 - onManyFrecenciesChanged doesn't get notified for the new Bookmarks eraseEverything and remove APIs. r?mak MozReview-Commit-ID: sEsJmfaIHk
toolkit/components/places/Bookmarks.jsm
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
@@ -1038,27 +1038,27 @@ var Bookmarks = Object.freeze({
   },
 });
 
 // Globals.
 
 /**
  * Sends a bookmarks notification through the given observers.
  *
- * @param observers
+ * @param {Array} observers
  *        array of nsINavBookmarkObserver objects.
- * @param notification
+ * @param {String} notification
  *        the notification name.
- * @param args
+ * @param {Array} [args]
  *        array of arguments to pass to the notification.
- * @param information
+ * @param {Object} [information]
  *        Information about the notification, so we can filter based
  *        based on the observer's preferences.
  */
-function notify(observers, notification, args, information = {}) {
+function notify(observers, notification, args = [], information = {}) {
   for (let observer of observers) {
     if (information.isTagging && observer.skipTags) {
       continue;
     }
 
     if (information.isDescendantRemoval && observer.skipDescendantsOnItemRemoval) {
       continue;
     }
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
@@ -1,11 +1,24 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
+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);
+  });
+}
+
 add_task(async function test_eraseEverything() {
   await PlacesTestUtils.addVisits({ uri: NetUtil.newURI("http://example.com/") });
   await PlacesTestUtils.addVisits({ uri: NetUtil.newURI("http://mozilla.org/") });
   let frecencyForExample = frecencyForUrl("http://example.com/");
   let frecencyForMozilla = frecencyForUrl("http://example.com/");
   Assert.ok(frecencyForExample > 0);
   Assert.ok(frecencyForMozilla > 0);
   let unfiledFolder = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
@@ -52,18 +65,23 @@ add_task(async function test_eraseEveryt
   checkBookmarkObject(toolbarBookmarkInFolder);
   PlacesUtils.annotations.setItemAnnotation((await PlacesUtils.promiseItemId(toolbarBookmarkInFolder.guid)),
                                             "testanno1", "testvalue1", 0, 0);
 
   await PlacesTestUtils.promiseAsyncUpdates();
   Assert.ok(frecencyForUrl("http://example.com/") > frecencyForExample);
   Assert.ok(frecencyForUrl("http://example.com/") > frecencyForMozilla);
 
+  let manyFrecenciesPromise = promiseManyFrecenciesChanged();
+
   await PlacesUtils.bookmarks.eraseEverything();
 
+  // Ensure we get an onManyFrecenciesChanged notification.
+  await manyFrecenciesPromise;
+
   Assert.equal(frecencyForUrl("http://example.com/"), frecencyForExample);
   Assert.equal(frecencyForUrl("http://example.com/"), frecencyForMozilla);
 
   // Check there are no orphan annotations.
   let conn = await PlacesUtils.promiseDBConnection();
   let annoAttrs = await conn.execute(`SELECT id, name FROM moz_anno_attributes`);
   // Bug 1306445 will eventually remove the mobile root anno.
   Assert.equal(annoAttrs.length, 1);
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -1,11 +1,44 @@
 /* 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) => {
+      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);
+  });
+}
+
+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(),
                 /Input should be a valid object/);
   Assert.throws(() => PlacesUtils.bookmarks.remove(null),
                 /Input should be a valid object/);
 
   Assert.throws(() => PlacesUtils.bookmarks.remove("test"),
                 /Invalid value for property 'guid'/);
@@ -61,32 +94,43 @@ add_task(async function remove_normal_fo
                                                     type: PlacesUtils.bookmarks.TYPE_FOLDER });
   checkBookmarkObject(folder);
   let removed_folder = await PlacesUtils.bookmarks.remove(folder);
   Assert.deepEqual(folder, removed_folder);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder.guid)), null);
 });
 
 add_task(async function remove_bookmark() {
+  // When removing a bookmark we need to check the frecency. First we confirm
+  // that there is a normal update when it is inserted.
+  let frecencyChangedPromise = promiseFrecencyChanged("http://example.com/",
+    UNVISITED_BOOKMARK_BONUS);
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                  url: "http://example.com/",
                                                  title: "a bookmark" });
   checkBookmarkObject(bm1);
 
+  await frecencyChangedPromise;
+
+  // This second one checks the frecency is changed when we remove the bookmark.
+  frecencyChangedPromise = promiseFrecencyChanged("http://example.com/", 0);
+
   let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid);
   checkBookmarkObject(bm2);
 
   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_BOOKMARK);
   Assert.equal(bm2.url.href, "http://example.com/");
   Assert.equal(bm2.title, "a bookmark");
+
+  await frecencyChangedPromise;
 });
 
 
 add_task(async function remove_bookmark_orphans() {
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                  url: "http://example.com/",
                                                  title: "a bookmark" });
@@ -125,26 +169,32 @@ add_task(async function remove_bookmark_
 });
 
 add_task(async function remove_folder() {
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                  title: "a folder" });
   checkBookmarkObject(bm1);
 
+  let manyFrencenciesPromise = promiseManyFrecenciesChanged();
+
   let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid);
   checkBookmarkObject(bm2);
 
   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));
+
+  // We should get an onManyFrecenciesChanged notification with the remove of
+  // a folder.
+  await manyFrencenciesPromise;
 });
 
 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,