94366 - rdf_MakeAbsoluteURI causes 382 NS_MakeAbsoluteURL creation on startup

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P3)

Description

[Suresh Duddi]

On startup, rdf_MakeAbsoluteURI causes 382 nsStdURL creations. All these urls
are already absolute.

Comment 1

[Suresh Duddi]

Attachment: Histogram of url creations by rdf_MakeAbsoluteURI

Comment 2

[Suresh Duddi]

There are only 12 file urls. I will eliminate this from my earlier patch. Will
attach patch next.

Need to do: eliminate the ToNewUTF8String() call in patch after talking to
scc/jaggernaut and then add the patch here.

Comment 3

[Suresh Duddi]

Attachment: Eliminates 370 calls to NS_MakeAbsoluteURI

Comment 4

[Suresh Duddi]

Review from waterson:

Good idea! Only two problems I can see...

1. We probably shouldn't include "file:", since there may be ".."
resolution issues involved. How many of the URLs you saved were "file:"
URLs?

2. There is no need to do ToNewUTF8String() -- which allocates! Talk to
[e-mail] about how to be clean up this ancient string fu.
scc has made great strides in your absence ;-).

Comment 5

[Chris Waterson]

Hey jag, got ideas for the string stuff here?

Comment 6

[jag (Peter Annema)]

Instead of doing ToNewUTF8String, you could make rdf_RequiresAbsoluteURI accept
a |const nsAString&| (or |const nsAReadableString&| if you prefer, same thing)
and do something like:

+ PRBool rdf_RequiresAbsoluteURI(const nsAString& uri)
+ {
+     // cheap shot at figuring out if this requires an absolute url translation
+     if (Substring(uri, 0, 4).Equals(NS_LITERAL_STRING("urn:")) ||
+         Substring(uri, 0, 9).Equals(NS_LITERAL_STRING("chrome:")) ||
+         !Compare(Substring(uri, 0, 3), NS_LITERAL_STRING("nc:"),
+                  nsCaseInsensitiveStringComparator())) {
+         return PR_FALSE;
+     }
+     return PR_TRUE;
+ }

Though at some point I hope we'll be able to say
StartsWith(uri, NS_LSTR("urn:")).

Comment 7

[Suresh Duddi]

Ok. I change nsAString() to provide strncmp() style functionality. Here is the
patch for both that and changes to my earlier patch.

Comment 8

[Suresh Duddi]

Attachment: Improves earlier patch by removing ToNewUTF8String()

Comment 9

[Suresh Duddi]

Attachment: nsAString() providing strncmp() style functionality

Comment 10

[Suresh Duddi]

Attachment: rdf/base/src/rdfutil.cpp patch only. Separating string patch and this.

Comment 11

[jag (Peter Annema)]

Deferring to scc for patch to nsAString

Comment 12

[Suresh Duddi]

The patch "Improves earlier patch ..." didn't have nsAString.h So ignore that. I
broke the patch into two rdf and string and attached them separately.

Comment 13

[jag (Peter Annema)]

For the record, I don't think adding a length parameter to Compare is the way to
go. Substrings already can do this part, and what we want is a convenience
StartsWith method (on which I filed a bug a while back).

Comment 14

[Chris Waterson]

Maybe we should just implement BeginsWith. rdf_BeginsWith() seems like a good
start to me.

Comment 15

[Suresh Duddi (gone)]

After talking to scc, I kind of agree that Compare(n) is not the way to go. I am
reverting to substring(). Will include new patch.

Comment 16

[Suresh Duddi (gone)]

Attachment: patch using substring to figure out if we need to create absolute url or not.

Comment 17

[Suresh Duddi (gone)]

I will take this. I have patch ready to go in.

Comment 18

[Suresh Duddi (gone)]

Waterson - r,sr blessing ?

Comment 19

[Chris Waterson]

Nits. 

- Use PRBool, PR_TRUE, PR_FALSE instead of int, 1, 0? 
- Follow local convention of prefixing arguments with `a' (e.g., |aBase|
  instead of just |base|)
- Put return on a separate line so you can set breakpoint there

Fix those and r= or sr= waterson. Also, `diff -u' is easier for most people to
read than `diff -c'. Thanks for fixing this!

Comment 20

[Suresh Duddi (gone)]

All accepted. Thanks.

Comment 21

[Suresh Duddi (gone)]

Fix checked into 0.9.4

Comment 22

[Tom Everingham]

verified