Bug 103226 – [JSC] Add support for overloaded constructors

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Tommy Widenflycht]

Bring the same functionality to JSC as V8.; see https://bugs.webkit.org/show_bug.cgi?id=100801

[Tommy Widenflycht]

Created attachment 179727 [details]
Patch

[Tommy Widenflycht]

Created attachment 179738 [details]
Patch

[Kentaro Hara]

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?

[Tommy Widenflycht]

Created attachment 179940 [details]
Patch

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

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.

[Kentaro Hara]

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.

[Tommy Widenflycht]

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.

[Kentaro Hara]

(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.

[Tommy Widenflycht]

Created attachment 180120 [details]
Patch

[WebKit Review Bot]

Comment on attachment 180120 [details]
Patch

Clearing flags on attachment: 180120

Committed r138138: <http://trac.webkit.org/changeset/138138>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.