Bug 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem. r=mak
authorMark Banner <standard8@mozilla.com>
Wed, 28 Mar 2018 21:05:27 +0100
changeset 410633 cb8cbe3c000f80ea886c6406cdad87e3c6236bfc
parent 410632 b2b99ec4dca66a9fce67a226ead6a4e8f82cb58d
child 410634 6bc3bc48fc87fa38855de66807720d900a6613ac
push id61924
push usermbanner@mozilla.com
push dateThu, 29 Mar 2018 16:35:51 +0000
treeherderautoland@729748e603e5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1047819
milestone61.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 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem. r=mak MozReview-Commit-ID: FXncB1TRFXw
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/nsAnnotationService.cpp
toolkit/components/places/nsAnnotationService.h
toolkit/components/places/nsIAnnotationService.idl
toolkit/components/places/tests/unit/test_annotations.js
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1181,26 +1181,25 @@ var PlacesUtils = {
    * @param aItemId
    *        The identifier of the itme for which annotations are to be
    *        retrieved.
    * @return Array of objects, each containing the following properties:
    *         name, flags, expires, mimeType, type, value
    */
   getAnnotationsForItem: function PU_getAnnotationsForItem(aItemId) {
     var annosvc = this.annotations;
-    var annos = [], val = null;
+    var annos = [];
     var annoNames = annosvc.getItemAnnotationNames(aItemId);
     for (var i = 0; i < annoNames.length; i++) {
-      var flags = {}, exp = {}, storageType = {};
-      annosvc.getItemAnnotationInfo(aItemId, annoNames[i], flags, exp, storageType);
-      val = annosvc.getItemAnnotation(aItemId, annoNames[i]);
+      let value = {}, flags = {}, exp = {}, storageType = {};
+      annosvc.getItemAnnotationInfo(aItemId, annoNames[i], value, flags, exp, storageType);
       annos.push({name: annoNames[i],
                   flags: flags.value,
                   expires: exp.value,
-                  value: val});
+                  value: value.value});
     }
     return annos;
   },
 
   /**
    * Annotate an item with a batch of annotations.
    * @param aItemId
    *        The identifier of the item for which annotations are to be set
--- a/toolkit/components/places/nsAnnotationService.cpp
+++ b/toolkit/components/places/nsAnnotationService.cpp
@@ -733,62 +733,68 @@ nsAnnotationService::GetPageAnnotation(n
 
   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);
+  switch (type) {
+    case nsIAnnotationService::TYPE_INT32:
+    case nsIAnnotationService::TYPE_INT64:
+    case nsIAnnotationService::TYPE_DOUBLE: {
+      rv = value->SetAsDouble(aStatement->AsDouble(kAnnoIndex_Content));
+      break;
+    }
+    case nsIAnnotationService::TYPE_STRING: {
+      nsAutoString valueString;
+      rv = aStatement->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;
+}
+
 
 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);
   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;
+  return GetValueFromStatement(statement, _retval);
 }
 
-
 NS_IMETHODIMP
 nsAnnotationService::GetPageAnnotationInt32(nsIURI* aURI,
                                         const nsACString& aName,
                                         int32_t* _retval)
 {
   NS_ENSURE_ARG(aURI);
   NS_ENSURE_ARG_POINTER(_retval);
 
@@ -981,21 +987,23 @@ nsAnnotationService::GetPageAnnotationIn
 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsAnnotationService::GetItemAnnotationInfo(int64_t aItemId,
                                            const nsACString& aName,
+                                           nsIVariant** _value,
                                            int32_t* _flags,
                                            uint16_t* _expiration,
                                            uint16_t* _storageType)
 {
   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);
   if (NS_FAILED(rv))
     return rv;
@@ -1008,17 +1016,17 @@ nsAnnotationService::GetItemAnnotationIn
     // For annotations created before explicit typing,
     // we can't determine type, just return as string type.
     *_storageType = nsIAnnotationService::TYPE_STRING;
   }
   else {
     *_storageType = type;
   }
 
-  return NS_OK;
+  return GetValueFromStatement(statement, _value);
 }
 
 
 NS_IMETHODIMP
 nsAnnotationService::GetPagesWithAnnotation(const nsACString& aName,
                                             uint32_t* _resultCount,
                                             nsIURI*** _results)
 {
--- a/toolkit/components/places/nsAnnotationService.h
+++ b/toolkit/components/places/nsAnnotationService.h
@@ -149,16 +149,21 @@ protected:
                                        int32_t aFlags,
                                        uint16_t aExpiration);
 
   nsresult RemoveAnnotationInternal(nsIURI* aURI,
                                     int64_t aItemId,
                                     BookmarkData* aBookmark,
                                     const nsACString& aName);
 
+  nsresult
+  GetValueFromStatement(nsCOMPtr<mozIStorageStatement>& aStatement,
+                        nsIVariant** _retval);
+
+
 public:
   nsresult GetPagesWithAnnotationCOMArray(const nsACString& aName,
                                           nsCOMArray<nsIURI>* _results);
   nsresult GetItemsWithAnnotationTArray(const nsACString& aName,
                                         nsTArray<int64_t>* _result);
   nsresult GetAnnotationNamesTArray(nsIURI* aURI,
                                     int64_t aItemId,
                                     nsTArray<nsCString>* _result);
--- a/toolkit/components/places/nsIAnnotationService.idl
+++ b/toolkit/components/places/nsIAnnotationService.idl
@@ -266,16 +266,17 @@ interface nsIAnnotationService : nsISupp
      */
     void getPageAnnotationInfo(in nsIURI aURI,
                                in AUTF8String aName,
                                out int32_t aFlags,
                                out unsigned short aExpiration,
                                out unsigned short aType);
     void getItemAnnotationInfo(in long long aItemId,
                                in AUTF8String aName,
+                               out nsIVariant aValue,
                                out long aFlags,
                                out unsigned short aExpiration,
                                out unsigned short aType);
 
     /**
      * Retrieves the type of an existing annotation
      * Use getAnnotationInfo if you need this along with the mime-type etc.
      *
--- a/toolkit/components/places/tests/unit/test_annotations.js
+++ b/toolkit/components/places/tests/unit/test_annotations.js
@@ -133,22 +133,23 @@ add_task(async function test_execute() {
     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 flags = {}, exp = {}, storageType = {};
+  var value = {}, flags = {}, exp = {}, storageType = {};
   annosvc.getPageAnnotationInfo(testURI, testAnnoName, flags, exp, storageType);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   Assert.equal(storageType.value, Ci.nsIAnnotationService.TYPE_STRING);
-  annosvc.getItemAnnotationInfo(testItemId, testAnnoName, flags, exp, storageType);
+  annosvc.getItemAnnotationInfo(testItemId, testAnnoName, value, flags, exp, storageType);
+  Assert.equal(value.value, testAnnoVal);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   Assert.equal(storageType.value, Ci.nsIAnnotationService.TYPE_STRING);
 
   // get annotation names for a uri
   var annoNames = annosvc.getPageAnnotationNames(testURI);
   Assert.equal(annoNames.length, 1);
   Assert.equal(annoNames[0], "moz-test-places/annotations");
@@ -221,60 +222,63 @@ add_task(async function test_execute() {
   Assert.ok(annosvc.itemHasAnnotation(newItemId, copiedAnno));
   Assert.equal(annosvc.getItemAnnotation(newItemId, "oldAnno"), "new");
 
   // test int32 anno type
   var int32Key = testAnnoName + "/types/Int32";
   var int32Val = 23;
   annosvc.setPageAnnotation(testURI, int32Key, int32Val, 0, 0);
   Assert.ok(annosvc.pageHasAnnotation(testURI, int32Key));
-  flags = {}, exp = {}, storageType = {};
+  value = {}, flags = {}, exp = {}, storageType = {};
   annosvc.getPageAnnotationInfo(testURI, int32Key, flags, exp, storageType);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   Assert.equal(storageType.value, Ci.nsIAnnotationService.TYPE_INT32);
   var storedVal = annosvc.getPageAnnotation(testURI, int32Key);
   Assert.ok(int32Val === storedVal);
   annosvc.setItemAnnotation(testItemId, int32Key, int32Val, 0, 0);
   Assert.ok(annosvc.itemHasAnnotation(testItemId, int32Key));
-  annosvc.getItemAnnotationInfo(testItemId, int32Key, flags, exp, storageType);
+  annosvc.getItemAnnotationInfo(testItemId, int32Key, value, flags, exp, storageType);
+  Assert.equal(value.value, int32Val);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   storedVal = annosvc.getItemAnnotation(testItemId, int32Key);
   Assert.ok(int32Val === storedVal);
 
   // test int64 anno type
   var int64Key = testAnnoName + "/types/Int64";
   var int64Val = 4294967296;
   annosvc.setPageAnnotation(testURI, int64Key, int64Val, 0, 0);
   annosvc.getPageAnnotationInfo(testURI, int64Key, flags, exp, storageType);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   storedVal = annosvc.getPageAnnotation(testURI, int64Key);
   Assert.ok(int64Val === storedVal);
   annosvc.setItemAnnotation(testItemId, int64Key, int64Val, 0, 0);
   Assert.ok(annosvc.itemHasAnnotation(testItemId, int64Key));
-  annosvc.getItemAnnotationInfo(testItemId, int64Key, flags, exp, storageType);
+  annosvc.getItemAnnotationInfo(testItemId, int64Key, value, flags, exp, storageType);
+  Assert.equal(value.value, int64Val);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   storedVal = annosvc.getItemAnnotation(testItemId, int64Key);
   Assert.ok(int64Val === storedVal);
 
   // test double anno type
   var doubleKey = testAnnoName + "/types/Double";
   var doubleVal = 0.000002342;
   annosvc.setPageAnnotation(testURI, doubleKey, doubleVal, 0, 0);
   annosvc.getPageAnnotationInfo(testURI, doubleKey, flags, exp, storageType);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   storedVal = annosvc.getPageAnnotation(testURI, doubleKey);
   Assert.ok(doubleVal === storedVal);
   annosvc.setItemAnnotation(testItemId, doubleKey, doubleVal, 0, 0);
   Assert.ok(annosvc.itemHasAnnotation(testItemId, doubleKey));
-  annosvc.getItemAnnotationInfo(testItemId, doubleKey, flags, exp, storageType);
+  annosvc.getItemAnnotationInfo(testItemId, doubleKey, value, flags, exp, storageType);
+  Assert.equal(value.value, doubleVal);
   Assert.equal(flags.value, 0);
   Assert.equal(exp.value, 0);
   Assert.equal(storageType.value, Ci.nsIAnnotationService.TYPE_DOUBLE);
   storedVal = annosvc.getItemAnnotation(testItemId, doubleKey);
   Assert.ok(doubleVal === storedVal);
 
   // test annotation removal
   annosvc.removePageAnnotation(testURI, int32Key);