Bug 100801 – Remove the V8 custom code for WebSockets constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tommy Widenflycht]

This requires the following:

Modifying the code generators to support overloaded constructors

Modify WebSocket.h/.cpp for the new constructors

Modify WebSocket.h/.cpp/.idl for nor having custom code for the send method.
There is a FIXME in the idl regarding this and the simple solution is to have the send(DOMString) declaration last since it acts as a catch-all.

[Tommy Widenflycht]

Created attachment 171561 [details]
Patch

[Tommy Widenflycht]

Reviewers, this is a patch in progress but I would be happy if you could take a look  so that I am not doing something wrong. This is by far the most perl code I have ever written ever.

Things that I do know is missing:
CodeGeneratorJS.pm needs the same changes as CodeGeneratorV8.pm
Bindings tests

[Early Warning System Bot]

Comment on attachment 171561 [details]
Patch

Attachment 171561 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14663057

[Early Warning System Bot]

Comment on attachment 171561 [details]
Patch

Attachment 171561 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14664068

[Build Bot]

Comment on attachment 171561 [details]
Patch

Attachment 171561 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14673001

[Build Bot]

Comment on attachment 171561 [details]
Patch

Attachment 171561 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14660068

[EFL EWS Bot]

Comment on attachment 171561 [details]
Patch

Attachment 171561 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14693007

[WebKit Review Bot]

Comment on attachment 171561 [details]
Patch

Attachment 171561 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14661080

New failing tests:
storage/indexeddb/index-basics.html
storage/indexeddb/index-basics-workers.html

[WebKit Review Bot]

Comment on attachment 171561 [details]
Patch

Attachment 171561 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14685020

New failing tests:
storage/indexeddb/index-basics.html
storage/indexeddb/index-basics-workers.html

[Adam Barth]

Comment on attachment 171561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171561&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1924
> +    my $name = "onstructor";

onstructor?  :)

It looks like $name is only used twice.  Perhaps we should just create a $overloadIndexString variable that is either the empty string or the stringification of the overload index.

[Adam Barth]

Looks like you've got some compile issues (and a couple test failures) to work through.  It would be good for haraken to review this patch once you've gotten green EWS bubbles.

[Tommy Widenflycht]

Created attachment 171712 [details]
Patch

[Tommy Widenflycht]

(In reply to comment #10)
> (From update of attachment 171561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171561&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1924
> > +    my $name = "onstructor";
> 
> onstructor?  :)
> 
> It looks like $name is only used twice.  Perhaps we should just create a $overloadIndexString variable that is either the empty string or the stringification of the overload index.

Yeah! :) Fixed.

[Tommy Widenflycht]

(In reply to comment #2)
> Reviewers, this is a patch in progress but I would be happy if you could take a look  so that I am not doing something wrong. This is by far the most perl code I have ever written ever.
> 
> Things that I do know is missing:
> CodeGeneratorJS.pm needs the same changes as CodeGeneratorV8.pm
> Bindings tests

CodeGeneratorJS.pm will get the changes in an followup patch, for now I have made sure it works with the IDLParser changes.

[Early Warning System Bot]

Comment on attachment 171712 [details]
Patch

Attachment 171712 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14670242

[Early Warning System Bot]

Comment on attachment 171712 [details]
Patch

Attachment 171712 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14685337

[EFL EWS Bot]

Comment on attachment 171712 [details]
Patch

Attachment 171712 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14678258

[Tommy Widenflycht]

Created attachment 171717 [details]
Patch

[Build Bot]

Comment on attachment 171717 [details]
Patch

Attachment 171717 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14693268

New failing tests:
fast/js/constructor-length.html

[Tommy Widenflycht]

Created attachment 171741 [details]
Patch

[Tommy Widenflycht]

I have now added a bindings test and the generated output from than makes up the size difference between the previous patch and this.

[WebKit Review Bot]

Attachment 171741 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverloadedConstructorsInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.h:58:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tommy Widenflycht]

FYI all the style failures are in the generated bindings test code.

(In reply to comment #22)
> Attachment 171741 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverloadedConstructorsInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.h:58:  More than one command on the same line in if  [whitespace/parens] [4]
> Total errors found: 3 in 22 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

@haraken: Any interest in reviewing this patch?

[Kentaro Hara]

(In reply to comment #24)
> @haraken: Any interest in reviewing this patch?

Sure. Will take a look in hours.

[Kentaro Hara]

Comment on attachment 171741 [details]
Patch

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?

[Tommy Widenflycht]

Comment on attachment 171741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171741&action=review

>> Source/WebCore/ChangeLog:15
>> +        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.

All send related changes have been removed.

>> Source/WebCore/Modules/websockets/WebSocket.cpp:178
>> +    }
> 
> 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.

Test added, and since all other url parsing checks throws a SYNTAX_ERR this is more consistent.

>> Source/WebCore/Modules/websockets/WebSocket.cpp:194
>> +        protocols.append(protocol);
> 
> This check has not been done before this patch.
> 
> If there is no test for this behavior, please add a test.

Removed check.

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

done.

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

fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3733
>> +    }
> 
> As mentioned above, I don't want to introduce [JSConstructorParameters].

done.

>> Source/WebCore/bindings/scripts/IDLAttributes.txt:64
>> +JSConstructorParameters=*
> 
> As mentioned above, we want to remove it.

done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:2409
>> +        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.

Can I do this in a follow-up patch, after JS support?

It would make this patch more complicated since attributes (which Constructors are) does not follow the same rules as functions and other members. And btw I don't do anything more than looping around the same code as before.

>> Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl:-30
>> -    Constructor(in DOMString hello, in SerializedScriptValue value),
> 
> Why did you remove this?

Because I think it is better if each file tests only one thing, in this case SerializedScriptValue, and because the the first constructor was ignored before and I wanted to let this file test exactly the same thing as before.

[Tommy Widenflycht]

Created attachment 171906 [details]
Patch

[WebKit Review Bot]

Attachment 171906 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverloadedConstructorsInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.h:58:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Looks much better

(In reply to comment #27)
> >> Source/WebCore/bindings/scripts/IDLParser.pm:2409
> >> +        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.
> 
> Can I do this in a follow-up patch, after JS support?
> 
> It would make this patch more complicated since attributes (which Constructors are) does not follow the same rules as functions and other members. And btw I don't do anything more than looping around the same code as before.

Wouldn't it be possible to keep the change in CodeGeneratorV8.pm as is and just move what you're doing in IDLParser.pm to CodeGenerator.pm ? I just do not like the fact that $dataNode->constructors is built up not in CodeGenerator.pm but in IDLParser.pm, which is messing up the parser. Looking at LinkOverloadedFunctions in CodeGenerator.pm, I guess you can do a similar thing for constructors. Am I missing something?

[Tommy Widenflycht]

(In reply to comment #30)
> Looks much better
> 
> (In reply to comment #27)
> > >> Source/WebCore/bindings/scripts/IDLParser.pm:2409
> > >> +        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.
> > 
> > Can I do this in a follow-up patch, after JS support?
> > 
> > It would make this patch more complicated since attributes (which Constructors are) does not follow the same rules as functions and other members. And btw I don't do anything more than looping around the same code as before.
> 
> Wouldn't it be possible to keep the change in CodeGeneratorV8.pm as is and just move what you're doing in IDLParser.pm to CodeGenerator.pm ? I just do not like the fact that $dataNode->constructors is built up not in CodeGenerator.pm but in IDLParser.pm, which is messing up the parser. Looking at LinkOverloadedFunctions in CodeGenerator.pm, I guess you can do a similar thing for constructors. Am I missing something?

The issue is that before the LinkOverloadedFunctions is called (in the boundary between IDLParser and CodeGenerator) the attributes are checked, and anything else than Constructor=string will cause an error. That's why in IDLParser the array of array of parameters have to be moved to the dataNode in one way or another before the transfer to CodeGenerator.

[Build Bot]

Comment on attachment 171906 [details]
Patch

Attachment 171906 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14692594

New failing tests:
http/tests/websocket/tests/hybi/url-parsing.html

[Kentaro Hara]

(In reply to comment #31)
> The issue is that before the LinkOverloadedFunctions is called (in the boundary between IDLParser and CodeGenerator) the attributes are checked, and anything else than Constructor=string will cause an error. That's why in IDLParser the array of array of parameters have to be moved to the dataNode in one way or another before the transfer to CodeGenerator.

It looks like that the problem is that anything other than Constructor="..." causes an error, which is not an expected behavior in our situation. Is it difficult to fix? (I'm happy to discuss the details with you offline.)

[Tommy Widenflycht]

(In reply to comment #33)
> (In reply to comment #31)
> > The issue is that before the LinkOverloadedFunctions is called (in the boundary between IDLParser and CodeGenerator) the attributes are checked, and anything else than Constructor=string will cause an error. That's why in IDLParser the array of array of parameters have to be moved to the dataNode in one way or another before the transfer to CodeGenerator.
> 
> It looks like that the problem is that anything other than Constructor="..." causes an error, which is not an expected behavior in our situation. Is it difficult to fix? (I'm happy to discuss the details with you offline.)

I don't think cleaning this up would difficult to fix but since it's not directly related to overloaded constructors I definitely prefer to do it in another patch, this one is large enough as it is.

[Kentaro Hara]

(In reply to comment #34)
> I don't think cleaning this up would difficult to fix but since it's not directly related to overloaded constructors I definitely prefer to do it in another patch, this one is large enough as it is.

So do you mean, in this patch you want to make the change to IDLParser.pm, and in a follow-up patch you want to move the change from IDLParser.pm to CodeGenerator.pm? (I don't much like the approach but would not object to it if you want to go that way.)

Either way, the goal should be (1) no change to the parser, and (2) both V8 and JSC support overloaded constructors.

[Kentaro Hara]

We discussed offline. I understand we need to make a change to IDLParser.pm but we're not sure what is the best way to do it.

tasak: We are trying to support overloaded constructors. IDLParser.pm should build up $dataNode->constructors (instead of $dataNode->constructor). Do you have any idea to implement it? (c.f. Look at the IDLParser.pm of https://bugs.webkit.org/attachment.cgi?id=171906&action=review It works as intended, but the solution is a bit dirty in that it introduces a temporal "Constructors" extended attribute to the parser.)

[Takashi Sakamoto]

> tasak: We are trying to support overloaded constructors. IDLParser.pm should build up $dataNode->constructors (instead of $dataNode->constructor). Do you have any idea to implement it? (c.f. Look at the IDLParser.pm of https://bugs.webkit.org/attachment.cgi?id=171906&action=review It works as intended, but the solution is a bit dirty in that it introduces a temporal "Constructors" extended attribute to the parser.)

I looked at https://bugs.webkit.org/attachment.cgi?id=171906&action=review.
If we don't need to support multiple values per one key except Constructor (I'm not sure... we don't need to support overloaded NamedConstructor?), I think, it's ok.
If we need, I think, it might be better to use an array per each key (key=extended attribute key).

Best regards,
Takashi Sakamoto

[Tommy Widenflycht]

(In reply to comment #37)
> > tasak: We are trying to support overloaded constructors. IDLParser.pm should build up $dataNode->constructors (instead of $dataNode->constructor). Do you have any idea to implement it? (c.f. Look at the IDLParser.pm of https://bugs.webkit.org/attachment.cgi?id=171906&action=review It works as intended, but the solution is a bit dirty in that it introduces a temporal "Constructors" extended attribute to the parser.)
> 
> I looked at https://bugs.webkit.org/attachment.cgi?id=171906&action=review.
> If we don't need to support multiple values per one key except Constructor (I'm not sure... we don't need to support overloaded NamedConstructor?), I think, it's ok.
> If we need, I think, it might be better to use an array per each key (key=extended attribute key).
> 
> Best regards,
> Takashi Sakamoto

Thanks for your answer. I would be surprised if NamedConstructor needs to be overloaded, but if that ever happens I would be happy to do the work.

[Tommy Widenflycht]

Created attachment 173039 [details]
Patch

[WebKit Review Bot]

Attachment 173039 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/ChangeLog:139:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverloadedConstructorsInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.h:58:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 4 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

I'm leaving and let me take a look at the patch later.

(In reply to comment #38)
> I would be surprised if NamedConstructor needs to be overloaded, but if that ever happens I would be happy to do the work.

Currently NamedConstructor is speced only in Audio, Image and Option. They are not overloaded. So practical speaking, overloaded NamedConstructor is not needed.

[Tommy Widenflycht]

(In reply to comment #41) 
> (In reply to comment #38)
> > I would be surprised if NamedConstructor needs to be overloaded, but if that ever happens I would be happy to do the work.
> 
> Currently NamedConstructor is speced only in Audio, Image and Option. They are not overloaded. So practical speaking, overloaded NamedConstructor is not needed.

Why do these classes need NamedConstructor? Just curious.

[Kentaro Hara]

(In reply to comment #42)
> (In reply to comment #41) 
> > (In reply to comment #38)
> > > I would be surprised if NamedConstructor needs to be overloaded, but if that ever happens I would be happy to do the work.
> > 
> > Currently NamedConstructor is speced only in Audio, Image and Option. They are not overloaded. So practical speaking, overloaded NamedConstructor is not needed.
> 
> Why do these classes need NamedConstructor? Just curious.

Boring answer: Because they are speced.

Practical answer: Although the interface name is HTMLImageElement, we want to make it constructable by "new Image()" not by "new HTMLImageElement()". NamedConstructor does the magic.

[Tommy Widenflycht]

Created attachment 173043 [details]
Patch

[WebKit Review Bot]

Attachment 173043 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverloadedConstructorsInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.h:58:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tommy Widenflycht]

Created attachment 173234 [details]
Patch (merge changes only)

[WebKit Review Bot]

Attachment 173234 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverloadedConstructorsInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.h:58:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Comment on attachment 173234 [details]
Patch (merge changes only)

View in context: https://bugs.webkit.org/attachment.cgi?id=173234&action=review

Looks OK with some nits. Thanks for a great patch!

> Source/WebCore/Modules/websockets/WebSocket.cpp:176
> +        ec = SYNTAX_ERR;

This change is non-trivial but looks OK for the following reasons:

(1) This changes the current behavior. The current implementation throws SyntaxError (one of JavaScript errors) instead of SYNTAX_ERR (one of DOM exceptions).

(2) But the spec says it should be SYNTAX_ERR. (http://www.w3.org/TR/2009/WD-websockets-20091222/) So the new implementation is conformed to the spec.

(3) As no developers would care about the difference, this change won't break compatibility.

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

Nit: Redundant ','

> Source/WebCore/bindings/scripts/IDLParser.pm:1326
> +sub copyAttributes

copyAttributes => copyExtendedAttributes ?

[Tommy Widenflycht]

Thanks for the review!

I will fix both comments and commit this patch on Monday morning.

(In reply to comment #48)
> (From update of attachment 173234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173234&action=review
> 
> Looks OK with some nits. Thanks for a great patch!
> 
> > Source/WebCore/Modules/websockets/WebSocket.cpp:176
> > +        ec = SYNTAX_ERR;
> 
> This change is non-trivial but looks OK for the following reasons:
> 
> (1) This changes the current behavior. The current implementation throws SyntaxError (one of JavaScript errors) instead of SYNTAX_ERR (one of DOM exceptions).
> 
> (2) But the spec says it should be SYNTAX_ERR. (http://www.w3.org/TR/2009/WD-websockets-20091222/) So the new implementation is conformed to the spec.
> 
> (3) As no developers would care about the difference, this change won't break compatibility.
> 
> > Source/WebCore/Modules/websockets/WebSocket.idl:43
> > +    ConstructorParameters=1,
> 
> Nit: Redundant ','
> 
> > Source/WebCore/bindings/scripts/IDLParser.pm:1326
> > +sub copyAttributes
> 
> copyAttributes => copyExtendedAttributes ?

[Tommy Widenflycht]

Created attachment 173606 [details]
Patch for landing

[WebKit Review Bot]

Attachment 173606 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h:61:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverloadedConstructorsInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.h:58:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 173606 [details]
Patch for landing

Rejecting attachment 173606 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/TestExpectations
Failed to merge in the changes.
Patch failed at 0001 Add tests for month picker

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 154.

Full output: http://queues.webkit.org/results/14811395

[Tommy Widenflycht]

Created attachment 173628 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173628 [details]
Patch for landing

Clearing flags on attachment: 173628

Committed r134221: <http://trac.webkit.org/changeset/134221>

[Kentaro Hara]

FYI, another IDL that has hoped overloaded constructors is Intent.idl. Though I'm not sure if it's easy to kill the custom binding.

[Tommy Widenflycht]

Will look at that one once I have fixed the JSC support.

[Kentaro Hara]

Sounds great!