Bug 1428040 - Allow PlacesUtils.isRootItem to take guids as well as ids. r=mak
authorMark Banner <standard8@mozilla.com>
Thu, 21 Dec 2017 09:16:48 +0000
changeset 397780 99f4d3702ed60412baf8b8cbd42fbf4c660d0b5b
parent 397779 5ce371fc492fba5200d03b05c00787ec82603ddb
child 397781 e43ed104f05649a0d9df22c4ea07008b995e0083
push id57536
push usermbanner@mozilla.com
push dateThu, 04 Jan 2018 16:49:15 +0000
treeherderautoland@99f4d3702ed6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1428040
milestone59.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 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]