Bug 1428040 - Allow PlacesUtils.isRootItem to take guids as well as ids. r?mak
MozReview-Commit-ID: 9ZMA2A879O8
--- 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]