4.61 KB, patch
|Details | Diff | Splinter Review|
216 bytes, text/html
4.44 KB, patch
|Details | Diff | Splinter Review|
In particular, it at least needs equalsExceptRef and cloneIgnoringRef. Are there other URI classes we missed?
oops! (Why doesn't it have a compile error, then, for lacking impls for those methods?) > Are there other URI classes we missed? I don't think so, at least not top-level ones. I searching for "public nsIURI" and ": nsIURI", and everything that turned up besides nsJSURI_base was covered in bug 308590. It's possible that we missed some non-top-level URI classes that need customizations of those methods, though. (In most cases I'd hope that the inherited versions would suffice, but good to be sure.) I'll look more thoroughly for anything like that after fixing up nsJSURI.  http://mxr.mozilla.org/mozilla-central/search?string=public%20nsIURI  http://mxr.mozilla.org/mozilla-central/search?string=%3A%20nsIURI
> Why doesn't it have a compile error, then Because it inherits from a class that implements all of nsIURI by forwarding to an nsSimpleURI instance. It then overrides some of the methods. That's about to change on cedar to nsJSURI just inheriting from nsSimpleURI and overriding some methods, but the basic setup is the same either way.
(In reply to comment #2) > > Why doesn't it have a compile error, then > > Because it inherits from a class that implements all of nsIURI by forwarding > to an nsSimpleURI instance. Ah, "NS_FORWARD_NSIURI". I see.
That's the only instance of NS_FORWARD_NSIUR* in our codebase, happily. http://mxr.mozilla.org/mozilla-central/search?string=NS_FORWARD_NSIUR
(In reply to comment #2) > That's about to change on cedar to nsJSURI just inheriting from nsSimpleURI (in Bug 647570, for reference) Marking this bug as depending on that bug, since I'm basing the patch here on top of that.
Depends on: 647570
Yes, I believe bug 308590 "breaks" such URIs. (unrelated to this bug here, though) I think you should be able to fix that by replacing "#" with "%23" in such URIs. (At least, that works fine in data URIs.)
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Here's a patch, with an xpcshell test included. It layers on top of Bug 647570. (In reply to comment #0) > In particular, it at least needs equalsExceptRef and cloneIgnoringRef. I don't think we actually need a specialized cloneIgnoringRef after Bug 647570, do we? The only custom data we have in nsJSURI is mBaseURI, and that already gets cloned in the StartClone() impl. (I added a comment in that method, clarifying its ref-handling behavior.)
Attachment #535244 - Attachment is obsolete: true
(Fixed a few small things I noticed when glancing over the first attachment.)
Attachment #535247 - Flags: review?(bzbarsky)
I've also changed the logic & up-front-expectations in nsJSURI::EqualsInternal to more closely match nsSimpleURI::EqualsInternal, since there's no real reason they should differ on those.
Comment on attachment 535247 [details] [diff] [review] fix v1a r=me. Thanks!
Attachment #535247 - Flags: review?(bzbarsky) → review+
Ah, yes. GetPath() is used to get the string to evaluate. OK, good.
Landed patch on cedar: http://hg.mozilla.org/projects/cedar/rev/01a96f067572
(In reply to comment #1) > It's possible that we missed some non-top-level URI classes that need > customizations of those methods, though. I'll look more > thoroughly for anything like that after fixing up nsJSURI. Just to follow up on this - I spent some time looking through mxr (searching ns.*URI, ns.*URL, 'public nsIURI' / ': nsIURI', public nsSimpleURI, public nsStandardURL, etc) and didn't find any other classes that I'd missed in bug 308590. (As noted in comment 2-4, I only missed this one because it forwarded its methods to a nsSimpleURI data member via NS_FORWARD_NSIURI, and it's the only class in our codebase to implement nsIURI or nsIURL in that way, so there shouldn't be any other issues like this hiding anywhere.)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
please nominate the patch for approval Aurora for 6. You have about 5 weeks to get it approved and landed there.
Comment on attachment 535247 [details] [diff] [review] fix v1a Requesting approval to land on aurora. This patch changes a method signature in one nsIURI implementation that I'd missed in bug 308590. I'd missed it because it blanket-forwarded all not-explicitly-overridden nsIURI methods to another object via the NS_FORWARD_NSIURI macro. I've verified that no other classes used that macro. Reward: Fixes regression in nsJSURI::Equals behavior. Includes testcase. Risk: Low.
Comment on attachment 535247 [details] [diff] [review] fix v1a Canceling approval request - as noted on bug 647570, I'm going to write an aurora-specific version of this that doesn't layer on top of bug 647570.
Here's the fix for aurora. As bug 308590 did for other nsIURI classes, this patch just shifts the existing Equals() & Clone() functionality into "Internal" methods, and then uses one-liners for the normal & ref-ignoring versions of those functions.
This includes the xpcshell test from the trunk patch. I verified that the test fails on current aurora, but passes with the fix applied.
Comment on attachment 536744 [details] [diff] [review] fix for aurora r=me
Attachment #536744 - Flags: review?(bzbarsky) → review+
Attachment #536744 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Landed on aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/84ef9983832e
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0 Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0 I have runned the tescase on these builds and it works fine. But i encountered a problem on Mac: clicking the alerts and hitting the Esc button makes my mouse disappear. Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.