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.
Created attachment 166417 [details] Patch
Comment on attachment 166417 [details] Patch Attachment 166417 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14074744
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.
Created attachment 166419 [details] Patch
Created attachment 166420 [details] Patch
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.
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.)
Created attachment 166426 [details] Patch
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.
Comment on attachment 166426 [details] Patch Looks OK.
Comment on attachment 166426 [details] Patch Thanks!
Comment on attachment 166426 [details] Patch Clearing flags on attachment: 166426 Committed r130019: <http://trac.webkit.org/changeset/130019>
All reviewed patches have been landed. Closing bug.
> 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.
(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.
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 yutak@chromium.org, 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. ========