707946 - Speed-up hasChildren for tags

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

We already know a tag has children and the tagging service has a nice cache we can leverage.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Will require SR but it's a small change

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 579486 [details] [diff] [review]
patch v1.0

Review of attachment 579486 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. could this cause the tag service to initialize earlier than previously? if so, should keep an eye on Ts.

Comment 3

[Marco Bonardo [:mak]]

Only if you have a copy of the Tags query in your bookmarks toolbar and the toolbar is visible. Otherwise that code path is hit when opening the Library.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 579486 [details] [diff] [review]
patch v1.0

Needs SR for the attribute addition.
Note that using allTags.length wouldn't be as performant since allTags does an expensive sorting to return tags sorted by name.

I expect in future we may just merge this interface into the bookmarks interface, since we allow to tag bookmarks and having them separated caused more troubles than wins.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 579486 [details] [diff] [review]
patch v1.0

>diff --git a/toolkit/components/places/nsITaggingService.idl b/toolkit/components/places/nsITaggingService.idl

> interface nsITaggingService : nsISupports

>+  /**
>+   * Whether there is any tag.
>+   */
>+  readonly attribute boolean hasTags;

"hasTags" seems a little awkward but I can't come up with anything better. I'd change the comment to "whether any tags exist", though.

Comment 6

[Marco Bonardo [:mak]]

fixed the comment, I also added a brief @note regarding the performance difference
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b51ee2ee388

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b51ee2ee388