399490 - perf: eliminate nsIURI usage in the places API

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

<Mano>	dietrich: i'm pondering making all places api takes uri spec rather than uris
<Mano>	all/most
<Mano>	dietrich: we've spec->uri->spec sequences across all places code
<dietrich> is perf the concern?
<Mano>	dietrich: yeah
<Mano>	dietrich: and code-sanity
<sayrer>	URIs are not as cheap as they could be, unfortunately
<Mano>	dietrich: see setPageAnnotation* and insertBookmark for example
<Mano>	as it happens, callers have the spec too
<Mano>	so we're doing newURI(spec), and then insertBookmarks takes the spec
<Mano>	usually with xpconnect overhead as well

Comment 1

[Jesse Ruderman]

Isn't the normal solution to this kind of problem to use nsIURI in more APIs, not fewer?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes.  Especially as we move into the mozilla2 world where URIs have a security context.  And especially because spec strings lead to people hand-writing parsing code.

Also, note that getting the spec from a URI is _really_ cheap.  At least if you're in C++.

Comment 3

[Asaf Romano (gone)]

Jesse: this is about API like insertBookmark/setPageAnnotation in which we're accessing the places db.

Comment 4

[Boris Zbarsky [:bzbarsky]]

sure, but who has a spec instead of a URI to start?

That is, we want to go URI->spec as late as possible, and only once.  Doesn't sound like we're succeeding.

Comment 5

[Asaf Romano (gone)]

A text box, for instance, or a href attribute (or, a hard-coded place: uri).

Comment 6

[Boris Zbarsky [:bzbarsky]]

How common are those cases?

For href, if it's common, perhaps we should expose an nsIURI getter.  As it is, HTMLAnchorElement.href creates an nsIURI and then returns its spec.

I can see how if all your input is opaque strings you might as well have all the APIs be strings.  But if most of your input is nsIURI at some point, we should work on getting that URI object to the end consumer.

Comment 7

[Asaf Romano (gone)]

(In reply to comment #6)
> How common are those cases?
> 
> For href, if it's common, perhaps we should expose an nsIURI getter.  As it is,
> HTMLAnchorElement.href creates an nsIURI and then returns its spec.
> 

For the href case, it's the places-import-export's content sink (in which performance really matters :-/).

Comment 8

[Asaf Romano (gone)]

See also http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#355

Comment 9

[Boris Zbarsky [:bzbarsky]]

So yeah.  I agree that using nsIURIs for place: URIs gets us nothing (mostly because place: is pretty much a hack as far as I can tell).

Does the content sink start our with strings that represent absolute URIs, basically?  Can you point me to that code?

If none of the consumers of this code really have an nsIURI to start with, then I agree the API should just use strings.

Comment 10

[Asaf Romano (gone)]

Sometimes they do (e.g. when bookmaking the uri loaded in a given browser), I'm considering of keeping both variants in place.

See http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#868 for the content-sink code.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, that code is using a base URI, so it needs to go through nsIURI no matter what....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Er, never mind that.  It's not using a base URI.  So as long as you're _sure_ that the string is already fully normalized you can use it as-is.

Comment 13

[Mike Connor [:mconnor]]

Not going to continue to block on this, please renominate with reasons why we can't ship with this in final

Comment 14

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 15

[Marco Bonardo [:mak]]

not going to do this blindly, in new APIs we are trying to allow passing URL or even href, but there's no point in changing the old APIs just for a non measurable gain.