Bug 1104796 - Bookmarks.remove disallows removing any direct child of the places root. r=mak
authorAsaf Romano <mano@mozilla.com>
Thu, 04 Dec 2014 10:57:20 -0800
changeset 218470 54f97b545e4c965e43b632708aff654f11d9b9f7
parent 218469 b0c1cd33f0eda2ed84efe775d6bbf26671937456
child 218471 f3278982666a4fb232548287b4a500f388f70d75
push idunknown
push userunknown
push dateunknown
reviewersmak
bugs1104796
milestone37.0a1
Bug 1104796 - Bookmarks.remove disallows removing any direct child of the places root. r=mak
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -399,33 +399,34 @@ let Bookmarks = Object.freeze({
    * @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) {
     let info = guidOrInfo;
     if (!info)
       throw new Error("Input should be a valid object");
-    if (typeof(guidOrInfo) != "object") {
+    if (typeof(guidOrInfo) != "object")
       info = { guid: guidOrInfo };
+
+    // Disallow removing the root folders.
+    if ([this.rootGuid, this.menuGuid, this.toolbarGuid, this.unfiledGuid,
+         this.tagsGuid].indexOf(info.guid) != -1) {
+      throw new Error("It's not possible to remove Places root folders.");
     }
 
     // Even if we ignore any other unneeded property, we still validate any
     // 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.");
 
-      // Disallow removing the root folders.
-      if (!item._parentId || item._parentId == PlacesUtils.placesRootId)
-        throw new Error("It's not possible to remove Places root folders.");
-
       item = yield removeBookmark(item);
 
       // 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 ]);
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -39,29 +39,34 @@ add_task(function* remove_nonexistent_gu
     yield PlacesUtils.bookmarks.remove({ guid: "123456789012"});
     Assert.ok(false, "Should have thrown");
   } catch (ex) {
     Assert.ok(/No bookmarks found for the provided GUID/.test(ex));
   }
 });
 
 add_task(function* remove_roots_fail() {
-  try {
-    yield PlacesUtils.bookmarks.remove(PlacesUtils.bookmarks.unfiledGuid);
-    Assert.ok(false, "Should have thrown");
-  } catch (ex) {
-    Assert.ok(/It's not possible to remove Places root folders/.test(ex));
+  let guids = [PlacesUtils.bookmarks.rootGuid,
+               PlacesUtils.bookmarks.unfiledGuid,
+               PlacesUtils.bookmarks.menuGuid,
+               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/);
   }
+});
 
-  try {
-    yield PlacesUtils.bookmarks.remove(PlacesUtils.bookmarks.rootGuid);
-    Assert.ok(false, "Should have thrown");
-  } catch (ex) {
-    Assert.ok(/It's not possible to remove Places root folders/.test(ex));
-  }
+add_task(function* remove_normal_folder_undes_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);
 });
 
 add_task(function* remove_bookmark() {
   let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                  url: "http://example.com/",
                                                  title: "a bookmark" });
   checkBookmarkObject(bm1);