author | bsilverberg <bsilverberg@mozilla.com> |
Thu, 03 Mar 2016 08:00:42 -0500 | |
changeset 286943 | bd154748b83a56ee103c21a324b5bd1c4cc28492 |
parent 286884 | a20f8c2cf8bb75075e198c499264bba973f5c37a |
child 286944 | 26fe286d51cd1282d310d0452c177373c9a439eb |
push id | 30058 |
push user | ryanvm@gmail.com |
push date | Sun, 06 Mar 2016 20:03:45 +0000 |
treeherder | mozilla-central@b6acf4d4fc20 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | kmag, mak |
bugs | 1252250 |
milestone | 47.0a1 |
first release with | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
last release without | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
--- a/browser/components/extensions/ext-bookmarks.js +++ b/browser/components/extensions/ext-bookmarks.js @@ -189,16 +189,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 @@ -395,17 +395,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"); @@ -329,19 +337,47 @@ function backgroundScript() { }).then(results => { browser.test.assertEq(5, results.length, "Expected number of results returned by getRecent"); browser.test.assertEq("Firefox", results[0].title, "Bookmark has the expected title"); browser.test.assertEq("Mozilla Corporation", results[1].title, "Bookmark has the expected title"); browser.test.assertEq("Mozilla", results[2].title, "Bookmark has the expected title"); browser.test.assertEq("Toolbar Item", results[3].title, "Bookmark has the expected title"); browser.test.assertEq("Menu Item", results[4].title, "Bookmark has the expected title"); + 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" + ); + }); + }); + }).then(() => { return 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 ]); @@ -1063,26 +1068,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(); }