Bug 1450223 - Remove unnecessary synchronous annotation lookups from the places context menu. r?mak draft
authorMark Banner <standard8@mozilla.com>
Thu, 29 Mar 2018 15:49:39 +0100
changeset 776592 2b99c43296bb87984ec7bfaf564374c20a9c17de
parent 776477 d75d996016dcf325c2db2ed8a47af512d07ffacd
push id104911
push userbmo:standard8@mozilla.com
push dateTue, 03 Apr 2018 11:15:07 +0000
reviewersmak
bugs1450223
milestone61.0a1
Bug 1450223 - Remove unnecessary synchronous annotation lookups from the places context menu. r?mak MozReview-Commit-ID: Ck2eaeVAjXs
browser/components/places/content/controller.js
browser/components/places/content/placesContextMenu.inc.xul
toolkit/components/places/nsAnnotationService.cpp
toolkit/components/places/nsAnnotationService.h
toolkit/components/places/nsIAnnotationService.idl
toolkit/components/places/tests/unit/test_annotations.js
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -395,17 +395,16 @@ PlacesController.prototype = {
   _buildSelectionMetadata: function PC__buildSelectionMetadata() {
     var metadata = [];
     var nodes = this._view.selectedNodes;
 
     for (var i = 0; i < nodes.length; i++) {
       var nodeData = {};
       var node = nodes[i];
       var nodeType = node.type;
-      var uri = null;
 
       // We don't use the nodeIs* methods here to avoid going through the type
       // property way too often
       switch (nodeType) {
         case Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY:
           nodeData.query = true;
           if (node.parent) {
             switch (PlacesUtils.asQuery(node.parent).queryOptions.resultType) {
@@ -417,50 +416,38 @@ PlacesController.prototype = {
                 nodeData.day = true;
                 break;
             }
           }
           break;
         case Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER:
         case Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT:
           nodeData.folder = true;
+          if (this.hasCachedLivemarkInfo(node)) {
+            nodeData.isLivemark = true;
+          }
           break;
         case Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR:
           nodeData.separator = true;
           break;
         case Ci.nsINavHistoryResultNode.RESULT_TYPE_URI:
           nodeData.link = true;
-          uri = Services.io.newURI(node.uri);
           if (PlacesUtils.nodeIsBookmark(node)) {
             nodeData.bookmark = true;
             var parentNode = node.parent;
             if (parentNode) {
               if (PlacesUtils.nodeIsTagQuery(parentNode))
                 nodeData.tagChild = true;
               else if (this.hasCachedLivemarkInfo(parentNode))
                 nodeData.livemarkChild = true;
             }
           }
           break;
       }
 
-      // annotations
-      if (uri) {
-        let names = PlacesUtils.annotations.getPageAnnotationNames(uri);
-        for (let j = 0; j < names.length; ++j)
-          nodeData[names[j]] = true;
-      }
-
-      // For items also include the item-specific annotations
-      if (node.itemId != -1) {
-        let names = PlacesUtils.annotations
-                               .getItemAnnotationNames(node.itemId);
-        for (let j = 0; j < names.length; ++j)
-          nodeData[names[j]] = true;
-      }
       metadata.push(nodeData);
     }
 
     return metadata;
   },
 
   /**
    * Determines if a context-menu item should be shown
--- a/browser/components/places/content/placesContextMenu.inc.xul
+++ b/browser/components/places/content/placesContextMenu.inc.xul
@@ -120,17 +120,17 @@
             accesskey="&cmd.context_sortby_name.accesskey;"
             closemenu="single"
             selection="folder"/>
   <menuitem id="placesContext_reload"
             command="placesCmd_reload"
             label="&cmd.reloadLivebookmark.label;"
             accesskey="&cmd.reloadLivebookmark.accesskey;"
             closemenu="single"
-            selection="livemark/feedURI"/>
+            selection="isLivemark"/>
   <menuseparator id="placesContext_sortSeparator"/>
   <menuitem id="placesContext_show:info"
             command="placesCmd_show:info"
             label="&cmd.properties.label;"
             accesskey="&cmd.properties.accesskey;"
             selection="bookmark|folder|query"
             forcehideselection="livemarkChild"/>
-</menupopup>
\ No newline at end of file
+</menupopup>
--- a/toolkit/components/places/nsAnnotationService.cpp
+++ b/toolkit/components/places/nsAnnotationService.cpp
@@ -1245,92 +1245,33 @@ nsAnnotationService::GetItemsWithAnnotat
     if (!_results->AppendElement(stmt->AsInt64(0)))
       return NS_ERROR_OUT_OF_MEMORY;
   }
 
   return NS_OK;
 }
 
 
-NS_IMETHODIMP
-nsAnnotationService::GetPageAnnotationNames(nsIURI* aURI,
-                                            uint32_t* _count,
-                                            nsIVariant*** _result)
-{
-  NS_ENSURE_ARG(aURI);
-  NS_ENSURE_ARG_POINTER(_count);
-  NS_ENSURE_ARG_POINTER(_result);
-
-  *_count = 0;
-  *_result = nullptr;
-
-  nsTArray<nsCString> names;
-  nsresult rv = GetAnnotationNamesTArray(aURI, 0, &names);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (names.Length() == 0)
-    return NS_OK;
-
-  *_result = static_cast<nsIVariant**>
-                        (moz_xmalloc(sizeof(nsIVariant*) * names.Length()));
-  NS_ENSURE_TRUE(*_result, NS_ERROR_OUT_OF_MEMORY);
-
-  for (uint32_t i = 0; i < names.Length(); i ++) {
-    nsCOMPtr<nsIWritableVariant> var = new nsVariant();
-    if (!var) {
-      // need to release all the variants we've already created
-      for (uint32_t j = 0; j < i; j ++)
-        NS_RELEASE((*_result)[j]);
-      free(*_result);
-      *_result = nullptr;
-      return NS_ERROR_OUT_OF_MEMORY;
-    }
-    var->SetAsAUTF8String(names[i]);
-    NS_ADDREF((*_result)[i] = var);
-  }
-  *_count = names.Length();
-
-  return NS_OK;
-}
-
-
 nsresult
-nsAnnotationService::GetAnnotationNamesTArray(nsIURI* aURI,
-                                              int64_t aItemId,
-                                              nsTArray<nsCString>* _result)
+nsAnnotationService::GetItemAnnotationNamesTArray(int64_t aItemId,
+                                                  nsTArray<nsCString>* _result)
 {
   _result->Clear();
 
-  bool isItemAnnotation = (aItemId > 0);
   nsCOMPtr<mozIStorageStatement> statement;
-  if (isItemAnnotation) {
-    statement = mDB->GetStatement(
-      "SELECT n.name "
-      "FROM moz_anno_attributes n "
-      "JOIN moz_items_annos a ON a.anno_attribute_id = n.id "
-      "WHERE a.item_id = :item_id"
-    );
-  }
-  else {
-    statement = mDB->GetStatement(
-      "SELECT n.name "
-      "FROM moz_anno_attributes n "
-      "JOIN moz_annos a ON a.anno_attribute_id = n.id "
-      "JOIN moz_places h ON h.id = a.place_id "
-      "WHERE h.url_hash = hash(:page_url) AND h.url = :page_url"
-    );
-  }
+  statement = mDB->GetStatement(
+    "SELECT n.name "
+    "FROM moz_anno_attributes n "
+    "JOIN moz_items_annos a ON a.anno_attribute_id = n.id "
+    "WHERE a.item_id = :item_id"
+  );
   NS_ENSURE_STATE(statement);
   mozStorageStatementScoper scoper(statement);
 
-  nsresult rv;
-  if (isItemAnnotation)
-    rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
-  else
-    rv = URIBinder::Bind(statement, NS_LITERAL_CSTRING("page_url"), aURI);
+  nsresult rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
   NS_ENSURE_SUCCESS(rv, rv);
 
   bool hasResult = false;
   while (NS_SUCCEEDED(statement->ExecuteStep(&hasResult)) &&
          hasResult) {
     nsAutoCString name;
     rv = statement->GetUTF8String(0, name);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -1350,17 +1291,17 @@ nsAnnotationService::GetItemAnnotationNa
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_count);
   NS_ENSURE_ARG_POINTER(_result);
 
   *_count = 0;
   *_result = nullptr;
 
   nsTArray<nsCString> names;
-  nsresult rv = GetAnnotationNamesTArray(nullptr, aItemId, &names);
+  nsresult rv = GetItemAnnotationNamesTArray(aItemId, &names);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (names.Length() == 0)
     return NS_OK;
 
   *_result = static_cast<nsIVariant**>
                         (moz_xmalloc(sizeof(nsIVariant*) * names.Length()));
   NS_ENSURE_TRUE(*_result, NS_ERROR_OUT_OF_MEMORY);
--- a/toolkit/components/places/nsAnnotationService.h
+++ b/toolkit/components/places/nsAnnotationService.h
@@ -159,15 +159,14 @@ protected:
                         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);
+  nsresult GetItemAnnotationNamesTArray(int64_t aItemId,
+                                        nsTArray<nsCString>* _result);
   nsresult RemoveItemAnnotationsWithoutNotifying(int64_t aItemId);
 };
 
 #endif /* nsAnnotationService_h___ */
--- a/toolkit/components/places/nsIAnnotationService.idl
+++ b/toolkit/components/places/nsIAnnotationService.idl
@@ -317,20 +317,16 @@ interface nsIAnnotationService : nsISupp
         [retval, array, size_is(count)] out mozIAnnotatedResult results);
 
     /**
      * Get the names of all annotations for this URI.
      *
      * example JS:
      *   var annotations = annotator.getPageAnnotations(myURI, {});
      */
-    void getPageAnnotationNames(
-      in nsIURI aURI,
-      [optional] out unsigned long count,
-      [retval, array, size_is(count)] out nsIVariant result);
     void getItemAnnotationNames(
       in long long aItemId,
       [optional] out unsigned long count,
       [retval, array, size_is(count)] out nsIVariant result);
 
     /**
      * Test for annotation existence.
      */
--- a/toolkit/components/places/tests/unit/test_annotations.js
+++ b/toolkit/components/places/tests/unit/test_annotations.js
@@ -144,23 +144,18 @@ add_task(async function test_execute() {
   Assert.equal(exp.value, 0);
   Assert.equal(storageType.value, Ci.nsIAnnotationService.TYPE_STRING);
   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");
-
   // get annotation names for an item
-  annoNames = annosvc.getItemAnnotationNames(testItemId);
+  let annoNames = annosvc.getItemAnnotationNames(testItemId);
   Assert.equal(annoNames.length, 1);
   Assert.equal(annoNames[0], "moz-test-places/annotations");
 
   // 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));