1047820 - Remove getItemAnnotationNames

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Mark Banner (: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.

Comment 2

[Marco Bonardo [:mak]]

(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.

Comment 3

[Marco Bonardo [:mak]]

I filed bug 1450223 for _buildSelectionMetadata, the remaining work here can wait

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Marco Bonardo [:mak]]

the plan is to remove item annotations.