Bug 1480049 - Remove nsIAnnotationService::getPageAnnotation. r=mak
authorMark Banner <standard8@mozilla.com>
Thu, 23 Aug 2018 09:59:50 +0000
changeset 831005 5412e9ef6314e378952186b398966a77b24edb7e
parent 831004 ce26d250b22ddb97ab1455126c08ac05af26e056
child 831006 613debd35609e10cd1d1404b2137514f8f9176d0
push id118868
push userbmo:zjz@zjz.name
push dateFri, 24 Aug 2018 07:04:39 +0000
reviewersmak
bugs1480049
milestone63.0a1
Bug 1480049 - Remove nsIAnnotationService::getPageAnnotation. r=mak This changes the meta data cache in DownloadHistory to store a copy of the meta data throughout the session, to avoid the need for async lookups when updating the UI. Differential Revision: https://phabricator.services.mozilla.com/D2597
toolkit/components/downloads/test/unit/common_test_Download.js
toolkit/components/places/nsAnnotationService.cpp
toolkit/components/places/nsAnnotationService.h
toolkit/components/places/nsIAnnotationService.idl
toolkit/components/places/tests/head_common.js
toolkit/components/places/tests/history/test_update.js
toolkit/components/places/tests/maintenance/test_preventive_maintenance.js
toolkit/components/places/tests/unit/test_415757.js
toolkit/components/places/tests/unit/test_annotations.js
toolkit/components/places/tests/unit/test_browserhistory.js
--- a/toolkit/components/downloads/test/unit/common_test_Download.js
+++ b/toolkit/components/downloads/test/unit/common_test_Download.js
@@ -2410,20 +2410,18 @@ add_task(async function test_history() {
 
   let expectedFile = new FileUtils.File(download.target.path);
 
   Assert.equal(time, download.startTime.getTime());
   Assert.equal(transitionType, Ci.nsINavHistoryService.TRANSITION_DOWNLOAD);
   Assert.equal(lastKnownTitle, expectedFile.leafName);
 
   let expectedFileURI = Services.io.newFileURI(expectedFile);
-  let destFileURI = PlacesUtils.annotations.getPageAnnotation(
-    Services.io.newURI(sourceUrl),
-    "downloads/destinationFileURI");
-  Assert.equal(destFileURI, expectedFileURI.spec,
+  let pageInfo = await PlacesUtils.history.fetch(sourceUrl, {includeAnnotations: true});
+  Assert.equal(pageInfo.annotations.get("downloads/destinationFileURI"), expectedFileURI.spec,
     "Should have saved the correct download target annotation.");
 
   // Restart and complete the download after clearing history.
   await PlacesUtils.history.clear();
   download.cancel();
   continueResponses();
   await download.start();
 
--- a/toolkit/components/places/nsAnnotationService.cpp
+++ b/toolkit/components/places/nsAnnotationService.cpp
@@ -375,60 +375,16 @@ nsAnnotationService::SetAnnotationDouble
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
 
-NS_IMETHODIMP
-nsAnnotationService::GetPageAnnotation(nsIURI* aURI,
-                                       const nsACString& aName,
-                                       nsIVariant** _retval)
-{
-  NS_ENSURE_ARG(aURI);
-  NS_ENSURE_ARG_POINTER(_retval);
-
-  nsCOMPtr<mozIStorageStatement> statement;
-  nsresult rv = StartGetAnnotation(aURI, 0, aName, statement);
-  if (NS_FAILED(rv))
-    return rv;
-
-  mozStorageStatementScoper scoper(statement);
-
-  nsCOMPtr<nsIWritableVariant> value = new nsVariant();
-  int32_t type = statement->AsInt32(kAnnoIndex_Type);
-  switch (type) {
-    case nsIAnnotationService::TYPE_INT32:
-    case nsIAnnotationService::TYPE_INT64:
-    case nsIAnnotationService::TYPE_DOUBLE: {
-      rv = value->SetAsDouble(statement->AsDouble(kAnnoIndex_Content));
-      break;
-    }
-    case nsIAnnotationService::TYPE_STRING: {
-      nsAutoString valueString;
-      rv = statement->GetString(kAnnoIndex_Content, valueString);
-      if (NS_SUCCEEDED(rv))
-        rv = value->SetAsAString(valueString);
-      break;
-    }
-    default: {
-      rv = NS_ERROR_UNEXPECTED;
-      break;
-    }
-  }
-
-  if (NS_SUCCEEDED(rv)) {
-    value.forget(_retval);
-  }
-
-  return rv;
-}
-
 nsresult
 nsAnnotationService::GetValueFromStatement(nsCOMPtr<mozIStorageStatement>& aStatement,
                                            nsIVariant** _retval)
 {
   nsresult rv;
 
   nsCOMPtr<nsIWritableVariant> value = new nsVariant();
   int32_t type = aStatement->AsInt32(kAnnoIndex_Type);
@@ -462,17 +418,17 @@ NS_IMETHODIMP
 nsAnnotationService::GetItemAnnotation(int64_t aItemId,
                                        const nsACString& aName,
                                        nsIVariant** _retval)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_retval);
 
   nsCOMPtr<mozIStorageStatement> statement;
-  nsresult rv = StartGetAnnotation(nullptr, aItemId, aName, statement);
+  nsresult rv = StartGetAnnotation(aItemId, aName, statement);
   if (NS_FAILED(rv))
     return rv;
 
   mozStorageStatementScoper scoper(statement);
 
   return GetValueFromStatement(statement, _retval);
 }
 
@@ -487,17 +443,17 @@ nsAnnotationService::GetItemAnnotationIn
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_value);
   NS_ENSURE_ARG_POINTER(_flags);
   NS_ENSURE_ARG_POINTER(_expiration);
   NS_ENSURE_ARG_POINTER(_storageType);
 
   nsCOMPtr<mozIStorageStatement> statement;
-  nsresult rv = StartGetAnnotation(nullptr, aItemId, aName, statement);
+  nsresult rv = StartGetAnnotation(aItemId, aName, statement);
   if (NS_FAILED(rv))
     return rv;
 
   mozStorageStatementScoper scoper(statement);
   *_flags = statement->AsInt32(kAnnoIndex_Flags);
   *_expiration = (uint16_t)statement->AsInt32(kAnnoIndex_Expiration);
   int32_t type = (uint16_t)statement->AsInt32(kAnnoIndex_Type);
   if (type == 0) {
@@ -817,52 +773,33 @@ nsAnnotationService::ItemHasAnnotation(i
 /**
  * This loads the statement and steps it once so you can get data out of it.
  *
  * @note You have to reset the statement when you're done if this succeeds.
  * @throws NS_ERROR_NOT_AVAILABLE if the annotation is not found.
  */
 
 nsresult
-nsAnnotationService::StartGetAnnotation(nsIURI* aURI,
-                                        int64_t aItemId,
+nsAnnotationService::StartGetAnnotation(int64_t aItemId,
                                         const nsACString& aName,
                                         nsCOMPtr<mozIStorageStatement>& aStatement)
 {
-  bool isItemAnnotation = (aItemId > 0);
-
-  if (isItemAnnotation) {
-    aStatement = mDB->GetStatement(
-      "SELECT a.id, a.item_id, :anno_name, a.content, a.flags, "
-             "a.expiration, a.type "
-      "FROM moz_anno_attributes n "
-      "JOIN moz_items_annos a ON a.anno_attribute_id = n.id "
-      "WHERE a.item_id = :item_id "
-      "AND n.name = :anno_name"
-    );
-  }
-  else {
-    aStatement = mDB->GetStatement(
-      "SELECT a.id, a.place_id, :anno_name, a.content, a.flags, "
-             "a.expiration, a.type "
-      "FROM moz_anno_attributes n "
-      "JOIN moz_annos a ON n.id = a.anno_attribute_id "
-      "JOIN moz_places h ON h.id = a.place_id "
-      "WHERE h.url_hash = hash(:page_url) AND h.url = :page_url "
-        "AND n.name = :anno_name"
-    );
-  }
+  aStatement = mDB->GetStatement(
+    "SELECT a.id, a.item_id, :anno_name, a.content, a.flags, "
+           "a.expiration, a.type "
+    "FROM moz_anno_attributes n "
+    "JOIN moz_items_annos a ON a.anno_attribute_id = n.id "
+    "WHERE a.item_id = :item_id "
+    "AND n.name = :anno_name"
+  );
   NS_ENSURE_STATE(aStatement);
   mozStorageStatementScoper getAnnoScoper(aStatement);
 
   nsresult rv;
-  if (isItemAnnotation)
-    rv = aStatement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
-  else
-    rv = URIBinder::Bind(aStatement, NS_LITERAL_CSTRING("page_url"), aURI);
+  rv = aStatement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = aStatement->BindUTF8StringByName(NS_LITERAL_CSTRING("anno_name"), aName);
   NS_ENSURE_SUCCESS(rv, rv);
 
   bool hasResult = false;
   rv = aStatement->ExecuteStep(&hasResult);
   if (NS_FAILED(rv) || !hasResult)
--- a/toolkit/components/places/nsAnnotationService.h
+++ b/toolkit/components/places/nsAnnotationService.h
@@ -92,18 +92,17 @@ protected:
   static const int kAnnoIndex_NameID;
   static const int kAnnoIndex_Content;
   static const int kAnnoIndex_Flags;
   static const int kAnnoIndex_Expiration;
   static const int kAnnoIndex_Type;
   static const int kAnnoIndex_DateAdded;
   static const int kAnnoIndex_LastModified;
 
-  nsresult StartGetAnnotation(nsIURI* aURI,
-                              int64_t aItemId,
+  nsresult StartGetAnnotation(int64_t aItemId,
                               const nsACString& aName,
                               nsCOMPtr<mozIStorageStatement>& aStatement);
 
   nsresult StartSetAnnotation(int64_t aItemId,
                               BookmarkData* aBookmark,
                               const nsACString& aName,
                               int32_t aFlags,
                               uint16_t aExpiration,
--- a/toolkit/components/places/nsIAnnotationService.idl
+++ b/toolkit/components/places/nsIAnnotationService.idl
@@ -78,18 +78,16 @@ interface nsIAnnotationService : nsISupp
     /**
      * Retrieves the value of a given annotation. Throws an error if the
      * annotation does not exist. C++ consumers may use the type-specific
      * methods.
      *
      * The type-specific methods throw if the given annotation is set in
      * a different type.
      */
-    nsIVariant getPageAnnotation(in nsIURI aURI,
-                                 in AUTF8String aName);
     nsIVariant getItemAnnotation(in long long aItemId,
                                  in AUTF8String aName);
 
     /**
      * Retrieves info about an existing annotation.
      *
      * aType will be one of TYPE_* constansts above
      *
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -968,8 +968,28 @@ function getItemsWithAnnotation(name) {
       JOIN moz_items_annos a ON n.id = a.anno_attribute_id
       JOIN moz_bookmarks b ON b.id = a.item_id
       WHERE n.name = :name
     `, {name});
 
     return rows.map(row => row.getResultByName("guid"));
   });
 }
+
+/**
+ * Checks there are no orphan page annotations in the database, and no
+ * orphan anno attribute names.
+ */
+async function assertNoOrphanPageAnnotations() {
+  let db = await PlacesUtils.promiseDBConnection();
+
+  let rows = await db.execute(`
+    SELECT place_id FROM moz_annos
+    WHERE place_id NOT IN (SELECT id FROM moz_places)
+  `);
+
+  Assert.equal(rows.length, 0, "Should not have any orphan page annotations");
+
+  rows = await db.execute(`
+    SELECT id FROM moz_anno_attributes
+    WHERE id NOT IN (SELECT anno_attribute_id FROM moz_annos) AND
+          id NOT IN (SELECT anno_attribute_id FROM moz_items_annos)`);
+}
--- a/toolkit/components/places/tests/history/test_update.js
+++ b/toolkit/components/places/tests/history/test_update.js
@@ -180,16 +180,24 @@ add_task(async function test_change_desc
   description = null;
   await PlacesUtils.history.update({ url: TEST_URL, description });
   descriptionInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "description");
   previewImageURLInDB = await PlacesTestUtils.fieldInDB(TEST_URL, "preview_image_url");
   Assert.strictEqual(description, descriptionInDB, "description should be updated via URL as expected");
   Assert.equal(previewImageURL, previewImageURLInDB, "previewImageURL should not be updated");
 });
 
+/**
+ * Gets annotation information from the database for the specified URL and
+ * annotation name.
+ *
+ * @param {String} pageUrl The URL to search for.
+ * @param {String} annoName The name of the annotation to search for.
+ * @return {Array} An array of objects containing the annotations found.
+ */
 async function getAnnotationInfoFromDB(pageUrl, annoName) {
   let db = await PlacesUtils.promiseDBConnection();
 
   let rows = await db.execute(`
     SELECT a.content, a.flags, a.expiration, a.type FROM moz_anno_attributes n
     JOIN moz_annos a ON n.id = a.anno_attribute_id
     JOIN moz_places h ON h.id = a.place_id
     WHERE h.url_hash = hash(:pageUrl) AND h.url = :pageUrl
--- a/toolkit/components/places/tests/maintenance/test_preventive_maintenance.js
+++ b/toolkit/components/places/tests/maintenance/test_preventive_maintenance.js
@@ -2170,17 +2170,18 @@ tests.push({
     Assert.equal((await bs.fetch(this._bookmark.guid)).url, this._uri1.spec);
     let folder = await bs.fetch(this._folder.guid);
     Assert.equal(folder.index, 0);
     Assert.equal(folder.type, bs.TYPE_FOLDER);
     Assert.equal((await bs.fetch(this._separator.guid)).type, bs.TYPE_SEPARATOR);
 
     Assert.equal(ts.getTagsForURI(this._uri1).length, 1);
     Assert.equal((await PlacesUtils.keywords.fetch({ url: this._uri1.spec })).keyword, "testkeyword");
-    Assert.equal(as.getPageAnnotation(this._uri2, "anno"), "anno");
+    let pageInfo = await PlacesUtils.history.fetch(this._uri2, {includeAnnotations: true});
+    Assert.equal(pageInfo.annotations.get("anno"), "anno");
     Assert.equal(as.getItemAnnotation(this._bookmarkId, "anno"), "anno");
 
     await new Promise(resolve => {
       fs.getFaviconURLForPage(this._uri2, aFaviconURI => {
         Assert.ok(aFaviconURI.equals(SMALLPNG_DATA_URI));
         resolve();
       });
     });
--- a/toolkit/components/places/tests/unit/test_415757.js
+++ b/toolkit/components/places/tests/unit/test_415757.js
@@ -26,39 +26,37 @@ function uri_in_db(aURI) {
 }
 
 const TOTAL_SITES = 20;
 
 // main
 add_task(async function test_execute() {
   // add pages to global history
   for (let i = 0; i < TOTAL_SITES; i++) {
-    let site = "http://www.test-" + i + ".com/";
-    let testURI = uri(site);
+    let uri = "http://www.test-" + i + ".com/";
     let when = Date.now() * 1000 + (i * TOTAL_SITES);
-    await PlacesTestUtils.addVisits({ uri: testURI, visitDate: when });
+    await PlacesTestUtils.addVisits({ uri, visitDate: when });
   }
   for (let i = 0; i < TOTAL_SITES; i++) {
-    let site = "http://www.test.com/" + i + "/";
-    let testURI = uri(site);
+    let uri = "http://www.test.com/" + i + "/";
     let when = Date.now() * 1000 + (i * TOTAL_SITES);
-    await PlacesTestUtils.addVisits({ uri: testURI, visitDate: when });
+    await PlacesTestUtils.addVisits({ uri, visitDate: when });
   }
 
   // set a page annotation on one of the urls that will be removed
-  var testAnnoDeletedURI = uri("http://www.test.com/1/");
+  var testAnnoDeletedURI = "http://www.test.com/1/";
   var testAnnoDeletedName = "foo";
   var testAnnoDeletedValue = "bar";
   await PlacesUtils.history.update({
     url: testAnnoDeletedURI,
     annotations: new Map([[testAnnoDeletedName, testAnnoDeletedValue]]),
   });
 
   // set a page annotation on one of the urls that will NOT be removed
-  var testAnnoRetainedURI = uri("http://www.test-1.com/");
+  var testAnnoRetainedURI = "http://www.test-1.com/";
   var testAnnoRetainedName = "foo";
   var testAnnoRetainedValue = "bar";
   await PlacesUtils.history.update({
     url: testAnnoRetainedURI,
     annotations: new Map([[testAnnoRetainedName, testAnnoRetainedValue]]),
   });
 
   // remove pages from www.test.com
@@ -74,23 +72,16 @@ add_task(async function test_execute() {
   // check that all pages in www.test-X.com have NOT been removed
   for (let i = 0; i < TOTAL_SITES; i++) {
     let site = "http://www.test-" + i + ".com/";
     let testURI = uri(site);
     Assert.ok(uri_in_db(testURI));
   }
 
   // check that annotation on the removed item does not exists
-  try {
-    PlacesUtils.annotations.getPageAnnotation(testAnnoDeletedURI, testAnnoDeletedName);
-    do_throw("fetching page-annotation that doesn't exist, should've thrown");
-  } catch (ex) {}
+  await assertNoOrphanPageAnnotations();
 
   // check that annotation on the NOT removed item still exists
-  try {
-    var annoVal = PlacesUtils.annotations.getPageAnnotation(testAnnoRetainedURI,
-                                                            testAnnoRetainedName);
-  } catch (ex) {
-    do_throw("The annotation has been removed erroneously");
-  }
-  Assert.equal(annoVal, testAnnoRetainedValue);
+  let pageInfo = await PlacesUtils.history.fetch(testAnnoRetainedURI, {includeAnnotations: true});
 
+  Assert.equal(pageInfo.annotations.get(testAnnoRetainedName), testAnnoRetainedValue,
+    "Should have kept the annotation for the non-removed items");
 });
--- a/toolkit/components/places/tests/unit/test_annotations.js
+++ b/toolkit/components/places/tests/unit/test_annotations.js
@@ -49,21 +49,16 @@ add_task(async function test_execute() {
   try {
     var annoVal = annosvc.getItemAnnotation(testItemId, testAnnoName);
     // verify the anno value
     Assert.ok(testAnnoVal === annoVal);
   } catch (ex) {
     do_throw("unable to get item annotation");
   }
 
-  // get annotation that doesn't exist
-  try {
-    annosvc.getPageAnnotation(testURI, "blah");
-    do_throw("fetching page-annotation that doesn't exist, should've thrown");
-  } catch (ex) {}
   try {
     annosvc.getItemAnnotation(testURI, "blah");
     do_throw("fetching item-annotation that doesn't exist, should've thrown");
   } catch (ex) {}
 
   // get annotation info
   var value = {}, flags = {}, exp = {}, storageType = {};
   annosvc.getItemAnnotationInfo(testItemId, testAnnoName, value, flags, exp, storageType);
--- a/toolkit/components/places/tests/unit/test_browserhistory.js
+++ b/toolkit/components/places/tests/unit/test_browserhistory.js
@@ -1,16 +1,16 @@
 /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim:set ts=2 sw=2 sts=2 et: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-const TEST_URI = NetUtil.newURI("http://mozilla.com/");
-const TEST_SUBDOMAIN_URI = NetUtil.newURI("http://foobar.mozilla.com/");
+const TEST_URI = "http://mozilla.com/";
+const TEST_SUBDOMAIN_URI = "http://foobar.mozilla.com/";
 
 async function checkEmptyHistory() {
   let db = await PlacesUtils.promiseDBConnection();
   let rows = await db.executeCached("SELECT count(*) FROM moz_historyvisits");
   return !rows[0].getResultByIndex(0);
 }
 
 add_task(async function test_addPage() {
@@ -21,17 +21,17 @@ add_task(async function test_addPage() {
 add_task(async function test_removePage() {
   await PlacesUtils.history.remove(TEST_URI);
   Assert.ok(await checkEmptyHistory(), "History is empty");
 });
 
 add_task(async function test_removePages() {
   let pages = [];
   for (let i = 0; i < 8; i++) {
-    pages.push(NetUtil.newURI(TEST_URI.spec + i));
+    pages.push(TEST_URI + i);
   }
 
   await PlacesTestUtils.addVisits(pages.map(uri => ({ uri })));
   // Bookmarked item should not be removed from moz_places.
   const ANNO_INDEX = 1;
   const ANNO_NAME = "testAnno";
   const ANNO_VALUE = "foo";
   const BOOKMARK_INDEX = 2;
@@ -50,52 +50,48 @@ add_task(async function test_removePages
   });
 
   await PlacesUtils.history.remove(pages);
   Assert.ok(await checkEmptyHistory(), "History is empty");
 
   // Check that the bookmark and its annotation still exist.
   let folder = await PlacesUtils.getFolderContents(PlacesUtils.bookmarks.unfiledGuid);
   Assert.equal(folder.root.childCount, 1);
-  Assert.equal(PlacesUtils.annotations.getPageAnnotation(pages[BOOKMARK_INDEX], ANNO_NAME),
-               ANNO_VALUE);
+  let pageInfo = await PlacesUtils.history.fetch(pages[BOOKMARK_INDEX], {includeAnnotations: true});
+  Assert.equal(pageInfo.annotations.get(ANNO_NAME), ANNO_VALUE);
 
   // Check the annotation on the non-bookmarked page does not exist anymore.
-  try {
-    PlacesUtils.annotations.getPageAnnotation(pages[ANNO_INDEX], ANNO_NAME);
-    do_throw("did not expire expire_never anno on a not bookmarked item");
-  } catch (ex) {}
+  await assertNoOrphanPageAnnotations();
 
   // Cleanup.
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesUtils.history.clear();
 });
 
 add_task(async function test_removePagesByTimeframe() {
   let visits = [];
   let startDate = (Date.now() - 10000) * 1000;
   for (let i = 0; i < 10; i++) {
     visits.push({
-      uri: NetUtil.newURI(TEST_URI.spec + i),
+      uri: TEST_URI + i,
       visitDate: startDate + i * 1000,
     });
   }
 
   await PlacesTestUtils.addVisits(visits);
 
   // Delete all pages except the first and the last.
   await PlacesUtils.history.removeByFilter({
     beginDate: PlacesUtils.toDate(startDate + 1000),
     endDate: PlacesUtils.toDate(startDate + 8000),
   });
 
   // Check that we have removed the correct pages.
   for (let i = 0; i < 10; i++) {
-    Assert.equal(page_in_database(NetUtil.newURI(TEST_URI.spec + i)) == 0,
-                 i > 0 && i < 9);
+    Assert.equal(page_in_database(TEST_URI + i) == 0, i > 0 && i < 9);
   }
 
   // Clear remaining items and check that all pages have been removed.
   await PlacesUtils.history.removeByFilter({
     beginDate: PlacesUtils.toDate(startDate),
     endDate: PlacesUtils.toDate(startDate + 9000),
   });
   Assert.ok(await checkEmptyHistory(), "History is empty");