author | Marco Bonardo <mbonardo@mozilla.com> |
Thu, 22 Mar 2018 14:13:07 +0100 | |
changeset 410770 | 147023fe220ca9b4235aa2900972a4affc79bab3 |
parent 410769 | 4b1ce6bf8518e00a5c5125d905173d28a11e72e2 |
child 410771 | fa72a782c21cb6b46204aab9c537579df09db81c |
push id | 33739 |
push user | nbeleuzu@mozilla.com |
push date | Fri, 30 Mar 2018 21:47:45 +0000 |
treeherder | mozilla-central@10c662d8416e [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | standard8 |
bugs | 1421664 |
milestone | 61.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
|
--- a/browser/components/places/content/browserPlacesViews.js +++ b/browser/components/places/content/browserPlacesViews.js @@ -200,20 +200,17 @@ PlacesViewBase.prototype = { // the insertion point is inside the folder, at the end. container = selectedNode; orientation = Ci.nsITreeView.DROP_ON; } else { // In all other cases the insertion point is before that node. container = selectedNode.parent; index = container.getChildIndex(selectedNode); if (PlacesUtils.nodeIsTagQuery(container)) { - tagName = container.title; - // TODO (Bug 1160193): properly support dropping on a tag root. - if (!tagName) - return null; + tagName = PlacesUtils.asQuery(container).query.tags[0]; } } } if (this.controller.disallowInsertion(container)) return null; return new PlacesInsertionPoint({
--- a/browser/components/places/content/controller.js +++ b/browser/components/places/content/controller.js @@ -170,17 +170,18 @@ PlacesController.prototype = { return this._canInsert(); case "placesCmd_new:separator": return this._canInsert() && !PlacesUtils.asQuery(this._view.result.root).queryOptions.excludeItems && this._view.result.sortingMode == Ci.nsINavHistoryQueryOptions.SORT_BY_NONE; case "placesCmd_show:info": { let selectedNode = this._view.selectedNode; - return selectedNode && PlacesUtils.getConcreteItemId(selectedNode) != -1; + return selectedNode && (PlacesUtils.nodeIsTagQuery(selectedNode) || + PlacesUtils.getConcreteItemId(selectedNode) != -1); } case "placesCmd_reload": { // Livemark containers let selectedNode = this._view.selectedNode; return selectedNode && this.hasCachedLivemarkInfo(selectedNode); } case "placesCmd_sortBy:name": { let selectedNode = this._view.selectedNode; @@ -802,46 +803,55 @@ PlacesController.prototype = { if (this._shouldSkipNode(node, removedFolders)) continue; totalItems++; if (PlacesUtils.nodeIsTagQuery(node.parent)) { // This is a uri node inside a tag container. It needs a special // untag transaction. - let tag = node.parent.title; + let tag = node.parent.title || ""; if (!tag) { - // TODO: Bug 1432405 Try using getConcreteItemGuid. - let tagItemId = PlacesUtils.getConcreteItemId(node.parent); - let tagGuid = await PlacesUtils.promiseItemGuid(tagItemId); - tag = (await PlacesUtils.bookmarks.fetch(tagGuid)).title; + // The parent may be the root node, that doesn't have a title. + // Until we fix bug 1293445, we have two ways to get tags: + if (node.parent.queryOptions.resultType == + Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS) { + // Get the tag using the bookmarks API. + let tagItemId = PlacesUtils.getConcreteItemId(node.parent); + let tagGuid = await PlacesUtils.promiseItemGuid(tagItemId); + tag = (await PlacesUtils.bookmarks.fetch(tagGuid)).title; + } else { + // Extract the tag from the query itself. + tag = node.parent.query.tags[0]; + } } transactions.push(PlacesTransactions.Untag({ urls: [node.uri], tag })); - } else if (PlacesUtils.nodeIsTagQuery(node) && node.parent && - PlacesUtils.nodeIsQuery(node.parent) && - PlacesUtils.asQuery(node.parent).queryOptions.resultType == - Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) { + } else if (PlacesUtils.nodeIsTagQuery(node) && + node.parent && + PlacesUtils.nodeIsQuery(node.parent) && + PlacesUtils.asQuery(node.parent).queryOptions.resultType == + Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) { // This is a tag container. // Untag all URIs tagged with this tag only if the tag container is // child of the "Tags" query in the library, in all other places we // must only remove the query node. let tag = node.title; let URIs = PlacesUtils.tagging.getURIsForTag(tag); transactions.push(PlacesTransactions.Untag({ tag, urls: URIs })); } else if (PlacesUtils.nodeIsURI(node) && - PlacesUtils.nodeIsQuery(node.parent) && - PlacesUtils.asQuery(node.parent).queryOptions.queryType == - Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) { + PlacesUtils.nodeIsQuery(node.parent) && + PlacesUtils.asQuery(node.parent).queryOptions.queryType == + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) { // This is a uri node inside an history query. PlacesUtils.history.remove(node.uri).catch(Cu.reportError); // History deletes are not undoable, so we don't have a transaction. } else if (node.itemId == -1 && - PlacesUtils.nodeIsQuery(node) && - PlacesUtils.asQuery(node).queryOptions.queryType == - Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) { + PlacesUtils.nodeIsQuery(node) && + PlacesUtils.asQuery(node).queryOptions.queryType == + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) { // This is a dynamically generated history query, like queries // grouped by site, time or both. Dynamically generated queries don't // have an itemId even if they are descendants of a bookmark. this._removeHistoryContainer(node); // History deletes are not undoable, so we don't have a transaction. } else { // This is a common bookmark item. if (PlacesUtils.nodeIsFolder(node)) {
--- a/browser/components/places/content/editBookmark.js +++ b/browser/components/places/content/editBookmark.js @@ -28,23 +28,22 @@ var gEditItemOverlay = { // folders), when the pane shows for them it's opened in read-only mode, showing the // properties of the target folder. let itemId = node ? node.itemId : -1; let itemGuid = node ? PlacesUtils.getConcreteItemGuid(node) : null; let isItem = itemId != -1; let isFolderShortcut = isItem && node.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT; let isTag = node && PlacesUtils.nodeIsTagQuery(node); + let tag = null; if (isTag) { - itemId = PlacesUtils.getConcreteItemId(node); - // For now we don't have access to the item guid synchronously for tags, - // so we'll need to fetch it later. + tag = PlacesUtils.asQuery(node).query.tags.length == 1 ? node.query.tags[0] : node.title; } let isURI = node && PlacesUtils.nodeIsURI(node); - let uri = isURI ? Services.io.newURI(node.uri) : null; + let uri = isURI || isTag ? Services.io.newURI(node.uri) : null; let title = node ? node.title : null; let isBookmark = isItem && isURI; let bulkTagging = !node; let uris = bulkTagging ? aInitInfo.uris : null; let visibleRows = new Set(); let isParentReadOnly = false; let postData = aInitInfo.postData; let parentId = -1; @@ -63,26 +62,26 @@ var gEditItemOverlay = { let focusedElement = aInitInfo.focusedElement; let onPanelReady = aInitInfo.onPanelReady; return this._paneInfo = { itemId, itemGuid, parentId, parentGuid, isItem, isURI, uri, title, isBookmark, isFolderShortcut, isParentReadOnly, bulkTagging, uris, visibleRows, postData, isTag, focusedElement, - onPanelReady }; + onPanelReady, tag }; }, get initialized() { return this._paneInfo != null; }, // Backwards-compatibility getters get itemId() { - if (!this.initialized || this._paneInfo.bulkTagging) + if (!this.initialized || this._paneInfo.isTag || this._paneInfo.bulkTagging) return -1; return this._paneInfo.itemId; }, get uri() { if (!this.initialized) return null; if (this._paneInfo.bulkTagging) @@ -115,17 +114,17 @@ var gEditItemOverlay = { // a certain item _firstEditedField: "", _initNamePicker() { if (this._paneInfo.bulkTagging) throw new Error("_initNamePicker called unexpectedly"); // title may by null, which, for us, is the same as an empty string. - this._initTextField(this._namePicker, this._paneInfo.title || ""); + this._initTextField(this._namePicker, this._paneInfo.title || this._paneInfo.tag || ""); }, _initLocationField() { if (!this._paneInfo.isURI) throw new Error("_initLocationField called unexpectedly"); this._initTextField(this._locationField, this._paneInfo.uri.spec); }, @@ -602,28 +601,38 @@ var gEditItemOverlay = { Services.prefs.setCharPref("browser.bookmarks.editDialog.firstEditField", aNewField); }, async onNamePickerChange() { if (this.readOnly || !(this._paneInfo.isItem || this._paneInfo.isTag)) return; // Here we update either the item title or its cached static title - let newTitle = this._namePicker.value; - if (!newTitle && this._paneInfo.isTag) { - // We don't allow setting an empty title for a tag, restore the old one. - this._initNamePicker(); - } else { - this._mayUpdateFirstEditField("namePicker"); + if (this._paneInfo.isTag) { + let tag = this._namePicker.value; + if (!tag || tag.includes("&")) { + // We don't allow setting an empty title for a tag, restore the old one. + this._initNamePicker(); + return; + } + // Get all the bookmarks for the old tag, tag them with the new tag, and + // untag them from the old tag. + let oldTag = this._paneInfo.tag; + this._paneInfo.tag = tag; + let title = this._paneInfo.title; + if (title == oldTag) { + this._paneInfo.title = tag; + } + await PlacesTransactions.RenameTag({ oldTag, tag }).transact(); + return; + } - let guid = this._paneInfo.isTag - ? (await PlacesUtils.promiseItemGuid(this._paneInfo.itemId)) - : this._paneInfo.itemGuid; - await PlacesTransactions.EditTitle({ guid, title: newTitle }).transact(); - } + this._mayUpdateFirstEditField("namePicker"); + await PlacesTransactions.EditTitle({ guid: this._paneInfo.itemGuid, + title: this._namePicker.value }).transact(); }, onDescriptionFieldChange() { if (this.readOnly || !this._paneInfo.isItem) return; let description = this._element("descriptionField").value; if (description != PlacesUIUtils.getItemDescription(this._paneInfo.itemId)) { @@ -1015,18 +1024,18 @@ var gEditItemOverlay = { this._initTagsField(); // Any tags change should be reflected in the tags selector. if (this._element("tagsSelector")) { this._rebuildTagsSelectorList(); } } }, - _onItemTitleChange(aItemId, aNewTitle) { - if (aItemId == this._paneInfo.itemId) { + _onItemTitleChange(aItemId, aNewTitle, aGuid) { + if (aItemId == this._paneInfo.itemId || aGuid == this._paneInfo.itemGuid) { this._paneInfo.title = aNewTitle; this._initTextField(this._namePicker, aNewTitle); } else if (this._paneInfo.visibleRows.has("folderRow")) { // If the title of a folder which is listed within the folders // menulist has been changed, we need to update the label of its // representing element. let menupopup = this._folderMenuList.menupopup; for (let menuitem of menupopup.childNodes) { @@ -1051,17 +1060,17 @@ var gEditItemOverlay = { onItemChanged(aItemId, aProperty, aIsAnnotationProperty, aValue, aLastModified, aItemType, aParentId, aGuid) { if (aProperty == "tags" && this._paneInfo.visibleRows.has("tagsRow")) { this._onTagsChange(aGuid).catch(Cu.reportError); return; } if (aProperty == "title" && (this._paneInfo.isItem || this._paneInfo.isTag)) { // This also updates titles of folders in the folder menu list. - this._onItemTitleChange(aItemId, aValue); + this._onItemTitleChange(aItemId, aValue, aGuid); return; } if (!this._paneInfo.isItem || this._paneInfo.itemId != aItemId) { return; } switch (aProperty) { @@ -1120,16 +1129,15 @@ var gEditItemOverlay = { }, onItemRemoved() { }, onBeginUpdateBatch() { }, onEndUpdateBatch() { }, onItemVisited() { }, }; - for (let elt of ["folderMenuList", "folderTree", "namePicker", "locationField", "descriptionField", "keywordField", "tagsField", "loadInSidebarCheckbox"]) { let eltScoped = elt; XPCOMUtils.defineLazyGetter(gEditItemOverlay, `_${eltScoped}`, () => gEditItemOverlay._element(eltScoped)); }
--- a/browser/components/places/content/tree.xml +++ b/browser/components/places/content/tree.xml @@ -78,18 +78,18 @@ // preserve grouping var queryNode = PlacesUtils.asQuery(this.result.root); var options = queryNode.queryOptions.clone(); // Make sure we're getting uri results. // We do not yet support searching into grouped queries or into // tag containers, so we must fall to the default case. if (PlacesUtils.nodeIsHistoryContainer(queryNode) || + PlacesUtils.nodeIsTagQuery(queryNode) || options.resultType == options.RESULTS_AS_TAG_QUERY || - options.resultType == options.RESULTS_AS_TAG_CONTENTS || options.resultType == options.RESULTS_AS_ROOTS_QUERY) options.resultType = options.RESULTS_AS_URI; var query = PlacesUtils.history.getNewQuery(); query.searchTerms = filterString; if (folderRestrict) { query.setFolders(folderRestrict, folderRestrict.length); @@ -523,23 +523,18 @@ index = orientation == Ci.nsITreeView.DROP_BEFORE ? lsi : lsi + 1; } } } if (this.controller.disallowInsertion(container)) return null; - // TODO (Bug 1160193): properly support dropping on a tag root. - let tagName = null; - if (PlacesUtils.nodeIsTagQuery(container)) { - tagName = container.title; - if (!tagName) - return null; - } + let tagName = PlacesUtils.nodeIsTagQuery(container) ? + PlacesUtils.asQuery(container).query.tags[0] : null; return new PlacesInsertionPoint({ parentId: PlacesUtils.getConcreteItemId(container), parentGuid: PlacesUtils.getConcreteItemGuid(container), index, orientation, tagName, dropNearNode }); ]]></body> </method>
--- a/browser/components/places/content/treeView.js +++ b/browser/components/places/content/treeView.js @@ -1346,18 +1346,18 @@ PlacesTreeView.prototype = { // Flat-lists may ignore expandQueries and other query options when // they are asked to open a container. if (this._flatList) return true; // Treat non-expandable childless queries as non-containers, unless they // are tags. if (PlacesUtils.nodeIsQuery(node) && !PlacesUtils.nodeIsTagQuery(node)) { - PlacesUtils.asQuery(node); - return node.queryOptions.expandQueries || node.hasChildren; + return PlacesUtils.asQuery(node).queryOptions.expandQueries || + node.hasChildren; } return true; }, isContainerOpen: function PTV_isContainerOpen(aRow) { if (this._flatList) return false; @@ -1462,23 +1462,18 @@ PlacesTreeView.prototype = { index = orientation == Ci.nsITreeView.DROP_BEFORE ? lsi : lsi + 1; } } } if (this._controller.disallowInsertion(container)) return null; - // TODO (Bug 1160193): properly support dropping on a tag root. - let tagName = null; - if (PlacesUtils.nodeIsTagQuery(container)) { - tagName = container.title; - if (!tagName) - return null; - } + let tagName = PlacesUtils.nodeIsTagQuery(container) ? + PlacesUtils.asQuery(container).query.tags[0] : null; return new PlacesInsertionPoint({ parentId: PlacesUtils.getConcreteItemId(container), parentGuid: PlacesUtils.getConcreteItemGuid(container), index, orientation, tagName, dropNearNode }); },
--- a/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js +++ b/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js @@ -26,59 +26,64 @@ add_task(async function() { let fooTag = tagsContainer.getChild(0); let tagNode = fooTag; tree.selectNode(fooTag); Assert.equal(tagNode.title, "tag1", "tagNode title is correct"); Assert.ok(tree.controller.isCommandEnabled("placesCmd_show:info"), "'placesCmd_show:info' on current selected node is enabled"); - let promiseTitleResetNotification = PlacesTestUtils.waitForNotification( - "onItemChanged", (itemId, prop, isAnno, val) => prop == "title" && val == "tag1"); + let promiseTagResetNotification = PlacesTestUtils.waitForNotification( + "onItemChanged", (itemId, prop) => { + let tags = PlacesUtils.tagging.getTagsForURI(uri); + return prop == "tags" && tags.length == 1 && tags[0] == "tag1"; + }); await withBookmarksDialog( true, function openDialog() { tree.controller.doCommand("placesCmd_show:info"); }, async function test(dialogWin) { // Check that the dialog is not read-only. Assert.ok(!dialogWin.gEditItemOverlay.readOnly, "Dialog should not be read-only"); // Check that name picker is not read only let namepicker = dialogWin.document.getElementById("editBMPanel_namePicker"); Assert.ok(!namepicker.readOnly, "Name field should not be read-only"); Assert.equal(namepicker.value, "tag1", "Node title is correct"); - - let promiseTitleChangeNotification = PlacesTestUtils.waitForNotification( - "onItemChanged", (itemId, prop, isAnno, val) => prop == "title" && val == "tag2"); + let promiseTagChangeNotification = PlacesTestUtils.waitForNotification( + "onItemChanged", (itemId, prop) => { + let tags = PlacesUtils.tagging.getTagsForURI(uri); + return prop == "tags" && tags.length == 1 && tags[0] == "tag2"; + }); fillBookmarkTextField("editBMPanel_namePicker", "tag2", dialogWin); - await promiseTitleChangeNotification; + await promiseTagChangeNotification; - Assert.equal(namepicker.value, "tag2", "Node title has been properly edited"); + Assert.equal(namepicker.value, "tag2", "The title field has been changed"); // Check the shortcut's title. Assert.equal(tree.selectedNode.title, "tag2", "The node has the correct title"); // Try to set an empty title, it should restore the previous one. fillBookmarkTextField("editBMPanel_namePicker", "", dialogWin); - Assert.equal(namepicker.value, "tag2", "Title has not been changed"); + Assert.equal(namepicker.value, "tag2", "The title field has been changed"); Assert.equal(tree.selectedNode.title, "tag2", "The node has the correct title"); // Check the tags have been edited. let tags = PlacesUtils.tagging.getTagsForURI(uri); Assert.equal(tags.length, 1, "Found the right number of tags"); Assert.ok(tags.includes("tag2"), "Found the expected tag"); // Ensure that the addition really is finished before we hit cancel. await PlacesTestUtils.promiseAsyncUpdates(); } ); - await promiseTitleResetNotification; + await promiseTagResetNotification; // Check the tag change has been reverted. let tags = PlacesUtils.tagging.getTagsForURI(uri); Assert.equal(tags.length, 1, "Found the right number of tags"); Assert.deepEqual(tags, ["tag1"], "Found the expected tag"); });
--- a/browser/components/places/tests/browser/browser_drag_bookmarks_on_toolbar.js +++ b/browser/components/places/tests/browser/browser_drag_bookmarks_on_toolbar.js @@ -111,24 +111,22 @@ function synthesizeDragWithDirection(aEl startingPoint.y + yIncrement * 9, { type: "mousemove" }); return promise; } function getToolbarNodeForItemId(itemGuid) { var children = document.getElementById("PlacesToolbarItems").childNodes; - var node = null; - for (var i = 0; i < children.length; i++) { - if (itemGuid == children[i]._placesNode.bookmarkGuid) { - node = children[i]; - break; + for (let child of children) { + if (itemGuid == child._placesNode.bookmarkGuid) { + return child; } } - return node; + return null; } function getExpectedDataForPlacesNode(aNode) { var wrappedNode = []; var flavors = ["text/x-moz-place", "text/x-moz-url", "text/plain", "text/html"];
--- a/browser/components/places/tests/browser/browser_library_delete_bookmarks_in_tags.js +++ b/browser/components/places/tests/browser/browser_library_delete_bookmarks_in_tags.js @@ -31,18 +31,18 @@ add_task(async function test_tags() { // display in "logical" order of bm0 at the top, and bm2 at the bottom. children = children.reverse(); await PlacesUtils.bookmarks.insertTree({ guid: PlacesUtils.bookmarks.unfiledGuid, children, }); - for (let i = 0; i < uris.length; i++) { - PlacesUtils.tagging.tagURI(uris[i], ["test"]); + for (let uri of uris) { + PlacesUtils.tagging.tagURI(uri, ["test"]); } let library = await promiseLibrary(); // Select and open the left pane "Bookmarks Toolbar" folder. let PO = library.PlacesOrganizer; PO.selectLeftPaneBuiltIn("Tags");
--- a/toolkit/components/places/PlacesTransactions.jsm +++ b/toolkit/components/places/PlacesTransactions.jsm @@ -903,17 +903,17 @@ DefineTransaction.verifyInput = function // Update the documentation at the top of this module if you add or // remove properties. DefineTransaction.defineInputProps(["url", "feedUrl", "siteUrl"], DefineTransaction.urlValidate, null); DefineTransaction.defineInputProps(["guid", "parentGuid", "newParentGuid"], DefineTransaction.guidValidate); DefineTransaction.defineInputProps(["title", "postData"], DefineTransaction.strOrNullValidate, null); -DefineTransaction.defineInputProps(["keyword", "oldKeyword", "tag", +DefineTransaction.defineInputProps(["keyword", "oldKeyword", "oldTag", "tag", "excludingAnnotation"], DefineTransaction.strValidate, ""); DefineTransaction.defineInputProps(["index", "newIndex"], DefineTransaction.indexValidate, PlacesUtils.bookmarks.DEFAULT_INDEX); DefineTransaction.defineInputProps(["annotation"], DefineTransaction.annotationObjectValidate); DefineTransaction.defineInputProps(["child"], @@ -1619,16 +1619,106 @@ PT.Untag.prototype = { for (let f of onRedo) { await f(); } }; } }; /** + * Transaction for renaming a tag. + * + * Required Input Properties: oldTag, tag. + */ +PT.RenameTag = DefineTransaction(["oldTag", "tag"]); +PT.RenameTag.prototype = { + async execute({ oldTag, tag }) { + // For now this is implemented by untagging and tagging all the bookmarks. + // We should create a specialized bookmarking API to just rename the tag. + let onUndo = [], onRedo = []; + let urls = PlacesUtils.tagging.getURIsForTag(oldTag); + if (urls.length > 0) { + let tagTxn = TransactionsHistory.getRawTransaction( + PT.Tag({ urls, tags: [tag] }) + ); + await tagTxn.execute(); + onUndo.unshift(tagTxn.undo.bind(tagTxn)); + onRedo.push(tagTxn.redo.bind(tagTxn)); + let untagTxn = TransactionsHistory.getRawTransaction( + PT.Untag({ urls, tags: [oldTag] }) + ); + await untagTxn.execute(); + onUndo.unshift(untagTxn.undo.bind(untagTxn)); + onRedo.push(untagTxn.redo.bind(untagTxn)); + + // Update all the place: queries that refer to this tag. + let db = await PlacesUtils.promiseDBConnection(); + let rows = await db.executeCached(` + SELECT h.url, b.guid, b.title + FROM moz_places h + JOIN moz_bookmarks b ON b.fk = h.id + WHERE url_hash BETWEEN hash("place", "prefix_lo") + AND hash("place", "prefix_hi") + AND url LIKE :tagQuery + `, { tagQuery: "%tag=%" }); + for (let row of rows) { + let url = row.getResultByName("url"); + try { + url = new URL(url); + let urlParams = new URLSearchParams(url.pathname); + let tags = urlParams.getAll("tag"); + if (!tags.includes(oldTag)) + continue; + if (tags.length > 1) { + // URLSearchParams cannot set more than 1 same-named param. + urlParams.delete("tag"); + urlParams.set("tag", tag); + url = new URL(url.protocol + urlParams + + "&tag=" + tags.filter(t => t != oldTag).join("&tag=")); + } else { + urlParams.set("tag", tag); + url = new URL(url.protocol + urlParams); + } + } catch (ex) { + Cu.reportError("Invalid bookmark url: " + row.getResultByName("url") + ": " + ex); + continue; + } + let guid = row.getResultByName("guid"); + let title = row.getResultByName("title"); + + let editUrlTxn = TransactionsHistory.getRawTransaction( + PT.EditUrl({ guid, url }) + ); + await editUrlTxn.execute(); + onUndo.unshift(editUrlTxn.undo.bind(editUrlTxn)); + onRedo.push(editUrlTxn.redo.bind(editUrlTxn)); + if (title == oldTag) { + let editTitleTxn = TransactionsHistory.getRawTransaction( + PT.EditTitle({ guid, title: tag }) + ); + await editTitleTxn.execute(); + onUndo.unshift(editTitleTxn.undo.bind(editTitleTxn)); + onRedo.push(editTitleTxn.redo.bind(editTitleTxn)); + } + } + } + this.undo = async function() { + for (let f of onUndo) { + await f(); + } + }; + this.redo = async function() { + for (let f of onRedo) { + await f(); + } + }; + } +}; + +/** * Transaction for copying an item. * * Required Input Properties: guid, newParentGuid * Optional Input Properties: newIndex, excludingAnnotations. */ PT.Copy = DefineTransaction(["guid", "newParentGuid"], ["newIndex", "excludingAnnotations"]); PT.Copy.prototype = {
--- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -777,19 +777,30 @@ var PlacesUtils = { /** * Determines whether or not a result-node is a tag container. * @param aNode * A result-node * @returns true if the node is a tag container, false otherwise */ nodeIsTagQuery: function PU_nodeIsTagQuery(aNode) { - return aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY && - asQuery(aNode).queryOptions.resultType == - Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS; + if (aNode.type != Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY) + return false; + // Direct child of RESULTS_AS_TAG_QUERY. + let parent = aNode.parent; + if (parent && PlacesUtils.asQuery(parent).queryOptions.resultType == + Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) + return true; + // We must also support the right pane of the Library, when the tag query + // is the root node. Unfortunately this is also valid for any tag query + // selected in the left pane that is not a direct child of RESULTS_AS_TAG_QUERY. + if (!parent && aNode == aNode.parentResult.root && + PlacesUtils.asQuery(aNode).query.tags.length == 1) + return true; + return false; }, /** * Determines whether or not a ResultNode is a container. * @param aNode * A result node * @returns true if the node is a container item, false otherwise */ @@ -817,25 +828,18 @@ var PlacesUtils = { this.nodeIsHost(aNode)); }, /** * Gets the concrete item-id for the given node. Generally, this is just * node.itemId, but for folder-shortcuts that's node.folderItemId. */ getConcreteItemId: function PU_getConcreteItemId(aNode) { - if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) - return asQuery(aNode).folderItemId; - else if (PlacesUtils.nodeIsTagQuery(aNode)) { - // RESULTS_AS_TAG_CONTENTS queries are similar to folder shortcuts - // so we can still get the concrete itemId for them. - let folders = aNode.query.getFolders(); - return folders[0]; - } - return aNode.itemId; + return aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT ? + asQuery(aNode).folderItemId : aNode.itemId; }, /** * Gets the concrete item-guid for the given node. For everything but folder * shortcuts, this is just node.bookmarkGuid. For folder shortcuts, this is * node.targetFolderGuid (see nsINavHistoryService.idl for the semantics). * * @param aNode
--- a/toolkit/components/places/nsNavHistory.cpp +++ b/toolkit/components/places/nsNavHistory.cpp @@ -1125,17 +1125,18 @@ bool NeedToFilterResultSet(const RefPtr< } // ** Helper class for ConstructQueryString **/ class PlacesSQLQueryBuilder { public: PlacesSQLQueryBuilder(const nsCString& aConditions, - nsNavHistoryQueryOptions* aOptions, + const RefPtr<nsNavHistoryQuery>& aQuery, + const RefPtr<nsNavHistoryQueryOptions>& aOptions, bool aUseLimit, nsNavHistory::StringHash& aAddParams, bool aHasSearchTerms); nsresult GetQueryString(nsCString& aQueryString); private: nsresult Select(); @@ -1168,37 +1169,44 @@ private: bool mIncludeHidden; uint16_t mSortingMode; uint32_t mMaxResults; nsCString mQueryString; nsCString mGroupBy; bool mHasDateColumns; bool mSkipOrderBy; + nsNavHistory::StringHash& mAddParams; }; PlacesSQLQueryBuilder::PlacesSQLQueryBuilder( const nsCString& aConditions, - nsNavHistoryQueryOptions* aOptions, + const RefPtr<nsNavHistoryQuery>& aQuery, + const RefPtr<nsNavHistoryQueryOptions>& aOptions, bool aUseLimit, nsNavHistory::StringHash& aAddParams, bool aHasSearchTerms) : mConditions(aConditions) , mUseLimit(aUseLimit) , mHasSearchTerms(aHasSearchTerms) , mResultType(aOptions->ResultType()) , mQueryType(aOptions->QueryType()) , mIncludeHidden(aOptions->IncludeHidden()) , mSortingMode(aOptions->SortingMode()) , mMaxResults(aOptions->MaxResults()) , mSkipOrderBy(false) , mAddParams(aAddParams) { mHasDateColumns = (mQueryType == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS); + // Force the default sorting mode for tag queries. + if (mSortingMode == nsINavHistoryQueryOptions::SORT_BY_NONE && + aQuery->Tags().Length() > 0) { + mSortingMode = nsINavHistoryQueryOptions::SORT_BY_TITLE_ASCENDING; + } } nsresult PlacesSQLQueryBuilder::GetQueryString(nsCString& aQueryString) { nsresult rv = Select(); NS_ENSURE_SUCCESS(rv, rv); rv = Where(); @@ -1649,16 +1657,19 @@ PlacesSQLQueryBuilder::SelectAsTag() { nsNavHistory *history = nsNavHistory::GetHistoryService(); NS_ENSURE_STATE(history); // This allows sorting by date fields what is not possible with // other history queries. mHasDateColumns = true; + // TODO (Bug 1449939): This is likely wrong, since the tag name should + // probably be urlencoded, and we have no util for that in SQL, yet. + // We could encode the tag when the user sets it though. mQueryString = nsPrintfCString( "SELECT null, 'place:tag=' || title, " "title, null, null, null, null, null, dateAdded, " "lastModified, null, null, null, null, null, null " "FROM moz_bookmarks " "WHERE parent = %" PRId64, history->GetTagsFolder() ); @@ -2014,17 +2025,17 @@ nsNavHistory::ConstructQueryString( conditions += queryClause; } // Determine whether we can push maxResults constraints into the query // as LIMIT, or if we need to do result count clamping later // using FilterResultSet() bool useLimitClause = !NeedToFilterResultSet(aQuery, aOptions); - PlacesSQLQueryBuilder queryStringBuilder(conditions, aOptions, + PlacesSQLQueryBuilder queryStringBuilder(conditions, aQuery, aOptions, useLimitClause, aAddParams, hasSearchTerms); rv = queryStringBuilder.GetQueryString(queryString); NS_ENSURE_SUCCESS(rv, rv); return NS_OK; } @@ -3035,18 +3046,19 @@ nsNavHistory::QueryToSelectClause(const Str("AND lower(tags.title) IN ("); for (uint32_t i = 0; i < tags.Length(); ++i) { nsPrintfCString param(":tag%d_", i); clause.Param(param.get()); if (i < tags.Length() - 1) clause.Str(","); } clause.Str(")"); - if (!aQuery->TagsAreNot()) + if (!aQuery->TagsAreNot()) { clause.Str("GROUP BY bms.fk HAVING count(*) >=").Param(":tag_count"); + } clause.Str(")"); } // transitions const nsTArray<uint32_t>& transitions = aQuery->Transitions(); for (uint32_t i = 0; i < transitions.Length(); ++i) { nsPrintfCString param(":transition%d_", i); clause.Condition("h.id IN (SELECT place_id FROM moz_historyvisits "
deleted file mode 100644 --- a/toolkit/components/places/tests/unit/test_419731.js +++ /dev/null @@ -1,123 +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/. */ - -add_task(async function test_tag_updates() { - const url = "http://foo.bar/"; - let lastModified = new Date(Date.now() - 10000); - - // create 2 bookmarks - let bm1 = await PlacesUtils.bookmarks.insert({ - parentGuid: PlacesUtils.bookmarks.menuGuid, - dateAdded: lastModified, - lastModified: new Date(), - title: "title 1", - url, - }); - let bm2 = await PlacesUtils.bookmarks.insert({ - parentGuid: PlacesUtils.bookmarks.menuGuid, - dateAdded: lastModified, - lastModified, - title: "title 2", - url, - }); - - // add a new tag - PlacesUtils.tagging.tagURI(uri(url), ["foo"]); - - // get tag folder id - let options = PlacesUtils.history.getNewQueryOptions(); - let query = PlacesUtils.history.getNewQuery(); - query.setFolders([PlacesUtils.tagsFolderId], 1); - let result = PlacesUtils.history.executeQuery(query, options); - let tagRoot = result.root; - tagRoot.containerOpen = true; - let tagNode = tagRoot.getChild(0) - .QueryInterface(Ci.nsINavHistoryContainerResultNode); - let tagItemGuid = tagNode.bookmarkGuid; - let tagItemId = tagNode.itemId; - tagRoot.containerOpen = false; - - // change bookmark 1 title - await PlacesUtils.bookmarks.update({ - guid: bm1.guid, - title: "new title 1", - }); - - // Workaround timers resolution and time skews. - bm2 = await PlacesUtils.bookmarks.fetch(bm2.guid); - - lastModified = new Date(lastModified.getTime() + 1000); - - await PlacesUtils.bookmarks.update({ - guid: bm1.guid, - lastModified, - }); - - // Query the tag. - options = PlacesUtils.history.getNewQueryOptions(); - options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS; - options.resultType = options.RESULTS_AS_TAG_QUERY; - - query = PlacesUtils.history.getNewQuery(); - result = PlacesUtils.history.executeQuery(query, options); - let root = result.root; - root.containerOpen = true; - Assert.equal(root.childCount, 1); - - let theTag = root.getChild(0) - .QueryInterface(Ci.nsINavHistoryContainerResultNode); - // Bug 524219: Check that renaming the tag shows up in the result. - Assert.equal(theTag.title, "foo"); - - await PlacesUtils.bookmarks.update({ - guid: tagItemGuid, - title: "bar", - }); - - // Check that the item has been replaced - Assert.notEqual(theTag, root.getChild(0)); - theTag = root.getChild(0) - .QueryInterface(Ci.nsINavHistoryContainerResultNode); - Assert.equal(theTag.title, "bar"); - - // Check that tag container contains new title - theTag.containerOpen = true; - Assert.equal(theTag.childCount, 1); - let node = theTag.getChild(0); - Assert.equal(node.title, "new title 1"); - theTag.containerOpen = false; - root.containerOpen = false; - - // Change bookmark 2 title. - PlacesUtils.bookmarks.update({ - guid: bm2.guid, - title: "new title 2" - }); - - // Workaround timers resolution and time skews. - lastModified = new Date(lastModified.getTime() + 1000); - - await PlacesUtils.bookmarks.update({ - guid: bm2.guid, - lastModified, - }); - - // Check that tag container contains new title - options = PlacesUtils.history.getNewQueryOptions(); - options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS; - options.resultType = options.RESULTS_AS_TAG_CONTENTS; - - query = PlacesUtils.history.getNewQuery(); - query.setFolders([tagItemId], 1); - result = PlacesUtils.history.executeQuery(query, options); - root = result.root; - - root.containerOpen = true; - Assert.equal(root.childCount, 1); - node = root.getChild(0); - Assert.equal(node.title, "new title 2"); - root.containerOpen = false; -});
--- a/toolkit/components/places/tests/unit/test_async_transactions.js +++ b/toolkit/components/places/tests/unit/test_async_transactions.js @@ -1854,8 +1854,72 @@ add_task(async function test_remove_mult // Redo remove. await PT.redo(); await ensureNonExistent(...guids); // Cleanup await PT.clearTransactionsHistory(); observer.reset(); }); + +add_task(async function test_renameTag() { + let url = "http://test.edit.keyword/"; + await PT.Tag({ url, tags: ["t1", "t2"] }).transact(); + ensureTagsForURI(url, ["t1", "t2"]); + + // Create bookmark queries that point to the modified tag. + let bm1 = await PlacesUtils.bookmarks.insert({ + url: "place:tag=t2", + parentGuid: PlacesUtils.bookmarks.unfiledGuid + }); + let bm2 = await PlacesUtils.bookmarks.insert({ + url: "place:tag=t2&sort=1", + parentGuid: PlacesUtils.bookmarks.unfiledGuid + }); + // This points to 2 tags, and as such won't be touched. + let bm3 = await PlacesUtils.bookmarks.insert({ + url: "place:tag=t2&tag=t1", + parentGuid: PlacesUtils.bookmarks.unfiledGuid + }); + + await PT.RenameTag({ oldTag: "t2", tag: "t3" }).transact(); + ensureTagsForURI(url, ["t1", "t3"]); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm1.guid)).url.href, "place:tag=t3", + "The fitst bookmark has been updated"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm2.guid)).url.href, "place:tag=t3&sort=1", + "The second bookmark has been updated"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm3.guid)).url.href, "place:tag=t3&tag=t1", + "The third bookmark has been updated"); + + await PT.undo(); + ensureTagsForURI(url, ["t1", "t2"]); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm1.guid)).url.href, "place:tag=t2", + "The fitst bookmark has been restored"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm2.guid)).url.href, "place:tag=t2&sort=1", + "The second bookmark has been restored"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm3.guid)).url.href, "place:tag=t2&tag=t1", + "The third bookmark has been restored"); + + await PT.redo(); + ensureTagsForURI(url, ["t1", "t3"]); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm1.guid)).url.href, "place:tag=t3", + "The fitst bookmark has been updated"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm2.guid)).url.href, "place:tag=t3&sort=1", + "The second bookmark has been updated"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm3.guid)).url.href, "place:tag=t3&tag=t1", + "The third bookmark has been updated"); + + await PT.undo(); + ensureTagsForURI(url, ["t1", "t2"]); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm1.guid)).url.href, "place:tag=t2", + "The fitst bookmark has been restored"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm2.guid)).url.href, "place:tag=t2&sort=1", + "The second bookmark has been restored"); + Assert.equal((await PlacesUtils.bookmarks.fetch(bm3.guid)).url.href, "place:tag=t2&tag=t1", + "The third bookmark has been restored"); + + await PT.undo(); + ensureTagsForURI(url, []); + + await PT.clearTransactionsHistory(); + ensureUndoState(); + await PlacesUtils.bookmarks.eraseEverything(); +});
--- a/toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js +++ b/toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js @@ -1,15 +1,15 @@ /* -*- 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/. */ -add_task(async function test_resolveNullBookmarkTitles() { +add_task(async function() { let uri1 = uri("http://foo.tld/"); let uri2 = uri("https://bar.tld/"); await PlacesTestUtils.addVisits([ { uri: uri1, title: "foo title" }, { uri: uri2, title: "bar title" } ]); await PlacesUtils.bookmarks.insert({ @@ -20,25 +20,38 @@ add_task(async function test_resolveNull await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.menuGuid, url: uri2, title: null }); PlacesUtils.tagging.tagURI(uri1, ["tag 1"]); PlacesUtils.tagging.tagURI(uri2, ["tag 2"]); +}); +add_task(async function testAsTagQuery() { let options = PlacesUtils.history.getNewQueryOptions(); options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS; options.resultType = options.RESULTS_AS_TAG_CONTENTS; let query = PlacesUtils.history.getNewQuery(); // if we don't set a tag folder, RESULTS_AS_TAG_CONTENTS will return all // tagged URIs let root = PlacesUtils.history.executeQuery(query, options).root; root.containerOpen = true; Assert.equal(root.childCount, 2); // actually RESULTS_AS_TAG_CONTENTS return results ordered by place_id DESC // so they are reversed Assert.equal(root.getChild(0).title, ""); Assert.equal(root.getChild(1).title, ""); root.containerOpen = false; }); + +add_task(async function testTagQuery() { + let options = PlacesUtils.history.getNewQueryOptions(); + let query = PlacesUtils.history.getNewQuery(); + query.tags = ["tag 1"]; + let root = PlacesUtils.history.executeQuery(query, options).root; + root.containerOpen = true; + Assert.equal(root.childCount, 1); + Assert.equal(root.getChild(0).title, ""); + root.containerOpen = false; +});
--- a/toolkit/components/places/tests/unit/xpcshell.ini +++ b/toolkit/components/places/tests/unit/xpcshell.ini @@ -28,17 +28,16 @@ skip-if = (os == 'win' && ccov) # Bug 14 # Bug 821781: test fails intermittently on Linux skip-if = os == "linux" [test_402799.js] [test_408221.js] [test_412132.js] [test_413784.js] [test_415460.js] [test_415757.js] -[test_419731.js] [test_419792_node_tags_property.js] [test_425563.js] [test_429505_remove_shortcuts.js] [test_433317_query_title_update.js] [test_433525_hasChildren_crash.js] [test_454977.js] [test_463863.js] [test_485442_crash_bug_nsNavHistoryQuery_GetUri.js]