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 449518 ed1d0d623ca4a66733383335b6fa19a5a3472b76
parent 449517 7fe4c38ae22f924209aacc4cb3e5177d79cdb9ef
child 449519 29e0ea0b247007a2760addeae2c8194686a8d35c
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [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]