Add Spdy support in WebSocketHandshake*Handler

Add Spdy support in WebSocketHandshake*Handler

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51109

[tyoshino (SeeGerritForStatus), 2010-06-09 02:55:02]

Could you add a test case to check "multiple values for the same key".

[Yuta Kitamura, 2010-06-09 03:39:30]

http://codereview.chromium.org/2743003/diff/1/2
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/2743003/diff/1/2#newcode247
net/websockets/websocket_handshake_handler.cc:247: std::string new_value =
(*headers)[name];
This code looks up a field for |name| twice. We can save an iterator obtained by
previous find() call and use it here.

Furthermore, we don't have to copy the string here:

  std::string* value = &iter->second;
  value->append(1, '\0');
  value->append(iter.values());

... because modifying a value of std::map is guaranteed to be safe (modifying a
key is dangerous on the other hand).

http://codereview.chromium.org/2743003/diff/1/2#newcode356
net/websockets/websocket_handshake_handler.cc:356: std::string value =
iter->second;
I think you can use const reference here.

http://codereview.chromium.org/2743003/diff/1/2#newcode362
net/websockets/websocket_handshake_handler.cc:362: if (end != value.npos)
std::string::npos?

[ukai, 2010-06-14 09:03:04]

http://codereview.chromium.org/2743003/diff/1/2
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/2743003/diff/1/2#newcode247
net/websockets/websocket_handshake_handler.cc:247: std::string new_value =
(*headers)[name];
On 2010/06/09 03:39:30, Yuta Kitamura wrote:
> This code looks up a field for |name| twice. We can save an iterator obtained
by
> previous find() call and use it here.
> 
> Furthermore, we don't have to copy the string here:
> 
>   std::string* value = &iter->second;
>   value->append(1, '\0');
>   value->append(iter.values());
> 
> ... because modifying a value of std::map is guaranteed to be safe (modifying
a
> key is dangerous on the other hand).

Done.

http://codereview.chromium.org/2743003/diff/1/2#newcode356
net/websockets/websocket_handshake_handler.cc:356: std::string value =
iter->second;
On 2010/06/09 03:39:30, Yuta Kitamura wrote:
> I think you can use const reference here.

Done.

http://codereview.chromium.org/2743003/diff/1/2#newcode362
net/websockets/websocket_handshake_handler.cc:362: if (end != value.npos)
On 2010/06/09 03:39:30, Yuta Kitamura wrote:
> std::string::npos?

Done.

[tyoshino (SeeGerritForStatus), 2010-06-24 08:11:14]

thank you for adding a test. Could you also cover GetRequestHeaderBlock for
multiple value for a key case.

http://codereview.chromium.org/2743003/diff/6001/7001
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/2743003/diff/6001/7001#newcode233
net/websockets/websocket_handshake_handler.cc:233: // Ignore
"Sec-WebSocket-Key1" header.
we doesn't ignore it. remove this comment?

http://codereview.chromium.org/2743003/diff/6001/7001#newcode238
net/websockets/websocket_handshake_handler.cc:238: // Ignore
"Sec-WebSocket-Key2" header.
ditto

[Paweł Hajdan Jr., 2010-06-24 08:14:50]

Drive-by with some test comments. No need to wait for another review by me.

http://codereview.chromium.org/2743003/diff/6001/7003
File net/websockets/websocket_handshake_handler_unittest.cc (right):

http://codereview.chromium.org/2743003/diff/6001/7003#newcode257
net/websockets/websocket_handshake_handler_unittest.cc:257:
CHECK(request_handler.GetRequestHeaderBlock(url, &headers, &challenge));
Uh-oh, why CHECK? Shouldn't this be an ASSERT_TRUE, to still make the test fail,
but don't crash the entire net_unittests?

http://codereview.chromium.org/2743003/diff/6001/7003#newcode324
net/websockets/websocket_handshake_handler_unittest.cc:324:
CHECK(request_handler.GetRequestHeaderBlock(url, &headers, &challenge));
Similarly here.

[ukai, 2010-06-29 05:34:26]

http://codereview.chromium.org/2743003/diff/6001/7001
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/2743003/diff/6001/7001#newcode233
net/websockets/websocket_handshake_handler.cc:233: // Ignore
"Sec-WebSocket-Key1" header.
On 2010/06/24 08:11:14, tyoshino wrote:
> we doesn't ignore it. remove this comment?

Done.

http://codereview.chromium.org/2743003/diff/6001/7001#newcode238
net/websockets/websocket_handshake_handler.cc:238: // Ignore
"Sec-WebSocket-Key2" header.
On 2010/06/24 08:11:14, tyoshino wrote:
> ditto

Done.

http://codereview.chromium.org/2743003/diff/6001/7003
File net/websockets/websocket_handshake_handler_unittest.cc (right):

http://codereview.chromium.org/2743003/diff/6001/7003#newcode257
net/websockets/websocket_handshake_handler_unittest.cc:257:
CHECK(request_handler.GetRequestHeaderBlock(url, &headers, &challenge));
On 2010/06/24 08:14:50, Paweł Hajdan Jr. wrote:
> Uh-oh, why CHECK? Shouldn't this be an ASSERT_TRUE, to still make the test
fail,
> but don't crash the entire net_unittests?

Done.

http://codereview.chromium.org/2743003/diff/6001/7003#newcode324
net/websockets/websocket_handshake_handler_unittest.cc:324:
CHECK(request_handler.GetRequestHeaderBlock(url, &headers, &challenge));
On 2010/06/24 08:14:50, Paweł Hajdan Jr. wrote:
> Similarly here.

Done.

[tyoshino (SeeGerritForStatus), 2010-06-29 07:00:47]

LGTM

[Yuta Kitamura, 2010-06-29 09:09:08]

LGTM with one nit.

http://codereview.chromium.org/2743003/diff/14001/15003
File net/websockets/websocket_handshake_handler_unittest.cc (right):

http://codereview.chromium.org/2743003/diff/14001/15003#newcode296
net/websockets/websocket_handshake_handler_unittest.cc:296:
"sec-websocket-location: ws://example.com/demo\r\n"
Why these header names are all lower-case? Is it realistic to preserve the
character case?

[ukai, 2010-06-29 09:14:18]

http://codereview.chromium.org/2743003/diff/14001/15003
File net/websockets/websocket_handshake_handler_unittest.cc (right):

http://codereview.chromium.org/2743003/diff/14001/15003#newcode296
net/websockets/websocket_handshake_handler_unittest.cc:296:
"sec-websocket-location: ws://example.com/demo\r\n"
On 2010/06/29 09:09:08, Yuta Kitamura wrote:
> Why these header names are all lower-case? Is it realistic to preserve the
> character case?

These comes from spdy and we'll use lower case field name in spdy protocol.
We don't care case (normalized in lower case to check the header field) in
WebSocket protocol, so lower-case would be fine.

[Yuta Kitamura, 2010-06-29 09:20:33]

http://codereview.chromium.org/2743003/diff/14001/15003
File net/websockets/websocket_handshake_handler_unittest.cc (right):

http://codereview.chromium.org/2743003/diff/14001/15003#newcode296
net/websockets/websocket_handshake_handler_unittest.cc:296:
"sec-websocket-location: ws://example.com/demo\r\n"
I know it's valid, but I'm a bit worried about Web Inspector show all-lower-case
header names which look ugly.

[ukai, 2010-06-29 09:29:29]

On 2010/06/29 09:20:33, Yuta Kitamura wrote:
> http://codereview.chromium.org/2743003/diff/14001/15003
> File net/websockets/websocket_handshake_handler_unittest.cc (right):
> 
> http://codereview.chromium.org/2743003/diff/14001/15003#newcode296
> net/websockets/websocket_handshake_handler_unittest.cc:296:
> "sec-websocket-location: ws://example.com/demo\r\n"
> I know it's valid, but I'm a bit worried about Web Inspector show
all-lower-case
> header names which look ugly.

Since case is not significant here, I think Web Inspector may show them with
capitalizing or so.

[Yuta Kitamura, 2010-06-29 09:51:36]

On 2010/06/29 09:29:29, ukai wrote:
> Since case is not significant here, I think Web Inspector may show them with
> capitalizing or so.

Capitalization is not trivial, "websocket" -> "WebSocket", "WWW-Authenticate" or
"Content-MD5" for example. Inspector currently shows exactly what the server
sent. If we are able to know "raw" header names, it's probably best.

Sounds like out-of-topic already, let's stop the discussion. I don't care about
the case much.