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

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

browser/components/places/content/controller.js

@@ -810,8 +810,9 @@ PlacesController.prototype = {
         // 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);
+        let urls = new Set();
-        transactions.push(PlacesTransactions.Untag({ tag, urls: URIs }));
+        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 ==

browser/components/places/tests/browser/browser_library_delete_bookmarks_in_tags.js

@@ -87,7 +87,7 @@ add_task(async function test_tags() {
   }
 
   // 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;

services/sync/tests/unit/test_engine_changes_during_sync.js

@@ -421,9 +421,10 @@ add_task(async function test_bookmark_change_during_sync() {
       "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();

toolkit/components/places/Bookmarks.jsm

@@ -780,15 +780,12 @@ var Bookmarks = Object.freeze({
           // 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 entry of (await fetchBookmarksByTags({ tags: [updatedItem.title] }, true))) {
-            for (let uri of URIs) {
+              notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
-              for (let entry of (await fetchBookmarksByURL({ url: new URL(uri.spec) }, true))) {
+                                                    PlacesUtils.toPRTime(entry.lastModified),
-                notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
+                                                    entry.type, entry._parentId,
-                                                     PlacesUtils.toPRTime(entry.lastModified),
+                                                    entry.guid, entry.parentGuid,
-                                                     entry.type, entry._parentId,
+                                                    "", updatedItem.source ]);
-                                                     entry.guid, entry.parentGuid,
-                                                     "", updatedItem.source ]);
-              }
             }
           }
         }
@@ -1200,6 +1197,13 @@ var Bookmarks = Object.freeze({
    *      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
@@ -1235,15 +1239,18 @@ var Bookmarks = Object.freeze({
       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}`);
@@ -1274,6 +1281,9 @@ var Bookmarks = Object.freeze({
         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;
@@ -2054,6 +2064,42 @@ async function fetchBookmarkByPosition(info, concurrent) {
                                            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(

toolkit/components/places/PlacesTransactions.jsm

@@ -1597,8 +1597,10 @@ PT.RenameTag.prototype = {
     // 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);
+    let urls = new Set();
-    if (urls.length > 0) {
+    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] })
       );

toolkit/components/places/PlacesUtils.jsm

@@ -252,7 +252,8 @@ const BOOKMARK_VALIDATORS = Object.freeze({
   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.

toolkit/components/places/nsITaggingService.idl

@@ -48,15 +48,6 @@ interface nsITaggingService : nsISupports
                 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.
    *

toolkit/components/places/nsTaggingService.js

@@ -229,42 +229,6 @@ TaggingService.prototype = {
     }
   },
 
-  // 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) {

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

@@ -1,7 +1,7 @@
 /* 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, []);
 
@@ -34,3 +34,55 @@ add_task(async function() {
     { 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");
+});

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

@@ -961,12 +961,12 @@ add_task(async function test_rewrite_tag_queries() {
 
   deepEqual(changesToUpload, {}, "Should not reupload any local records");
 
-  let urisWithTaggy = PlacesUtils.tagging.getURIsForTag("taggy");
+  let bmWithTaggy = await PlacesUtils.bookmarks.fetch({tags: ["taggy"]});
-  deepEqual(urisWithTaggy.map(uri => uri.spec).sort(), ["http://example.com/e"],
+  equal(bmWithTaggy.url.href, "http://example.com/e",
     "Should insert bookmark with new tag");
 
-  let urisWithKitty = PlacesUtils.tagging.getURIsForTag("kitty");
+  let bmWithKitty = await PlacesUtils.bookmarks.fetch({tags: ["kitty"]});
-  deepEqual(urisWithKitty.map(uri => uri.spec).sort(), ["http://example.com/d"],
+  equal(bmWithKitty.url.href, "http://example.com/d",
     "Should retain existing tag");
 
   let { root: toolbarContainer } = PlacesUtils.getFolderContents(

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

@@ -37,9 +37,11 @@ function shuffle(array) {
   return results;
 }
 
-function assertTagForURLs(tag, urls, message) {
+async function assertTagForURLs(tag, urls, message) {
-  let taggedURLs = PlacesUtils.tagging.getURIsForTag(tag).map(uri => uri.spec);
+  let taggedURLs = new Set();
-  deepEqual(taggedURLs.sort(compareAscending), urls.sort(compareAscending), message);
+  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) {
@@ -581,7 +583,7 @@ add_task(async function test_update_tags() {
     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");
@@ -666,7 +668,7 @@ add_task(async function test_pullChanges_tags() {
     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);
   }
@@ -715,17 +717,17 @@ add_task(async function test_pullChanges_tags() {
     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);
   }
@@ -1313,13 +1315,13 @@ add_task(async function test_insert_tags() {
     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");
+  await assertTagForURLs("bar", ["https://example.com/"], "1 URL with existing tag");
-  assertTagForURLs("baz", ["https://example.org/",
+  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();
@@ -1353,7 +1355,7 @@ add_task(async function test_insert_tags_whitespace() {
   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"]);

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

@@ -58,12 +58,6 @@ function run_test() {
   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);