Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
234323003:
Make WebSocket.fromSocket public. (Closed)
|
Created:
6 years, 8 months ago by nweiz Modified:
6 years, 7 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMake WebSocket.fromUpgradedSocket public.
This allows dart:io's WebSocket protocol code to be used in contexts
where dart:io HttpRequest and HttpResponse objects aren't available.
BUG=18172
R=ajohnsen@google.com
Committed: https://code.google.com/p/dart/source/detail?r=35540
Patch Set 1 #
Total comments: 8
Patch Set 2 : code review #Patch Set 3 : fix test #
Total comments: 5
Patch Set 4 : code review #
Total comments: 7
Messages
Total messages: 14 (0 generated)
I'm not sure this is something we want to expose. A request should only be upgraded in a very specific way, as exposed by WebSocketTransformer.upgrade. It can be very confusing for people, as this may indicate that a newly connected Socket can be upgraded to a WebSocket. I suggest you instead file an issue against Area-IO, so we can have a discussion for addressing concerns, before exposing this.
On 2014/04/11 07:24:02, Anders Johnsen wrote: > I'm not sure this is something we want to expose. > > A request should only be upgraded in a very specific way, as exposed by > WebSocketTransformer.upgrade. It can be very confusing for people, as this may > indicate that a newly connected Socket can be upgraded to a WebSocket. > > I suggest you instead file an issue against Area-IO, so we can have a discussion > for addressing concerns, before exposing this. Done: https://code.google.com/p/dart/issues/detail?id=18172
After long discussions, we've come to the same conclusions as you have, that this indeed is the place for such a mechanism. With the comments addressed, this should be in a good shape, where we warn those users who just want to create a WebSocket and avoid the accidental usage. Adding sgjesse@ as reviewer as well. https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:137: * Creates a WebSocket from an existing socket. [...] an already upgraded socket. https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:139: * The initial WebSocket handshake must have occurred prior to this call. This comment should be expanded to include references to: WebSocket.connect and WebSocketTransformer.upgrade & HttpResponse.detachSocket To be sure we point people to where a upgraded socket is easily created from. https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:140: * [protocol] should be the protocol negotiated by this handshake, if any. protocol and serverSide should be in separate paragraphs. Mention what happens if serverSide is either true or false (e.g. frame masking is performed in the direction of client->server). https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:143: factory WebSocket.fromSocket(Socket socket, {String protocol, Please change the name to something along the lines of: WebSocket.fromUpgradedSocket WebSocket.fromDetachedSocket or something else. fromSocket is too generic, and the user may be tempted to do: Socket.connect(...).then((socket) => new WebSocket.fromSocket(socket)) which will not work.
https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:137: * Creates a WebSocket from an existing socket. On 2014/04/29 10:43:44, Anders Johnsen wrote: > [...] an already upgraded socket. Done. https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:139: * The initial WebSocket handshake must have occurred prior to this call. On 2014/04/29 10:43:44, Anders Johnsen wrote: > This comment should be expanded to include references to: > > WebSocket.connect > > and > > WebSocketTransformer.upgrade & HttpResponse.detachSocket > > To be sure we point people to where a upgraded socket is easily created from. Done. https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:140: * [protocol] should be the protocol negotiated by this handshake, if any. On 2014/04/29 10:43:44, Anders Johnsen wrote: > protocol and serverSide should be in separate paragraphs. Mention what happens > if serverSide is either true or false (e.g. frame masking is performed in the > direction of client->server). Done. https://codereview.chromium.org/234323003/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:143: factory WebSocket.fromSocket(Socket socket, {String protocol, On 2014/04/29 10:43:44, Anders Johnsen wrote: > Please change the name to something along the lines of: > > WebSocket.fromUpgradedSocket > WebSocket.fromDetachedSocket > > or something else. > > fromSocket is too generic, and the user may be tempted to do: > > Socket.connect(...).then((socket) => new WebSocket.fromSocket(socket)) > > which will not work. Done.
LGTM, with comments. (consider changing CL topic to match rename) https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:148: * the messages it sends. If it's `false`, it will act as the client and will It's the other way around :) https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:152: bool serverSide: true}) => Maybe serverSide should be a mandatory argument? The user should always consider what to use.
Message was sent while issue was closed.
Committed patchset #4 manually as r35540 (presubmit successful).
Message was sent while issue was closed.
DBC https://codereview.chromium.org/234323003/diff/60001/tests/standalone/io/web_... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/234323003/diff/60001/tests/standalone/io/web_... tests/standalone/io/web_socket_test.dart:404: return request.response.detachSocket() The 'return' here has no effect right? https://codereview.chromium.org/234323003/diff/60001/tests/standalone/io/web_... tests/standalone/io/web_socket_test.dart:415: var completer = new Completer(); [client] and [completer] seem to be unused.
Message was sent while issue was closed.
https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:148: * the messages it sends. If it's `false`, it will act as the client and will On 2014/04/29 20:05:10, Anders Johnsen wrote: > It's the other way around :) Done. https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:152: bool serverSide: true}) => On 2014/04/29 20:05:10, Anders Johnsen wrote: > Maybe serverSide should be a mandatory argument? The user should always consider > what to use. It's good to encourage the user to consider, but I think that's outweighed by the benefit when reading the code of seeing the name of the parameter. Unnamed boolean parameters are difficult to understand when reading code. https://codereview.chromium.org/234323003/diff/60001/tests/standalone/io/web_... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/234323003/diff/60001/tests/standalone/io/web_... tests/standalone/io/web_socket_test.dart:404: return request.response.detachSocket() On 2014/04/29 20:36:50, kustermann wrote: > The 'return' here has no effect right? Removed. https://codereview.chromium.org/234323003/diff/60001/tests/standalone/io/web_... tests/standalone/io/web_socket_test.dart:415: var completer = new Completer(); On 2014/04/29 20:36:50, kustermann wrote: > [client] and [completer] seem to be unused. Removed.
Message was sent while issue was closed.
https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/40001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:152: bool serverSide: true}) => On 2014/04/30 22:33:27, nweiz wrote: > On 2014/04/29 20:05:10, Anders Johnsen wrote: > > Maybe serverSide should be a mandatory argument? The user should always > consider > > what to use. > > It's good to encourage the user to consider, but I think that's outweighed by > the benefit when reading the code of seeing the name of the parameter. Unnamed > boolean parameters are difficult to understand when reading code. That is a good argument - I sometimes forget the caller benefits of names optional arguments.
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
https://codereview.chromium.org/234323003/diff/60001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:136: /** How bad would it be to add a (deprecated) default constructor for this so that people can subclass this for the time being?
Message was sent while issue was closed.
https://codereview.chromium.org/234323003/diff/60001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:136: /** On 2014/05/05 09:31:51, kasperl wrote: > How bad would it be to add a (deprecated) default constructor for this so that > people can subclass this for the time being? Would it be okay if the WebSocket constructor throw a UnimplementedError?
Message was sent while issue was closed.
https://codereview.chromium.org/234323003/diff/60001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/234323003/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:136: /** On 2014/05/05 09:33:49, Anders Johnsen wrote: > On 2014/05/05 09:31:51, kasperl wrote: > > How bad would it be to add a (deprecated) default constructor for this so that > > people can subclass this for the time being? > > Would it be okay if the WebSocket constructor throw a UnimplementedError? Wouldn't that also break users using "extend WebSocket"? |
