Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak
authorbsilverberg <bsilverberg@mozilla.com>
Thu, 03 Mar 2016 08:00:42 -0500
changeset 286944 bd154748b83a56ee103c21a324b5bd1c4cc28492
parent 286939 a20f8c2cf8bb75075e198c499264bba973f5c37a
child 286945 26fe286d51cd1282d310d0452c177373c9a439eb
push id18025
push userryanvm@gmail.com
push dateSun, 06 Mar 2016 20:04:05 +0000
treeherderfx-team@b6acf4d4fc20 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, mak
bugs1252250
milestone47.0a1
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");
@@ -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();
 }