Bug 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 28 Mar 2018 21:05:27 +0100
changeset 774805 a6506c7edd7041b2541fab68771e551cdabb8d52
parent 774659 1a87c28df4f83ae73f8378a44311d0dd6c240236
child 774806 d5b7ef502b44948e021bfbbeb5eb4caaad7b510c
push id104510
push userbmo:standard8@mozilla.com
push dateThu, 29 Mar 2018 16:11:54 +0000
reviewersmak
bugs1047819
milestone61.0a1
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
@@ -1180,26 +1180,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);