Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
954933003:
Sync the WebSocket interface with the spec (Closed)
|
Created:
5 years, 10 months ago by jsbell Modified:
5 years, 9 months ago Reviewers:
philipj_slow CC:
Paritosh Kumar, blink-reviews, yhirano+watch_chromium.org, tyoshino+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSync the WebSocket interface with the spec
https://html.spec.whatwg.org/multipage/comms.html#websocket
Changes:
* Replace overloaded contructors with type union (per spec)
* DOMString -> USVString in two places (replaces explicit conversions)
This does not update the binaryType attribute; see
https://codereview.chromium.org/871013007/
R=philipj@opera.com
BUG=460722
TEST=http/tests/websocket
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191127
Patch Set 1 #
Total comments: 21
Patch Set 2 : Review feedback #Patch Set 3 : Add TypeChecking=Interface #
Messages
Total messages: 13 (4 generated)
jsbell@chromium.org changed reviewers: + paritosh.in@samsung.com - philipj@opera.com
jsbell@chromium.org changed reviewers: + philipj@opera.com - paritosh.in@samsung.com
philipj@ - please take a look? https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/DO... File Source/modules/websockets/DOMWebSocket.cpp (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/DO... Source/modules/websockets/DOMWebSocket.cpp:475: // Bindings specify USVString, so unpaired surrogates are already replaced with U+FFFD. Is this comment helpful here, or just let the bindings (and existing tests) document this? https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... File Source/modules/websockets/WebSocket.idl (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:35: // FIXME: use BinaryType in binaryType See https://codereview.chromium.org/871013007/ by Paritosh Kumar for a fix for this https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:40: Constructor(DOMString url, optional (DOMString or sequence<DOMString>) protocols), Spec has (DOMString or DOMString[]) but it's unclear why T[] would be used instead of sequence<T>.
https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... File Source/modules/websockets/WebSocket.idl (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:40: Constructor(DOMString url, optional (DOMString or sequence<DOMString>) protocols), On 2015/02/25 18:51:11, jsbell wrote: > Spec has (DOMString or DOMString[]) but it's unclear why T[] would be used > instead of sequence<T>. I filed this against the HTML spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28102
LGTM with some more FIXMEs and possibly a test needed. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/DO... File Source/modules/websockets/DOMWebSocket.cpp (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/DO... Source/modules/websockets/DOMWebSocket.cpp:475: // Bindings specify USVString, so unpaired surrogates are already replaced with U+FFFD. On 2015/02/25 18:51:11, jsbell wrote: > Is this comment helpful here, or just let the bindings (and existing tests) > document this? Someone very familiar with bindings and the distinction between DOMString and USVString may find this redundant, but it looks good to me. Is there a way to assert it instead? Also, have you looked at the generated code to see if it now does StrictUTF8ConversionReplacingUnpairedSurrogatesWithFFFD? If it's not exactly equivalent, some test exposing the change in behavior might be nice. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... File Source/modules/websockets/WebSocket.idl (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:32: // http://dev.w3.org/html5/websockets/#websocket I'm pretty sure this copy of the spec is abandoned, see the date. You can also tell from the indentation of the IDL block that it isn't being kept in sync with the source document. In other words, drop this link? https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:33: // https://html.spec.whatwg.org/multipage/comms.html#websocket Can you link to https://html.spec.whatwg.org/multipage/comms.html#the-websocket-interface instead? Not all interfaces have such a section handy, but when there is one, it means you get to see the constructor and any enums without scrolling. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:40: Constructor(DOMString url, optional (DOMString or sequence<DOMString>) protocols), On 2015/02/25 23:18:55, jsbell wrote: > On 2015/02/25 18:51:11, jsbell wrote: > > Spec has (DOMString or DOMString[]) but it's unclear why T[] would be used > > instead of sequence<T>. > > I filed this against the HTML spec: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28102 Can you add a FIXME here linking to that spec bug, just in case it should be rejected? It can just be removed if Hixie does what you ask. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:47: [DeprecateAs=WebSocketURL] readonly attribute DOMString URL; // Lowercased .url is the one in the spec, but leaving .URL for compatibility reasons. So that it's easy to find all the differences, can you convert this to a FIXME? I would also LGTM a Intent to Remove it, since it's been deprecated for a long time and has acceptably low usage: https://www.chromestatus.com/metrics/feature/timeline/popularity/255 https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:62: [TreatReturnedNullStringAs=Undefined] readonly attribute DOMString extensions; Add FIXMEs for these two [TreatReturnedNullStringAs=Undefined], per spec it looks like these should always be strings. If there are cases where a null string is returned internally, we should probably just return an empty string. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:70: [RaisesException] void send(Blob data); Note that without [TypeChecking=Interface], some non-Blob object or null will result in null being seen on the C++ side. That should never happen per the spec's IDL. There's some risk to that change, but if you want to add it in a separate (easily reverted) CL that would be great!
https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... File Source/modules/websockets/WebSocket.idl (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:70: [RaisesException] void send(Blob data); On 2015/02/26 03:02:53, philipj_UTC7 wrote: > Note that without [TypeChecking=Interface], some non-Blob object or null will > result in null being seen on the C++ side. That should never happen per the > spec's IDL. There's some risk to that change, but if you want to add it in a > separate (easily reverted) CL that would be great! Hmm, I'm actually not sure what happens when there are same-name methods with argument type overloading, there's no single C++ function to call with a null argument. Maybe I'm just wrong, but it would be worth checking if [TypeChecking=Interface] changes the generated code.
Thanks for the awesome feedback. Quick notes for tonight, I'll investigate/address the FIXME additions tomorrow. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/DO... File Source/modules/websockets/DOMWebSocket.cpp (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/DO... Source/modules/websockets/DOMWebSocket.cpp:475: // Bindings specify USVString, so unpaired surrogates are already replaced with U+FFFD. On 2015/02/26 03:02:53, philipj_UTC7 wrote: > On 2015/02/25 18:51:11, jsbell wrote: > > Is this comment helpful here, or just let the bindings (and existing tests) > > document this? > > Someone very familiar with bindings and the distinction between DOMString and > USVString may find this redundant, but it looks good to me. That'd be me (I implemented our USVString handling), fortunately. Or unfortunately, for getting a second opinion. > Is there a way to assert it instead? > > Also, have you looked at the generated code to see if it now does > StrictUTF8ConversionReplacingUnpairedSurrogatesWithFFFD? If it's not exactly > equivalent, some test exposing the change in behavior might be nice. I verified that it's covered in: http/tests/websocket/unpaired-surrogates-in-close-reason.html .. and the other one in: http/tests/websocket/unpaired-surrogates-in-message.html https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... File Source/modules/websockets/WebSocket.idl (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:70: [RaisesException] void send(Blob data); On 2015/02/26 03:05:59, philipj_UTC7 wrote: > On 2015/02/26 03:02:53, philipj_UTC7 wrote: > > Note that without [TypeChecking=Interface], some non-Blob object or null will > > result in null being seen on the C++ side. That should never happen per the > > spec's IDL. There's some risk to that change, but if you want to add it in a > > separate (easily reverted) CL that would be great! > > Hmm, I'm actually not sure what happens when there are same-name methods with > argument type overloading, there's no single C++ function to call with a null > argument. Maybe I'm just wrong, but it would be worth checking if > [TypeChecking=Interface] changes the generated code. I'll take a look at the generated code and report back.
Take another peek? https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... File Source/modules/websockets/WebSocket.idl (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:32: // http://dev.w3.org/html5/websockets/#websocket On 2015/02/26 03:02:53, philipj_UTC7 wrote: > I'm pretty sure this copy of the spec is abandoned, see the date. You can also > tell from the indentation of the IDL block that it isn't being kept in sync with > the source document. In other words, drop this link? Done. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:33: // https://html.spec.whatwg.org/multipage/comms.html#websocket On 2015/02/26 03:02:53, philipj_UTC7 wrote: > Can you link to > https://html.spec.whatwg.org/multipage/comms.html#the-websocket-interface > instead? Not all interfaces have such a section handy, but when there is one, it > means you get to see the constructor and any enums without scrolling. Done. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:40: Constructor(DOMString url, optional (DOMString or sequence<DOMString>) protocols), On 2015/02/26 03:02:53, philipj_UTC7 wrote: > On 2015/02/25 23:18:55, jsbell wrote: > > On 2015/02/25 18:51:11, jsbell wrote: > > > Spec has (DOMString or DOMString[]) but it's unclear why T[] would be used > > > instead of sequence<T>. > > > > I filed this against the HTML spec: > > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28102 > > Can you add a FIXME here linking to that spec bug, just in case it should be > rejected? It can just be removed if Hixie does what you ask. Done. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:47: [DeprecateAs=WebSocketURL] readonly attribute DOMString URL; // Lowercased .url is the one in the spec, but leaving .URL for compatibility reasons. On 2015/02/26 03:02:53, philipj_UTC7 wrote: > So that it's easy to find all the differences, can you convert this to a FIXME? > I would also LGTM a Intent to Remove it, since it's been deprecated for a long > time and has acceptably low usage: > https://www.chromestatus.com/metrics/feature/timeline/popularity/255 Done. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:62: [TreatReturnedNullStringAs=Undefined] readonly attribute DOMString extensions; On 2015/02/26 03:02:53, philipj_UTC7 wrote: > Add FIXMEs for these two [TreatReturnedNullStringAs=Undefined], per spec it > looks like these should always be strings. If there are cases where a null > string is returned internally, we should probably just return an empty string. Done. I traced the code back through from Chromium, and they should never be null. But left the FIXMEs in there for now. https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:70: [RaisesException] void send(Blob data); On 2015/02/26 03:05:59, philipj_UTC7 wrote: > On 2015/02/26 03:02:53, philipj_UTC7 wrote: > > Note that without [TypeChecking=Interface], some non-Blob object or null will > > result in null being seen on the C++ side. That should never happen per the > > spec's IDL. There's some risk to that change, but if you want to add it in a > > separate (easily reverted) CL that would be great! > > Hmm, I'm actually not sure what happens when there are same-name methods with > argument type overloading, there's no single C++ function to call with a null > argument. Maybe I'm just wrong, but it would be worth checking if > [TypeChecking=Interface] changes the generated code. The generated code does change, but there's no behavior difference. With the overload, the type checks are done before dispatching using V8$TYPE$::hasInstance(), falling back to the string overload. Then in the dispatched calls, the conversion is done with data = V8$TYPE$::toImplWithTypeCheck(). If [TypeChecking=Interface] is specified, this is followed by `if (!data) { ... throwTypeError ... }`. But that should never be true. I can add in [TypeChecking=Interface] since we ideally would have that everywhere, fix the cases where it causes problems, then make it the default. Or leave it for now to avoid clutter - I have no strong preference.
LGTM https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... File Source/modules/websockets/WebSocket.idl (right): https://codereview.chromium.org/954933003/diff/1/Source/modules/websockets/We... Source/modules/websockets/WebSocket.idl:70: [RaisesException] void send(Blob data); On 2015/02/26 19:56:54, jsbell wrote: > On 2015/02/26 03:05:59, philipj_UTC7 wrote: > > On 2015/02/26 03:02:53, philipj_UTC7 wrote: > > > Note that without [TypeChecking=Interface], some non-Blob object or null > will > > > result in null being seen on the C++ side. That should never happen per the > > > spec's IDL. There's some risk to that change, but if you want to add it in a > > > separate (easily reverted) CL that would be great! > > > > Hmm, I'm actually not sure what happens when there are same-name methods with > > argument type overloading, there's no single C++ function to call with a null > > argument. Maybe I'm just wrong, but it would be worth checking if > > [TypeChecking=Interface] changes the generated code. > > The generated code does change, but there's no behavior difference. > > With the overload, the type checks are done before dispatching using > V8$TYPE$::hasInstance(), falling back to the string overload. Then in the > dispatched calls, the conversion is done with data = > V8$TYPE$::toImplWithTypeCheck(). If [TypeChecking=Interface] is specified, this > is followed by `if (!data) { ... throwTypeError ... }`. But that should never be > true. > > I can add in [TypeChecking=Interface] since we ideally would have that > everywhere, fix the cases where it causes problems, then make it the default. Or > leave it for now to avoid clutter - I have no strong preference. Oh, overloading also changes things a bit. [TypeChecking=Interface] on the interface would be great, in this CL or later as you see fit.
The CQ bit was checked by jsbell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/954933003/#ps40001 (title: "Add TypeChecking=Interface")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/954933003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191127 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
