Closed Bug 94366 Opened 23 years ago Closed 23 years ago

rdf_MakeAbsoluteURI causes 382 NS_MakeAbsoluteURL creation on startup

Categories

(Core Graveyard :: RDF, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: dpsuresh, Assigned: dp)

References

Details

(Keywords: perf)

Attachments

(6 files)

On startup, rdf_MakeAbsoluteURI causes 382 nsStdURL creations. All these urls
are already absolute.
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.
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
jaggernaut@netscape.com about how to be clean up this ancient string fu.
scc has made great strides in your absence ;-).
Hey jag, got ideas for the string stuff here?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla0.9.4
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:")).
Keywords: perf
Ok. I change nsAString() to provide strncmp() style functionality. Here is the
patch for both that and changes to my earlier patch.
Deferring to scc for patch to nsAString
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.
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).
Blocks: 7251
Maybe we should just implement BeginsWith. rdf_BeginsWith() seems like a good
start to me.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
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.
I will take this. I have patch ready to go in.
Assignee: waterson → dp
Status: ASSIGNED → NEW
Waterson - r,sr blessing ?
Status: NEW → ASSIGNED
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!


All accepted. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Fix checked into 0.9.4
verified
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.