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
--- 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;