[chromedriver] Write websocket client and sync websocket client.

[chromedriver] Write websocket client and sync websocket client.
BUG=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170255

[kkania, 2012-11-21 02:42:54]

If either of you don't feel comfortable reviewing, can you assign to another
websocket owner? Feel free to email/ping me with questions.

[kkania, 2012-11-21 04:47:11]

For now we're just using the full net library. In the future we may pull out and
use a select few files by themselves.

ChromeDriver is a shared library which exports a synchronous ExecuteCommand
function, which is why we need the SyncWebSocket. Otherwise we'd have to create
a new thread for each WebDriver session, which complicates things. I'll probably
add timeouts to the sync socket connect and read functions in the future, but
for now I just want to get something basic working.

[Takashi Toyoshima, 2012-11-27 07:57:36]

This is first review comments on web_socket*.
It will affect sync_web_socket*, so I'll look them later.

You might want to get another review on url_request_context by net/OWNERS.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:30: web_socket_->Close();
This Close() may not perform closing handshake correctly.
If you need to close the connection gracefully, you should expose Close method
as WebSocket::Close(). Caller should call WebSocket::Close() and wait OnClose()
delegate call before deleting WebSocket object.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:78: "Upgrade: WebSocket\r\n"
The value is case-insensitive, but I recommend to use 'websocket' here.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:80: "Sec-WebSocket-Key:
dGhlIHNhbXBsZSBub25jZQ==\r\n"
This value must not be a constant string.
It's a base64-encoded 16-bytes randomized values.

std::string sec_key;
static const size_t nonce_size = 16;
char key[nonce_size];
base::RandBytes(key, nonce_size);
base::StringPieve key_piece(key, nonce_size);
base::Base64Encode(key_piece, sec_key);

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:81: "Sec-WebSocket-Version: 13\r\n"
If ChromeDriver works over the internet, I suggest to add following two headers
like other browser implementations do.

- Pragma: no-cache
- Cache-Control: no-cache

These headers are useful to connect over some kinds of poor proxies.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:102: }
You should not expect 'WebSocket Protocol Handshake' here. Just check status
code here.
And should check Upgrade, Connection, Sec-WebSocket-Accept headers.

Maybe you can use net/http/http_response_headers to parse response.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket.h (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Other WebSocket* classes have a filename websocket_* respectively. So, this file
should be renamed as websocket.h.
Ditto on other files.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:21: }
}  // namespace net

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:27: class WebSocket : public
net::SocketStream::Delegate {
Memo:
SocketStream will be obsolete and replaced with WebSocketStream in the future.
But, currently using SocketStream here looks reasonable.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:36: void Connect(const
base::Callback<void(bool)>& callback);
Using net::CompletionCallback is usual style for net/ though argument is not
bool, but int representing net::Error.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:39: bool Write(const std::string&
message);
Send() looks consistent.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:59: base::Callback<void(bool)>
connect_callback_;
net::CompletionCallback

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket_unittest.cc:81: 
I think DevTools uses net::HttpServer and ChromeDriver will connect to this,
right?
So, this test seems good for testing real cases, but I think you should use
net::TestServer for testing client implementation.

net::TestSerevr supports WebSocket and performs strict protocol check.

What do you think about this?

[kkania, 2012-11-27 19:58:22]

In patch 2 I ONLY renamed all the web_socket files to websocket. In patch 3 I
made the actual code changes. I broke it into 2 patches so that you can look at
the diff between 2 and 3 without losing history due to the rename.

+rdsmith for review of url_request_context_getter.cc/h

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:30: web_socket_->Close();
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> This Close() may not perform closing handshake correctly.
> If you need to close the connection gracefully, you should expose Close method
> as WebSocket::Close(). Caller should call WebSocket::Close() and wait
OnClose()
> delegate call before deleting WebSocket object.

Ok. Right now we don't have any need to close the connection gracefully, since
this will only be called when terminating ChromeDriver/Chrome. I may add this in
the future. Let me know if there's a reason I should add it now.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:78: "Upgrade: WebSocket\r\n"
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> The value is case-insensitive, but I recommend to use 'websocket' here.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:80: "Sec-WebSocket-Key:
dGhlIHNhbXBsZSBub25jZQ==\r\n"
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> This value must not be a constant string.
> It's a base64-encoded 16-bytes randomized values.
> 
> std::string sec_key;
> static const size_t nonce_size = 16;
> char key[nonce_size];
> base::RandBytes(key, nonce_size);
> base::StringPieve key_piece(key, nonce_size);
> base::Base64Encode(key_piece, sec_key);

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:81: "Sec-WebSocket-Version: 13\r\n"
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> If ChromeDriver works over the internet, I suggest to add following two
headers
> like other browser implementations do.
> 
> - Pragma: no-cache
> - Cache-Control: no-cache
> 
> These headers are useful to connect over some kinds of poor proxies.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:102: }
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> You should not expect 'WebSocket Protocol Handshake' here. Just check status
> code here.
> And should check Upgrade, Connection, Sec-WebSocket-Accept headers.
> 
> Maybe you can use net/http/http_response_headers to parse response.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket.h (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> Other WebSocket* classes have a filename websocket_* respectively. So, this
file
> should be renamed as websocket.h.
> Ditto on other files.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:21: }
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> }  // namespace net

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:27: class WebSocket : public
net::SocketStream::Delegate {
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> Memo:
> SocketStream will be obsolete and replaced with WebSocketStream in the future.
> But, currently using SocketStream here looks reasonable.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:36: void Connect(const
base::Callback<void(bool)>& callback);
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> Using net::CompletionCallback is usual style for net/ though argument is not
> bool, but int representing net::Error.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:39: bool Write(const std::string&
message);
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> Send() looks consistent.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.h:59: base::Callback<void(bool)>
connect_callback_;
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> net::CompletionCallback

Done.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket_unittest.cc:81: 
On 2012/11/27 07:57:37, Takashi Toyoshima (chromium) wrote:
> I think DevTools uses net::HttpServer and ChromeDriver will connect to this,
> right?
> So, this test seems good for testing real cases, but I think you should use
> net::TestServer for testing client implementation.
> 
> net::TestSerevr supports WebSocket and performs strict protocol check.
> 
> What do you think about this?

Yes, DevTools uses net::HttpServer, which ChromeDriver will connect to.

I'm not sure I see the benefit of using net::TestServer, except I suppose all
the other net test code uses it. It'll probably be slower (since it starts
python), and it's not the real server I care about. Plus, is it as easy to
simulate different conditions such as returning 404 or closing the socket in the
middle of a request or message?

This WebSocket class is meant to work with net::HttpServer and is not intended
to work in all circumstances like Chrome websockets are. If for some reason this
class wants to be generalized to handle all WebSocket servers, we could migrate
to net::TestServer.

[Takashi Toyoshima, 2012-11-28 09:11:29]

LGTM on WebSocket with some suggestions.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket.cc:30: web_socket_->Close();
Without closing handshake, we have no confidence that all sent packages are
received by a server. If your application level protocol has a kind of ACK
mechanism, or it's not problem that a server misses to receive some data, it's
not mandatory. You can omit this now.

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
File chrome/test/chromedriver/net/web_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/2002/chrome/test/chromed...
chrome/test/chromedriver/net/web_socket_unittest.cc:81: 
I'm just afraid that DevTools and ChromeDriver use not WebSocket, but
WebSocket-like protocol. Once the tool is distributed, migration to the right
protocol will be difficult.

From the viewpoint of testing, net::TestServer has features to simulate various
situation.

But, as you said, it may be overkill for this. I agree that this test use
net::HttpServer for now. We may want to use net::TestServer when we integrate
this implementation to net/websocket/websocket_stream or something.

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
File chrome/test/chromedriver/net/sync_websocket.h (right):

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/sync_websocket.h:45: bool
ReadNextMessage(std::string* message);
ReceiveNextMessage looks better here.
Same on other Read* functions.

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
File chrome/test/chromedriver/net/websocket.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/websocket.cc:80:
CHECK(base::Base64Encode(base::RandBytesAsString(16), &sec_key_));
Ah, this is smarter than my suggestion :)

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/websocket.cc:104: if (!connect_callback_.is_null())
{
Can you add some comment here, as WebSocketJob guarantees the first
OnReceivedData call contains all response headers at once.

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/websocket.cc:124: net::WebSocketFrameParser parser;
I suggest you make net::WebSocketFrameParser a member of this class. The parser
can hold incomplete incoming data instead of incoming_msg_buf. You can just pass
data and len to parser.Decode().

[kkania, 2012-11-28 16:02:12]

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
File chrome/test/chromedriver/net/sync_websocket.h (right):

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/sync_websocket.h:45: bool
ReadNextMessage(std::string* message);
On 2012/11/28 09:11:30, Takashi Toyoshima (chromium) wrote:
> ReceiveNextMessage looks better here.
> Same on other Read* functions.

Done.

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
File chrome/test/chromedriver/net/websocket.cc (right):

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/websocket.cc:80:
CHECK(base::Base64Encode(base::RandBytesAsString(16), &sec_key_));
On 2012/11/28 09:11:30, Takashi Toyoshima (chromium) wrote:
> Ah, this is smarter than my suggestion :)

Done.

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/websocket.cc:104: if (!connect_callback_.is_null())
{
On 2012/11/28 09:11:30, Takashi Toyoshima (chromium) wrote:
> Can you add some comment here, as WebSocketJob guarantees the first
> OnReceivedData call contains all response headers at once.

Good idea. Done.

https://chromiumcodereview.appspot.com/11316115/diff/13023/chrome/test/chrome...
chrome/test/chromedriver/net/websocket.cc:124: net::WebSocketFrameParser parser;
On 2012/11/28 09:11:30, Takashi Toyoshima (chromium) wrote:
> I suggest you make net::WebSocketFrameParser a member of this class. The
parser
> can hold incomplete incoming data instead of incoming_msg_buf. You can just
pass
> data and len to parser.Decode().

Ah, I wasn't aware of that. Thanks! Done.

[Randy Smith (Not in Mondays), 2012-11-28 16:40:47]

On 2012/11/27 19:58:22, kkania wrote:
> +rdsmith for review of url_request_context_getter.cc/h

[+eroman, who I think is a better reviewer for this.  ]

My apologies, but I don't think I'm a good reviewer for this.  Despite being
listed as a reviewer, I haven't done that much work in the network stack, and
adding a new class counts as an architectural change that should at least get a
glance from someone who knows the stack fairly well.  Eric, you willing to take
a look?

[eroman, 2012-11-28 22:56:44]

the urlrequestcontextgetter looks fine.
i suggest adding assertion checks that geturlrequest is only called from the
network_task_runner thread.

https://chromiumcodereview.appspot.com/11316115/diff/9021/chrome/test/chromed...
File chrome/test/chromedriver/net/url_request_context_getter.h (right):

https://chromiumcodereview.appspot.com/11316115/diff/9021/chrome/test/chromed...
chrome/test/chromedriver/net/url_request_context_getter.h:24:
URLRequestContextGetter(
mark as explicit

[commit-bot: I haz the power, 2012-11-29 16:31:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/11316115/8014

[commit-bot: I haz the power, 2012-11-29 22:21:34]

Change committed as 170255