Bug 1433307 - Remove synchronous Bookmarks::getKeywordForBookmark. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 09 Feb 2018 17:10:26 +0100
changeset 758886 d12756b14dd8917acd49676c43de9e05d0d3aec8
parent 758885 e0cf06d47f28c6e52409c4d24e4d48e0a0832136
push id100214
push usermak77@bonardo.net
push dateFri, 23 Feb 2018 09:05:58 +0000
reviewersstandard8
bugs1433307
milestone60.0a1
Bug 1433307 - Remove synchronous Bookmarks::getKeywordForBookmark. r=standard8 MozReview-Commit-ID: 1XR5N2WJ7P7
toolkit/components/places/nsINavBookmarksService.idl
toolkit/components/places/nsINavHistoryService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/nsNavHistoryResult.h
toolkit/components/places/tests/bookmarks/test_keywords.js
toolkit/components/places/tests/queries/test_sorting.js
toolkit/components/places/tests/unit/test_result_sort.js
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -532,24 +532,17 @@ interface nsINavBookmarksService : nsISu
   /**
 
   /**
    * Get the parent folder's id for an item.
    */
   long long getFolderIdForItem(in long long aItemId);
 
   /**
-   * Retrieves the keyword for the given bookmark. Will be void string
-   * (null in JS) if no such keyword is found.
-   *
-   * @deprecated Use PlacesUtils.keywords.fetch() API instead.
-   */
-  AString getKeywordForBookmark(in long long aItemId);
 
-  /**
    * Adds a bookmark observer. If ownsWeak is false, the bookmark service will
    * keep an owning reference to the observer.  If ownsWeak is true, then
    * aObserver must implement nsISupportsWeakReference, and the bookmark
    * service will keep a weak reference to the observer.
    */
   void addObserver(in nsINavBookmarkObserver observer,
                    [optional] in boolean ownsWeak);
 
--- a/toolkit/components/places/nsINavHistoryService.idl
+++ b/toolkit/components/places/nsINavHistoryService.idl
@@ -995,18 +995,16 @@ interface nsINavHistoryQueryOptions : ns
   const unsigned short SORT_BY_TITLE_ASCENDING = 1;
   const unsigned short SORT_BY_TITLE_DESCENDING = 2;
   const unsigned short SORT_BY_DATE_ASCENDING = 3;
   const unsigned short SORT_BY_DATE_DESCENDING = 4;
   const unsigned short SORT_BY_URI_ASCENDING = 5;
   const unsigned short SORT_BY_URI_DESCENDING = 6;
   const unsigned short SORT_BY_VISITCOUNT_ASCENDING = 7;
   const unsigned short SORT_BY_VISITCOUNT_DESCENDING = 8;
-  const unsigned short SORT_BY_KEYWORD_ASCENDING = 9;
-  const unsigned short SORT_BY_KEYWORD_DESCENDING = 10;
   const unsigned short SORT_BY_DATEADDED_ASCENDING = 11;
   const unsigned short SORT_BY_DATEADDED_DESCENDING = 12;
   const unsigned short SORT_BY_LASTMODIFIED_ASCENDING = 13;
   const unsigned short SORT_BY_LASTMODIFIED_DESCENDING = 14;
   const unsigned short SORT_BY_TAGS_ASCENDING = 17;
   const unsigned short SORT_BY_TAGS_DESCENDING = 18;
   const unsigned short SORT_BY_ANNOTATION_ASCENDING = 19;
   const unsigned short SORT_BY_ANNOTATION_DESCENDING = 20;
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -2245,54 +2245,16 @@ nsNavBookmarks::GetBookmarksForURI(nsIUR
   }
 
   return NS_OK;
 }
 
 
 
 NS_IMETHODIMP
-nsNavBookmarks::GetKeywordForBookmark(int64_t aBookmarkId, nsAString& aKeyword)
-{
-  NS_ENSURE_ARG_MIN(aBookmarkId, 1);
-  aKeyword.Truncate(0);
-
-  // We can have multiple keywords for the same uri, here we'll just return the
-  // last created one.
-  nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(NS_LITERAL_CSTRING(
-    "SELECT k.keyword "
-    "FROM moz_bookmarks b "
-    "JOIN moz_keywords k ON k.place_id = b.fk "
-    "WHERE b.id = :item_id "
-    "ORDER BY k.ROWID DESC "
-    "LIMIT 1"
-  ));
-  NS_ENSURE_STATE(stmt);
-  mozStorageStatementScoper scoper(stmt);
-
-  nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"),
-                             aBookmarkId);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  bool hasMore;
-  if (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) {
-    nsAutoString keyword;
-    rv = stmt->GetString(0, keyword);
-    NS_ENSURE_SUCCESS(rv, rv);
-    aKeyword = keyword;
-    return NS_OK;
-  }
-
-  aKeyword.SetIsVoid(true);
-
-  return NS_OK;
-}
-
-
-NS_IMETHODIMP
 nsNavBookmarks::RunInBatchMode(nsINavHistoryBatchCallback* aCallback,
                                nsISupports* aUserData) {
   AUTO_PROFILER_LABEL("nsNavBookmarks::RunInBatchMode", OTHER);
 
   NS_ENSURE_ARG(aCallback);
 
   mBatching = true;
 
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -737,20 +737,16 @@ nsNavHistoryContainerResultNode::GetSort
     case nsINavHistoryQueryOptions::SORT_BY_URI_ASCENDING:
       return &SortComparison_URILess;
     case nsINavHistoryQueryOptions::SORT_BY_URI_DESCENDING:
       return &SortComparison_URIGreater;
     case nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_ASCENDING:
       return &SortComparison_VisitCountLess;
     case nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_DESCENDING:
       return &SortComparison_VisitCountGreater;
-    case nsINavHistoryQueryOptions::SORT_BY_KEYWORD_ASCENDING:
-      return &SortComparison_KeywordLess;
-    case nsINavHistoryQueryOptions::SORT_BY_KEYWORD_DESCENDING:
-      return &SortComparison_KeywordGreater;
     case nsINavHistoryQueryOptions::SORT_BY_ANNOTATION_ASCENDING:
       return &SortComparison_AnnotationLess;
     case nsINavHistoryQueryOptions::SORT_BY_ANNOTATION_DESCENDING:
       return &SortComparison_AnnotationGreater;
     case nsINavHistoryQueryOptions::SORT_BY_DATEADDED_ASCENDING:
       return &SortComparison_DateAddedLess;
     case nsINavHistoryQueryOptions::SORT_BY_DATEADDED_DESCENDING:
       return &SortComparison_DateAddedGreater;
@@ -1026,53 +1022,16 @@ int32_t nsNavHistoryContainerResultNode:
   return value;
 }
 int32_t nsNavHistoryContainerResultNode::SortComparison_URIGreater(
     nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure)
 {
   return -SortComparison_URILess(a, b, closure);
 }
 
-
-int32_t nsNavHistoryContainerResultNode::SortComparison_KeywordLess(
-    nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure)
-{
-  int32_t value = 0;
-  if (a->mItemId != -1 || b->mItemId != -1) {
-    // compare the keywords
-    nsAutoString keywordA, keywordB;
-    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
-    NS_ENSURE_TRUE(bookmarks, 0);
-
-    nsresult rv;
-    if (a->mItemId != -1) {
-      rv = bookmarks->GetKeywordForBookmark(a->mItemId, keywordA);
-      NS_ENSURE_SUCCESS(rv, 0);
-    }
-    if (b->mItemId != -1) {
-      rv = bookmarks->GetKeywordForBookmark(b->mItemId, keywordB);
-      NS_ENSURE_SUCCESS(rv, 0);
-    }
-
-    value = SortComparison_StringLess(keywordA, keywordB);
-  }
-
-  // Fall back to title sorting.
-  if (value == 0)
-    value = SortComparison_TitleLess(a, b, closure);
-
-  return value;
-}
-
-int32_t nsNavHistoryContainerResultNode::SortComparison_KeywordGreater(
-    nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure)
-{
-  return -SortComparison_KeywordLess(a, b, closure);
-}
-
 int32_t nsNavHistoryContainerResultNode::SortComparison_AnnotationLess(
     nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure)
 {
   nsAutoCString annoName(static_cast<char*>(closure));
   NS_ENSURE_TRUE(!annoName.IsEmpty(), 0);
 
   bool a_itemAnno = false;
   bool b_itemAnno = false;
--- a/toolkit/components/places/nsNavHistoryResult.h
+++ b/toolkit/components/places/nsNavHistoryResult.h
@@ -533,22 +533,16 @@ public:
                                            nsNavHistoryResultNode* b,
                                            void* closure);
   static int32_t SortComparison_VisitCountLess(nsNavHistoryResultNode* a,
                                                nsNavHistoryResultNode* b,
                                                void* closure);
   static int32_t SortComparison_VisitCountGreater(nsNavHistoryResultNode* a,
                                                   nsNavHistoryResultNode* b,
                                                   void* closure);
-  static int32_t SortComparison_KeywordLess(nsNavHistoryResultNode* a,
-                                            nsNavHistoryResultNode* b,
-                                            void* closure);
-  static int32_t SortComparison_KeywordGreater(nsNavHistoryResultNode* a,
-                                               nsNavHistoryResultNode* b,
-                                               void* closure);
   static int32_t SortComparison_AnnotationLess(nsNavHistoryResultNode* a,
                                                nsNavHistoryResultNode* b,
                                                void* closure);
   static int32_t SortComparison_AnnotationGreater(nsNavHistoryResultNode* a,
                                                   nsNavHistoryResultNode* b,
                                                   void* closure);
   static int32_t SortComparison_DateAddedLess(nsNavHistoryResultNode* a,
                                               nsNavHistoryResultNode* b,
--- a/toolkit/components/places/tests/bookmarks/test_keywords.js
+++ b/toolkit/components/places/tests/bookmarks/test_keywords.js
@@ -7,34 +7,27 @@ ChromeUtils.defineModuleGetter(this, "Pr
 const URI1 = "http://test1.mozilla.org/";
 const URI2 = "http://test2.mozilla.org/";
 const URI3 = "http://test3.mozilla.org/";
 
 async function check_keyword(aURI, aKeyword) {
   if (aKeyword)
     aKeyword = aKeyword.toLowerCase();
 
-  let bms = [];
-  await PlacesUtils.bookmarks.fetch({ url: aURI }, bm => bms.push(bm));
-  for (let bm of bms) {
-    let itemId = await PlacesUtils.promiseItemId(bm.guid);
-    let keyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId);
-    if (keyword && !aKeyword) {
-      throw (`${aURI} should not have a keyword, found keyword "${keyword}"`);
-    } else if (aKeyword && keyword == aKeyword) {
-      Assert.equal(keyword, aKeyword);
-    }
-  }
-
   if (aKeyword) {
     let uri = await PlacesUtils.keywords.fetch(aKeyword);
     Assert.equal(uri.url, aURI);
     // Check case insensitivity.
     uri = await PlacesUtils.keywords.fetch(aKeyword.toUpperCase());
     Assert.equal(uri.url, aURI);
+  } else {
+    let entry = await PlacesUtils.keywords.fetch({ url: aURI });
+    if (entry) {
+      throw (`${aURI.spec} should not have a keyword`);
+    }
   }
 }
 
 async function check_orphans() {
   let db = await PlacesUtils.promiseDBConnection();
   let rows = await db.executeCached(
     `SELECT id FROM moz_keywords k
      WHERE NOT EXISTS (SELECT 1 FROM moz_places WHERE id = k.place_id)
@@ -70,20 +63,16 @@ function expectNotifications() {
       return target[name];
     }
   });
   PlacesUtils.bookmarks.addObserver(observer);
   return observer;
 }
 
 add_task(function test_invalid_input() {
-  Assert.throws(() => PlacesUtils.bookmarks.getKeywordForBookmark(null),
-                /NS_ERROR_ILLEGAL_VALUE/);
-  Assert.throws(() => PlacesUtils.bookmarks.getKeywordForBookmark(0),
-                /NS_ERROR_ILLEGAL_VALUE/);
 });
 
 add_task(async function test_addBookmarkAndKeyword() {
   let timerPrecision = Preferences.get("privacy.reduceTimerPrecision");
   Preferences.set("privacy.reduceTimerPrecision", false);
 
   registerCleanupFunction(function() {
     Preferences.set("privacy.reduceTimerPrecision", timerPrecision);
--- a/toolkit/components/places/tests/queries/test_sorting.js
+++ b/toolkit/components/places/tests/queries/test_sorting.js
@@ -13,33 +13,30 @@ tests.push({
     info("Sorting test 1: SORT BY NONE");
 
     this._unsortedData = [
       { isBookmark: true,
         uri: "http://example.com/b",
         parentGuid: PlacesUtils.bookmarks.toolbarGuid,
         index: PlacesUtils.bookmarks.DEFAULT_INDEX,
         title: "y",
-        keyword: "b",
         isInQuery: true },
 
       { isBookmark: true,
         uri: "http://example.com/a",
         parentGuid: PlacesUtils.bookmarks.toolbarGuid,
         index: PlacesUtils.bookmarks.DEFAULT_INDEX,
         title: "z",
-        keyword: "a",
         isInQuery: true },
 
       { isBookmark: true,
         uri: "http://example.com/c",
         parentGuid: PlacesUtils.bookmarks.toolbarGuid,
         index: PlacesUtils.bookmarks.DEFAULT_INDEX,
         title: "x",
-        keyword: "c",
         isInQuery: true },
     ];
 
     this._sortedData = this._unsortedData;
 
     // This function in head_queries.js creates our database with the above data
     await task_populateDB(this._unsortedData);
   },
@@ -437,113 +434,16 @@ tests.push({
   check_reverse() {
     this._sortingMode = Ci.nsINavHistoryQueryOptions.SORT_BY_VISITCOUNT_DESCENDING;
     this._sortedData.reverse();
     this.check();
   }
 });
 
 tests.push({
-  _sortingMode: Ci.nsINavHistoryQueryOptions.SORT_BY_KEYWORD_ASCENDING,
-
-  async setup() {
-    info("Sorting test 6: SORT BY KEYWORD");
-
-    this._unsortedData = [
-      { isBookmark: true,
-        uri: "http://example.com/a",
-        parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-        index: PlacesUtils.bookmarks.DEFAULT_INDEX,
-        title: "z",
-        keyword: "a",
-        isInQuery: true },
-
-      { isBookmark: true,
-        uri: "http://example.com/c",
-        parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-        index: PlacesUtils.bookmarks.DEFAULT_INDEX,
-        title: "x",
-        keyword: "c",
-        isInQuery: true },
-
-      { isBookmark: true,
-        uri: "http://example.com/b1",
-        parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-        index: PlacesUtils.bookmarks.DEFAULT_INDEX,
-        title: "y9",
-        keyword: "b",
-        isInQuery: true },
-
-      // without a keyword, should fall back to title
-      { isBookmark: true,
-        uri: "http://example.com/null2",
-        parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-        index: PlacesUtils.bookmarks.DEFAULT_INDEX,
-        title: "null8",
-        keyword: null,
-        isInQuery: true },
-
-      // without a keyword, should fall back to title
-      { isBookmark: true,
-        uri: "http://example.com/null1",
-        parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-        index: PlacesUtils.bookmarks.DEFAULT_INDEX,
-        title: "null9",
-        keyword: null,
-        isInQuery: true },
-
-      // if keywords are equal, should fall back to title
-      { isBookmark: true,
-        uri: "http://example.com/b1",
-        parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-        index: PlacesUtils.bookmarks.DEFAULT_INDEX,
-        title: "y8",
-        keyword: "b",
-        isInQuery: true },
-    ];
-
-    this._sortedData = [
-      this._unsortedData[3],
-      this._unsortedData[4],
-      this._unsortedData[0],
-      this._unsortedData[5],
-      this._unsortedData[2],
-      this._unsortedData[1],
-    ];
-
-    // This function in head_queries.js creates our database with the above data
-    await task_populateDB(this._unsortedData);
-  },
-
-  check() {
-    // Query
-    var query = PlacesUtils.history.getNewQuery();
-    query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);
-    query.onlyBookmarked = true;
-
-    // query options
-    var options = PlacesUtils.history.getNewQueryOptions();
-    options.sortingMode = this._sortingMode;
-
-    // Results - this gets the result set and opens it for reading and modification.
-    var result = PlacesUtils.history.executeQuery(query, options);
-    var root = result.root;
-    root.containerOpen = true;
-    compareArrayToResult(this._sortedData, root);
-    root.containerOpen = false;
-  },
-
-  check_reverse() {
-    this._sortingMode = Ci.nsINavHistoryQueryOptions.SORT_BY_KEYWORD_DESCENDING;
-    this._sortedData.reverse();
-    this.check();
-  }
-});
-
-tests.push({
   _sortingMode: Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_ASCENDING,
 
   async setup() {
     info("Sorting test 7: SORT BY DATEADDED");
 
     var timeInMicroseconds = Date.now() * 1000;
     this._unsortedData = [
       { isBookmark: true,
--- a/toolkit/components/places/tests/unit/test_result_sort.js
+++ b/toolkit/components/places/tests/unit/test_result_sort.js
@@ -91,24 +91,16 @@ add_task(async function test() {
   });
   checkOrder(guid3, guid1, guid2);
   await PlacesUtils.bookmarks.update({
     guid: guid1,
     url: uri1,
   });
   checkOrder(guid1, guid3, guid2);
 
-  // keyword sort
-  info("Sort by keyword asc");
-  result.sortingMode = NHQO.SORT_BY_KEYWORD_ASCENDING;
-  checkOrder(guid3, guid2, guid1); // no keywords set - falling back to title sort
-  await PlacesUtils.keywords.insert({ url: uri1, keyword: "a" });
-  await PlacesUtils.keywords.insert({ url: uri2, keyword: "z" });
-  checkOrder(guid3, guid1, guid2);
-
   // XXXtodo: test history sortings (visit count, visit date)
   // XXXtodo: test different item types once folderId and bookmarkId are merged.
   // XXXtodo: test sortingAnnotation functionality with non-bookmark nodes
 
   info("Sort by annotation desc");
   let ids = await PlacesUtils.promiseManyItemIds([guid1, guid3]);
   PlacesUtils.annotations.setItemAnnotation(ids.get(guid1), "testAnno", "a", 0, 0);
   PlacesUtils.annotations.setItemAnnotation(ids.get(guid3), "testAnno", "b", 0, 0);