Closed Bug 1047820 Opened 10 years ago Closed 6 years ago

Remove getItemAnnotationNames

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mak, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

We should check the consumers of getAnnotationNames and figure what they try to do, it's possible they are doing something very unefficient like getting all names and then looping through them
Blocks: PlacesJank
Priority: -- → P3
Keywords: perf
Summary: Investigate usage of getAnnotationNames → Investigate usage of getPage/ItemAnnotationNames
Priority: P3 → P2
Depends on: 1047819
Assignee: nobody → standard8
After bug 1047819 lands, we'll be in this state: - getItemAnnotationNames Used by PlacesUtils.getAnnotationsForItem and PlacesController._buildSelectionMetadata. - getPageAnnotationNames Used by PlacesController._buildSelectionMetadata The _buildSelectionMetaData cases are getting annotation details for the items being selected. This is then used when building the context menu to decide what is/isn't displayed based on the 'selection' attribute: https://searchfox.org/mozilla-central/search?q=selection&case=true&regexp=false&path=placesContextMenu.inc.xul The only annotation that's currently listed in the selection attribute is "livemark/feedURI" - we can obtain that via hasCachedLivemarkInfo (the code already does for finding out if there's a livemark parent), so then we don't actually need to get any of the annotations. For the remaining getItemAnnotationNames case, I think this could be combined with a new version of getItemAnnotationInfo - which would save going twice across the xpcom boundaries, and two separate SQL lookups. However, what I'm not quite sure about with that is the idl interface, as can we return an array as a variant, or are we going to need something like mozIAnnotatedResult? If it is another interface, then I'm not sure it is worth combining these functions at the moment, given the uncertain futures of annotations etc.
(In reply to Mark Banner (:standard8) (afk until 3rd April) from comment #1) > The only annotation that's currently listed in the selection attribute is > "livemark/feedURI" - we can obtain that via hasCachedLivemarkInfo (the code > already does for finding out if there's a livemark parent), so then we don't > actually need to get any of the annotations. What about getPageAnnotationNames? I don't see any rule that uses this info, can it be removed already? The only page anno that come to my mind are: downloads/destinationFileURI downloads/metaData" URIProperties/characterSet None of these seem to be involved with _buildSelectionMetadata Removing usage from _buildSelectionMetadata seems to go a long way in the right direction. > However, what I'm not quite sure about with that is the idl interface, as > can we return an array as a variant, or are we going to need something like > mozIAnnotatedResult? > > If it is another interface, then I'm not sure it is worth combining these > functions at the moment, given the uncertain futures of annotations etc. Likely yes, we should wait for this.
Depends on: 1450223
I filed bug 1450223 for _buildSelectionMetadata, the remaining work here can wait
Priority: P2 → P3
Assignee: standard8 → nobody
morphing. This is not trivial because it's used by D&D & clipboard code (through wrapNode and serializeNode), but long term we may discover we don't really need item annotations... and then this would not be a problem.
Summary: Investigate usage of getPage/ItemAnnotationNames → Remove getItemAnnotationNames
the plan is to remove item annotations.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.