493636 - when possible, use cached left pane query ids instead of the anno service directly

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

currently the treeview queries the annotation service to see if a container is one of the left pane containers.

since we already cache those in PlacesUIUtils.leftPaneQueries, we should use that instead.

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 378144 [details] [diff] [review]
v1

>diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js
>--- a/browser/components/places/content/treeView.js
>+++ b/browser/components/places/content/treeView.js
>@@ -918,25 +918,19 @@ PlacesTreeView.prototype = {
>         else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER ||
>                  nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) {
>           if (PlacesUtils.annotations.itemHasAnnotation(itemId,
>                                                         LMANNO_FEEDURI))
>             properties.push(this._getAtomFor("livemark"));
>         }
> 
>         if (itemId != -1) {
>-          var oqAnno;
>-          try {
>-            oqAnno = PlacesUtils.annotations
>-                                .getItemAnnotation(itemId,
>-                                                   ORGANIZER_QUERY_ANNO);
>+          var oqAnno = PlacesUIUtils.itemIsLeftPaneQuery(itemId);
>+          if (oqAnno)
>             properties.push(this._getAtomFor("OrganizerQuery_" + oqAnno));

while here, and since we are removing anno code form here, i suggest to expand "oqAnno" var name, it's not clear what it does mean.
if we are abstracting that to "get name of the query", it should not even contain the "anno" word

>diff --git a/browser/components/places/content/utils.js b/browser/components/places/content/utils.js
>--- a/browser/components/places/content/utils.js
>+++ b/browser/components/places/content/utils.js
>@@ -1294,21 +1294,50 @@ var PlacesUIUtils = {
>       }
>     };
>     bs.runInBatchMode(callback, null);
> 
>     delete this.leftPaneFolderId;
>     return this.leftPaneFolderId = leftPaneRoot;
>   },
> 
>+  /**
>+   * Get the folder id for the organizer left-pane folder.
>+   */
>   get allBookmarksFolderId() {
>     // ensure the left-pane root is initialized;
>     this.leftPaneFolderId;
>     delete this.allBookmarksFolderId;
>     return this.allBookmarksFolderId = this.leftPaneQueries["AllBookmarks"];
>+  },
>+
>+  /**
>+   * Determines if an item id is one of the left-pane queries
>+   * in the organizer.
>+   * @param aItemId id of a container
>+   * @returns the name of the query, or empty string if not a left-pane query
>+   */
>+  itemIsLeftPaneQuery: function PU_itemIsLeftPaneQuery(aItemId) {

The name is confusing, i know one can simply read the javadoc, but seeing a method called isSomething i expect to get a boolean (same would be for hasSomething).
Maybe GetLeftPaneQueryNameFromId, returns the name, or an empty string if this is not a left pane query.

>+    // If the let pane hasn't been built, use the annotation service
>+    // directly, to avoid building the left pane too early.
>+    if (this.__lookupGetter__("leftPaneFolderId")) {
>+      try {
>+        return PlacesUtils.annotations.
>+                           getItemAnnotation(itemId, ORGANIZER_QUERY_ANNO);
>+      }
>+      catch (ex) { /* doesn't have the annotation */ }
>+      return "";

instead of the early return you could probably if/else and return "" only at the end

>+    }
>+
>+    // If the left pane has already been built, use the cached obj

"use the cached obj" should be better expanded (which cached obj?)
Please end comments with "."

>+    for (let [name, id] in Iterator(this.leftPaneQueries)) { 

nit: trailing space

>+      if (aItemId == id)
>+        return name;
>+    }
>+    return "";
>   },

r=me with those fixed.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: v1.1 (for check-in)

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 379727 [details] [diff] [review]
v1.1 (for check-in)

asking approval, reduces queries load when using Library, due to trying to recognize left pane queries in tree views.

Comment 4

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/9cb4a746d8b0