Bug 653816 - returning only nontags for GetBookmarkIdsForURI and fixing consumers, r=mak
authormilindl <i.milind.luthra@gmail.com>
Tue, 30 May 2017 19:48:17 +0530
changeset 361918 291fe1a6a375d8f2a95908f3a28f93bfe16ec4f3
parent 361917 5c407f01edac0fe3837946e6e094529441caee94
child 361919 0a94b9164a649af95e7e645669bf6b2a1883745c
push id31952
push usercbook@mozilla.com
push dateFri, 02 Jun 2017 12:17:25 +0000
treeherdermozilla-central@194c009d6295 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs653816
milestone55.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 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
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/nsTaggingService.js
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1327,56 +1327,31 @@ this.PlacesUtils = {
     }
   },
 
   /**
    * Get all bookmarks for a URL, excluding items under tags.
    */
   getBookmarksForURI:
   function PU_getBookmarksForURI(aURI) {
-    var bmkIds = 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);
+    return this.bookmarks.getBookmarkIdsForURI(aURI);
   },
 
   /**
    * Get the most recently added/modified bookmark for a URL, excluding items
    * under tags.
    *
    * @param aURI
    *        nsIURI of the page we will look for.
    * @returns itemId of the found bookmark, or -1 if nothing is found.
    */
   getMostRecentBookmarkForURI:
   function PU_getMostRecentBookmarkForURI(aURI) {
-    var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI);
-    for (var i = 0; i < bmkIds.length; i++) {
-      // 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;
+    let bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI);
+    return bmkIds.length ? bmkIds[0] : -1;
   },
 
   /**
    * Returns a nsNavHistoryContainerResultNode with forced excludeItems and
    * expandQueries.
    * @param   aNode
    *          The node to convert
    * @param   [optional] excludeItems
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -2569,49 +2569,42 @@ nsNavBookmarks::GetFolderIdForItem(int64
 
   *_parentId = bookmark.parentId;
   return NS_OK;
 }
 
 
 nsresult
 nsNavBookmarks::GetBookmarkIdsForURITArray(nsIURI* aURI,
-                                           nsTArray<int64_t>& aResult,
-                                           bool aSkipTags)
+                                           nsTArray<int64_t>& aResult)
 {
   NS_ENSURE_ARG(aURI);
 
   // Double ordering covers possible lastModified ties, that could happen when
   // importing, syncing or due to extensions.
   // 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);
   mozStorageStatementScoper scoper(stmt);
 
   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);
     NS_ENSURE_TRUE(aResult.AppendElement(bookmarkId), NS_ERROR_OUT_OF_MEMORY);
   }
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
@@ -2680,18 +2673,17 @@ nsNavBookmarks::GetBookmarkIdsForURI(nsI
   NS_ENSURE_ARG_POINTER(aCount);
   NS_ENSURE_ARG_POINTER(aBookmarks);
 
   *aCount = 0;
   *aBookmarks = nullptr;
   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, false);
+  nsresult rv = GetBookmarkIdsForURITArray(aURI, bookmarks);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Copy the results into a new array for output
   if (bookmarks.Length()) {
     *aBookmarks =
       static_cast<int64_t*>(moz_xmalloc(sizeof(int64_t) * bookmarks.Length()));
     if (!*aBookmarks)
       return NS_ERROR_OUT_OF_MEMORY;
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -395,22 +395,19 @@ private:
    * TArray version of getBookmarksIdForURI for ease of use in C++ code.
    * Pass in a reference to a TArray; it will get filled with the
    * resulting list of bookmark IDs.
    *
    * @param aURI
    *        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,
-                                      bool aSkipTags);
+                                      nsTArray<int64_t>& aResult);
 
   nsresult GetBookmarksForURI(nsIURI* aURI,
                               nsTArray<BookmarkData>& _bookmarks);
 
   int64_t RecursiveFindRedirectedBookmark(int64_t aPlaceId);
 
   class RemoveFolderTransaction final : public nsITransaction {
   public:
--- a/toolkit/components/places/nsTaggingService.js
+++ b/toolkit/components/places/nsTaggingService.js
@@ -291,30 +291,45 @@ TaggingService.prototype = {
     return uris;
   },
 
   // nsITaggingService
   getTagsForURI: function TS_getTagsForURI(aURI, aCount) {
     if (!aURI)
       throw Cr.NS_ERROR_INVALID_ARG;
 
-    var tags = [];
-    var bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(aURI);
-    for (var i = 0; i < bookmarkIds.length; i++) {
-      var folderId = PlacesUtils.bookmarks.getFolderIdForItem(bookmarkIds[i]);
-      if (this._tagFolders[folderId])
-        tags.push(this._tagFolders[folderId]);
+    let tags = [];
+    let db = PlacesUtils.history.DBConnection;
+    let stmt = db.createStatement(
+      `SELECT t.id AS folderId
+       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(: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;
   },
 
   __tagFolders: null,
   get _tagFolders() {
     if (!this.__tagFolders) {
       this.__tagFolders = [];