Bug 97999 – [WebSocket] Setting wrong value to binaryType should not raise exception

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

See http://code.google.com/p/chromium/issues/detail?id=153240 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=18749 for details.

[Kenichi Ishibashi]

Created attachment 166417 [details]
Patch

[Build Bot]

Comment on attachment 166417 [details]
Patch

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

[Yuta Kitamura]

Comment on attachment 166417 [details]
Patch

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

r- for unused function argument.

> Source/WebCore/Modules/websockets/WebSocket.cpp:417
> +    scriptExecutionContext()->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "'" + binaryType + "' is the wrong binaryType");

- Error message sounds a bit strange and unhelpful; I would suggest:
    "'" + binaryType + "'" is not a valid value for binaryType; binaryType remains unchanged."

- You can now remove the argument |ec|. You need to touch WebSocket.idl to remove this.

[Kenichi Ishibashi]

Created attachment 166419 [details]
Patch

[Kenichi Ishibashi]

Created attachment 166420 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 166417 [details]
Patch

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

Thank you for review!

>> Source/WebCore/Modules/websockets/WebSocket.cpp:417
>> +    scriptExecutionContext()->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "'" + binaryType + "' is the wrong binaryType");
> 
> - Error message sounds a bit strange and unhelpful; I would suggest:
>     "'" + binaryType + "'" is not a valid value for binaryType; binaryType remains unchanged."
> 
> - You can now remove the argument |ec|. You need to touch WebSocket.idl to remove this.

Done.

[Yuta Kitamura]

Comment on attachment 166420 [details]
Patch

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

r- for unneeded |ExceptionCode&| argument.

> Source/WebCore/Modules/websockets/WebSocket.cpp:407
> +void WebSocket::setBinaryType(const String& binaryType, ExceptionCode&)

Well, what I meant was to update the following line in WebSocket.idl:

    attribute [TreatReturnedNullStringAs=Undefined] DOMString binaryType
        setter raises(DOMException);

If you remove "setter raises(DOMException)", you will not need the second argument
of setBinaryType().

(You can also remove [TreatReturnedNullStringAs=Undefined]. It was used to handle
 the old hixie76 protocol, which is no longer supported.)

[Kenichi Ishibashi]

Created attachment 166426 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 166420 [details]
Patch

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

>> Source/WebCore/Modules/websockets/WebSocket.cpp:407
>> +void WebSocket::setBinaryType(const String& binaryType, ExceptionCode&)
> 
> Well, what I meant was to update the following line in WebSocket.idl:
> 
>     attribute [TreatReturnedNullStringAs=Undefined] DOMString binaryType
>         setter raises(DOMException);
> 
> If you remove "setter raises(DOMException)", you will not need the second argument
> of setBinaryType().
> 
> (You can also remove [TreatReturnedNullStringAs=Undefined]. It was used to handle
>  the old hixie76 protocol, which is no longer supported.)

Thanks. Removed.

[Yuta Kitamura]

Comment on attachment 166426 [details]
Patch

Looks OK.

[Kenichi Ishibashi]

Comment on attachment 166426 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 166426 [details]
Patch

Clearing flags on attachment: 166426

Committed r130019: <http://trac.webkit.org/changeset/130019>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> See http://code.google.com/p/chromium/issues/detail?id=153240 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=18749 for details.

Please always post relevant information into Bugzilla. It's not polite to make everyone reading a bug click through to 3rd party sites to research rationale. It's good to provide links for additional background, but rationale should be explained locally.

[Kenichi Ishibashi]

(In reply to comment #14)
> Please always post relevant information into Bugzilla. It's not polite to make everyone reading a bug click through to 3rd party sites to research rationale. It's good to provide links for additional background, but rationale should be explained locally.

I see. I'll post enough information into Bugzilla next time.

[Yuta Kitamura]

You just should have pasted my comment in the Chromium bug.

For the record, I'm pasting it here.

http://code.google.com/p/chromium/issues/detail?id=153240

========
Reported by project member [e-mail], Yesterday (19 hours ago)
Background: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18749

Now WebSocket API defines an enum type BinaryType, and binaryType attribute has that type.
http://dev.w3.org/html5/websockets/

As defined in WebIDL spec, setting a wrong value to enum attribute should not throw
an exception:
http://dev.w3.org/2006/webapi/WebIDL/#es-attributes
> - If the type of the attribute is an enumeration, then:
>     1. Let S be the result of calling ToString(V).
>     2. If S is not of the enumeration’s values, then return undefined.
>     3. The value of idlValue is the enumeration value equal to S.

Current implementation raises a SYNTAX_ERR (based on the older WS spec), so we'll need to
fix this.

To help developers to find their mistakes, we would also add a console message when
a wrong value is being set to binaryType.

I don't have a compatibility concern because it doesn't really make sense to handle
the SYNTAX_ERR thrown for this.
========