Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak
MozReview-Commit-ID: HyjJrEjcsZu
--- a/browser/components/extensions/ext-bookmarks.js
+++ b/browser/components/extensions/ext-bookmarks.js
@@ -185,16 +185,28 @@ extensions.registerSchemaAPI("bookmarks"
remove: function(id) {
let info = {
guid: id,
};
// The API doesn't give you the old bookmark at the moment
try {
+ return Bookmarks.remove(info, {preventRemovalOfNonEmptyFolders: true}).then(result => {});
+ } catch (e) {
+ return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
+ }
+ },
+
+ removeTree: function(id) {
+ let info = {
+ guid: id,
+ };
+
+ try {
return Bookmarks.remove(info).then(result => {});
} catch (e) {
return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
}
},
},
};
});
--- a/browser/components/extensions/schemas/bookmarks.json
+++ b/browser/components/extensions/schemas/bookmarks.json
@@ -396,17 +396,16 @@
"name": "callback",
"optional": true,
"parameters": []
}
]
},
{
"name": "removeTree",
- "unsupported": true,
"type": "function",
"description": "Recursively removes a bookmark folder.",
"async": "callback",
"parameters": [
{
"type": "string",
"name": "id"
},
--- a/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
@@ -151,17 +151,25 @@ function backgroundScript() {
browser.bookmarks.create({title: "Mozilla Corporation", url: "http://allizom.com", parentId: createdFolderId}),
browser.bookmarks.create({title: "Firefox", url: "http://allizom.org/firefox", parentId: createdFolderId}),
]).then(() => {
// returns all items on empty object
return browser.bookmarks.search({});
}).then(results => {
browser.test.assertTrue(results.length >= 9, "At least as many bookmarks as added were returned by search({})");
- return browser.bookmarks.getSubTree(createdFolderId);
+ return Promise.resolve().then(() => {
+ return browser.bookmarks.remove(createdFolderId);
+ }).then(expectedError, error => {
+ browser.test.assertTrue(
+ error.message.includes("Cannot remove a non-empty folder"),
+ "Expected error thrown when trying to remove a non-empty folder"
+ );
+ return browser.bookmarks.getSubTree(createdFolderId);
+ });
});
}).then(results => {
browser.test.assertEq(1, results.length, "Expected number of nodes returned by getSubTree");
browser.test.assertEq("Mozilla Folder", results[0].title, "Folder has the expected title");
let children = results[0].children;
browser.test.assertEq(3, children.length, "Expected number of bookmarks returned by getSubTree");
browser.test.assertEq("Firefox", children[0].title, "Bookmark has the expected title");
browser.test.assertEq("Mozilla Corporation", children[1].title, "Bookmark has the expected title");
@@ -302,19 +310,46 @@ function backgroundScript() {
"Expected number of results returned for non-matching title and query fields"
);
// returns an empty array on item not found
return browser.bookmarks.search("microsoft");
}).then(results => {
browser.test.assertEq(0, results.length, "Expected number of results returned for non-matching search");
- browser.test.notifyPass("bookmarks");
+ return browser.bookmarks.search({});
+ }).then(results => {
+ let startBookmarkCount = results.length;
+ return browser.bookmarks.search({title: "Mozilla Folder"}).then(result => {
+ return browser.bookmarks.removeTree(result[0].id);
+ }).then(() => {
+ return browser.bookmarks.search({}).then(results => {
+ browser.test.assertEq(
+ startBookmarkCount - 4,
+ results.length,
+ "Expected number of results returned after removeTree");
+ });
+ });
+ }).then(() => {
+ return browser.bookmarks.create({title: "Empty Folder"});
+ }).then(result => {
+ let emptyFolderId = result.id;
+ browser.test.assertEq("Empty Folder", result.title, "Folder has the expected title");
+ return browser.bookmarks.remove(emptyFolderId).then(() => {
+ return browser.bookmarks.get(emptyFolderId).then(expectedError, error => {
+ browser.test.assertTrue(
+ error.message.includes("Bookmark not found"),
+ "Expected error thrown when trying to get a removed folder"
+ );
+ browser.test.notifyPass("bookmarks");
+ });
+ });
}).catch(error => {
browser.test.fail(`Error: ${String(error)} :: ${error.stack}`);
+ browser.test.notifyFail("bookmarks");
});
}
let extensionData = {
background: `(${backgroundScript})()`,
manifest: {
permissions: ["bookmarks"],
},
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -367,23 +367,28 @@ var Bookmarks = Object.freeze({
},
/**
* Removes a bookmark-item.
*
* @param guidOrInfo
* The globally unique identifier of the item to remove, or an
* object representing it, as defined above.
+ * @param {Object} [options={}]
+ * Additional options that can be passed to the function.
+ * Currently supports preventRemovalOfNonEmptyFolders which
+ * will cause an exception to be thrown if attempting to remove
+ * a folder that is not empty.
*
* @return {Promise} resolved when the removal is complete.
* @resolves to an object representing the removed bookmark.
* @rejects if the provided guid doesn't match any existing bookmark.
* @throws if the arguments are invalid.
*/
- remove(guidOrInfo) {
+ remove(guidOrInfo, options={}) {
let info = guidOrInfo;
if (!info)
throw new Error("Input should be a valid object");
if (typeof(guidOrInfo) != "object")
info = { guid: guidOrInfo };
// Disallow removing the root folders.
if ([this.rootGuid, this.menuGuid, this.toolbarGuid, this.unfiledGuid,
@@ -395,17 +400,17 @@ var Bookmarks = Object.freeze({
// known property to reduce likelihood of hidden bugs.
let removeInfo = validateBookmarkObject(info);
return Task.spawn(function* () {
let item = yield fetchBookmark(removeInfo);
if (!item)
throw new Error("No bookmarks found for the provided GUID.");
- item = yield removeBookmark(item);
+ item = yield removeBookmark(item, options);
// Notify onItemRemoved to listeners.
let observers = PlacesUtils.bookmarks.getObservers();
let uri = item.hasOwnProperty("url") ? toURI(item.url) : null;
notify(observers, "onItemRemoved", [ item._id, item._parentId, item.index,
item.type, uri, item.guid,
item.parentGuid ]);
@@ -1017,26 +1022,30 @@ function fetchBookmarksByParent(info) {
return rowsToItemsArray(rows);
}));
}
////////////////////////////////////////////////////////////////////////////////
// Remove implementation.
-function removeBookmark(item) {
+function removeBookmark(item, options) {
return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: updateBookmark",
Task.async(function*(db) {
let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
yield db.executeTransaction(function* transaction() {
// If it's a folder, remove its contents first.
- if (item.type == Bookmarks.TYPE_FOLDER)
+ if (item.type == Bookmarks.TYPE_FOLDER) {
+ if (options.preventRemovalOfNonEmptyFolders && item._childCount > 0) {
+ throw new Error("Cannot remove a non-empty folder.");
+ }
yield removeFoldersContents(db, [item.guid]);
+ }
// Remove annotations first. If it's a tag, we can avoid paying that cost.
if (!isUntagging) {
// We don't go through the annotations service for this cause otherwise
// we'd get a pointless onItemChanged notification and it would also
// set lastModified to an unexpected value.
yield removeAnnotationsForItem(db, item._id);
}
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -50,17 +50,17 @@ add_task(function* remove_roots_fail() {
PlacesUtils.bookmarks.toolbarGuid,
PlacesUtils.bookmarks.tagsGuid];
for (let guid of guids) {
Assert.throws(() => PlacesUtils.bookmarks.remove(guid),
/It's not possible to remove Places root folders/);
}
});
-add_task(function* remove_normal_folder_undes_root_succeeds() {
+add_task(function* remove_normal_folder_under_root_succeeds() {
let folder = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.rootGuid,
type: PlacesUtils.bookmarks.TYPE_FOLDER });
checkBookmarkObject(folder);
let removed_folder = yield PlacesUtils.bookmarks.remove(folder);
Assert.deepEqual(folder, removed_folder);
Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(folder.guid)), null);
});
@@ -146,16 +146,17 @@ add_task(function* test_nested_contents_
title: "a folder" });
let sep = yield PlacesUtils.bookmarks.insert({ parentGuid: folder2.guid,
type: PlacesUtils.bookmarks.TYPE_SEPARATOR });
yield PlacesUtils.bookmarks.remove(folder1);
Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(folder1.guid)), null);
Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(folder2.guid)), null);
Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(sep.guid)), null);
});
+
add_task(function* remove_folder_empty_title() {
let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
type: PlacesUtils.bookmarks.TYPE_FOLDER,
title: "" });
checkBookmarkObject(bm1);
let bm2 = yield PlacesUtils.bookmarks.remove(bm1.guid);
checkBookmarkObject(bm2);
@@ -177,11 +178,22 @@ add_task(function* remove_separator() {
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_SEPARATOR);
Assert.ok(!("url" in bm2));
Assert.ok(!("title" in bm2));
});
+add_task(function* test_nested_content_fails_when_not_allowed() {
+ let folder1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "a folder" });
+ let folder2 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "a folder" });
+ Assert.rejects(PlacesUtils.bookmarks.remove(folder1, {preventRemovalOfNonEmptyFolders: true}),
+ /Cannot remove a non-empty folder./);
+});
+
function run_test() {
run_next_test();
}