Closed
Bug 492794
Opened 15 years ago
Closed 15 years ago
microsummary service should cache bookmark ids for fast access
Categories
(Firefox Graveyard :: Microsummaries, defect)
Firefox Graveyard
Microsummaries
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 3.6a1
People
(Reporter: dietrich, Assigned: dietrich)
References
Details
(Keywords: perf, Whiteboard: [TSnappiness])
Attachments
(1 file, 2 obsolete files)
13.68 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 1•15 years ago
|
||
working
Attachment #377206 -
Attachment is obsolete: true
Attachment #378263 -
Flags: review?(mak77)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [TSnappiness] → [TSnappiness][needs review mak]
Updated•15 years ago
|
Attachment #378263 -
Flags: review?(mak77) → review+
Comment 2•15 years ago
|
||
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 <myk@mozilla.org> (Original Author) > # Simon Bünzli <zeniko@gmail.com> > # Asaf Romano <mano@mozilla.com> > # Dan Mills <thunder@mozilla.com> > # Ryan Flint <rflint@dslr.net> >+# Dietrich <dietrich@mozilla.com> 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
Updated•15 years ago
|
Whiteboard: [TSnappiness][needs review mak] → [TSnappiness]
Assignee | ||
Comment 3•15 years ago
|
||
Attachment #378263 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Whiteboard: [TSnappiness] → [TSnappiness][has review][can land]
Updated•15 years ago
|
Flags: wanted-firefox3.5?
Updated•15 years ago
|
Attachment #379730 -
Flags: approval1.9.1?
Comment 4•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #379730 -
Flags: approval1.9.1?
Assignee | ||
Updated•15 years ago
|
Whiteboard: [TSnappiness][has review][can land] → [TSnappiness]
Target Milestone: --- → Firefox 3.6a1
Assignee | ||
Updated•15 years ago
|
Flags: wanted-firefox3.5?
Assignee | ||
Comment 5•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e2c6142749d8
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•