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)
Core Graveyard
RDF
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.4
People
(Reporter: dpsuresh, Assigned: dp)
References
Details
(Keywords: perf)
Attachments
(6 files)
8.48 KB,
text/plain
|
Details | |
2.71 KB,
patch
|
Details | Diff | Splinter Review | |
4.44 KB,
patch
|
Details | Diff | Splinter Review | |
1.16 KB,
patch
|
Details | Diff | Splinter Review | |
3.01 KB,
patch
|
Details | Diff | Splinter Review | |
2.69 KB,
patch
|
Details | Diff | Splinter Review |
On startup, rdf_MakeAbsoluteURI causes 382 nsStdURL creations. All these urls are already absolute.
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
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.
Reporter | ||
Comment 3•23 years ago
|
||
Reporter | ||
Comment 4•23 years ago
|
||
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 ;-).
Comment 5•23 years ago
|
||
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
Comment 6•23 years ago
|
||
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:")).
Reporter | ||
Comment 7•23 years ago
|
||
Ok. I change nsAString() to provide strncmp() style functionality. Here is the patch for both that and changes to my earlier patch.
Reporter | ||
Comment 8•23 years ago
|
||
Reporter | ||
Comment 9•23 years ago
|
||
Reporter | ||
Comment 10•23 years ago
|
||
Comment 11•23 years ago
|
||
Deferring to scc for patch to nsAString
Reporter | ||
Comment 12•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Maybe we should just implement BeginsWith. rdf_BeginsWith() seems like a good start to me.
Updated•23 years ago
|
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Assignee | ||
Comment 15•23 years ago
|
||
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.
Assignee | ||
Comment 16•23 years ago
|
||
Assignee | ||
Comment 17•23 years ago
|
||
I will take this. I have patch ready to go in.
Assignee: waterson → dp
Status: ASSIGNED → NEW
Assignee | ||
Comment 18•23 years ago
|
||
Waterson - r,sr blessing ?
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Comment 19•23 years ago
|
||
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!
Assignee | ||
Comment 20•23 years ago
|
||
All accepted. Thanks.
Assignee | ||
Updated•23 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Assignee | ||
Comment 21•23 years ago
|
||
Fix checked into 0.9.4
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•