rdf_MakeAbsoluteURI causes 382 NS_MakeAbsoluteURL creation on startup

VERIFIED FIXED in mozilla0.9.4

Status

defect
P3
normal
VERIFIED FIXED
18 years ago
11 months ago

People

(Reporter: dpsuresh, Assigned: dp)

Tracking

({perf})

Trunk
mozilla0.9.4
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Reporter

Description

18 years ago
On startup, rdf_MakeAbsoluteURI causes 382 nsStdURL creations. All these urls
are already absolute.
Reporter

Comment 2

18 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 4

18 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

18 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

18 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:")).

Updated

18 years ago
Keywords: perf
Reporter

Comment 7

18 years ago
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
Reporter

Comment 12

18 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.
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).

Updated

18 years ago
Blocks: 7251

Comment 14

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

Updated

18 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Assignee

Comment 15

18 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 17

18 years ago
I will take this. I have patch ready to go in.
Assignee: waterson → dp
Status: ASSIGNED → NEW
Assignee

Comment 18

18 years ago
Waterson - r,sr blessing ?
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 19

18 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

18 years ago
All accepted. Thanks.
Assignee

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Assignee

Comment 21

18 years ago
Fix checked into 0.9.4

Comment 22

18 years ago
verified
Status: RESOLVED → VERIFIED

Updated

11 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.