isLivemark should use the livemark cache, instead of the db

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Places
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: dietrich, Assigned: dietrich)

Tracking

({perf})

Trunk
mozilla1.9.2a1
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [TSnappiness])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 377207 [details] [diff] [review]
v1

isLivemark() should use the cached set of folder ids, instead of hitting the db every time. that will allow the front-end to use it.

also, nodeIsLivemark() should use the livemark service, post-startup.
(Assignee)

Updated

9 years ago
Whiteboard: [TSnappiness]
(Assignee)

Updated

9 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

9 years ago
Attachment #377207 - Flags: review?(mak77)
(Assignee)

Comment 1

9 years ago
Comment on attachment 377207 [details] [diff] [review]
v1

also improves some smart getters, and removes a hasAnno check (queries--). ignore the debug info there.
Comment on attachment 377207 [details] [diff] [review]
v1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1346,17 +1346,26 @@ function delayedStartup(isLoadingBlank, 
>     }
>   }, 4000);
> 
>   // Delayed initialization of the livemarks update timer.
>   // Livemark updates don't need to start until after bookmark UI 
>   // such as the toolbar has initialized. Starting 5 seconds after
>   // delayedStartup in order to stagger this after the microsummary
>   // service (see above) and before the download manager starts (see below).
>-  setTimeout(function() PlacesUtils.livemarks.start(), 5000);
>+  setTimeout(function() {
>+    // Until now, used the annotations service directly to avoid instantiating
>+    // the Livemark service on startup. (bug 398300)
>+    PlacesUtils.nodeIsLivemarkContainer = function PU_nodeIsLivemarkContainer(aNode) {
>+      return PlacesUtils.nodeIsFolder(aNode) &&
>+             PlacesUtils.livemarks.isLivemark(aNode.itemId);
>+    };

this seems a bit hacky/error-prone, would not be possible to make nodeIsLivemarkContainer ALWAYS use livemarks.isLivemark and make isLivemark check annotation till the folder hash has been inited?


>diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js

>         if (itemId != -1) {
>+          dump("CHECKING OQ>>>>>>\n");

cleanup

>   getSiteURI: function LS_getSiteURI(aFolderId) {
>diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js
>--- a/toolkit/components/places/src/utils.js
>+++ b/toolkit/components/places/src/utils.js
> 
>+  /**
>+   * TODO: this should use the livemark service's cache of folder ids.
>+   */

file a bug and annotate number here please, we tend to forget about TODOs till bugs are filed

waiting for answer on isLivemark before setting review flag
so, as you said me on IRC we can't move the logic to livemark service because we don't want to init the service till start() is called...

So, we must ensure every place where we use livemarks.isLivemark is called after start(), or we should use PlacesUtils helper.

Alternatively, we could remove everything from the service constructor, and call all the constructor code on first start() call. So we would be sure we don't lose time executing anything till start() has been called at least once.
I'm thinking to the possibility that a livemark is on the bookmarks toolbar when we start.
(In reply to comment #3)

> Alternatively, we could remove everything from the service constructor, and
> call all the constructor code on first start() call.

But this way we would force any service user to call start() at least once, and this sounds bad, definately, so discard this idea.
so, either with __lookupGetter__ or with a local _livemarksInitialized private property in PlacesUtils (set to true in the getter), we should self contain code in nodeIsLivemarkContainer.

Please ensure that places where you have replaced checking the anno with livemarks.isLivemark do not initialize the service before we want to... eventually redirect all of them to nodeIsLivemarkContainer.
(Assignee)

Comment 6

9 years ago
Created attachment 377290 [details] [diff] [review]
v2
Attachment #377207 - Attachment is obsolete: true
Attachment #377290 - Flags: review?(mak77)
Attachment #377207 - Flags: review?(mak77)
Comment on attachment 377290 [details] [diff] [review]
v2


>       var grandparent = this.bookmarks.getFolderIdForItem(parent);
>       if (grandparent != this.tagsFolderId &&
>-          !this.annotations.itemHasAnnotation(parent, LMANNO_FEEDURI))
>+          !this.livemarks.isLivemark(parent))

yesterday we discussed about the fact this could init livemarks service in init path, we could provide PlacesUtils.itemIsLivemark, and nodeIsLivemark could call itemIsLivemark(node.itemId) after checking for nodeIsFolder.
clearing request.
(Assignee)

Comment 8

9 years ago
Created attachment 378130 [details] [diff] [review]
v3

added the itemIsLivemark part
Attachment #377290 - Attachment is obsolete: true
Attachment #378130 - Flags: review?(mak77)
Attachment #378130 - Flags: review?(mak77) → review+
Comment on attachment 378130 [details] [diff] [review]
v3

>diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js
>--- a/toolkit/components/places/src/utils.js
>+++ b/toolkit/components/places/src/utils.js
>@@ -354,28 +354,41 @@ var PlacesUtils = {
>    * @returns true if the node is a dynamic container item, false otherwise
>    */
>   nodeIsDynamicContainer: function PU_nodeIsDynamicContainer(aNode) {
>     if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_DYNAMIC_CONTAINER)
>       return true;
>     return false;
>   },
> 
>- /**
>-  * Determines whether a result node is a remote container registered by the
>-  * livemark service.
>-  * @param aNode
>-  *        A result Node
>-  * @returns true if the node is a livemark container item
>-  */
>+  /**
>+   * Determines if an folder id is a livemark.

nit: an folder id.
Should maybe say "a container itemId" since in future could not be a folder anymore.

>+   * @param aItemId

missing description (should the param be aFolderId  or better aContainerItemId then?)

>+   * @returns true if the item is a livemark.
>+   */
>+  itemIsLivemark: function PU_itemIsLivemark(aItemId) {
>+    // Use the annotations service directly to avoid instanciating
>+    // the Livemark service on startup. (bug 398300)
>+    if (this.__lookupGetter__("livemarks"))
>+      return this.annotations.itemHasAnnotation(aItemId, LMANNO_FEEDURI);

the comment should be inside the if, or reference to the check ("if Livemarks service has not yet been initialized...")

>+    // If the livemark service has already been instanciated, use it.
>+    return this.livemarks.isLivemark(aItemId);
>+  },
>+
>+  /**
>+   * Determines whether a result node is a remote container registered by the
>+   * livemark service.

i think "is a livemark container" is much simpler since this comment is too much toward implementation details, and could be we won't even ask to livemarks service.

>+   * @param aNode
>+   *        A result Node
>+   * @returns true if the node is a livemark container item
>+   */
>   nodeIsLivemarkContainer: function PU_nodeIsLivemarkContainer(aNode) {
>-    // Use the annotations service directly to avoid instantiating
>-    // the Livemark service on startup. (bug 398300)
>-    return this.nodeIsFolder(aNode) &&
>-           this.annotations.itemHasAnnotation(aNode.itemId, LMANNO_FEEDURI);
>+    if (!this.nodeIsFolder(aNode))
>+      return false;
>+    return this.itemIsLivemark(aNode.itemId);

maybe return nodeIsFolder && itemIsLivemark?
any reason for the separate if?

>@@ -892,17 +905,17 @@ var PlacesUtils = {
>   getBookmarksForURI:
>   function PU_getBookmarksForURI(aURI) {
>     var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI, {});
> 
>     // filter the ids list
>     return bmkIds.filter(function(aID) {
>       var parent = this.bookmarks.getFolderIdForItem(aID);
>       // Livemark child
>-      if (this.annotations.itemHasAnnotation(parent, LMANNO_FEEDURI))
>+      if (this.livemarks.isLivemark(parent))

ugh, should not this use the util above instead of calling livemarks service?

>         return false;
>       var grandparent = this.bookmarks.getFolderIdForItem(parent);
>       // item under a tag container
>       if (grandparent == this.tagsFolderId)
>         return false;
>       return true;
>     }, this);
>   },
>@@ -912,28 +925,31 @@ var PlacesUtils = {
>    * under tag or livemark containers. -1 is returned if no item is found.
>    */
>   getMostRecentBookmarkForURI:
>   function PU_getMostRecentBookmarkForURI(aURI) {
>     var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI, {});
>     for (var i = 0; i < bmkIds.length; i++) {
>       // Find the first folder which isn't a tag container
>       var bk = bmkIds[i];
>-      var parent = this.bookmarks.getFolderIdForItem(bk);
>-      if (parent == this.unfiledBookmarksFolderId)
>+      var parentId = this.bookmarks.getFolderIdForItem(bk);
>+      if (parentId == this.unfiledBookmarksFolderId)
>         return bk;
> 
>-      var grandparent = this.bookmarks.getFolderIdForItem(parent);
>+      var grandparent = this.bookmarks.getFolderIdForItem(parentId);

since you are changing this, it should be grandParentId

r=me with the above fixed.
please ensure there are no direct calls to livemarks.isLivemark apart the one in itemIsLivemark through all the browser UI codebase.
(Assignee)

Comment 10

9 years ago
Created attachment 378152 [details] [diff] [review]
v4

comments addressed
Attachment #378130 - Attachment is obsolete: true
Attachment #378152 - Flags: review?(mak77)
Comment on attachment 378152 [details] [diff] [review]
v4

>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
>@@ -912,18 +912,17 @@ PlacesTreeView.prototype = {
>             properties.push(this._getAtomFor("tagContainer"));
>           else if (PlacesUtils.nodeIsDay(node))
>             properties.push(this._getAtomFor("dayContainer"));
>           else if (PlacesUtils.nodeIsHost(node))
>             properties.push(this._getAtomFor("hostContainer"));
>         }
>         else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER ||
>                  nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) {
>-          if (PlacesUtils.annotations.itemHasAnnotation(itemId,
>-                                                        LMANNO_FEEDURI))
>+          if (PlacesUtils.nodeIsLivemarkContainer(node))

we already know this is a folder, so i would directly call itemIsLivemarkContainer(node.itemId). Won't change much though, so feel free to ignore me here.

looks good
Attachment #378152 - Flags: review?(mak77) → review+
(Assignee)

Updated

9 years ago
Whiteboard: [TSnappiness] → [TSnappiness][has patch][has review][can land]
Flags: wanted1.9.1?
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Comment on attachment 378152 [details] [diff] [review]
v4

askin approval for this patch, reduces queries load while using Library, avoiding to hit annotations service for items we do already know are livemarks.
Attachment #378152 - Flags: approval1.9.1?
(Assignee)

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/d9417f32e674
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [TSnappiness][has patch][has review][can land] → [TSnappiness]
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

9 years ago
Attachment #378152 - Flags: approval1.9.1?
(Assignee)

Updated

9 years ago
Duplicate of this bug: 420078
You need to log in before you can comment on or make changes to this bug.