[webkit-reviews] review granted: [Bug 34008] Assertion failure in KURL::setProtocol when running DOM Fuzzer : [Attachment 47212] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 34008: Assertion failure in KURL::setProtocol when running DOM Fuzzer
https://bugs.webkit.org/show_bug.cgi?id=34008

Attachment 47212: proposed fix
https://bugs.webkit.org/attachment.cgi?id=47212&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
It seems strange to move the "ignore everything after the first colon" behavior
inside KURL::setProtocol. On the other hand, since it needs to be shared with
location.protocol, I guess that was the handiest place to put it.

In HTMLAnchorElement::setProtocol you could do an early return if setProtocol
returns false. This would avoid the question of why you can ignore the return
value here, and it would slightly improve speed in the error case. But I
suppose it's not all that great.

I think that at some point should rename from protocol to scheme in the URL
class itself and related functions such as protocol/schemeIsJavaScript. Even if
the DOM API calls it protocol, the URL specifications call it scheme and I
think that's a clearer term.

invalid-protocol.js seems to have a UTF-8 BOM at the top of the file. I think a
better way to do this would be to make the script-tests wrapper specify UTF-8
for the test script or use code to make the strings instead of using literal
strings. But if you think we need to stick with the BOM then I think you should
add a comment to the file letting people know the BOM is there and should not
be removed.

r=me assuming you make a cut at writing the Google-specific code too or at
least notify someone about fixing the Chromium build