Bug 103226

Created attachment 179727 [details] Patch

Created attachment 179738 [details] Patch

Comment on attachment 179738 [details] Patch 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?

Created attachment 179940 [details] Patch

Comment on attachment 179738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179738&action=review >> Source/WebCore/ChangeLog:8 >> + Adding patch adds the same support for overloaded constructors to JSC as V8. > > Typo: Adding patch => This patch Fixed. >> Source/WebCore/ChangeLog:9 >> + As proof of implementation soundness WebSockets costom constrictor is removed. > > Typo: constrictor => constructor Fixed. >> Source/WebCore/ChangeLog:12 >> + 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. Fixed. >> LayoutTests/http/tests/websocket/tests/hybi/url-parsing-expected.txt:1 >> +CONSOLE MESSAGE: Wrong url scheme for WebSocket http://127.0.0.1:8000/websocket/tests/hybi/null > > Are we changing the current behavior? No, just unifying the output between JSC and V8.

There's a bit more cleanup that could be done but I'll leave that to a follow-up patch to keep this one reasonable in size.

Comment on attachment 179940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179940&action=review This is a great improvement. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3774 > + next if exists $fetchedArguments{$parameterIndex}; Why can the same parameterIndex appear twice in @neededArguments ? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3980 > + $numberOfConstructorParameters = 1024; Nit: Let's use 255 as we're using 255 in other places.

Comment on attachment 179940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179940&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3774 >> + next if exists $fetchedArguments{$parameterIndex}; > > Why can the same parameterIndex appear twice in @neededArguments ? Honestly I have no idea... This is copied from the function overload generation and I kept it as is. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3980 >> + $numberOfConstructorParameters = 1024; > > Nit: Let's use 255 as we're using 255 in other places. Will fix.

(In reply to comment #8) > > Why can the same parameterIndex appear twice in @neededArguments ? > > Honestly I have no idea... This is copied from the function overload generation and I kept it as is. OK, then let' fix it in a follow-up patch if needed.

Created attachment 180120 [details] Patch

Comment on attachment 180120 [details] Patch Clearing flags on attachment: 180120 Committed r138138: <http://trac.webkit.org/changeset/138138>

All reviewed patches have been landed. Closing bug.