Bug 1452067 - Move AllTags to an async method in the bookmarking API. r=Standard8

The tagging API is moving to the bookmarking API, this is one step

Differential Revision: https://phabricator.services.mozilla.com/D2316

--HG--
extra : moz-landing-system : lando

browser/components/places/content/editBookmark.js

@@ -233,7 +233,7 @@ var gEditItemOverlay = {
     if (showOrCollapse("tagsRow", isURI || bulkTagging, "tags"))
       this._initTagsField();
     else if (!this._element("tagsSelectorRow").collapsed)
-      this.toggleTagsSelector();
+      this.toggleTagsSelector().catch(Cu.reportError);
 
     // Folder picker.
     // Technically we should check that the item is not moveable, but that's
@@ -435,7 +435,7 @@ var gEditItemOverlay = {
       // Hide the tag selector if it was previously visible.
       var tagsSelectorRow = this._element("tagsSelectorRow");
       if (!tagsSelectorRow.collapsed)
-        this.toggleTagsSelector();
+        this.toggleTagsSelector().catch(Cu.reportError);
     }
 
     if (this._observersAdded) {
@@ -741,7 +741,7 @@ var gEditItemOverlay = {
     folderItem.doCommand();
   },
 
-  _rebuildTagsSelectorList() {
+  async _rebuildTagsSelectorList() {
     let tagsSelector = this._element("tagsSelector");
     let tagsSelectorRow = this._element("tagsSelectorRow");
     if (tagsSelectorRow.collapsed)
@@ -756,10 +756,10 @@ var gEditItemOverlay = {
     }
 
     let tagsInField = this._getTagsArrayFromTagsInputField();
-    let allTags = PlacesUtils.tagging.allTags;
+    let allTags = await PlacesUtils.bookmarks.fetchTags();
     let fragment = document.createDocumentFragment();
-    for (var i = 0; i < allTags.length; i++) {
+    for (let i = 0; i < allTags.length; i++) {
-      let tag = allTags[i];
+      let tag = allTags[i].name;
       let elt = document.createElement("richlistitem");
       elt.appendChild(document.createElement("image"));
       let label = document.createElement("label");
@@ -778,9 +778,11 @@ var gEditItemOverlay = {
       tagsSelector.selectedIndex = selectedIndex;
       tagsSelector.ensureIndexIsVisible(selectedIndex);
     }
+    let event = new CustomEvent("BookmarkTagsSelectorUpdated", { bubbles: true });
+    tagsSelector.dispatchEvent(event);
   },
 
-  toggleTagsSelector() {
+  async toggleTagsSelector() {
     var tagsSelector = this._element("tagsSelector");
     var tagsSelectorRow = this._element("tagsSelectorRow");
     var expander = this._element("tagsSelectorExpander");
@@ -789,7 +791,7 @@ var gEditItemOverlay = {
       expander.setAttribute("tooltiptext",
                             expander.getAttribute("tooltiptextup"));
       tagsSelectorRow.collapsed = false;
-      this._rebuildTagsSelectorList();
+      await this._rebuildTagsSelectorList();
 
       // This is a no-op if we've added the listener.
       tagsSelector.addEventListener("mousedown", this);
@@ -934,7 +936,7 @@ var gEditItemOverlay = {
       this._initTagsField();
       // Any tags change should be reflected in the tags selector.
       if (this._element("tagsSelector")) {
-        this._rebuildTagsSelectorList();
+        await this._rebuildTagsSelectorList();
       }
     }
   },

browser/components/places/content/editBookmarkPanel.inc.xul

@@ -102,7 +102,7 @@
                 tooltiptext="&editBookmarkOverlay.tagsExpanderDown.tooltip;"
                 tooltiptextdown="&editBookmarkOverlay.tagsExpanderDown.tooltip;"
                 tooltiptextup="&editBookmarkOverlay.expanderUp.tooltip;"
-                oncommand="gEditItemOverlay.toggleTagsSelector();"/>
+                oncommand="gEditItemOverlay.toggleTagsSelector().catch(Cu.reportError);"/>
       </hbox>
     </vbox>
 

browser/components/places/tests/browser/browser_bug631374_tags_selector_scroll.js

@@ -78,10 +78,10 @@ add_task(async function() {
     let selectedTag = listItem.label;
 
     // Uncheck the tag.
-    let promiseNotification = PlacesTestUtils.waitForNotification(
+    let promise = BrowserTestUtils.waitForEvent(tagsSelector,
-      "onItemChanged", (id, property) => property == "tags");
+                                                "BookmarkTagsSelectorUpdated");
     EventUtils.synthesizeMouseAtCenter(listItem.firstChild, {});
-    await promiseNotification;
+    await promise;
     is(scrollTop, tagsSelector.scrollTop, "Scroll position did not change");
 
     // The listbox is rebuilt, so we have to get the new element.
@@ -91,10 +91,10 @@ add_task(async function() {
     is(newItem.label, selectedTag, "Correct tag is still selected");
 
     // Check the tag.
-    promiseNotification = PlacesTestUtils.waitForNotification(
+    promise = BrowserTestUtils.waitForEvent(tagsSelector,
-      "onItemChanged", (id, property) => property == "tags");
+                                            "BookmarkTagsSelectorUpdated");
     EventUtils.synthesizeMouseAtCenter(newItem.firstChild, {});
-    await promiseNotification;
+    await promise;
     is(scrollTop, tagsSelector.scrollTop, "Scroll position did not change");
   }
 
@@ -114,10 +114,10 @@ add_task(async function() {
     ok(listItem.hasAttribute("checked"), "Item is checked " + i);
 
     // Uncheck the tag.
-    let promiseNotification = PlacesTestUtils.waitForNotification(
+    let promise = BrowserTestUtils.waitForEvent(tagsSelector,
-      "onItemChanged", (id, property) => property == "tags");
+                                                "BookmarkTagsSelectorUpdated");
     EventUtils.synthesizeMouseAtCenter(listItem.firstChild, {});
-    await promiseNotification;
+    await promise;
 
     // The listbox is rebuilt, so we have to get the new element.
     let topItem = [...tagsSelector.children].find(e => e.label == topTag);
@@ -140,13 +140,9 @@ add_task(async function() {
 });
 
 function openTagSelector() {
-  // Wait for the tags selector to be open.
+  let promise = BrowserTestUtils.waitForEvent(
-  let promise = new Promise(resolve => {
+    document.getElementById("editBMPanel_tagsSelector"),
-    let row = document.getElementById("editBMPanel_tagsSelectorRow");
+    "BookmarkTagsSelectorUpdated");
-    row.addEventListener("DOMAttrModified", function onAttrModified() {
-      resolve();
-    }, {once: true});
-  });
   // Open the tags selector.
   document.getElementById("editBMPanel_tagsSelectorExpander").doCommand();
   return promise;

browser/components/places/tests/browser/browser_editBookmark_tags_liveUpdate.js

@@ -1,8 +1,8 @@
 "use strict";
 
-function checkTagsSelector(aAvailableTags, aCheckedTags) {
+async function checkTagsSelector(aAvailableTags, aCheckedTags) {
-  is(PlacesUtils.tagging.allTags.length, aAvailableTags.length,
+  let tags = await PlacesUtils.bookmarks.fetchTags();
-     "tagging service is in sync.");
+  is(tags.length, aAvailableTags.length, "Check tags list");
   let tagsSelector = document.getElementById("editBMPanel_tagsSelector");
   let children = tagsSelector.childNodes;
   is(children.length, aAvailableTags.length,
@@ -49,7 +49,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == TEST_TAG,
     "Editing a single bookmark shows the added tag.");
-  checkTagsSelector([TEST_TAG], [TEST_TAG]);
+  await checkTagsSelector([TEST_TAG], [TEST_TAG]);
 
   // Remove tag.
   PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
@@ -58,7 +58,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing a single bookmark should not show any tag");
-  checkTagsSelector([], []);
+  await checkTagsSelector([], []);
 
   // Add a second bookmark.
   let bm2 = await PlacesUtils.bookmarks.insert({
@@ -79,7 +79,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing multiple bookmarks without matching tags should not show any tag.");
-  checkTagsSelector([TEST_TAG], []);
+  await checkTagsSelector([TEST_TAG], []);
 
   // Add a tag to the second uri.
   PlacesUtils.tagging.tagURI(TEST_URI2, [TEST_TAG]);
@@ -88,7 +88,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == TEST_TAG,
     "Editing multiple bookmarks should show matching tags.");
-  checkTagsSelector([TEST_TAG], [TEST_TAG]);
+  await checkTagsSelector([TEST_TAG], [TEST_TAG]);
 
   // Remove tag from the first bookmark.
   PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
@@ -97,7 +97,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing multiple bookmarks without matching tags should not show any tag.");
-  checkTagsSelector([TEST_TAG], []);
+  await checkTagsSelector([TEST_TAG], []);
 
   // Remove tag from the second bookmark.
   PlacesUtils.tagging.untagURI(TEST_URI2, [TEST_TAG]);
@@ -106,7 +106,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing multiple bookmarks without matching tags should not show any tag.");
-  checkTagsSelector([], []);
+  await checkTagsSelector([], []);
 
   // Init panel with a nsIURI entry.
   gEditItemOverlay.initPanel({ uris: [TEST_URI] });
@@ -118,7 +118,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == TEST_TAG,
     "Editing a single nsIURI entry shows the added tag.");
-  checkTagsSelector([TEST_TAG], [TEST_TAG]);
+  await checkTagsSelector([TEST_TAG], [TEST_TAG]);
 
   // Remove tag.
   PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
@@ -127,7 +127,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing a single nsIURI entry should not show any tag.");
-  checkTagsSelector([], []);
+  await checkTagsSelector([], []);
 
   // Init panel with multiple nsIURI entries.
   gEditItemOverlay.initPanel({ uris: [TEST_URI, TEST_URI2] });
@@ -139,7 +139,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing multiple nsIURIs without matching tags should not show any tag.");
-  checkTagsSelector([TEST_TAG], []);
+  await checkTagsSelector([TEST_TAG], []);
 
   // Add a tag to the second entry.
   PlacesUtils.tagging.tagURI(TEST_URI2, [TEST_TAG]);
@@ -148,7 +148,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == TEST_TAG,
     "Editing multiple nsIURIs should show matching tags.");
-  checkTagsSelector([TEST_TAG], [TEST_TAG]);
+  await checkTagsSelector([TEST_TAG], [TEST_TAG]);
 
   // Remove tag from the first entry.
   PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
@@ -157,7 +157,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing multiple nsIURIs without matching tags should not show any tag.");
-  checkTagsSelector([TEST_TAG], []);
+  await checkTagsSelector([TEST_TAG], []);
 
   // Remove tag from the second entry.
   PlacesUtils.tagging.untagURI(TEST_URI2, [TEST_TAG]);
@@ -166,7 +166,7 @@ add_task(async function() {
   await BrowserTestUtils.waitForCondition(
     () => document.getElementById("editBMPanel_tagsField").value == "",
     "Editing multiple nsIURIs without matching tags should not show any tag.");
-  checkTagsSelector([], []);
+  await checkTagsSelector([], []);
 
   // Cleanup.
   await PlacesUtils.bookmarks.remove(bm.guid);

toolkit/components/places/Bookmarks.jsm

@@ -1365,6 +1365,34 @@ var Bookmarks = Object.freeze({
     throw new Error("Not yet implemented");
   },
 
+  /**
+   * Fetch all the existing tags, sorted alphabetically.
+   * @return {Promise} resolves to an array of objects representing tags, when
+   *         fetching is complete.
+   *         Each object looks like {
+   *           name: the name of the tag,
+   *           count: number of bookmarks with this tag
+   *         }
+   */
+  async fetchTags() {
+    // TODO: Once the tagging API is implemented in Bookmarks.jsm, we can cache
+    // the list of tags, instead of querying every time.
+    let db = await PlacesUtils.promiseDBConnection();
+    let rows = await db.executeCached(`
+      SELECT b.title AS name, count(*) AS count
+      FROM moz_bookmarks b
+      JOIN moz_bookmarks p ON b.parent = p.id
+      JOIN moz_bookmarks c ON c.parent = b.id
+      WHERE p.guid = :tagsGuid
+      GROUP BY name
+      ORDER BY name COLLATE nocase ASC 
+    `, { tagsGuid: this.tagsGuid });
+    return rows.map(r => ({
+      name: r.getResultByName("name"),
+      count: r.getResultByName("count")
+    }));
+  },
+
   /**
    * Reorders contents of a folder based on a provided array of GUIDs.
    *

toolkit/components/places/nsTaggingService.js

@@ -325,18 +325,6 @@ TaggingService.prototype = {
     return this.__tagFolders;
   },
 
-  // nsITaggingService
-  get allTags() {
-    var allTags = [];
-    for (var i in this._tagFolders)
-      allTags.push(this._tagFolders[i]);
-    // sort the tag list
-    allTags.sort(function(a, b) {
-        return a.toLowerCase().localeCompare(b.toLowerCase());
-      });
-    return allTags;
-  },
-
   // nsITaggingService
   get hasTags() {
     return this._tagFolders.length > 0;
@@ -473,7 +461,6 @@ TagAutoCompleteSearch.prototype = {
    * @param listener - A listener to notify when the search is complete
    */
   startSearch(searchString, searchParam, previousResult, listener) {
-    let searchResults = PlacesUtils.tagging.allTags;
     this._stopped = false;
 
     // only search on characters for the last tag
@@ -503,29 +490,33 @@ TagAutoCompleteSearch.prototype = {
       return;
     }
 
-    // Chunk the search results via a generator.
+    (async () => {
-    let gen = (function* () {
+      let tags = (await PlacesUtils.bookmarks.fetchTags())
-      for (let i = 0; i < searchResults.length; ++i) {
+        .filter(t => t.name.toLowerCase().startsWith(searchString.toLowerCase()))
-        if (this._stopped)
+        .map(t => t.name);
-          yield false;
+
+      // Chunk the search results via a generator.
+      let gen = (function* () {
+        for (let i = 0; i < tags.length; ++i) {
+          if (this._stopped)
+            yield false;
 
-        if (searchResults[i].toLowerCase().startsWith(searchString.toLowerCase())) {
           // For each match, prepend what the user has typed so far.
           count++;
-          result.appendMatch(before + searchResults[i], searchResults[i]);
+          result.appendMatch(before + tags[i], tags[i]);
-        }
 
-        // In case of many tags, notify once every 50 loops.
+          // In case of many tags, notify once every 10 loops.
-        if ((i % 10) == 0) {
+          if ((i % 10) == 0) {
-          this.notifyResult(result, count, listener, true);
+            this.notifyResult(result, count, listener, true);
-          yield true;
+            yield true;
+          }
         }
-      }
+        yield false;
-      yield false;
+      }.bind(this))();
-    }.bind(this))();
 
-    while (gen.next().value);
+      while (gen.next().value);
-    this.notifyResult(result, count, listener, false);
+      this.notifyResult(result, count, listener, false);
+    })();
   },
 
   /**

toolkit/components/places/tests/bookmarks/test_tags.js

@@ -0,0 +1,36 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+add_task(async function() {
+  let tags = await PlacesUtils.bookmarks.fetchTags();
+  Assert.deepEqual(tags, []);
+
+  let bm = await PlacesUtils.bookmarks.insert({
+    url: "http://page1.com/",
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+  });
+
+  PlacesUtils.tagging.tagURI(Services.io.newURI(bm.url.href), ["1", "2"]);
+  tags = await PlacesUtils.bookmarks.fetchTags();
+  Assert.deepEqual(tags, [
+    { name: "1", count: 1 },
+    { name: "2", count: 1 },
+  ]);
+
+  PlacesUtils.tagging.untagURI(Services.io.newURI(bm.url.href), ["1"]);
+  tags = await PlacesUtils.bookmarks.fetchTags();
+  Assert.deepEqual(tags, [
+    { name: "2", count: 1 },
+  ]);
+
+  let bm2 = await PlacesUtils.bookmarks.insert({
+    url: "http://page2.com/",
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+  });
+  PlacesUtils.tagging.tagURI(Services.io.newURI(bm2.url.href), ["2", "3"]);
+  tags = await PlacesUtils.bookmarks.fetchTags();
+  Assert.deepEqual(tags, [
+    { name: "2", count: 2 },
+    { name: "3", count: 1 },
+  ]);
+});

toolkit/components/places/tests/bookmarks/xpcshell.ini

@@ -41,4 +41,5 @@ firefox-appdir = browser
 [test_removeFolderTransaction_reinsert.js]
 [test_savedsearches.js]
 [test_sync_fields.js]
+[test_tags.js]
 [test_untitled.js]

toolkit/components/places/tests/sync/test_sync_utils.js

@@ -679,7 +679,7 @@ add_task(async function test_pullChanges_tags() {
   info("Rename tag folder using Bookmarks.setItemTitle");
   {
     PlacesUtils.bookmarks.setItemTitle(tagFolderId, "sneaky");
-    deepEqual(PlacesUtils.tagging.allTags, ["sneaky"],
+    deepEqual((await PlacesUtils.bookmarks.fetchTags()).map(t => t.name), ["sneaky"],
       "Tagging service should update cache with new title");
     let changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
@@ -694,7 +694,7 @@ add_task(async function test_pullChanges_tags() {
       guid: tagFolderGuid,
       title: "tricky",
     });
-    deepEqual(PlacesUtils.tagging.allTags, ["tricky"],
+    deepEqual((await PlacesUtils.bookmarks.fetchTags()).map(t => t.name), ["tricky"],
       "Tagging service should update cache after updating tag folder");
     let changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
@@ -1358,7 +1358,7 @@ add_task(async function test_insert_tags_whitespace() {
 
   PlacesUtils.tagging.untagURI(uri("https://example.org"), ["untrimmed", "taggy"]);
   PlacesUtils.tagging.untagURI(uri("https://example.net"), ["taggy"]);
-  deepEqual(PlacesUtils.tagging.allTags, [], "Should clean up all tags");
+  deepEqual((await PlacesUtils.bookmarks.fetchTags()).map(t => t.name), [], "Should clean up all tags");
 
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
@@ -1423,7 +1423,7 @@ add_task(async function test_insert_tag_query() {
     ok(!params.has("type"), "Should not preserve query type");
     ok(!params.has("folder"), "Should not preserve folder");
     equal(params.get("tag"), "nonexisting", "Should add tag");
-    deepEqual(PlacesUtils.tagging.allTags, ["taggy"],
+    deepEqual((await PlacesUtils.bookmarks.fetchTags()).map(t => t.name), ["taggy"],
               "The nonexisting tag should not be added");
   }
 
@@ -1443,7 +1443,7 @@ add_task(async function test_insert_tag_query() {
     ok(!params.has("folder"), "Should not preserve folder");
     equal(params.get("maxResults"), "15", "Should preserve additional params");
     equal(params.get("tag"), "taggy", "Should add tag");
-    deepEqual(PlacesUtils.tagging.allTags, ["taggy"],
+    deepEqual((await PlacesUtils.bookmarks.fetchTags()).map(t => t.name), ["taggy"],
               "Should not duplicate existing tags");
   }
 
@@ -1451,7 +1451,7 @@ add_task(async function test_insert_tag_query() {
 
   info("Removing the tag should clean up the tag folder");
   PlacesUtils.tagging.untagURI(uri("https://mozilla.org"), null);
-  deepEqual(PlacesUtils.tagging.allTags, [],
+  deepEqual((await PlacesUtils.bookmarks.fetchTags()).map(t => t.name), [],
     "Should remove tag folder once last item is untagged");
 
   await PlacesUtils.bookmarks.eraseEverything();

toolkit/components/places/tests/unit/test_tagging.js

@@ -64,12 +64,6 @@ function run_test() {
   Assert.ok(tag1uris[0].equals(uri1));
   Assert.ok(tag1uris[1].equals(uri2));
 
-  // test allTags attribute
-  var allTags = tagssvc.allTags;
-  Assert.equal(allTags.length, 2);
-  Assert.equal(allTags[0], "Tag 1");
-  Assert.equal(allTags[1], "Tag 2");
-
   // test untagging
   tagssvc.untagURI(uri1, ["tag 1"]);
   Assert.equal(tag1node.childCount, 1);