Bug 1477996 - Move getURIsForTag to the bookmarking API. r=Standard8,lina
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 27 Jul 2018 15:16:57 +0000
changeset 428759 fae2d182cacf047eacbd91b8a932f3d3344fb2f7
parent 428758 abfa418aed59ff1bb89d02b8b145f07d44dabab4
child 428760 fde63f5ba297b6405eb9ddca0f6c926c2a90b77d
push id66995
push usermak77@bonardo.net
push dateFri, 27 Jul 2018 15:47:32 +0000
treeherderautoland@fae2d182cacf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8, lina
bugs1477996
milestone63.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 1477996 - Move getURIsForTag to the bookmarking API. r=Standard8,lina The tagging API is being merged into the bookmarking API. This is part of it. Differential Revision: https://phabricator.services.mozilla.com/D2450
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser_library_delete_bookmarks_in_tags.js
services/sync/tests/unit/test_engine_changes_during_sync.js
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/PlacesTransactions.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/nsITaggingService.idl
toolkit/components/places/nsTaggingService.js
toolkit/components/places/tests/bookmarks/test_tags.js
toolkit/components/places/tests/sync/test_bookmark_value_changes.js
toolkit/components/places/tests/sync/test_sync_utils.js
toolkit/components/places/tests/unit/test_tagging.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -805,18 +805,19 @@ PlacesController.prototype = {
                  PlacesUtils.nodeIsQuery(node.parent) &&
                  PlacesUtils.asQuery(node.parent).queryOptions.resultType ==
                    Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAGS_ROOT) {
         // 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 }));
+        let urls = new Set();
+        await PlacesUtils.bookmarks.fetch({tags: [tag]}, b => urls.add(b.url));
+        transactions.push(PlacesTransactions.Untag({ tag, urls: Array.from(urls) }));
       } else if (PlacesUtils.nodeIsURI(node) &&
                  PlacesUtils.nodeIsQuery(node.parent) &&
                  PlacesUtils.asQuery(node.parent).queryOptions.queryType ==
                    Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) {
         // This is a uri node inside an history query.
         await 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 &&
--- 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
@@ -82,15 +82,15 @@ add_task(async function test_tags() {
       } 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"), [],
+  Assert.equal(await PlacesUtils.bookmarks.fetch({tags: ["test"]}), null,
                    "There should be no URIs remaining for the tag");
 
   tagsNode.containerOpen = false;
 
   library.close();
 });
--- a/services/sync/tests/unit/test_engine_changes_during_sync.js
+++ b/services/sync/tests/unit/test_engine_changes_during_sync.js
@@ -416,19 +416,20 @@ add_task(async function test_bookmark_ch
     let bmk2 = await PlacesUtils.bookmarks.fetch({
       guid: bmk2_guid,
     });
     ok(bmk2, "Remote bookmark should be applied during first sync");
     await assertChildGuids(folder1.guid, [tbBmk.guid, bmk2_guid, bmk3.guid],
       "Folder 1 should have 3 children after first sync");
     await assertChildGuids(folder2_guid, [bmk4_guid, tagQuery_guid],
       "Folder 2 should have 2 children after first sync");
-    let taggedURIs = PlacesUtils.tagging.getURIsForTag("taggy");
+    let taggedURIs = [];
+    await PlacesUtils.bookmarks.fetch({tags: ["taggy"]}, b => taggedURIs.push(b.url));
     equal(taggedURIs.length, 1, "Should have 1 tagged URI");
-    equal(taggedURIs[0].spec, "https://example.org/",
+    equal(taggedURIs[0].href, "https://example.org/",
       "Synced tagged bookmark should appear in tagged URI list");
 
     changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(changes, {},
       "Should have already uploaded changes in follow-up sync");
 
     // First ping won't include validation data, since we've changed bookmarks
     // and `canValidate` will indicate it can't proceed.
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -775,25 +775,22 @@ var Bookmarks = Object.freeze({
                                                updatedItem._parentId,
                                                updatedItem.guid,
                                                updatedItem.parentGuid, "",
                                                updatedItem.source ],
                                                { isTagging });
           // If we're updating a tag, we must notify all the tagged bookmarks
           // about the change.
           if (isTagging) {
-            let URIs = PlacesUtils.tagging.getURIsForTag(updatedItem.title);
-            for (let uri of URIs) {
-              for (let entry of (await fetchBookmarksByURL({ url: new URL(uri.spec) }, true))) {
-                notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
-                                                     PlacesUtils.toPRTime(entry.lastModified),
-                                                     entry.type, entry._parentId,
-                                                     entry.guid, entry.parentGuid,
-                                                     "", updatedItem.source ]);
-              }
+            for (let entry of (await fetchBookmarksByTags({ tags: [updatedItem.title] }, true))) {
+              notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
+                                                    PlacesUtils.toPRTime(entry.lastModified),
+                                                    entry.type, entry._parentId,
+                                                    entry.guid, entry.parentGuid,
+                                                    "", updatedItem.source ]);
             }
           }
         }
         if (updateInfo.hasOwnProperty("url")) {
           await PlacesUtils.keywords.reassign(item.url, updatedItem.url,
                                               updatedItem.source);
           notify(observers, "onItemChanged", [ updatedItem._id, "uri",
                                                false, updatedItem.url.href,
@@ -1195,16 +1192,23 @@ var Bookmarks = Object.freeze({
    *  - url
    *      retrieves the most recent bookmark having the given URL.
    *      To retrieve ALL of the bookmarks for that URL, you must pass in an
    *      onResult callback, that will be invoked once for each found bookmark.
    *  - guidPrefix
    *      retrieves the most recent item with the specified guid prefix.
    *      To retrieve ALL of the bookmarks for that guid prefix, you must pass
    *      in an onResult callback, that will be invoked once for each bookmark.
+   *  - tags
+   *      Retrieves the most recent item with all the specified tags.
+   *      The tags are matched in a case-insensitive way.
+   *      To retrieve ALL of the bookmarks having these tags, pass in an
+   *      onResult callback, that will be invoked once for each bookmark.
+   *      Note, there can be multiple bookmarks for the same url, if you need
+   *      unique tagged urls you can filter duplicates by accumulating in a Set.
    *
    * @param guidOrInfo
    *        The globally unique identifier of the item to fetch, or an
    *        object representing it, as defined above.
    * @param onResult [optional]
    *        Callback invoked for each found bookmark.
    * @param options [optional]
    *        an optional object whose properties describe options for the fetch:
@@ -1230,25 +1234,28 @@ var Bookmarks = Object.freeze({
       throw new Error("onResult callback must be a valid function");
     let info = guidOrInfo;
     if (!info)
       throw new Error("Input should be a valid object");
     if (typeof(info) != "object") {
       info = { guid: guidOrInfo };
     } else if (Object.keys(info).length == 1) {
       // Just a faster code path.
-      if (!["url", "guid", "parentGuid", "index", "guidPrefix"].includes(Object.keys(info)[0]))
+      if (!["url", "guid", "parentGuid",
+            "index", "guidPrefix", "tags"].includes(Object.keys(info)[0])) {
         throw new Error(`Unexpected number of conditions provided: 0`);
+      }
     } else {
       // Only one condition at a time can be provided.
       let conditionsCount = [
         v => v.hasOwnProperty("guid"),
         v => v.hasOwnProperty("parentGuid") && v.hasOwnProperty("index"),
         v => v.hasOwnProperty("url"),
-        v => v.hasOwnProperty("guidPrefix")
+        v => v.hasOwnProperty("guidPrefix"),
+        v => v.hasOwnProperty("tags")
       ].reduce((old, fn) => old + fn(info) | 0, 0);
       if (conditionsCount != 1)
         throw new Error(`Unexpected number of conditions provided: ${conditionsCount}`);
     }
 
     let behavior = {};
     if (info.hasOwnProperty("parentGuid") || info.hasOwnProperty("index")) {
       behavior = {
@@ -1269,16 +1276,19 @@ var Bookmarks = Object.freeze({
       if (fetchInfo.hasOwnProperty("url"))
         results = await fetchBookmarksByURL(fetchInfo, options && options.concurrent);
       else if (fetchInfo.hasOwnProperty("guid"))
         results = await fetchBookmark(fetchInfo, options && options.concurrent);
       else if (fetchInfo.hasOwnProperty("parentGuid") && fetchInfo.hasOwnProperty("index"))
         results = await fetchBookmarkByPosition(fetchInfo, options && options.concurrent);
       else if (fetchInfo.hasOwnProperty("guidPrefix"))
         results = await fetchBookmarksByGUIDPrefix(fetchInfo, options && options.concurrent);
+      else if (fetchInfo.hasOwnProperty("tags")) {
+        results = await fetchBookmarksByTags(fetchInfo, options && options.concurrent);
+      }
 
       if (!results)
         return null;
 
       if (!Array.isArray(results))
         results = [results];
       // Remove non-enumerable properties.
       results = results.map(r => Object.assign({}, r));
@@ -2049,16 +2059,52 @@ async function fetchBookmarkByPosition(i
   if (concurrent) {
     let db = await PlacesUtils.promiseDBConnection();
     return query(db);
   }
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmarkByPosition",
                                            query);
 }
 
+async function fetchBookmarksByTags(info, concurrent) {
+  let query = async function(db) {
+    let rows = await db.executeCached(
+      `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
+              b.dateAdded, b.lastModified, b.type, IFNULL(b.title, "") AS title,
+              h.url AS url, b.id AS _id, b.parent AS _parentId,
+              NULL AS _childCount,
+              p.parent AS _grandParentId, b.syncStatus AS _syncStatus
+       FROM moz_bookmarks b
+       JOIN moz_bookmarks p ON p.id = b.parent
+       JOIN moz_bookmarks g ON g.id = p.parent
+       JOIN moz_places h ON h.id = b.fk
+       WHERE g.guid <> ? AND b.fk IN (
+          SELECT b2.fk FROM moz_bookmarks b2
+          JOIN moz_bookmarks p2 ON p2.id = b2.parent
+          JOIN moz_bookmarks g2 ON g2.id = p2.parent
+          WHERE g2.guid = ?
+                AND lower(p2.title) IN (
+                  ${new Array(info.tags.length).fill("?").join(",")}
+                )
+          GROUP BY b2.fk HAVING count(*) = ${info.tags.length}
+       )
+       ORDER BY b.lastModified DESC
+      `, [Bookmarks.tagsGuid, Bookmarks.tagsGuid].concat(info.tags.map(t => t.toLowerCase())));
+
+    return rows.length ? rowsToItemsArray(rows) : null;
+  };
+
+  if (concurrent) {
+    let db = await PlacesUtils.promiseDBConnection();
+    return query(db);
+  }
+  return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmarksByTags",
+                                           query);
+}
+
 async function fetchBookmarksByGUIDPrefix(info, concurrent) {
   let query = async function(db) {
     let rows = await db.executeCached(
       `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
               b.dateAdded, b.lastModified, b.type, IFNULL(b.title, "") AS title,
               h.url AS url, b.id AS _id, b.parent AS _parentId,
               NULL AS _childCount,
               p.parent AS _grandParentId, b.syncStatus AS _syncStatus
--- a/toolkit/components/places/PlacesTransactions.jsm
+++ b/toolkit/components/places/PlacesTransactions.jsm
@@ -1592,18 +1592,20 @@ PT.Untag.prototype = {
  * 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 urls = new Set();
+    await PlacesUtils.bookmarks.fetch({tags: [oldTag]}, b => urls.add(b.url));
+    if (urls.size > 0) {
+      urls = Array.from(urls);
       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] })
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -247,17 +247,18 @@ const BOOKMARK_VALIDATORS = Object.freez
     return v;
   },
   source: simpleValidateFunc(v => Number.isInteger(v) &&
                                   Object.values(PlacesUtils.bookmarks.SOURCES).includes(v)),
   annos: simpleValidateFunc(v => Array.isArray(v) && v.length),
   keyword: simpleValidateFunc(v => (typeof(v) == "string") && v.length),
   charset: simpleValidateFunc(v => (typeof(v) == "string") && v.length),
   postData: simpleValidateFunc(v => (typeof(v) == "string") && v.length),
-  tags: simpleValidateFunc(v => Array.isArray(v) && v.length),
+  tags: simpleValidateFunc(v => Array.isArray(v) && v.length &&
+                                v.every(item => item && typeof item == "string")),
 });
 
 // Sync bookmark records can contain additional properties.
 const SYNC_BOOKMARK_VALIDATORS = Object.freeze({
   // Sync uses Places GUIDs for all records except roots.
   recordId: simpleValidateFunc(v => typeof v == "string" && (
                                 (PlacesSyncUtils.bookmarks.ROOTS.includes(v) || PlacesUtils.isValidGuid(v)))),
   parentRecordId: v => SYNC_BOOKMARK_VALIDATORS.recordId(v),
--- a/toolkit/components/places/nsITaggingService.idl
+++ b/toolkit/components/places/nsITaggingService.idl
@@ -44,25 +44,16 @@ interface nsITaggingService : nsISupport
    *        A change source constant from nsINavBookmarksService::SOURCE_*.
    *        Defaults to SOURCE_DEFAULT if omitted.
    */
   void untagURI(in nsIURI aURI,
                 in nsIVariant aTags,
                 [optional] in unsigned short aSource);
 
   /**
-   * Retrieves all URLs tagged with the given tag.
-   *
-   * @param aTag
-   *        tag name
-   * @returns Array of uris tagged with aTag.
-   */
-  nsIVariant getURIsForTag(in AString aTag);
-
-  /**
    * Retrieves all tags set for the given URL.
    *
    * @param aURI
    *        a URL.
    * @returns array of tags (sorted by name).
    */
   void getTagsForURI(in nsIURI aURI,
                      [optional] out unsigned long length,
--- a/toolkit/components/places/nsTaggingService.js
+++ b/toolkit/components/places/nsTaggingService.js
@@ -225,52 +225,16 @@ TaggingService.prototype = {
           // There is a tagged item.
           PlacesUtils.bookmarks.removeItem(itemId, aSource);
         }
       }
     }
   },
 
   // nsITaggingService
-  getURIsForTag: function TS_getURIsForTag(aTagName) {
-    if (!aTagName || aTagName.length == 0) {
-      throw Components.Exception("Invalid tag name", Cr.NS_ERROR_INVALID_ARG);
-    }
-
-    if (/^\s|\s$/.test(aTagName)) {
-      throw Components.Exception("Tag passed to getURIsForTag was not trimmed",
-                                 Cr.NS_ERROR_INVALID_ARG);
-    }
-
-    let uris = [];
-    let tagId = this._getItemIdForTag(aTagName);
-    if (tagId == -1)
-      return uris;
-
-    let db = PlacesUtils.history.DBConnection;
-    let stmt = db.createStatement(
-      `SELECT h.url FROM moz_places h
-       JOIN moz_bookmarks b ON b.fk = h.id
-       WHERE b.parent = :tag_id`
-    );
-    stmt.params.tag_id = tagId;
-    try {
-      while (stmt.executeStep()) {
-        try {
-          uris.push(Services.io.newURI(stmt.row.url));
-        } catch (ex) {}
-      }
-    } finally {
-      stmt.finalize();
-    }
-
-    return uris;
-  },
-
-  // nsITaggingService
   getTagsForURI: function TS_getTagsForURI(aURI, aCount) {
     if (!aURI) {
       throw Components.Exception("Invalid uri", Cr.NS_ERROR_INVALID_ARG);
     }
 
     let tags = [];
     let db = PlacesUtils.history.DBConnection;
     let stmt = db.createStatement(
--- a/toolkit/components/places/tests/bookmarks/test_tags.js
+++ b/toolkit/components/places/tests/bookmarks/test_tags.js
@@ -1,12 +1,12 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
-add_task(async function() {
+add_task(async function test_fetchTags() {
   let tags = await PlacesUtils.bookmarks.fetchTags();
   Assert.deepEqual(tags, []);
 
   let bm = await PlacesUtils.bookmarks.insert({
     url: "http://page1.com/",
     parentGuid: PlacesUtils.bookmarks.unfiledGuid,
   });
 
@@ -29,8 +29,60 @@ add_task(async function() {
   });
   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 },
   ]);
 });
+
+add_task(async function test_fetch_by_tags() {
+  Assert.throws(() => PlacesUtils.bookmarks.fetch({ tags: "" }),
+                /Invalid value for property 'tags'/);
+  Assert.throws(() => PlacesUtils.bookmarks.fetch({ tags: [] }),
+                /Invalid value for property 'tags'/);
+  Assert.throws(() => PlacesUtils.bookmarks.fetch({ tags: null }),
+                /Invalid value for property 'tags'/);
+  Assert.throws(() => PlacesUtils.bookmarks.fetch({ tags: [""] }),
+                /Invalid value for property 'tags'/);
+  Assert.throws(() => PlacesUtils.bookmarks.fetch({ tags: ["valid", null] }),
+                /Invalid value for property 'tags'/);
+
+  info("Add bookmarks with tags.");
+  let bm1 = await PlacesUtils.bookmarks.insert({
+    url: "http://bacon.org/",
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+  });
+  PlacesUtils.tagging.tagURI(Services.io.newURI(bm1.url.href), ["egg", "ratafià"]);
+  let bm2 = await PlacesUtils.bookmarks.insert({
+    url: "http://mushroom.org/",
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+  });
+  PlacesUtils.tagging.tagURI(Services.io.newURI(bm2.url.href), ["egg"]);
+
+  info("Fetch a single tag.");
+  let bms = [];
+  Assert.equal((await PlacesUtils.bookmarks.fetch({tags: ["egg"]}, b => bms.push(b))).guid,
+               bm2.guid, "Found the expected recent bookmark");
+  Assert.deepEqual(bms.map(b => b.guid), [bm2.guid, bm1.guid],
+                   "Found the expected bookmarks");
+
+  info("Fetch multiple tags.");
+  bms = [];
+  Assert.equal((await PlacesUtils.bookmarks.fetch({tags: ["egg", "ratafià"]}, b => bms.push(b))).guid,
+               bm1.guid, "Found the expected recent bookmark");
+  Assert.deepEqual(bms.map(b => b.guid), [bm1.guid],
+                   "Found the expected bookmarks");
+
+  info("Fetch a nonexisting tag.");
+  bms = [];
+  Assert.equal(await PlacesUtils.bookmarks.fetch({tags: ["egg", "tomato"]}, b => bms.push(b)),
+               null, "Should not find any bookmark");
+  Assert.deepEqual(bms, [], "Should not find any bookmark");
+
+  info("Check case insensitive");
+  bms = [];
+  Assert.equal((await PlacesUtils.bookmarks.fetch({tags: ["eGg", "raTafiÀ"]}, b => bms.push(b))).guid,
+               bm1.guid, "Found the expected recent bookmark");
+  Assert.deepEqual(bms.map(b => b.guid), [bm1.guid],
+                   "Found the expected bookmarks");
+});
--- a/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
@@ -956,22 +956,22 @@ add_task(async function test_rewrite_tag
   }]);
 
   info("Apply remote");
   let changesToUpload = await buf.apply();
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
 
   deepEqual(changesToUpload, {}, "Should not reupload any local records");
 
-  let urisWithTaggy = PlacesUtils.tagging.getURIsForTag("taggy");
-  deepEqual(urisWithTaggy.map(uri => uri.spec).sort(), ["http://example.com/e"],
+  let bmWithTaggy = await PlacesUtils.bookmarks.fetch({tags: ["taggy"]});
+  equal(bmWithTaggy.url.href, "http://example.com/e",
     "Should insert bookmark with new tag");
 
-  let urisWithKitty = PlacesUtils.tagging.getURIsForTag("kitty");
-  deepEqual(urisWithKitty.map(uri => uri.spec).sort(), ["http://example.com/d"],
+  let bmWithKitty = await PlacesUtils.bookmarks.fetch({tags: ["kitty"]});
+  equal(bmWithKitty.url.href, "http://example.com/d",
     "Should retain existing tag");
 
   let { root: toolbarContainer } = PlacesUtils.getFolderContents(
     PlacesUtils.bookmarks.toolbarGuid, false, true);
   equal(toolbarContainer.childCount, 3,
     "Should add queries and bookmark to toolbar");
 
   let containerForB = PlacesUtils.asContainer(toolbarContainer.getChild(0));
--- a/toolkit/components/places/tests/sync/test_sync_utils.js
+++ b/toolkit/components/places/tests/sync/test_sync_utils.js
@@ -32,19 +32,21 @@ function shuffle(array) {
   for (let i = 0; i < array.length; ++i) {
     let randomIndex = Math.floor(Math.random() * (i + 1));
     results[i] = results[randomIndex];
     results[randomIndex] = array[i];
   }
   return results;
 }
 
-function assertTagForURLs(tag, urls, message) {
-  let taggedURLs = PlacesUtils.tagging.getURIsForTag(tag).map(uri => uri.spec);
-  deepEqual(taggedURLs.sort(compareAscending), urls.sort(compareAscending), message);
+async function assertTagForURLs(tag, urls, message) {
+  let taggedURLs = new Set();
+  await PlacesUtils.bookmarks.fetch({tags: [tag]}, b => taggedURLs.add(b.url.href));
+  deepEqual(Array.from(taggedURLs).sort(compareAscending),
+            urls.sort(compareAscending), message);
 }
 
 function assertURLHasTags(url, tags, message) {
   let actualTags = PlacesUtils.tagging.getTagsForURI(uri(url));
   deepEqual(actualTags.sort(compareAscending), tags, message);
 }
 
 var populateTree = async function populate(parentGuid, ...items) {
@@ -576,17 +578,17 @@ add_task(async function test_update_tags
   {
     let updatedItem = await PlacesSyncUtils.bookmarks.update({
       recordId: item.recordId,
       tags: ["foo", "baz"],
     });
     deepEqual(updatedItem.tags, ["foo", "baz"], "Should return updated tags");
     assertURLHasTags("https://mozilla.org", ["baz", "foo"],
       "Should update tags for URL");
-    assertTagForURLs("bar", [], "Should remove existing tag");
+    await assertTagForURLs("bar", [], "Should remove existing tag");
   }
 
   info("Tags with whitespace");
   {
     let updatedItem = await PlacesSyncUtils.bookmarks.update({
       recordId: item.recordId,
       tags: [" leading", "trailing ", " baz ", " "],
     });
@@ -661,17 +663,17 @@ add_task(async function test_pullChanges
 
   info("Change tag case");
   {
     PlacesUtils.tagging.tagURI(uri("https://mozilla.org"), ["TaGgY"]);
     let changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
       [firstItem.recordId, secondItem.recordId, taggedItem.recordId].sort(),
       "Should include tagged bookmarks after changing case");
-    assertTagForURLs("TaGgY", ["https://example.org/", "https://mozilla.org/"],
+    await assertTagForURLs("TaGgY", ["https://example.org/", "https://mozilla.org/"],
       "Should add tag for new URL");
     await setChangesSynced(changes);
   }
 
   // These tests change a tag item directly, without going through the tagging
   // service. This behavior isn't supported, but the tagging service registers
   // an observer to handle these cases, so we make sure we handle them
   // correctly.
@@ -710,27 +712,27 @@ add_task(async function test_pullChanges
       index: 0
     });
     bm.url = "https://bugzilla.org/";
     await PlacesUtils.bookmarks.update(bm);
     let changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
       [firstItem.recordId, secondItem.recordId, untaggedItem.recordId].sort(),
       "Should include tagged bookmarks after changing tag entry URI");
-    assertTagForURLs("tricky", ["https://bugzilla.org/", "https://mozilla.org/"],
+    await assertTagForURLs("tricky", ["https://bugzilla.org/", "https://mozilla.org/"],
       "Should remove tag entry for old URI");
     await setChangesSynced(changes);
 
-    bm.url = "https://example.com/";
+    bm.url = "https://example.org/";
     await PlacesUtils.bookmarks.update(bm);
     changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
-      [untaggedItem.recordId].sort(),
+      [firstItem.recordId, secondItem.recordId, untaggedItem.recordId].sort(),
       "Should include tagged bookmarks after changing tag entry URL");
-    assertTagForURLs("tricky", ["https://example.com/", "https://mozilla.org/"],
+    await assertTagForURLs("tricky", ["https://example.org/", "https://mozilla.org/"],
       "Should remove tag entry for old URL");
     await setChangesSynced(changes);
   }
 
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
 
@@ -1308,23 +1310,23 @@ add_task(async function test_insert_tags
     url: "place:queryType=1&sort=12&maxResults=10",
     recordId: makeGuid(),
     parentRecordId: "toolbar",
     folder: "bar",
     tags: ["baz", "qux"],
     title: "bar",
   }].map(info => PlacesSyncUtils.bookmarks.insert(info)));
 
-  assertTagForURLs("foo", ["https://example.com/", "https://example.org/"],
+  await assertTagForURLs("foo", ["https://example.com/", "https://example.org/"],
     "2 URLs with new tag");
-  assertTagForURLs("bar", ["https://example.com/"], "1 URL with existing tag");
-  assertTagForURLs("baz", ["https://example.org/",
+  await assertTagForURLs("bar", ["https://example.com/"], "1 URL with existing tag");
+  await assertTagForURLs("baz", ["https://example.org/",
     "place:queryType=1&sort=12&maxResults=10"],
     "Should support tagging URLs and tag queries");
-  assertTagForURLs("qux", ["place:queryType=1&sort=12&maxResults=10"],
+  await assertTagForURLs("qux", ["place:queryType=1&sort=12&maxResults=10"],
     "Should support tagging tag queries");
 
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(async function test_insert_tags_whitespace() {
   info("Untrimmed and blank tags");
@@ -1348,17 +1350,17 @@ add_task(async function test_insert_tags
     parentRecordId: "toolbar",
     tags: [" taggy", "taggy ", " taggy ", "taggy"],
   });
   deepEqual(taggedDupes.tags, ["taggy", "taggy", "taggy", "taggy"],
     "Should return trimmed and dupe tags");
   assertURLHasTags("https://example.net/", ["taggy"],
     "Should ignore dupes when setting tags");
 
-  assertTagForURLs("taggy", ["https://example.net/", "https://example.org/"],
+  await assertTagForURLs("taggy", ["https://example.net/", "https://example.org/"],
     "Should exclude falsy tags");
 
   PlacesUtils.tagging.untagURI(uri("https://example.org"), ["untrimmed", "taggy"]);
   PlacesUtils.tagging.untagURI(uri("https://example.net"), ["taggy"]);
   deepEqual((await PlacesUtils.bookmarks.fetchTags()).map(t => t.name), [], "Should clean up all tags");
 
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
--- a/toolkit/components/places/tests/unit/test_tagging.js
+++ b/toolkit/components/places/tests/unit/test_tagging.js
@@ -53,22 +53,16 @@ function run_test() {
   var uri1tags = tagssvc.getTagsForURI(uri1);
   Assert.equal(uri1tags.length, 2);
   Assert.equal(uri1tags[0], "Tag 1");
   Assert.equal(uri1tags[1], "Tag 2");
   var uri2tags = tagssvc.getTagsForURI(uri2);
   Assert.equal(uri2tags.length, 1);
   Assert.equal(uri2tags[0], "Tag 1");
 
-  // test getURIsForTag
-  var tag1uris = tagssvc.getURIsForTag("tag 1");
-  Assert.equal(tag1uris.length, 2);
-  Assert.ok(tag1uris[0].equals(uri1));
-  Assert.ok(tag1uris[1].equals(uri2));
-
   // test untagging
   tagssvc.untagURI(uri1, ["tag 1"]);
   Assert.equal(tag1node.childCount, 1);
 
   // removing the last uri from a tag should remove the tag-container
   tagssvc.untagURI(uri2, ["tag 1"]);
   Assert.equal(tagRoot.childCount, 1);