Accept new WebSocket handshake format (hybi-04 and later).

Accept new WebSocket handshake format (hybi-04 and later).

To update WebSocket protocol implementation in WebKit,
WebSocketRequestHandshakeHandler and WebSocketResponseHandler need to be able
to understand both old handshake (hybi-03 and prior, including hixie-76) and
new handshake (hybi-04 and later).

BUG=64470
TEST=net_unittests --gtest_filter="WebSocketHandshake*HandlerTest.*"


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

[Yuta Kitamura, 2011-04-13 02:49:15]

Hi Ukai-san, could you review this? Also, could you suggest anyone in net/OWNERS
list who can review this?
http://src.chromium.org/viewvc/chrome/trunk/src/net/OWNERS?view=markup

Yoshino-san, Toyoshima-san: FYI.

[ukai, 2011-04-13 03:53:27]

On 2011/04/13 02:49:15, Yuta Kitamura wrote:
> Hi Ukai-san, could you review this? Also, could you suggest anyone in
net/OWNERS
> list who can review this?
> http://src.chromium.org/viewvc/chrome/trunk/src/net/OWNERS?view=markup

let's add websocket owners http://codereview.chromium.org/6837012/


> 
> Yoshino-san, Toyoshima-san: FYI.

[ukai, 2011-04-13 04:09:00]

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
File net/websockets/websocket_handshake_handler.h (right):

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.h:81: bool is_hybi04_handshake()
const;
maybe, HasSecWebSocketKey() or HasKey3Data() ?

or use value of Sec-WebSocket-Version?

[Yuta Kitamura, 2011-04-13 09:57:52]

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.cc:402:
DCHECK(base::Base64Encode(hash, &websocket_accept));
Call to Base64Encode should not be inside DCHECK. Will fix.

[Yuta Kitamura, 2011-04-13 11:23:50]

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.cc:402:
DCHECK(base::Base64Encode(hash, &websocket_accept));
On 2011/04/13 09:57:52, Yuta Kitamura wrote:
> Call to Base64Encode should not be inside DCHECK. Will fix.

Done.

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.cc:442:
DCHECK(base::Base64Encode(hash, &websocket_accept));
Fixed here, too.

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
File net/websockets/websocket_handshake_handler.h (right):

http://codereview.chromium.org/6823075/diff/2001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.h:81: bool is_hybi04_handshake()
const;
Used Sec-WebSocket-Version. Done.

[Yuta Kitamura, 2011-05-06 04:35:24]

Hi Ukai-san, could you review this?

[ukai, 2011-05-06 05:02:52]

LGTM

http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.cc:153: DCHECK(conversion_success);
here, we're checking server's response, so it is expected to fail with bad
servers and browser should not die for such response.
Maybe, it would be better to LOG_IF(ERROR, !conversion_success) or so.

[Yuta Kitamura, 2011-05-06 08:30:32]

http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
File net/websockets/websocket_handshake_handler.cc (right):

http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.cc:153: DCHECK(conversion_success);
This function is only used for a request (not for a response). I think this
DCHECK is okay.

However, I have noticed that I should fix DCHECKs elsewhere (see comments
below).

http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.cc:318: DCHECK(old_key2.empty());
These DCHECKs should probably be |VLOG_IF(1, !old_keyX.empty())|.

It's normal to receive a corrupt response from a server, thus LOG_IF(ERROR, ...)
is not appropriate, I think.

http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
net/websockets/websocket_handshake_handler.cc:321: DCHECK(new_key.empty());
Same here.

[ukai, 2011-05-06 08:41:42]

Did you upload new patch set?

On 2011/05/06 08:30:32, Yuta Kitamura wrote:
>
http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
> File net/websockets/websocket_handshake_handler.cc (right):
> 
>
http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
> net/websockets/websocket_handshake_handler.cc:153: DCHECK(conversion_success);
> This function is only used for a request (not for a response). I think this
> DCHECK is okay.
> 
> However, I have noticed that I should fix DCHECKs elsewhere (see comments
> below).
> 
>
http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
> net/websockets/websocket_handshake_handler.cc:318: DCHECK(old_key2.empty());
> These DCHECKs should probably be |VLOG_IF(1, !old_keyX.empty())|.
> 
> It's normal to receive a corrupt response from a server, thus LOG_IF(ERROR,
...)
> is not appropriate, I think.
> 
>
http://codereview.chromium.org/6823075/diff/9001/net/websockets/websocket_han...
> net/websockets/websocket_handshake_handler.cc:321: DCHECK(new_key.empty());
> Same here.

[Yuta Kitamura, 2011-05-06 08:46:52]

Uploaded a new patch set.

[ukai, 2011-05-06 08:49:36]

LGTM

[commit-bot: I haz the power, 2011-05-06 10:14:29]

Change committed as 84427