Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak
☠☠ backed out by c9435de8c24c ☠ ☠
authorbsilverberg <bsilverberg@mozilla.com>
Fri, 04 Mar 2016 11:02:28 -0800
changeset 325049 a67f0b208af6351f1f804aeac976fa1814c00c75
parent 325048 1cfb0f3e893ebe7fe0979b3f74b3dac4b7bb7752
child 325050 27bf11a167ff9d6ac59015329d893148ede3f9a8
push id1128
push userjlund@mozilla.com
push dateWed, 01 Jun 2016 01:31:59 +0000
treeherdermozilla-release@fe0d30de989d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, mak
bugs1252250
milestone47.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
Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak MozReview-Commit-ID: HyjJrEjcsZu
browser/components/extensions/ext-bookmarks.js
browser/components/extensions/schemas/bookmarks.json
toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
--- 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");
@@ -330,18 +338,45 @@ function backgroundScript() {
     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.test.notifyPass("bookmarks");
+  }).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 ]);
 
@@ -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();
 }