From 896f516dd787e5dcd793502bce80db4a08105926 Mon Sep 17 00:00:00 2001 From: Asaf Romano Date: Wed, 1 Oct 2014 12:19:06 +0300 Subject: [PATCH] Bug 1068671 - Remove 'read-only' folders support from places. r=mak --- browser/components/places/PlacesUIUtils.jsm | 107 +++++++++++++++++- .../places/content/browserPlacesViews.js | 4 +- .../components/places/content/controller.js | 92 +++------------ browser/components/places/content/menu.xml | 6 +- browser/components/places/content/treeView.js | 36 ++++-- .../places/tests/browser/browser_423515.js | 50 +------- toolkit/components/places/PlacesUtils.jsm | 84 +------------- .../places/nsINavBookmarksService.idl | 26 +---- .../places/nsINavHistoryService.idl | 22 +--- .../components/places/nsLivemarkService.js | 1 - toolkit/components/places/nsNavBookmarks.cpp | 53 ++------- toolkit/components/places/nsNavBookmarks.h | 9 ++ .../components/places/nsNavHistoryResult.cpp | 43 +------ .../components/places/nsNavHistoryResult.h | 15 +-- .../places/tests/queries/head_queries.js | 3 - .../places/tests/queries/test_async.js | 1 - .../queries/test_excludeReadOnlyFolders.js | 48 -------- .../tests/queries/test_querySerialization.js | 11 -- .../places/tests/queries/xpcshell.ini | 1 - 19 files changed, 195 insertions(+), 417 deletions(-) delete mode 100644 toolkit/components/places/tests/queries/test_excludeReadOnlyFolders.js diff --git a/browser/components/places/PlacesUIUtils.jsm b/browser/components/places/PlacesUIUtils.jsm index e506d89ba436..b00554d2f3ec 100644 --- a/browser/components/places/PlacesUIUtils.jsm +++ b/browser/components/places/PlacesUIUtils.jsm @@ -44,6 +44,43 @@ XPCOMUtils.defineLazyModuleGetter(this, "Weave", // copied from utilityOverlay.js const TAB_DROP_TYPE = "application/x-moz-tabbrowser-tab"; +// This function isn't public both because it's synchronous and because it is +// going to be removed in bug 1072833. +function IsLivemark(aItemId) { + // Since this check may be done on each dragover event, it's worth maintaining + // a cache. + let self = IsLivemark; + if (!("ids" in self)) { + const LIVEMARK_ANNO = PlacesUtils.LMANNO_FEEDURI; + + let idsVec = PlacesUtils.annotations.getItemsWithAnnotation(LIVEMARK_ANNO); + self.ids = new Set(idsVec); + + let obs = Object.freeze({ + QueryInterface: XPCOMUtils.generateQI(Ci.nsIAnnotationObserver), + + onItemAnnotationSet(itemId, annoName) { + if (annoName == LIVEMARK_ANNO) + self.ids.add(itemId); + }, + + onItemAnnotationRemoved(itemId, annoName) { + // If annoName is set to an empty string, the item is gone. + if (annoName == LIVEMARK_ANNO || annoName == "") + self.ids.delete(itemId); + }, + + onPageAnnotationSet() { }, + onPageAnnotationRemoved() { }, + }); + PlacesUtils.annotations.addObserver(obs); + PlacesUtils.registerShutdownFunction(() => { + PlacesUtils.annotations.removeObserver(obs); + }); + } + return self.ids.has(aItemId); +} + this.PlacesUIUtils = { ORGANIZER_LEFTPANE_VERSION: 7, ORGANIZER_FOLDER_ANNO: "PlacesOrganizer/OrganizerFolder", @@ -560,6 +597,74 @@ this.PlacesUIUtils = { return ""; }, + /** + * Check whether or not the given node represents a removable entry (either in + * history or in bookmarks). + * + * @param aNode + * a node, except the root node of a query. + * @return true if the aNode represents a removable entry, false otherwise. + */ + canUserRemove: function (aNode) { + let parentNode = aNode.parent; + if (!parentNode) + throw new Error("canUserRemove doesn't accept root nodes"); + + // If it's not a bookmark, we can remove it unless it's a child of a + // livemark. + if (aNode.itemId == -1) { + // Rather than executing a db query, checking the existence of the feedURI + // annotation, detect livemark children by the fact that they are the only + // direct non-bookmark children of bookmark folders. + return !PlacesUtils.nodeIsFolder(parentNode); + } + + // Otherwise it has to be a child of an editable folder. + return !this.isContentsReadOnly(parentNode); + }, + + /** + * DO NOT USE THIS API IN ADDONS. IT IS VERY LIKELY TO CHANGE WHEN THE SWITCH + * TO GUIDS IS COMPLETE (BUG 1071511). + * + * Check whether or not the given node or item-id points to a folder which + * should not be modified by the user (i.e. its children should be unremovable + * and unmovable, new children should be disallowed, etc). + * These semantics are not inherited, meaning that read-only folder may + * contain editable items (for instance, the places root is read-only, but all + * of its direct children aren't). + * + * You should only pass folder item ids or folder nodes for aNodeOrItemId. + * While this is only enforced for the node case (if an item id of a separator + * or a bookmark is passed, false is returned), it's considered the caller's + * job to ensure that it checks a folder. + * Also note that folder-shortcuts should only be passed as result nodes. + * Otherwise they are just treated as bookmarks (i.e. false is returned). + * + * @param aNodeOrItemId + * any item id or result node. + * @throws if aNodeOrItemId is neither an item id nor a folder result node. + * @note livemark "folders" are considered read-only (but see bug 1072833). + * @return true if aItemId points to a read-only folder, false otherwise. + */ + isContentsReadOnly: function (aNodeOrItemId) { + let itemId; + if (typeof(aNodeOrItemId) == "number") { + itemId = aNodeOrItemId; + } + else if (PlacesUtils.nodeIsFolder(aNodeOrItemId)) { + itemId = PlacesUtils.getConcreteItemId(aNodeOrItemId); + } + else { + throw new Error("invalid value for aNodeOrItemId"); + } + + return itemId == this.leftPaneFolderId || + itemId == this.allBookmarksFolderId || + itemId == PlacesUtils.placesRootId || + IsLivemark(itemId); + }, + /** * Gives the user a chance to cancel loading lots of tabs at once */ @@ -962,8 +1067,6 @@ this.PlacesUIUtils = { // We should never backup this, since it changes between profiles. as.setItemAnnotation(folderId, PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO, 1, 0, as.EXPIRE_NEVER); - // Disallow manipulating this folder within the organizer UI. - bs.setFolderReadonly(folderId, true); if (aIsRoot) { // Mark as special left pane root. diff --git a/browser/components/places/content/browserPlacesViews.js b/browser/components/places/content/browserPlacesViews.js index 385c16d54824..e93d60e5d2cc 100644 --- a/browser/components/places/content/browserPlacesViews.js +++ b/browser/components/places/content/browserPlacesViews.js @@ -1376,8 +1376,8 @@ PlacesToolbar.prototype = { elt.localName != "menupopup") { let eltRect = elt.getBoundingClientRect(); let eltIndex = Array.indexOf(this._rootElt.childNodes, elt); - if (PlacesUtils.nodeIsFolder(elt._placesNode) && - !PlacesUtils.nodeIsReadOnly(elt._placesNode)) { + if (PlacesUIUtils.nodeIsFolder(elt._placesNode) && + !PlacesUIUtils.isContentsReadOnly(elt._placesNode)) { // This is a folder. // If we are in the middle of it, drop inside it. // Otherwise, drop before it, with regards to RTL mode. diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js index 43ec8b11f22c..6d76e54f8f44 100644 --- a/browser/components/places/content/controller.js +++ b/browser/components/places/content/controller.js @@ -200,7 +200,7 @@ PlacesController.prototype = { var selectedNode = this._view.selectedNode; return selectedNode && PlacesUtils.nodeIsFolder(selectedNode) && - !PlacesUtils.nodeIsReadOnly(selectedNode) && + !PlacesUIUtils.isContentsReadOnly(selectedNode) && this._view.result.sortingMode == Ci.nsINavHistoryQueryOptions.SORT_BY_NONE; case "placesCmd_createBookmark": @@ -330,21 +330,7 @@ PlacesController.prototype = { if (nodes[i] == root) return false; - if (PlacesUtils.nodeIsFolder(nodes[i]) && - !PlacesControllerDragHelper.canMoveNode(nodes[i])) - return false; - - // We don't call nodeIsReadOnly here, because nodeIsReadOnly means that - // a node has children that cannot be edited, reordered or removed. Here, - // we don't care if a node's children can't be reordered or edited, just - // that they're removable. All history results have removable children - // (based on the principle that any URL in the history table should be - // removable), but some special bookmark folders may have non-removable - // children, e.g. live bookmark folder children. It doesn't make sense - // to delete a child of a live bookmark folder, since when the folder - // refreshes, the child will return. - var parent = nodes[i].parent || root; - if (PlacesUtils.isReadonlyFolder(parent)) + if (!PlacesUIUtils.canUserRemove(nodes[i])) return false; } } @@ -1560,10 +1546,9 @@ let PlacesControllerDragHelper = { canMoveUnwrappedNode: function (aUnwrappedNode) { return aUnwrappedNode.id > 0 && !PlacesUtils.isRootItem(aUnwrappedNode.id) && - aUnwrappedNode.parent != PlacesUtils.placesRootId && + !PlacesUIUtils.isContentsReadOnly(aUnwrappedNode.parent) || aUnwrappedNode.parent != PlacesUtils.tagsFolderId && - aUnwrappedNode.grandParentId != PlacesUtils.tagsFolderId && - !aUnwrappedNode.parentReadOnly; + aUnwrappedNode.grandParentId != PlacesUtils.tagsFolderId; }, /** @@ -1575,58 +1560,17 @@ let PlacesControllerDragHelper = { */ canMoveNode: function PCDH_canMoveNode(aNode) { - // Can't move query root. - if (!aNode.parent) + // Only bookmark items are movable. + if (aNode.itemId == -1) return false; - let parentId = PlacesUtils.getConcreteItemId(aNode.parent); - let concreteId = PlacesUtils.getConcreteItemId(aNode); - - // Can't move children of tag containers. - if (PlacesUtils.nodeIsTagQuery(aNode.parent)) - return false; - - // Can't move children of read-only containers. - if (PlacesUtils.nodeIsReadOnly(aNode.parent)) - return false; - - // Check for special folders, etc. - if (PlacesUtils.nodeIsContainer(aNode) && - !this.canMoveContainer(aNode.itemId, parentId)) - return false; - - return true; - }, - - /** - * Determines if a container node can be moved. - * - * @param aId - * A bookmark folder id. - * @param [optional] aParentId - * The parent id of the folder. - * @return True if the container can be moved to the target. - */ - canMoveContainer: - function PCDH_canMoveContainer(aId, aParentId) { - if (aId == -1) - return false; - - // Disallow moving of roots and special folders. - const ROOTS = [PlacesUtils.placesRootId, PlacesUtils.bookmarksMenuFolderId, - PlacesUtils.tagsFolderId, PlacesUtils.unfiledBookmarksFolderId, - PlacesUtils.toolbarFolderId]; - if (ROOTS.indexOf(aId) != -1) - return false; - - // Get parent id if necessary. - if (aParentId == null || aParentId == -1) - aParentId = PlacesUtils.bookmarks.getFolderIdForItem(aId); - - if (PlacesUtils.bookmarks.getFolderReadonly(aParentId)) - return false; - - return true; + // Once tags and bookmarked are divorced, the tag-query check should be + // removed. + let parentNode = aNode.parent; + return parentNode != null && + !(PlacesUtils.nodeIsFolder(parentNode) && + PlacesUIUtils.isContentsReadOnly(parentNode)) && + !PlacesUtils.nodeIsTagQuery(parentNode); }, /** @@ -1726,12 +1670,10 @@ let PlacesControllerDragHelper = { */ disallowInsertion: function(aContainer) { NS_ASSERT(aContainer, "empty container"); - // Allow dropping into Tag containers. - if (PlacesUtils.nodeIsTagQuery(aContainer)) - return false; - // Disallow insertion of items under readonly folders. - return (!PlacesUtils.nodeIsFolder(aContainer) || - PlacesUtils.nodeIsReadOnly(aContainer)); + // Allow dropping into Tag containers and editable folders. + return !PlacesUtils.nodeIsTagQuery(aContainer) && + (!PlacesUtils.nodeIsFolder(aContainer) || + PlacesUIUtils.isContentsReadOnly(aContainer)); } }; diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml index 02598a071f86..6ab30c474b80 100644 --- a/browser/components/places/content/menu.xml +++ b/browser/components/places/content/menu.xml @@ -106,9 +106,9 @@ let tagName = PlacesUtils.nodeIsTagQuery(elt._placesNode) ? elt._placesNode.title : null; - if ((PlacesUtils.nodeIsFolder(elt._placesNode) || - PlacesUtils.nodeIsTagQuery(elt._placesNode)) && - !PlacesUtils.nodeIsReadOnly(elt._placesNode)) { + if ((PlacesUtils.nodeIsFolder(elt._placesNode) && + !PlacesUIUtils.isContentsReadOnly(elt._placesNode) || + PlacesUtils.nodeIsTagQuery(elt._placesNode)) { // This is a folder or a tag container. if (eventY - eltY < eltHeight * 0.20) { // If mouse is in the top part of the element, drop above folder. diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js index abe4805ab3b8..dd4f4f815c23 100644 --- a/browser/components/places/content/treeView.js +++ b/browser/components/places/content/treeView.js @@ -1649,23 +1649,39 @@ PlacesTreeView.prototype = { if (aColumn.index != 0) return false; - // Only bookmark-nodes are editable, and those are never built lazily let node = this._rows[aRow]; - if (!node || node.itemId == -1) + if (!node) { + Cu.reportError("isEditable called for an unbuilt row."); + return false; + } + let itemId = node.itemId; + + // Only bookmark-nodes are editable. Fortunately, this checks also takes + // care of livemark children. + if (itemId == -1) return false; - // The following items are never editable: - // * Read-only items. + // 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 - if (PlacesUtils.nodeIsReadOnly(node) || - PlacesUtils.nodeIsSeparator(node)) + // + // 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)) return false; - if (PlacesUtils.nodeIsFolder(node)) { - let itemId = PlacesUtils.getConcreteItemId(node); - if (PlacesUtils.isRootItem(itemId)) - return false; + let parentId = node.parent.itemId; + if (parentId == PlacesUIUtils.leftPaneFolderId || + parentId == PlacesUIUtils.allBallBookmarksFolderId) { + // 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 + // descendants of the left pane folder. + return false; } return true; diff --git a/browser/components/places/tests/browser/browser_423515.js b/browser/components/places/tests/browser/browser_423515.js index 6be928881e66..c3b4da4339d0 100644 --- a/browser/components/places/tests/browser/browser_423515.js +++ b/browser/components/places/tests/browser/browser_423515.js @@ -27,8 +27,6 @@ function test() { validate: function() { is(rootNode.childCount, 1, "populate added data to the test root"); - is(PlacesControllerDragHelper.canMoveContainer(this.id), - true, "can move regular folder id"); is(PlacesControllerDragHelper.canMoveNode(rootNode.getChild(0)), true, "can move regular folder node"); } @@ -57,9 +55,6 @@ function test() { var concreteId = PlacesUtils.getConcreteItemId(shortcutNode); is(concreteId, folderNode.itemId, "shortcut node id and concrete id match"); - is(PlacesControllerDragHelper.canMoveContainer(this.shortcutId), - true, "can move folder shortcut id"); - is(PlacesControllerDragHelper.canMoveNode(shortcutNode), true, "can move folder shortcut node"); } @@ -83,9 +78,6 @@ function test() { var queryNode = rootNode.getChild(1); is(queryNode.itemId, this.queryId, "query id and query node item id match"); - is(PlacesControllerDragHelper.canMoveContainer(this.queryId), - true, "can move query id"); - is(PlacesControllerDragHelper.canMoveNode(queryNode), true, "can move query node"); } @@ -127,9 +119,6 @@ function test() { for (var i = 0; i < this.folders.length; i++) { var id = this.folders[i]; - is(PlacesControllerDragHelper.canMoveContainer(id), - false, "shouldn't be able to move special folder id"); - var node = getRootChildNode(id); isnot(node, null, "Node found"); is(PlacesControllerDragHelper.canMoveNode(node), @@ -140,10 +129,6 @@ function test() { is(shortcutNode.itemId, shortcutId, "shortcut id and shortcut node item id match"); - dump("can move shortcut id?\n"); - is(PlacesControllerDragHelper.canMoveContainer(shortcutId), - true, "should be able to move special folder shortcut id"); - dump("can move shortcut node?\n"); is(PlacesControllerDragHelper.canMoveNode(shortcutNode), true, "should be able to move special folder shortcut node"); @@ -169,46 +154,13 @@ function test() { is(tagsNode.childCount, 1, "has new tag"); var tagNode = tagsNode.getChild(0); - + is(PlacesControllerDragHelper.canMoveNode(tagNode), false, "should not be able to move tag container node"); - tagsNode.containerOpen = false; } }); - // test that any child of a read-only node cannot be moved - tests.push({ - populate: function() { - this.id = - PlacesUtils.bookmarks.createFolder(rootId, "foo", IDX); - PlacesUtils.bookmarks.createFolder(this.id, "bar", IDX); - PlacesUtils.bookmarks.setFolderReadonly(this.id, true); - }, - validate: function() { - is(rootNode.childCount, 1, - "populate added data to the test root"); - var readOnlyFolder = rootNode.getChild(0); - - // test that we can move the read-only folder - is(PlacesControllerDragHelper.canMoveContainer(this.id), - true, "can move read-only folder id"); - is(PlacesControllerDragHelper.canMoveNode(readOnlyFolder), - true, "can move read-only folder node"); - - // test that we cannot move the child of a read-only folder - readOnlyFolder.QueryInterface(Ci.nsINavHistoryContainerResultNode); - readOnlyFolder.containerOpen = true; - var childFolder = readOnlyFolder.getChild(0); - - is(PlacesControllerDragHelper.canMoveContainer(childFolder.itemId), - false, "cannot move a child of a read-only folder"); - is(PlacesControllerDragHelper.canMoveNode(childFolder), - false, "cannot move a child node of a read-only folder node"); - readOnlyFolder.containerOpen = false; - } - }); - tests.forEach(function(aTest) { PlacesUtils.bookmarks.removeFolderChildren(rootId); aTest.populate(); diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index bf34c2c4695e..aa2bf2c7175e 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -206,35 +206,8 @@ this.PlacesUtils = { } }, - /** - * Cache array of read-only item IDs. - * - * The first time this property is called: - * - the cache is filled with all ids with the RO annotation - * - an annotation observer is added - * - a shutdown observer is added - * - * When the annotation observer detects annotations added or - * removed that are the RO annotation name, it adds/removes - * the ids from the cache. - * - * At shutdown, the annotation and shutdown observers are removed. - */ - get _readOnly() { - // Add annotations observer. - this.annotations.addObserver(this, false); - this.registerShutdownFunction(function () { - this.annotations.removeObserver(this); - }); - - var readOnly = this.annotations.getItemsWithAnnotation(this.READ_ONLY_ANNO); - this.__defineGetter__("_readOnly", function() readOnly); - return this._readOnly; - }, - QueryInterface: XPCOMUtils.generateQI([ - Ci.nsIAnnotationObserver - , Ci.nsIObserver + Ci.nsIObserver , Ci.nsITransactionListener ]), @@ -273,24 +246,6 @@ this.PlacesUtils = { } }, - ////////////////////////////////////////////////////////////////////////////// - //// nsIAnnotationObserver - - onItemAnnotationSet: function PU_onItemAnnotationSet(aItemId, aAnnotationName) - { - if (aAnnotationName == this.READ_ONLY_ANNO && - this._readOnly.indexOf(aItemId) == -1) - this._readOnly.push(aItemId); - }, - - onItemAnnotationRemoved: - function PU_onItemAnnotationRemoved(aItemId, aAnnotationName) - { - var index = this._readOnly.indexOf(aItemId); - if (aAnnotationName == this.READ_ONLY_ANNO && index > -1) - delete this._readOnly[index]; - }, - onPageAnnotationSet: function() {}, onPageAnnotationRemoved: function() {}, @@ -340,27 +295,6 @@ this.PlacesUtils = { willMerge: function PU_willMerge() {}, didMerge: function PU_didMerge() {}, - - /** - * Determines if a node is read only (children cannot be inserted, sometimes - * they cannot be removed depending on the circumstance) - * @param aNode - * A result node - * @returns true if the node is readonly, false otherwise - */ - nodeIsReadOnly: function PU_nodeIsReadOnly(aNode) { - let itemId = aNode.itemId; - if (itemId != -1) { - return this._readOnly.indexOf(itemId) != -1; - } - - if (this.nodeIsQuery(aNode) && - asQuery(aNode).queryOptions.resultType != - Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS) - return aNode.childrenReadOnly; - return false; - }, - /** * Determines whether or not a ResultNode is a host container. * @param aNode @@ -431,17 +365,6 @@ this.PlacesUtils = { this.nodeIsHost(aNode)); }, - /** - * Determines whether or not a node is a readonly folder. - * @param aNode - * The node to test. - * @returns true if the node is a readonly folder. - */ - isReadonlyFolder: function(aNode) { - return this.nodeIsFolder(aNode) && - this._readOnly.indexOf(asQuery(aNode).folderItemId) != -1; - }, - /** * Gets the concrete item-id for the given node. Generally, this is just * node.itemId, but for folder-shortcuts that's node.folderItemId. @@ -1111,10 +1034,9 @@ this.PlacesUtils = { if (guid) { aJSNode.itemGuid = guid; var parent = aPlacesNode.parent; - if (parent) { + if (parent) aJSNode.parent = parent.itemId; - aJSNode.parentReadOnly = PlacesUtils.nodeIsReadOnly(parent); - } + var dateAdded = aPlacesNode.dateAdded; if (dateAdded) aJSNode.dateAdded = dateAdded; diff --git a/toolkit/components/places/nsINavBookmarksService.idl b/toolkit/components/places/nsINavBookmarksService.idl index e7fec3b7702a..8944a52751e2 100644 --- a/toolkit/components/places/nsINavBookmarksService.idl +++ b/toolkit/components/places/nsINavBookmarksService.idl @@ -223,7 +223,7 @@ interface nsINavBookmarkObserver : nsISupports * folders. A URI in history can be contained in one or more such folders. */ -[scriptable, uuid(A78EA368-E28E-462E-897A-26606D4DDCE6)] +[scriptable, uuid(4C309044-B6DA-4511-AF57-E8940DB00045)] interface nsINavBookmarksService : nsISupports { /** @@ -466,30 +466,6 @@ interface nsINavBookmarksService : nsISupports */ unsigned short getItemType(in long long aItemId); - /** - * Checks whether a folder is marked as read-only. - * If this is set to true, UI will not allow the user to add, remove, - * or reorder children in this folder. The default for all folders is false. - * Note: This does not restrict API calls, only UI actions. - * - * @param aItemId - * the item-id of the folder. - */ - boolean getFolderReadonly(in long long aItemId); - - /** - * Sets or unsets the readonly flag from a folder. - * If this is set to true, UI will not allow the user to add, remove, - * or reorder children in this folder. The default for all folders is false. - * Note: This does not restrict API calls, only UI actions. - * - * @param aFolder - * the item-id of the folder. - * @param aReadOnly - * the read-only state (boolean). - */ - void setFolderReadonly(in long long aFolder, in boolean aReadOnly); - /** * Returns true if the given URI is in any bookmark folder. If you want the * results to be redirect-aware, use getBookmarkedURIFor() diff --git a/toolkit/components/places/nsINavHistoryService.idl b/toolkit/components/places/nsINavHistoryService.idl index e80a9010e6f7..34c645e1d095 100644 --- a/toolkit/components/places/nsINavHistoryService.idl +++ b/toolkit/components/places/nsINavHistoryService.idl @@ -175,7 +175,7 @@ interface nsINavHistoryResultNode : nsISupports * Bookmark folders and places queries will be QueryResultNodes which extends * these items. */ -[scriptable, uuid(5bac9734-c0ff-44eb-8d19-da88462ff6da)] +[scriptable, uuid(3E9CC95F-0D93-45F1-894F-908EEB9866D7)] interface nsINavHistoryContainerResultNode : nsINavHistoryResultNode { @@ -256,15 +256,6 @@ interface nsINavHistoryContainerResultNode : nsINavHistoryResultNode in PRTime aTime, in long long aItemId, in boolean aRecursive); - - /** - * Returns false if this node's list of children can be modified - * (adding or removing children, or reordering children), or true if - * the UI should not allow the list of children to be modified. - * This is false for bookmark folder nodes unless setFolderReadOnly() has - * been called to override it, and true for non-folder nodes. - */ - readonly attribute boolean childrenReadOnly; }; @@ -275,7 +266,7 @@ interface nsINavHistoryContainerResultNode : nsINavHistoryResultNode * generated this node, this item will report it has no children and never try * to populate itself. */ -[scriptable, uuid(a4144c3e-8125-46d5-a719-831bec8095f4)] +[scriptable, uuid(91AC5E59-3F5C-4ACD-AB3B-325FC425A5A1)] interface nsINavHistoryQueryResultNode : nsINavHistoryContainerResultNode { /** @@ -1118,12 +1109,9 @@ interface nsINavHistoryQueryOptions : nsISupports attribute boolean excludeQueries; /** - * Set to true to exclude read-only folders from the query results. This is - * designed for cases where you want to give the user the option of filing - * something into a list of folders. It only affects cases where the actual - * folder result node would appear in its parent folder and filters it out. - * It doesn't affect the query at all, and doesn't affect more complex - * queries (such as "folders with annotation X"). + * DO NOT USE THIS API. IT'LL BE REMOVED IN BUG 1072833. + * + * Set to true to exclude live bookmarks from the query results. */ attribute boolean excludeReadOnlyFolders; diff --git a/toolkit/components/places/nsLivemarkService.js b/toolkit/components/places/nsLivemarkService.js index 3f3955d8be92..1141aadd15c9 100644 --- a/toolkit/components/places/nsLivemarkService.js +++ b/toolkit/components/places/nsLivemarkService.js @@ -547,7 +547,6 @@ function Livemark(aLivemarkInfo) aLivemarkInfo.title, aLivemarkInfo.index, aLivemarkInfo.guid); - PlacesUtils.bookmarks.setFolderReadonly(this.id, true); this.writeFeedURI(aLivemarkInfo.feedURI); if (aLivemarkInfo.siteURI) { this.writeSiteURI(aLivemarkInfo.siteURI); diff --git a/toolkit/components/places/nsNavBookmarks.cpp b/toolkit/components/places/nsNavBookmarks.cpp index 134385af383a..38a56122f74f 100644 --- a/toolkit/components/places/nsNavBookmarks.cpp +++ b/toolkit/components/places/nsNavBookmarks.cpp @@ -60,7 +60,7 @@ PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsNavBookmarks, gBookmarksService) #define BOOKMARKS_ANNO_PREFIX "bookmarks/" #define BOOKMARKS_TOOLBAR_FOLDER_ANNO NS_LITERAL_CSTRING(BOOKMARKS_ANNO_PREFIX "toolbarFolder") -#define READ_ONLY_ANNO NS_LITERAL_CSTRING("placesInternal/READ_ONLY") +#define FEED_URI_ANNO NS_LITERAL_CSTRING("livemark/feedURI") namespace { @@ -790,46 +790,18 @@ nsNavBookmarks::CreateFolder(int64_t aParent, const nsACString& aName, return NS_OK; } -NS_IMETHODIMP -nsNavBookmarks::GetFolderReadonly(int64_t aFolder, bool* aResult) +bool nsNavBookmarks::IsLivemark(int64_t aFolderId) { - NS_ENSURE_ARG_MIN(aFolder, 1); - NS_ENSURE_ARG_POINTER(aResult); - nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); - NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); - nsresult rv = annosvc->ItemHasAnnotation(aFolder, READ_ONLY_ANNO, aResult); - NS_ENSURE_SUCCESS(rv, rv); - return NS_OK; + NS_ENSURE_TRUE(annosvc, false); + bool isLivemark; + nsresult rv = annosvc->ItemHasAnnotation(aFolderId, + FEED_URI_ANNO, + &isLivemark); + NS_ENSURE_SUCCESS(rv, false); + return isLivemark; } - -NS_IMETHODIMP -nsNavBookmarks::SetFolderReadonly(int64_t aFolder, bool aReadOnly) -{ - NS_ENSURE_ARG_MIN(aFolder, 1); - - nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); - NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); - nsresult rv; - if (aReadOnly) { - rv = annosvc->SetItemAnnotationInt32(aFolder, READ_ONLY_ANNO, 1, 0, - nsAnnotationService::EXPIRE_NEVER); - NS_ENSURE_SUCCESS(rv, rv); - } - else { - bool hasAnno; - rv = annosvc->ItemHasAnnotation(aFolder, READ_ONLY_ANNO, &hasAnno); - NS_ENSURE_SUCCESS(rv, rv); - if (hasAnno) { - rv = annosvc->RemoveItemAnnotation(aFolder, READ_ONLY_ANNO); - NS_ENSURE_SUCCESS(rv, rv); - } - } - return NS_OK; -} - - nsresult nsNavBookmarks::CreateContainerWithID(int64_t aItemId, int64_t aParent, @@ -1866,11 +1838,10 @@ nsNavBookmarks::ProcessFolderNodeRow( } } else if (itemType == TYPE_FOLDER) { + // ExcludeReadOnlyFolders currently means "ExcludeLivemarks" (to be fixed in + // bug 1072833) if (aOptions->ExcludeReadOnlyFolders()) { - // If the folder is read-only, skip it. - bool readOnly = false; - GetFolderReadonly(id, &readOnly); - if (readOnly) + if (IsLivemark(id)) return NS_OK; } diff --git a/toolkit/components/places/nsNavBookmarks.h b/toolkit/components/places/nsNavBookmarks.h index a44eae3852bd..dd06e7d0c8a7 100644 --- a/toolkit/components/places/nsNavBookmarks.h +++ b/toolkit/components/places/nsNavBookmarks.h @@ -234,6 +234,15 @@ private: ~nsNavBookmarks(); + /** + * Checks whether or not aFolderId points to a live bookmark. + * + * @param aFolderId + * the item-id of the folder to check. + * @return true if aFolderId points to live bookmarks, false otherwise. + */ + bool IsLivemark(int64_t aFolderId); + /** * Locates the root items in the bookmarks folder hierarchy assigning folder * ids to the root properties that are exposed through the service interface. diff --git a/toolkit/components/places/nsNavHistoryResult.cpp b/toolkit/components/places/nsNavHistoryResult.cpp index b920d230950c..301cd3e1edee 100644 --- a/toolkit/components/places/nsNavHistoryResult.cpp +++ b/toolkit/components/places/nsNavHistoryResult.cpp @@ -322,13 +322,12 @@ NS_INTERFACE_MAP_END_INHERITING(nsNavHistoryResultNode) nsNavHistoryContainerResultNode::nsNavHistoryContainerResultNode( const nsACString& aURI, const nsACString& aTitle, - const nsACString& aIconURI, uint32_t aContainerType, bool aReadOnly, + const nsACString& aIconURI, uint32_t aContainerType, nsNavHistoryQueryOptions* aOptions) : nsNavHistoryResultNode(aURI, aTitle, 0, 0, aIconURI), mResult(nullptr), mContainerType(aContainerType), mExpanded(false), - mChildrenReadOnly(aReadOnly), mOptions(aOptions), mAsyncCanceledState(NOT_CANCELED) { @@ -337,13 +336,12 @@ nsNavHistoryContainerResultNode::nsNavHistoryContainerResultNode( nsNavHistoryContainerResultNode::nsNavHistoryContainerResultNode( const nsACString& aURI, const nsACString& aTitle, PRTime aTime, - const nsACString& aIconURI, uint32_t aContainerType, bool aReadOnly, + const nsACString& aIconURI, uint32_t aContainerType, nsNavHistoryQueryOptions* aOptions) : nsNavHistoryResultNode(aURI, aTitle, 0, aTime, aIconURI), mResult(nullptr), mContainerType(aContainerType), mExpanded(false), - mChildrenReadOnly(aReadOnly), mOptions(aOptions), mAsyncCanceledState(NOT_CANCELED) { @@ -1716,16 +1714,6 @@ nsNavHistoryContainerResultNode::FindNodeByDetails(const nsACString& aURIString, return NS_OK; } -/** - * @note Overridden for folders to query the bookmarks service directly. - */ -NS_IMETHODIMP -nsNavHistoryContainerResultNode::GetChildrenReadOnly(bool *aChildrenReadOnly) -{ - *aChildrenReadOnly = mChildrenReadOnly; - return NS_OK; -} - /** * HOW QUERY UPDATING WORKS * @@ -1753,7 +1741,7 @@ nsNavHistoryQueryResultNode::nsNavHistoryQueryResultNode( const nsACString& aQueryURI) : nsNavHistoryContainerResultNode(aQueryURI, aTitle, aIconURI, nsNavHistoryResultNode::RESULT_TYPE_QUERY, - true, nullptr), + nullptr), mLiveUpdate(QUERYUPDATE_COMPLEX_WITH_BOOKMARKS), mHasSearchTerms(false), mContentsValid(false), @@ -1767,7 +1755,7 @@ nsNavHistoryQueryResultNode::nsNavHistoryQueryResultNode( nsNavHistoryQueryOptions* aOptions) : nsNavHistoryContainerResultNode(EmptyCString(), aTitle, aIconURI, nsNavHistoryResultNode::RESULT_TYPE_QUERY, - true, aOptions), + aOptions), mQueries(aQueries), mContentsValid(false), mBatchChanges(0), @@ -1800,7 +1788,7 @@ nsNavHistoryQueryResultNode::nsNavHistoryQueryResultNode( nsNavHistoryQueryOptions* aOptions) : nsNavHistoryContainerResultNode(EmptyCString(), aTitle, aTime, aIconURI, nsNavHistoryResultNode::RESULT_TYPE_QUERY, - true, aOptions), + aOptions), mQueries(aQueries), mContentsValid(false), mBatchChanges(0), @@ -2988,7 +2976,7 @@ nsNavHistoryFolderResultNode::nsNavHistoryFolderResultNode( int64_t aFolderId) : nsNavHistoryContainerResultNode(EmptyCString(), aTitle, EmptyCString(), nsNavHistoryResultNode::RESULT_TYPE_FOLDER, - false, aOptions), + aOptions), mContentsValid(false), mQueryItemId(-1), mIsRegisteredFolderObserver(false) @@ -3090,25 +3078,6 @@ nsNavHistoryFolderResultNode::GetItemId(int64_t* aItemId) return NS_OK; } -/** - * Here, we override the getter and ignore the value stored in our object. - * The bookmarks service can tell us whether this folder should be read-only - * or not. - * - * It would be nice to put this code in the folder constructor, but the - * database was complaining. I believe it is because most folders are created - * while enumerating the bookmarks table and having a statement open, and doing - * another statement might make it unhappy in some cases. - */ -NS_IMETHODIMP -nsNavHistoryFolderResultNode::GetChildrenReadOnly(bool *aChildrenReadOnly) -{ - nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); - NS_ENSURE_TRUE(bookmarks, NS_ERROR_UNEXPECTED); - return bookmarks->GetFolderReadonly(mItemId, aChildrenReadOnly); -} - - NS_IMETHODIMP nsNavHistoryFolderResultNode::GetFolderItemId(int64_t* aItemId) { diff --git a/toolkit/components/places/nsNavHistoryResult.h b/toolkit/components/places/nsNavHistoryResult.h index 25baafe30e5a..e28a6fad7318 100644 --- a/toolkit/components/places/nsNavHistoryResult.h +++ b/toolkit/components/places/nsNavHistoryResult.h @@ -401,7 +401,7 @@ NS_DEFINE_STATIC_IID_ACCESSOR(nsNavHistoryResultNode, NS_NAVHISTORYRESULTNODE_II // derived classes each provide their own implementation of has children and // forward the rest to us using this macro -#define NS_FORWARD_CONTAINERNODE_EXCEPT_HASCHILDREN_AND_READONLY \ +#define NS_FORWARD_CONTAINERNODE_EXCEPT_HASCHILDREN \ NS_IMETHOD GetState(uint16_t* _state) \ { return nsNavHistoryContainerResultNode::GetState(_state); } \ NS_IMETHOD GetContainerOpen(bool *aContainerOpen) \ @@ -430,12 +430,12 @@ public: nsNavHistoryContainerResultNode( const nsACString& aURI, const nsACString& aTitle, const nsACString& aIconURI, uint32_t aContainerType, - bool aReadOnly, nsNavHistoryQueryOptions* aOptions); + nsNavHistoryQueryOptions* aOptions); nsNavHistoryContainerResultNode( const nsACString& aURI, const nsACString& aTitle, PRTime aTime, const nsACString& aIconURI, uint32_t aContainerType, - bool aReadOnly, nsNavHistoryQueryOptions* aOptions); + nsNavHistoryQueryOptions* aOptions); virtual nsresult Refresh(); @@ -479,8 +479,6 @@ public: // Filled in by the result type generator in nsNavHistory. nsCOMArray mChildren; - bool mChildrenReadOnly; - nsCOMPtr mOptions; void FillStats(); @@ -644,10 +642,8 @@ public: NS_IMETHOD GetType(uint32_t* type) { *type = nsNavHistoryResultNode::RESULT_TYPE_QUERY; return NS_OK; } NS_IMETHOD GetUri(nsACString& aURI); // does special lazy creation - NS_FORWARD_CONTAINERNODE_EXCEPT_HASCHILDREN_AND_READONLY + NS_FORWARD_CONTAINERNODE_EXCEPT_HASCHILDREN NS_IMETHOD GetHasChildren(bool* aHasChildren); - NS_IMETHOD GetChildrenReadOnly(bool *aChildrenReadOnly) - { return nsNavHistoryContainerResultNode::GetChildrenReadOnly(aChildrenReadOnly); } NS_DECL_NSINAVHISTORYQUERYRESULTNODE bool CanExpand(); @@ -725,9 +721,8 @@ public: return NS_OK; } NS_IMETHOD GetUri(nsACString& aURI); - NS_FORWARD_CONTAINERNODE_EXCEPT_HASCHILDREN_AND_READONLY + NS_FORWARD_CONTAINERNODE_EXCEPT_HASCHILDREN NS_IMETHOD GetHasChildren(bool* aHasChildren); - NS_IMETHOD GetChildrenReadOnly(bool *aChildrenReadOnly); NS_IMETHOD GetItemId(int64_t *aItemId); NS_DECL_NSINAVHISTORYQUERYRESULTNODE diff --git a/toolkit/components/places/tests/queries/head_queries.js b/toolkit/components/places/tests/queries/head_queries.js index 932734d06794..c7023333f80e 100644 --- a/toolkit/components/places/tests/queries/head_queries.js +++ b/toolkit/components/places/tests/queries/head_queries.js @@ -152,8 +152,6 @@ function task_populateDB(aArray) let folderId = PlacesUtils.bookmarks.createFolder(qdata.parentFolder, qdata.title, qdata.index); - if (qdata.readOnly) - PlacesUtils.bookmarks.setFolderReadonly(folderId, true); } if (qdata.isLivemark) { @@ -246,7 +244,6 @@ function queryData(obj) { this.dateAdded = obj.dateAdded ? obj.dateAdded : today; this.keyword = obj.keyword ? obj.keyword : ""; this.visitCount = obj.visitCount ? obj.visitCount : 0; - this.readOnly = obj.readOnly ? obj.readOnly : false; this.isSeparator = obj.hasOwnProperty("isSeparator") && obj.isSeparator; // And now, the attribute for whether or not this object should appear in the diff --git a/toolkit/components/places/tests/queries/test_async.js b/toolkit/components/places/tests/queries/test_async.js index 643068f5299b..72c64ba39243 100644 --- a/toolkit/components/places/tests/queries/test_async.js +++ b/toolkit/components/places/tests/queries/test_async.js @@ -313,7 +313,6 @@ let DataHelper = { case "folder": return { isFolder: true, - readOnly: false, parentFolder: dat.parent, index: PlacesUtils.bookmarks.DEFAULT_INDEX, title: dat.title, diff --git a/toolkit/components/places/tests/queries/test_excludeReadOnlyFolders.js b/toolkit/components/places/tests/queries/test_excludeReadOnlyFolders.js deleted file mode 100644 index 787eb6b39540..000000000000 --- a/toolkit/components/places/tests/queries/test_excludeReadOnlyFolders.js +++ /dev/null @@ -1,48 +0,0 @@ -/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ -/* vim:set ts=2 sw=2 sts=2 et: */ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -// The test data for our database, note that the ordering of the results that -// will be returned by the query (the isInQuery: true objects) is IMPORTANT. -// see compareArrayToResult in head_queries.js for more info. -var testData = [ - // Normal folder - { isInQuery: true, isFolder: true, title: "Folder 1", - parentFolder: PlacesUtils.toolbarFolderId }, - - // Read only folder - { isInQuery: false, isFolder: true, title: "Folder 2 RO", - parentFolder: PlacesUtils.toolbarFolderId, readOnly: true } -]; - -function run_test() -{ - run_next_test(); -} - -add_task(function test_excludeReadOnlyFolders() -{ - yield task_populateDB(testData); - - var query = PlacesUtils.history.getNewQuery(); - query.setFolders([PlacesUtils.toolbarFolderId], 1); - - // Options - var options = PlacesUtils.history.getNewQueryOptions(); - options.excludeQueries = true; - options.excludeReadOnlyFolders = true; - - // Results - var result = PlacesUtils.history.executeQuery(query, options); - var root = result.root; - root.containerOpen = true; - - displayResultSet(root); - // The readonly folder should not be in our result set. - do_check_eq(1, root.childCount); - do_check_eq("Folder 1", root.getChild(0).title); - - root.containerOpen = false; -}); diff --git a/toolkit/components/places/tests/queries/test_querySerialization.js b/toolkit/components/places/tests/queries/test_querySerialization.js index 541c0ceec6e3..8aab959f656a 100644 --- a/toolkit/components/places/tests/queries/test_querySerialization.js +++ b/toolkit/components/places/tests/queries/test_querySerialization.js @@ -414,17 +414,6 @@ const queryOptionSwitches = [ } ] }, - // excludeReadOnlyFolders - { - property: "excludeReadOnlyFolders", - desc: "nsINavHistoryQueryOptions.excludeReadOnlyFolders", - matches: simplePropertyMatches, - runs: [ - function (aQuery, aQueryOptions) { - aQueryOptions.excludeReadOnlyFolders = true; - } - ] - }, // expandQueries { property: "expandQueries", diff --git a/toolkit/components/places/tests/queries/xpcshell.ini b/toolkit/components/places/tests/queries/xpcshell.ini index 704ae46b2850..f4a6bf9fae4a 100644 --- a/toolkit/components/places/tests/queries/xpcshell.ini +++ b/toolkit/components/places/tests/queries/xpcshell.ini @@ -7,7 +7,6 @@ tail = [test_abstime-annotation-uri.js] [test_async.js] [test_containersQueries_sorting.js] -[test_excludeReadOnlyFolders.js] [test_history_queries_tags_liveUpdate.js] [test_history_queries_titles_liveUpdate.js] [test_onlyBookmarked.js]