492794 - microsummary service should cache bookmark ids for fast access

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

instead of hitting the annotation db every time, the service should cache the list of bookmark ids that are microsummarized, and use an annotation observer to manage the cache.

there's some error right now after deleting a microsummarized bookmark, so not ready for review yet.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v2

working

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 378263 [details] [diff] [review]
v2

>diff --git a/browser/components/microsummaries/src/nsMicrosummaryService.js b/browser/components/microsummaries/src/nsMicrosummaryService.js
>--- a/browser/components/microsummaries/src/nsMicrosummaryService.js
>+++ b/browser/components/microsummaries/src/nsMicrosummaryService.js
>@@ -18,16 +18,17 @@
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #  Myk Melez <[e-mail]> (Original Author)
> #  Simon Bünzli <[e-mail]>
> #  Asaf Romano <[e-mail]>
> #  Dan Mills <[e-mail]>
> #  Ryan Flint <[e-mail]>
>+#  Dietrich <[e-mail]>

i do know you're famous and unique, but adding last name could be a good idea :)

>@@ -558,53 +559,49 @@ MicrosummaryService.prototype = {
> 
>   /**
>    * Get the set of bookmarks with microsummaries.
>    *
>    * This is the internal version of this method, which is not accessible

since you changed name from _getbookmarks to _bookmarks the comment "of this method" does not make much sense (which method?)

>    * via XPCOM but is more performant; inside this component, use this version.
>    * Outside the component, use getBookmarks (no underscore prefix) instead.
>    *
>-   * @returns an array of place: uris representing bookmarks items
>+   * This caches the list of microsummarized bookmarks. The cache is
>+   * managed by observing the annotation service, and updating 
>+   * when a microsummary annotation is added or removed.
>+   *
>+   * @returns an array of place ids for bookmarks items

those don't look like place ids, not as we inted in Places at least. should be bookmark itemIds or bookmark ids

>@@ -690,17 +687,17 @@ MicrosummaryService.prototype = {
>    * @param   bookmarkID
>    *          the bookmark for which to set the current microsummary
>    *
>    * @returns a boolean representing whether or not the given bookmark
>    *          has a current microsummary
>    *
>    */
>   hasMicrosummary: function MSS_hasMicrosummary(bookmarkID) {
>-    return this._ans.itemHasAnnotation(bookmarkID, ANNO_MICSUM_GEN_URI);
>+    return (this._bookmarks.indexOf(bookmarkID) > -1);

while here, param should be called aBookmarkID
why checking > -1 instead of the usual != -1? Looking around in mxr i see != -1 is the most used style.

>@@ -780,17 +780,32 @@ MicrosummaryService.prototype = {
>     };
> 
>     // Register the observer with the microsummary and trigger the microsummary
>     // to update itself.
>     microsummary.addObserver(observer);
>     microsummary.update();
>     
>     return microsummary;
>-  }
>+  },
>+    

nit: trailing spaces

>+  // nsIAnnotationObserver
>+  onItemAnnotationSet: function(aItemId, aAnnotationName) {
>+    if (aAnnotationName == ANNO_MICSUM_GEN_URI &&
>+        this._bookmarks.indexOf(aItemId) == -1)
>+      this._bookmarks.push(aItemId);
>+  },
>+  onItemAnnotationRemoved: function(aItemId, aAnnotationName) {
>+    var index = this._bookmarks.indexOf(aItemId);
>+    if (index > -1 && (!aAnnotationName.length /* removed all annos */ ||
>+        aAnnotationName == ANNO_MICSUM_GEN_URI))

i would prefer having a temp var for isMicrosummaryAnnotation, or better indentation.

r=me with those fixed

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: v2.1 (for checkin)

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 379730 [details] [diff] [review]
v2.1 (for checkin)

asking pre approval, reduces queries load due to microsummary service, every time we check if a bookmark has a microsummary.

Comment 5

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/e2c6142749d8