Bug 1401242 - Fix removing bookmarks from tag groups - when getting the tagGuid from the node, we must get it async, rather than using the concrete guid. r=mak
authorMark Banner <standard8@mozilla.com>
Tue, 19 Sep 2017 20:47:26 +0100
changeset 381914 c2de01dc7c105214ffa0f75b61a7e5e0df2b98a5
parent 381913 39b1badf8930068d09478b89a21621c17244f06e
child 381915 648e06c8b1b557c199d7c0ea0cf153cc1ad1bfe2
push id32542
push userkwierso@gmail.com
push dateWed, 20 Sep 2017 21:07:55 +0000
treeherdermozilla-central@319a34bea9e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1401242
milestone57.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 1401242 - Fix removing bookmarks from tag groups - when getting the tagGuid from the node, we must get it async, rather than using the concrete guid. r=mak MozReview-Commit-ID: 7fGS6jFNVPl
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_library_delete_bookmarks_in_tags.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -885,20 +885,20 @@ PlacesController.prototype = {
       if (PlacesUtils.nodeIsTagQuery(node.parent)) {
         // This is a uri node inside a tag container.  It needs a special
         // untag transaction.
         var tagItemId = PlacesUtils.getConcreteItemId(node.parent);
         var uri = NetUtil.newURI(node.uri);
         if (PlacesUIUtils.useAsyncTransactions) {
           let tag = node.parent.title;
           if (!tag) {
-            let tagGuid = PlacesUtils.getConcreteItemGuid(node.parent);
+            let tagGuid = await PlacesUtils.promiseItemGuid(tagItemId);
             tag = (await PlacesUtils.bookmarks.fetch(tagGuid)).title;
           }
-          transactions.push(PlacesTransactions.Untag({ uri, tag }));
+          transactions.push(PlacesTransactions.Untag({ urls: [uri], tag }));
         } else {
           let txn = new PlacesUntagURITransaction(uri, [tagItemId]);
           transactions.push(txn);
         }
       } else if (PlacesUtils.nodeIsTagQuery(node) && node.parent &&
                PlacesUtils.nodeIsQuery(node.parent) &&
                PlacesUtils.asQuery(node.parent).queryOptions.resultType ==
                  Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) {
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -33,16 +33,17 @@ subsuite = clipboard
 [browser_copy_folder_tree.js]
 [browser_copy_query_without_tree.js]
 subsuite = clipboard
 [browser_drag_bookmarks_on_toolbar.js]
 [browser_forgetthissite_single.js]
 [browser_history_sidebar_search.js]
 [browser_library_batch_delete.js]
 [browser_library_commands.js]
+[browser_library_delete_bookmarks_in_tags.js]
 [browser_library_delete_tags.js]
 [browser_library_downloads.js]
 [browser_library_infoBox.js]
 [browser_library_left_pane_fixnames.js]
 [browser_library_left_pane_middleclick.js]
 [browser_library_left_pane_select_hierarchy.js]
 [browser_library_middleclick.js]
 [browser_library_move_bookmarks.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_library_delete_bookmarks_in_tags.js
@@ -0,0 +1,96 @@
+/* -*- 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/. */
+
+/**
+ *  Test deleting bookmarks from within tags.
+ */
+
+registerCleanupFunction(async function() {
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesTestUtils.clearHistory();
+});
+
+add_task(async function test_tags() {
+  const uris = [
+    Services.io.newURI("http://example.com/1"),
+    Services.io.newURI("http://example.com/2"),
+    Services.io.newURI("http://example.com/3"),
+  ];
+
+  let children = uris.map((uri, index, arr) => {
+    return {
+      title: `bm${index}`,
+      url: uri,
+    }
+  })
+
+  // Note: we insert the uris in reverse order, so that we end up with the
+  // 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"]);
+  }
+
+  let library = await promiseLibrary();
+
+  // Select and open the left pane "Bookmarks Toolbar" folder.
+  let PO = library.PlacesOrganizer;
+
+  PO.selectLeftPaneQuery("Tags");
+  let tagsNode = PO._places.selectedNode;
+  Assert.notEqual(tagsNode, null, "Should have a valid selection");
+  let tagsTitle = PlacesUtils.getString("TagsFolderTitle");
+  Assert.equal(tagsNode.title, tagsTitle,
+               "Should have selected the Tags node");
+
+  // Now select the tag.
+  PlacesUtils.asContainer(tagsNode).containerOpen = true;
+  let tag = tagsNode.getChild(0);
+  PO._places.selectNode(tag);
+  Assert.equal(PO._places.selectedNode.title, "test",
+               "Should have selected the created tag");
+
+  let ContentTree = library.ContentTree;
+
+  for (let i = 0; i < uris.length; i++) {
+    ContentTree.view.selectNode(ContentTree.view.result.root.getChild(0));
+
+    Assert.equal(ContentTree.view.selectedNode.title, `bm${i}`,
+                 `Should have selected bm${i}`);
+
+    let promiseNotification = PlacesTestUtils.waitForNotification("onItemChanged",
+      (id, property) => property == "tags");
+
+    ContentTree.view.controller.doCommand("cmd_delete");
+
+    await promiseNotification;
+
+    for (let j = 0; j < uris.length; j++) {
+      let tags = PlacesUtils.tagging.getTagsForURI(uris[j]);
+      if (j <= i) {
+        Assert.equal(tags.length, 0,
+                     `There should be no tags for the URI: ${uris[j].spec}`);
+      } else {
+        Assert.equal(tags.length, 1,
+                     `There should be one tag for the URI: ${uris[j].spec}`);
+      }
+    }
+  }
+
+  // The tag should now not exist.
+  Assert.deepEqual(PlacesUtils.tagging.getURIsForTag("test"), [],
+                   "There should be no URIs remaining for the tag");
+
+  tagsNode.containerOpen = false;
+
+  library.close();
+});