[webkit-reviews] review denied: [Bug 103226] [JSC] Add support for overloaded constructors : [Attachment 179738] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 103226: [JSC] Add support for overloaded constructors
https://bugs.webkit.org/show_bug.cgi?id=103226

Attachment 179738: Patch
https://bugs.webkit.org/attachment.cgi?id=179738&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179738&action=review


The overall approach looks good. But it looks like this patch is doing a lot of
things at a breath. We might want to land the patch incrementally.

> Source/WebCore/ChangeLog:8
> +	   Adding patch adds the same support for overloaded constructors to
JSC as V8.

Typo: Adding patch => This patch

> Source/WebCore/ChangeLog:9
> +	   As proof of implementation soundness WebSockets costom constrictor
is removed.

Typo: constrictor => constructor

> Source/WebCore/ChangeLog:12
> +	   The changes to bindings/scripts/CodeGeneratorJS.pm is quite
extensive since
> +	   I split up the enormous "doEverything()" function into a few
smaller; this also meant
> +	   that some generated code has been shuffled around.

Would you land these refactoring patches first? It's a bit difficult to verify
that this patch is correct.

> LayoutTests/http/tests/websocket/tests/hybi/url-parsing-expected.txt:1
> -CONSOLE MESSAGE: Invalid url for WebSocket null
> +CONSOLE MESSAGE: Wrong url scheme for WebSocket
http://127.0.0.1:8000/websocket/tests/hybi/null

Are we changing the current behavior?