Bug 65367 - WebSocket: Receive URL and subprotocol in WebSocketChannel::connect()
Summary: WebSocket: Receive URL and subprotocol in WebSocketChannel::connect()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
Depends on:
Blocks: 65247
  Show dependency treegraph
 
Reported: 2011-07-29 04:37 PDT by Yuta Kitamura
Modified: 2011-08-01 04:16 PDT (History)
4 users (show)

See Also:


Attachments
Patch (21.31 KB, patch)
2011-07-29 05:00 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (22.31 KB, patch)
2011-08-01 01:50 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v3 (22.45 KB, patch)
2011-08-01 02:21 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-07-29 04:37:53 PDT
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.
Comment 1 Yuta Kitamura 2011-07-29 05:00:41 PDT
Created attachment 102348 [details]
Patch
Comment 2 Kent Tamura 2011-07-31 19:37:48 PDT
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.
Comment 3 Yuta Kitamura 2011-08-01 01:50:14 PDT
Created attachment 102494 [details]
Patch v2
Comment 4 Yuta Kitamura 2011-08-01 01:52:45 PDT
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.
Comment 5 Kent Tamura 2011-08-01 02:00:29 PDT
(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().
Comment 6 Yuta Kitamura 2011-08-01 02:21:27 PDT
Created attachment 102498 [details]
Patch v3
Comment 7 Kent Tamura 2011-08-01 02:22:23 PDT
Comment on attachment 102498 [details]
Patch v3

ok
Comment 8 Yuta Kitamura 2011-08-01 02:22:41 PDT
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().
Comment 9 WebKit Review Bot 2011-08-01 04:16:31 PDT
Comment on attachment 102498 [details]
Patch v3

Clearing flags on attachment: 102498

Committed r92116: <http://trac.webkit.org/changeset/92116>
Comment 10 WebKit Review Bot 2011-08-01 04:16:36 PDT
All reviewed patches have been landed.  Closing bug.