Bug 1428040 - Allow PlacesUtils.isRootItem to take guids as well as ids. r?mak draft
authorMark Banner <standard8@mozilla.com>
Thu, 21 Dec 2017 09:16:48 +0000
changeset 715745 72ae81cbcd2b957caa6b9693bdc33acc1b3ea098
parent 715663 f78a83244fbebe8a469ae3512fce7f638cab7e1f
child 744875 f2cc539c796063a15c166ce85bc19303064aa64a
push id94248
push userbmo:standard8@mozilla.com
push dateThu, 04 Jan 2018 15:39:56 +0000
reviewersmak
bugs1428040
milestone59.0a1
Bug 1428040 - Allow PlacesUtils.isRootItem to take guids as well as ids. r?mak MozReview-Commit-ID: 9ZMA2A879O8
browser/components/places/content/treeView.js
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
toolkit/components/places/tests/bookmarks/test_protectRoots.js
toolkit/components/places/tests/bookmarks/xpcshell.ini
toolkit/components/places/tests/legacy/test_protectRoots.js
toolkit/components/places/tests/legacy/xpcshell.ini
toolkit/components/places/tests/unit/test_PlacesUtils_isRootItem.js
toolkit/components/places/tests/unit/xpcshell.ini
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -1733,34 +1733,34 @@ PlacesTreeView.prototype = {
     if (aColumn.index != 0)
       return false;
 
     let node = this._rows[aRow];
     if (!node) {
       Cu.reportError("isEditable called for an unbuilt row.");
       return false;
     }
-    let itemId = node.itemId;
+    let itemGuid = node.bookmarkGuid;
 
     // Only bookmark-nodes are editable.  Fortunately, this checks also takes
     // care of livemark children.
-    if (itemId == -1)
+    if (itemGuid == "")
       return false;
 
     // The following items are also not editable, even though they are bookmark
     // items.
     // * places-roots
     // * the left pane special folders and queries (those are place: uri
     //   bookmarks)
     // * separators
     //
     // Note that concrete itemIds aren't used intentionally.  For example, we
     // have no reason to disallow renaming a shortcut to the Bookmarks Toolbar,
     // except for the one under All Bookmarks.
-    if (PlacesUtils.nodeIsSeparator(node) || PlacesUtils.isRootItem(itemId))
+    if (PlacesUtils.nodeIsSeparator(node) || PlacesUtils.isRootItem(itemGuid))
       return false;
 
     let parentId = PlacesUtils.getConcreteItemId(node.parent);
     if (parentId == PlacesUIUtils.leftPaneFolderId ||
         parentId == PlacesUIUtils.allBookmarksFolderId) {
       // Note that the for the time being this is the check that actually
       // blocks renaming places "roots", and not the isRootItem check above.
       // That's because places root are only exposed through folder shortcuts
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -598,16 +598,19 @@ var Bookmarks = Object.freeze({
                                         b.lastModified >= (b.dateAdded || item.dateAdded) },
           dateAdded: { defaultValue: item.dateAdded }
         });
 
       return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: update",
         async db => {
         let parent;
         if (updateInfo.hasOwnProperty("parentGuid")) {
+          if (PlacesUtils.isRootItem(item.guid)) {
+            throw new Error("It's not possible to move Places root folders.");
+          }
           if (item.type == this.TYPE_FOLDER) {
             // Make sure we are not moving a folder into itself or one of its
             // descendants.
             let rows = await db.executeCached(
               `WITH RECURSIVE
                descendants(did) AS (
                  VALUES(:id)
                  UNION ALL
@@ -623,16 +626,19 @@ var Bookmarks = Object.freeze({
           }
 
           parent = await fetchBookmark({ guid: updateInfo.parentGuid });
           if (!parent)
             throw new Error("No bookmarks found for the provided parentGuid");
         }
 
         if (updateInfo.hasOwnProperty("index")) {
+          if (PlacesUtils.isRootItem(item.guid)) {
+            throw new Error("It's not possible to move Places root folders.");
+          }
           // If at this point we don't have a parent yet, we are moving into
           // the same container.  Thus we know it exists.
           if (!parent)
             parent = await fetchBookmark({ guid: item.parentGuid });
 
           if (updateInfo.index >= parent._childCount ||
               updateInfo.index == this.DEFAULT_INDEX) {
              updateInfo.index = parent._childCount;
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1227,29 +1227,36 @@ this.PlacesUtils = {
   },
 
   get mobileFolderId() {
     delete this.mobileFolderId;
     return this.mobileFolderId = this.bookmarks.mobileFolder;
   },
 
   /**
-   * Checks if aItemId is a root.
+   * Checks if item is a root.
    *
-   *   @param aItemId
-   *          item id to look for.
-   *   @returns true if aItemId is a root, false otherwise.
+   * @param {Number|String} guid The guid or id of the item to look for.
+   * @returns {Boolean} true if guid is a root, false otherwise.
    */
-  isRootItem: function PU_isRootItem(aItemId) {
-    return aItemId == PlacesUtils.bookmarksMenuFolderId ||
-           aItemId == PlacesUtils.toolbarFolderId ||
-           aItemId == PlacesUtils.unfiledBookmarksFolderId ||
-           aItemId == PlacesUtils.tagsFolderId ||
-           aItemId == PlacesUtils.placesRootId ||
-           aItemId == PlacesUtils.mobileFolderId;
+  isRootItem(guid) {
+    if (typeof guid === "string") {
+      return guid == PlacesUtils.bookmarks.menuGuid ||
+             guid == PlacesUtils.bookmarks.toolbarGuid ||
+             guid == PlacesUtils.bookmarks.unfiledGuid ||
+             guid == PlacesUtils.bookmarks.tagsGuid ||
+             guid == PlacesUtils.bookmarks.rootGuid ||
+             guid == PlacesUtils.bookmarks.mobileGuid;
+    }
+    return guid == PlacesUtils.bookmarksMenuFolderId ||
+           guid == PlacesUtils.toolbarFolderId ||
+           guid == PlacesUtils.unfiledBookmarksFolderId ||
+           guid == PlacesUtils.tagsFolderId ||
+           guid == PlacesUtils.placesRootId ||
+           guid == PlacesUtils.mobileFolderId;
   },
 
   /**
    * Set the POST data associated with a bookmark, if any.
    * Used by POST keywords.
    *   @param aBookmarkId
    *
    * @deprecated Use PlacesUtils.keywords.insert() API instead.
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
@@ -73,16 +73,32 @@ add_task(async function invalid_input_th
   Assert.throws(() => PlacesUtils.bookmarks.update({ guid: "123456789012" }),
                 /Not enough properties to update/);
 
   Assert.throws(() => PlacesUtils.bookmarks.update({ guid: "123456789012",
                                                      parentGuid: "012345678901" }),
                 /The following properties were expected: index/);
 });
 
+add_task(async function move_roots_fail() {
+  let guids = [PlacesUtils.bookmarks.unfiledGuid,
+               PlacesUtils.bookmarks.menuGuid,
+               PlacesUtils.bookmarks.toolbarGuid,
+               PlacesUtils.bookmarks.tagsGuid,
+               PlacesUtils.bookmarks.mobileGuid];
+  for (let guid of guids) {
+    Assert.rejects(PlacesUtils.bookmarks.update({
+      guid,
+      index: -1,
+      parentGuid: PlacesUtils.bookmarks.rootGuid,
+    }), /It's not possible to move Places root folders\./,
+    `Should reject when attempting to move ${guid}`);
+  }
+});
+
 add_task(async function nonexisting_bookmark_throws() {
   try {
     await PlacesUtils.bookmarks.update({ guid: "123456789012",
                                          title: "test" });
     Assert.ok(false, "Should have thrown");
   } catch (ex) {
     Assert.ok(/No bookmarks found for the provided GUID/.test(ex));
   }
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -35,13 +35,12 @@ skip-if = toolkit == 'android'
 [test_bookmarks_notifications.js]
 [test_bookmarks_remove.js]
 [test_bookmarks_reorder.js]
 [test_bookmarks_search.js]
 [test_bookmarks_update.js]
 [test_insertTree_fixupOrSkipInvalidEntries.js]
 [test_keywords.js]
 [test_nsINavBookmarkObserver.js]
-[test_protectRoots.js]
 [test_removeFolderTransaction_reinsert.js]
 [test_savedsearches.js]
 [test_sync_fields.js]
 [test_untitled.js]
rename from toolkit/components/places/tests/bookmarks/test_protectRoots.js
rename to toolkit/components/places/tests/legacy/test_protectRoots.js
--- a/toolkit/components/places/tests/legacy/xpcshell.ini
+++ b/toolkit/components/places/tests/legacy/xpcshell.ini
@@ -4,9 +4,10 @@
 [DEFAULT]
 head = head_legacy.js
 firefox-appdir = browser
 
 [test_418643_removeFolderChildren.js]
 [test_bookmarks_setNullTitle.js]
 [test_changeBookmarkURI.js]
 [test_placesTxn.js]
+[test_protectRoots.js]
 [test_removeItem.js]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_PlacesUtils_isRootItem.js
@@ -0,0 +1,15 @@
+"use strict";
+
+const GUIDS = [
+  PlacesUtils.bookmarks.rootGuid,
+  ...PlacesUtils.bookmarks.userContentRoots,
+  PlacesUtils.bookmarks.tagsGuid,
+];
+
+add_task(async function test_isRootItem() {
+  for (let guid of GUIDS) {
+    Assert.ok(PlacesUtils.isRootItem(guid), `Should correctly identify root item ${guid}`);
+  }
+
+  Assert.ok(!PlacesUtils.isRootItem("fakeguid1234"), "Should not identify other items as root.");
+});
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -107,16 +107,17 @@ skip-if = (os == 'win' && ccov) # Bug 14
 # Bug 902248: intermittent timeouts on all platforms
 skip-if = true
 [test_null_interfaces.js]
 [test_onItemChanged_tags.js]
 [test_pageGuid_bookmarkGuid.js]
 [test_frecency_observers.js]
 [test_placeURIs.js]
 [test_PlacesUtils_invalidateCachedGuidFor.js]
+[test_PlacesUtils_isRootItem.js]
 [test_preventive_maintenance.js]
 [test_preventive_maintenance_checkAndFixDatabase.js]
 [test_preventive_maintenance_runTasks.js]
 [test_promiseBookmarksTree.js]
 [test_resolveNullBookmarkTitles.js]
 [test_result_sort.js]
 [test_resultsAsVisit_details.js]
 [test_sql_guid_functions.js]