Bug 653816 - returning only nontags for GetBookmarkIdsForURI and fixing consumers, r=mak

Most consumers of `GetBookmarkIdsForURI` already don't need tags, the only
consumer which does (`TaggingService`) has been changed to use a separate
database query.

MozReview-Commit-ID: LabjaA6Q0GF

--HG--
extra : rebase_source : e13dc730a53b5b46ca1766bf896112aa65aa00af

toolkit/components/places/PlacesUtils.jsm

@@ -1332,17 +1332,7 @@ this.PlacesUtils = {
    */
   getBookmarksForURI:
   function PU_getBookmarksForURI(aURI) {
-    var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI);
+    return this.bookmarks.getBookmarkIdsForURI(aURI);
-
-    // filter the ids list
-    return bmkIds.filter(function(aID) {
-      var parentId = this.bookmarks.getFolderIdForItem(aID);
-      var grandparentId = this.bookmarks.getFolderIdForItem(parentId);
-      // item under a tag container
-      if (grandparentId == this.tagsFolderId)
-        return false;
-      return true;
-    }, this);
   },
 
   /**
@@ -1355,23 +1345,8 @@ this.PlacesUtils = {
    */
   getMostRecentBookmarkForURI:
   function PU_getMostRecentBookmarkForURI(aURI) {
-    var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI);
+    let bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI);
-    for (var i = 0; i < bmkIds.length; i++) {
+    return bmkIds.length ? bmkIds[0] : -1;
-      // Find the first folder which isn't a tag container
-      var itemId = bmkIds[i];
-      var parentId = this.bookmarks.getFolderIdForItem(itemId);
-      // Optimization: if this is a direct child of a root we don't need to
-      // check if its grandparent is a tag.
-      if (parentId == this.unfiledBookmarksFolderId ||
-          parentId == this.toolbarFolderId ||
-          parentId == this.bookmarksMenuFolderId)
-        return itemId;
-
-      var grandparentId = this.bookmarks.getFolderIdForItem(parentId);
-      if (grandparentId != this.tagsFolderId)
-        return itemId;
-    }
-    return -1;
   },
 
   /**

toolkit/components/places/nsNavBookmarks.cpp

@@ -2574,8 +2574,7 @@ nsNavBookmarks::GetFolderIdForItem(int64_t aItemId, int64_t* _parentId)
 
 nsresult
 nsNavBookmarks::GetBookmarkIdsForURITArray(nsIURI* aURI,
-                                           nsTArray<int64_t>& aResult,
+                                           nsTArray<int64_t>& aResult)
-                                           bool aSkipTags)
 {
   NS_ENSURE_ARG(aURI);
 
@@ -2584,10 +2583,11 @@ nsNavBookmarks::GetBookmarkIdsForURITArray(nsIURI* aURI,
   // Note: not using a JOIN is cheaper in this case.
   nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
     "/* do not warn (bug 1175249) */ "
-    "SELECT b.id, b.guid, b.parent, b.lastModified, t.guid, t.parent "
+    "SELECT b.id "
     "FROM moz_bookmarks b "
     "JOIN moz_bookmarks t on t.id = b.parent "
-    "WHERE b.fk = (SELECT id FROM moz_places WHERE url_hash = hash(:page_url) AND url = :page_url) "
+    "WHERE b.fk = (SELECT id FROM moz_places WHERE url_hash = hash(:page_url) AND url = :page_url) AND "
+    "t.parent IS NOT :tags_root "
     "ORDER BY b.lastModified DESC, b.id DESC "
   );
   NS_ENSURE_STATE(stmt);
@@ -2595,18 +2595,11 @@ nsNavBookmarks::GetBookmarkIdsForURITArray(nsIURI* aURI,
 
   nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), aURI);
   NS_ENSURE_SUCCESS(rv, rv);
+  rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("tags_root"), mTagsRoot);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   bool more;
   while (NS_SUCCEEDED((rv = stmt->ExecuteStep(&more))) && more) {
-    if (aSkipTags) {
-      // Skip tags, for the use-cases of this async getter they are useless.
-      int64_t grandParentId;
-      nsresult rv = stmt->GetInt64(5, &grandParentId);
-      NS_ENSURE_SUCCESS(rv, rv);
-      if (grandParentId == mTagsRoot) {
-        continue;
-      }
-    }
     int64_t bookmarkId;
     rv = stmt->GetInt64(0, &bookmarkId);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -2685,8 +2678,7 @@ nsNavBookmarks::GetBookmarkIdsForURI(nsIURI* aURI, uint32_t* aCount,
   nsTArray<int64_t> bookmarks;
 
   // Get the information from the DB as a TArray
-  // TODO (bug 653816): make this API skip tags by default.
+  nsresult rv = GetBookmarkIdsForURITArray(aURI, bookmarks);
-  nsresult rv = GetBookmarkIdsForURITArray(aURI, bookmarks, false);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Copy the results into a new array for output

toolkit/components/places/nsNavBookmarks.h

@@ -400,12 +400,9 @@ private:
    *        URI to get bookmarks for.
    * @param aResult
    *        Array of bookmark ids.
-   * @param aSkipTags
-   *        If true ids of tags-as-bookmarks entries will be excluded.
    */
   nsresult GetBookmarkIdsForURITArray(nsIURI* aURI,
-                                      nsTArray<int64_t>& aResult,
+                                      nsTArray<int64_t>& aResult);
-                                      bool aSkipTags);
 
   nsresult GetBookmarksForURI(nsIURI* aURI,
                               nsTArray<BookmarkData>& _bookmarks);

toolkit/components/places/nsTaggingService.js

@@ -296,20 +296,35 @@ TaggingService.prototype = {
     if (!aURI)
       throw Cr.NS_ERROR_INVALID_ARG;
 
-    var tags = [];
+    let tags = [];
-    var bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(aURI);
+    let db = PlacesUtils.history.DBConnection;
-    for (var i = 0; i < bookmarkIds.length; i++) {
+    let stmt = db.createStatement(
-      var folderId = PlacesUtils.bookmarks.getFolderIdForItem(bookmarkIds[i]);
+      `SELECT t.id AS folderId
-      if (this._tagFolders[folderId])
+       FROM moz_bookmarks b
-        tags.push(this._tagFolders[folderId]);
+       JOIN moz_bookmarks t on t.id = b.parent
+       WHERE b.fk = (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url) AND
+       t.parent = :tags_root
+       ORDER BY b.lastModified DESC, b.id DESC`
+    );
+    stmt.params.url = aURI.spec;
+    stmt.params.tags_root = PlacesUtils.tagsFolderId;
+    try {
+      while (stmt.executeStep()) {
+        try {
+          tags.push(this._tagFolders[stmt.row.folderId]);
+        } catch (ex) {}
+      }
+    } finally {
+      stmt.finalize();
     }
 
     // sort the tag list
     tags.sort(function(a, b) {
         return a.toLowerCase().localeCompare(b.toLowerCase());
       });
-    if (aCount)
+    if (aCount) {
       aCount.value = tags.length;
+    }
     return tags;
   },