[webkit-reviews] review denied: [Bug 100801] Remove all custom code for WebSockets : [Attachment 171741] 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 100801: Remove all custom code for WebSockets
https://bugs.webkit.org/show_bug.cgi?id=100801

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

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


Looks great to remove custom bindings!

> Source/WebCore/ChangeLog:15
> +	   2) Modifies WebSocket.h/.cpp for the new constructors.
> +
> +	   3) Modifies WebSocket.h/.cpp/.idl to not have custom code for the
send method.

The patch is a bit too big to review. Can you split a patch for constructors
from a patch for send()?

Below, I'm commenting on the constructors part.

> Source/WebCore/Modules/websockets/WebSocket.cpp:178
> +    if (url.isNull()) {
> +	   ec = SYNTAX_ERR;
> +	   return 0;
> +    }

This has been throwing throwError(SyntaxError, "Empty URL") before this patch.
SYNTAX_ERR is a DOM exception, which is different from a SyntaxError, which is
a JavaScript error.

If there is no test for this behavior, please add a test.

> Source/WebCore/Modules/websockets/WebSocket.cpp:194
> +    if (!protocol.isNull())
> +	   protocols.append(protocol);

This check has not been done before this patch.

If there is no test for this behavior, please add a test.

> Source/WebCore/Modules/websockets/WebSocket.idl:43
> +    JSConstructorParameters=1,

Let's use [ConstructorParameters=1] as is, without introducing
[JSConstructorParameters]. Having a [ConstructorParameters=X] IDL attribute is
not a problem even for non-custom constructors (unless V8 and JSC want to
implement constructors that have a different number of parameters).

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3724
> +    # FIXME: Add support for overloaded constructors to jS as well.

Nit: jS => JS

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3733
>      if (!defined $numberOfConstructorParameters) {
> +	   $numberOfConstructorParameters =
$dataNode->extendedAttributes->{"JSConstructorParameters"};
> +    }

As mentioned above, I don't want to introduce [JSConstructorParameters].

> Source/WebCore/bindings/scripts/IDLAttributes.txt:64
> +JSConstructorParameters=*

As mentioned above, we want to remove it.

> Source/WebCore/bindings/scripts/IDLParser.pm:2409
> +    if (defined $extendedAttributeList->{"Constructors"}) {
> +	   my @constructorParams = @{$extendedAttributeList->{"Constructors"}};

> +	   my $index = (@constructorParams == 1) ? 0 : 1;
> +	   foreach my $param (@constructorParams) {
> +	       my $constructor = new domFunction();
> +	       $constructor->signature(new domSignature());
> +	       $constructor->signature->name("Constructor");
> +	      
$constructor->signature->extendedAttributes($extendedAttributeList);
> +	       $constructor->parameters($param);
> +	       $constructor->{overloadedIndex} = $index++;
> +	       push(@{$dataNode->constructors}, $constructor);
> +	   }
> +	   delete $extendedAttributeList->{"Constructors"};

Currently we are resolving overloaded functions in CodeGenerator.pm, in order
not to introduce additional logic to IDLParser.pm (IDLParser.pm should do only
parsing). Similarly, can you resolve overloaded constructors in
CodeGenerator.pm? You can implement LinkOverloadedConstructors, just like
LinkOverloadedFunctions.

>
Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl:-30

> -    Constructor(in DOMString hello, in SerializedScriptValue value),

Why did you remove this?