Bug 65367 – WebSocket: Receive URL and subprotocol in WebSocketChannel::connect()

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

[Yuta Kitamura]

Receive URL and subprotocol in WebSocketChannel::connect(), rather than WebSocketChannel constructor.

We need to check the arguments of WebSocket constructor (in WebSocket::connect()). This should be done between WebSocketChannel construction and WebSocketChannel::conncect() call, because we need to know which WebSocket protocol is used via WebSocketChannel to check the arguments, and then create a connection after checking the arguments.

This is part of bug 65247.

[Yuta Kitamura]

Created attachment 102348 [details]
Patch

[Kent Tamura]

Comment on attachment 102348 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        WebSocket::connect() needs to check the value of subprotocols provided by the script depending
> +        on the value of WebSocketChannel::useHixie76Protocol(), thus WebSocketChannel has to receive

Isn't The subprotocol check implemented in connect() yet?

> Source/WebCore/ChangeLog:36
> +        * websockets/ThreadableWebSocketChannel.cpp:
> +        (WebCore::ThreadableWebSocketChannel::create):
> +        * websockets/ThreadableWebSocketChannel.h:
> +        * websockets/WebSocket.cpp:
> +        (WebCore::WebSocket::connect):
> +        * websockets/WebSocketChannel.cpp:
> +        (WebCore::WebSocketChannel::WebSocketChannel):
> +        (WebCore::WebSocketChannel::connect):
> +        * websockets/WebSocketChannel.h:
> +        (WebCore::WebSocketChannel::create):
> +        * websockets/WorkerThreadableWebSocketChannel.cpp:
> +        (WebCore::WorkerThreadableWebSocketChannel::WorkerThreadableWebSocketChannel):
> +        (WebCore::WorkerThreadableWebSocketChannel::connect):
> +        (WebCore::WorkerThreadableWebSocketChannel::Peer::Peer):
> +        (WebCore::WorkerThreadableWebSocketChannel::Peer::connect):
> +        (WebCore::WorkerThreadableWebSocketChannel::Bridge::mainThreadCreateWebSocketChannel):
> +        (WebCore::WorkerThreadableWebSocketChannel::Bridge::Bridge):
> +        (WebCore::WorkerThreadableWebSocketChannel::mainThreadConnect):
> +        (WebCore::WorkerThreadableWebSocketChannel::Bridge::connect):
> +        * websockets/WorkerThreadableWebSocketChannel.h:
> +        (WebCore::WorkerThreadableWebSocketChannel::create):
> +        (WebCore::WorkerThreadableWebSocketChannel::Peer::create):
> +        (WebCore::WorkerThreadableWebSocketChannel::Bridge::create):

You should write at least:
 * Removed the URL argument and the protocol argument
 for factory functions and constructors
 * Add the URL argument and the protocol argument
 for connect() functions.

[Yuta Kitamura]

Created attachment 102494 [details]
Patch v2

[Yuta Kitamura]

Comment on attachment 102348 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        on the value of WebSocketChannel::useHixie76Protocol(), thus WebSocketChannel has to receive
> 
> Isn't The subprotocol check implemented in connect() yet?

Not yet. It will be implemented later. Rephrased the description completely to make sure the point becomes clearer.

>> Source/WebCore/ChangeLog:36
>> +        (WebCore::WorkerThreadableWebSocketChannel::Bridge::create):
> 
> You should write at least:
>  * Removed the URL argument and the protocol argument
>  for factory functions and constructors
>  * Add the URL argument and the protocol argument
>  for connect() functions.

Done.

[Kent Tamura]

(In reply to comment #4)
> (From update of attachment 102348 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102348&action=review
> 
> >> Source/WebCore/ChangeLog:9
> >> +        on the value of WebSocketChannel::useHixie76Protocol(), thus WebSocketChannel has to receive
> > 
> > Isn't The subprotocol check implemented in connect() yet?
> 
> Not yet. It will be implemented later. Rephrased the description completely to make sure the point becomes clearer.

ok, so we should have a FIXME comment in connect().

[Yuta Kitamura]

Created attachment 102498 [details]
Patch v3

[Kent Tamura]

Comment on attachment 102498 [details]
Patch v3

ok

[Yuta Kitamura]

Comment on attachment 102348 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:9
>>>> +        on the value of WebSocketChannel::useHixie76Protocol(), thus WebSocketChannel has to receive
>>> 
>>> Isn't The subprotocol check implemented in connect() yet?
>> 
>> Not yet. It will be implemented later. Rephrased the description completely to make sure the point becomes clearer.
> 
> ok, so we should have a FIXME comment in connect().

Added FIXME in WebSocket::connect().

[WebKit Review Bot]

Comment on attachment 102498 [details]
Patch v3

Clearing flags on attachment: 102498

Committed r92116: <http://trac.webkit.org/changeset/92116>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.