Bug 1480049 - Remove nsIAnnotationService::getPageAnnotation. r=mak
authorMark Banner <standard8@mozilla.com>
Thu, 23 Aug 2018 09:59:50 +0000
changeset 433004 5412e9ef6314e378952186b398966a77b24edb7e
parent 433003 ce26d250b22ddb97ab1455126c08ac05af26e056
child 433005 613debd35609e10cd1d1404b2137514f8f9176d0
push id34498
push usercsabou@mozilla.com
push dateThu, 23 Aug 2018 21:39:18 +0000
treeherdermozilla-central@7b78aeca32ed [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1480049
milestone63.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 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");