WebSocket over SPDY: handshake support for both of SPDY/2 and SPDY/3

WebSocket over SPDY: handshake support for both of SPDY/2 and SPDY/3

If negotiated protocol is SPDY/3 or later, well defined name of SYN_STREAM and
SYN_REPLY must be colon prefixed name.

BUG=42320
TEST=net_uinttests


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

[Takashi Toyoshima, 2012-08-02 12:45:27]

Yuta,

I notice that a header name rule is different between spdy/2 and later.
I should add colon character prefix to well defined header names.

headers which should have colon prefix are defined in my spec.
https://docs.google.com/document/d/1zUEFzz7NCls3Yms8hXxY4wGXJ3EEvoZc3GihrqPQc...


Ryan,

After yutak's review, please review this CL as a net/ OWNERS. yutak is a
net/websockets OWNERS, but he can not review net/net.gyp.

Thanks

[Ryan Hamilton, 2012-08-02 16:35:14]

On 2012/08/02 12:45:27, toyoshim wrote:
> Yuta,
> 
> I notice that a header name rule is different between spdy/2 and later.
> I should add colon character prefix to well defined header names.
> 
> headers which should have colon prefix are defined in my spec.
>
https://docs.google.com/document/d/1zUEFzz7NCls3Yms8hXxY4wGXJ3EEvoZc3GihrqPQc...
> 
> 
> Ryan,
> 
> After yutak's review, please review this CL as a net/ OWNERS. yutak is a
> net/websockets OWNERS, but he can not review net/net.gyp.
> 
> Thanks

I will be happy to review after yutak's review.  Please ping me when it is
complete, so I don't miss it.

[Yuta Kitamura, 2012-08-03 07:31:51]

Almost all my comments are nits, except the first one.

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

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:275: if (protocol_negotiated ==
kProtoSPDY2) {
Can we assume |protocol_negotiated| is either kProtoSPDY2 or kProtoSPDY3? Or it
may take other values like kProtoUnknown, kProtoHTTP11 or kProtoSPDY1?

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:316: // TODO(toyoshim): Some
WebSocket extensions may be not not compatible
"may be not not" -> "may not be"

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:487:
LowerCaseEqualsASCII(iter->first, "status")) ||
nit: This line needs to align the opening paren, i.e.:

  if ((foo &&
       bar) ||
      baz) {
  }

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:489:
LowerCaseEqualsASCII(iter->first, ":status"))) {
Ditto.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:506:
LowerCaseEqualsASCII(iter->first, ":sec-websocket-extensions")))
Ditto.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
File net/websockets/websocket_handshake_handler_spdy2_unittest.cc (right):

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy2_unittest.cc:15: 
nit: This blank line is not needed.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy2_unittest.cc:65: const char
expected_challenge[] = "dGhlIHNhbXBsZSBub25jZQ==";
static

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
File net/websockets/websocket_handshake_handler_spdy3_unittest.cc (right):

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy3_unittest.cc:15: 
Same here.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy3_unittest.cc:68: const char
expected_challenge[] = "dGhlIHNhbXBsZSBub25jZQ==";
Same here.

[Takashi Toyoshima, 2012-08-06 13:01:53]

Thank you, Yuta.

Ryan,
Could you review this CL?
I'd like to confirm the way to know spdy version which is used in the current
session. These modules are used iff spdy session is established correctly. I
think I can use SpdySession::GetProtocolVersion().

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

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:275: if (protocol_negotiated ==
kProtoSPDY2) {
Good point.
It doesn't seem to be kProtoHTTP11 or kProtoSPDY1 because these negotiation
never create right spdy session for WebSocket. But, kProtoUnknown can be.

SpdySession want to use g_default_protocol for kProtoUnknown to initiate a new
session. I guess this default value is kProtoSPDY2, or kProtoSPDY3 if it is
enabled. So using this value looks right way. I'll fix this change to hold this
initiating protocol version, and query it from WebSocketJob to SpdySession.

ps. SpdySession::GetProtocolVersion looks right function to know the actually
used protocol version.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:316: // TODO(toyoshim): Some
WebSocket extensions may be not not compatible
Thanks.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:487:
LowerCaseEqualsASCII(iter->first, "status")) ||
On 2012/08/03 07:31:51, Yuta Kitamura wrote:
> nit: This line needs to align the opening paren, i.e.:
> 
>   if ((foo &&
>        bar) ||
>       baz) {
>   }

Done.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:489:
LowerCaseEqualsASCII(iter->first, ":status"))) {
On 2012/08/03 07:31:51, Yuta Kitamura wrote:
> Ditto.

Done.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler.cc:506:
LowerCaseEqualsASCII(iter->first, ":sec-websocket-extensions")))
On 2012/08/03 07:31:51, Yuta Kitamura wrote:
> Ditto.

Done.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
File net/websockets/websocket_handshake_handler_spdy2_unittest.cc (right):

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy2_unittest.cc:15: 
On 2012/08/03 07:31:51, Yuta Kitamura wrote:
> nit: This blank line is not needed.

Done.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy2_unittest.cc:65: const char
expected_challenge[] = "dGhlIHNhbXBsZSBub25jZQ==";
On 2012/08/03 07:31:51, Yuta Kitamura wrote:
> static

Done.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
File net/websockets/websocket_handshake_handler_spdy3_unittest.cc (right):

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy3_unittest.cc:15: 
On 2012/08/03 07:31:51, Yuta Kitamura wrote:
> Same here.

Done.

http://codereview.chromium.org/10843050/diff/2001/net/websockets/websocket_ha...
net/websockets/websocket_handshake_handler_spdy3_unittest.cc:68: const char
expected_challenge[] = "dGhlIHNhbXBsZSBub25jZQ==";
On 2012/08/03 07:31:51, Yuta Kitamura wrote:
> Same here.

Done.

[Yuta Kitamura, 2012-08-07 07:02:22]

LGTM with a nit.

http://codereview.chromium.org/10843050/diff/7011/net/websockets/websocket_job.h
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/10843050/diff/7011/net/websockets/websocket_jo...
net/websockets/websocket_job.h:141: int protocol_version_;
"spdy_protocol_version_" should sound better.

[Takashi Toyoshima, 2012-08-13 11:34:55]

rch: ping.

Hi Ryan.
I fixed the point Yuta suggested.
Could you do an OWNERS review on this change?

[Ryan Hamilton, 2012-08-13 17:03:20]

On 2012/08/13 11:34:55, toyoshim wrote:
> rch: ping.
> 
> Hi Ryan.
> I fixed the point Yuta suggested.
> Could you do an OWNERS review on this change?

net.gyp LGTM

[commit-bot: I haz the power, 2012-08-14 01:53:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10843050/12002

[commit-bot: I haz the power, 2012-08-14 05:33:57]

Try job failure for 10843050-12002 (retry) on linux_chromeos for steps
"browser_tests, content_browsertests".
It's a second try, previously, steps "browser_tests, content_browsertests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-14 07:09:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10843050/12002

[commit-bot: I haz the power, 2012-08-14 10:32:23]

Change committed as 151453