[webkit-reviews] review denied: [Bug 10323] REGRESSION: javascript: URL containing '\\' gets passed as '//' : [Attachment 10429] Patch - First attempt

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Thu Sep 7 07:48:53 PDT 2006


Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 10323: REGRESSION: javascript: URL containing '\\' gets passed as '//'
http://bugzilla.opendarwin.org/show_bug.cgi?id=10323

Attachment 10429: Patch - First attempt
http://bugzilla.opendarwin.org/attachment.cgi?id=10429&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for tackling this!

The patch has an object lifetime problem. The result of substituteBackslashes,
with this code change, will only live until the end of the expression that
initializes rel, leaving rel referring to a deleted object. There's a reason
that the subsitutedRelative variable was declared outside the if statement in
the old code, and this new code is broken.

I am surprised to see an explicit check for the javascript scheme here. I
thought we were going to do this based on hierarchical vs. non-hierarchical
URLs. And I'd really like the backslash hack to be an exception rather than the
rule -- it would be better to have limited cases where it *does* happen rather
than limited cases where it does not.

That having been said, I think this approach is OK, but not great.

There's also a missing space before "false" in the function call.

review- just for the object lifetime problem; otherwise this is fine.



More information about the webkit-reviews mailing list