SPDY - default to SPDY/2.1 protocol in unittests

SPDY - Change all tests to use SPDY/2.1 protocol. Made
changes to SPDY code to support enabling or disabling
flow_control independent of NPN protocol negotiated.

TEST=network unit tests
R=wtc

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

[ramant (doing other things), 2012-01-25 04:43:32]

Hi Wan-Teh and Hasan,
  This is a first cut at using spdy 2.1 as the default protocol for most of the
tests. Couple of ServerPush Tests didn't work with SPDY 2.1 (thus used no flow
control for those tests).

  Server could decided either to use spdy/2 or spdy/2.1. I think it is good to
test spdy/2 and spdy/2.1 in the tests that were modified. What you all think?

  Would appreciate if you could take a quick look.

thanks much,
raman

[ramant (doing other things), 2012-01-25 22:27:33]

Hi Wan-Teh,
  Removed the flip server changes (as they are not related to this CL).

[wtc, 2012-01-26 00:13:25]

Patch Set 6 LGTM.

http://codereview.chromium.org/9252029/diff/18014/net/tools/flip_server/const...
File net/tools/flip_server/constants.h (right):

http://codereview.chromium.org/9252029/diff/18014/net/tools/flip_server/const...
net/tools/flip_server/constants.h:21: #define NEXT_PROTO_STRING
"\x06spdy/2\x08spdy/2.1\x08http/1.1\x08http/1.0"

Should we list spdy/2.1 first?  Does the order matter?

This question also applies to net/tools/flip_server/spdy_ssl.cc.

http://codereview.chromium.org/9252029/diff/20010/net/http/http_stream_factor...
File net/http/http_stream_factory_impl.cc (right):

http://codereview.chromium.org/9252029/diff/20010/net/http/http_stream_factor...
net/http/http_stream_factory_impl.cc:159: if (alternate.protocol != NPN_SPDY_2
&& alternate.protocol != NPN_SPDY_21)

Can we test for a range of protocol version here to be future proof?

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:297: // In unittests, check if use_flow_control_ is
enabled or disabled.

Nit: change all occurrences of "unittests" to "unit tests"

The code here means flow_control_ is false for
SpdySession::kFlowControlBasedOnNPN.  Is that intended?

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:358: 

Delete this blank line.

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:365: }

This logic is hard to understand.  It would be nice to explain
it.  I think this logic says in SPDY 2.1 flow control is
enabled by default, whereas in SPDY 2 flow control is disabled
by default.  Correct?

To support SPDY 3 in the future, I think the test on
line 352 should be
  ssl_socket->protocol_negotiated() >= SSLClientSocket::kProtoSPDY21

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:51: // FlowControl provides the ability for unittests to
change |flow_control_|.

It would be nice to document the three enumeration constants.
For example, it is not clear to me what kFlowControlBasedOnNPN
means.

Nit: unittests => unit tests

Did you mean flow_control_ or use_flow_control_ here?

http://codereview.chromium.org/9252029/diff/20010/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/9252029/diff/20010/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:189: * length-prefixed). For example:
"\008http/1.1\006spdy/2\008spdy/2.1". */

Since this comment is just an example, it is not necessary to change it. I
recommend omitting this change.

[ramant (doing other things), 2012-01-26 18:58:47]

Hi Wan-Teh,
  Made changes you had suggested. Would appreciate if you could take another
look at this CL.

thanks much,
raman

http://codereview.chromium.org/9252029/diff/18014/net/tools/flip_server/const...
File net/tools/flip_server/constants.h (right):

http://codereview.chromium.org/9252029/diff/18014/net/tools/flip_server/const...
net/tools/flip_server/constants.h:21: #define NEXT_PROTO_STRING
"\x06spdy/2\x08spdy/2.1\x08http/1.1\x08http/1.0"
On 2012/01/26 00:13:25, wtc wrote:
> 
> Should we list spdy/2.1 first?  Does the order matter?
> 
> This question also applies to net/tools/flip_server/spdy_ssl.cc.

Hasan said order doesn't matter. Removed this file from this CL.

Done.

http://codereview.chromium.org/9252029/diff/20010/net/http/http_stream_factor...
File net/http/http_stream_factory_impl.cc (right):

http://codereview.chromium.org/9252029/diff/20010/net/http/http_stream_factor...
net/http/http_stream_factory_impl.cc:159: if (alternate.protocol != NPN_SPDY_2
&& alternate.protocol != NPN_SPDY_21)
On 2012/01/26 00:13:26, wtc wrote:
> 
> Can we test for a range of protocol version here to be future proof?

Done.

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:297: // In unittests, check if use_flow_control_ is
enabled or disabled.
On 2012/01/26 00:13:26, wtc wrote:
> 
> Nit: change all occurrences of "unittests" to "unit tests"
> 
> The code here means flow_control_ is false for
> SpdySession::kFlowControlBasedOnNPN.  Is that intended?

Done.

Until the server is ready to support flow control, we will initialize to false
(afterwards we will initialize to true). We determine the flow_control based on
the protocol version that is negotiated.

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:358: 
On 2012/01/26 00:13:26, wtc wrote:
> 
> Delete this blank line.

Done.

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:365: }
On 2012/01/26 00:13:26, wtc wrote:
> 
> This logic is hard to understand.  It would be nice to explain
> it.  I think this logic says in SPDY 2.1 flow control is
> enabled by default, whereas in SPDY 2 flow control is disabled
> by default.  Correct?
> 
> To support SPDY 3 in the future, I think the test on
> line 352 should be
>   ssl_socket->protocol_negotiated() >= SSLClientSocket::kProtoSPDY21

Done.

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/9252029/diff/20010/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:51: // FlowControl provides the ability for unittests to
change |flow_control_|.
On 2012/01/26 00:13:26, wtc wrote:
> 
> It would be nice to document the three enumeration constants.
> For example, it is not clear to me what kFlowControlBasedOnNPN
> means.
> 
> Nit: unittests => unit tests
> 
> Did you mean flow_control_ or use_flow_control_ here?

Done.

http://codereview.chromium.org/9252029/diff/20010/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/9252029/diff/20010/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:189: * length-prefixed). For example:
"\008http/1.1\006spdy/2\008spdy/2.1". */
On 2012/01/26 00:13:26, wtc wrote:
> 
> Since this comment is just an example, it is not necessary to change it. I
> recommend omitting this change.

Fixed the error in the comment. It should have been 008 not 010.

Done.

[wtc, 2012-01-26 22:03:25]

Patch Set 8 LGTM.  Please make the changes I suggested below
before you check this in.  Thanks.

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:354: // for all other protocols, flow control is
disabled by default.

Nit: for all other protocols => for older versions ?

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:359: flow_control_ = false;

Nit: you can just set flow_control_ to the logical expression:

  flow_control_ = (use_flow_control_ != SpdySession::kDisableFlowControl)

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:365: flow_control_ = true;

Similarly, you can just say
  flow_control_ = (use_flow_control_ == SpdySession::kEnableFlowControl);

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:56: kUninitialized = 0,

IMPORTANT: kUninitialized => kFlowControlUninitialized

otherwise it is not clear what is uninitialized when you
see the SpdySession::kUninitialized enum constant.

I think the original name kFlowControlBasedOnNPN is better.
It just needs to be documented.

http://codereview.chromium.org/9252029/diff/15012/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/9252029/diff/15012/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:189: * length-prefixed). For example:
"\008http/1.1\006spdy/2". */

BUG: please undo this change.  \0dd is in octal notation, so
\010 is 8.

[ramant (doing other things), 2012-01-26 22:39:20]

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:354: // for all other protocols, flow control is
disabled by default.
On 2012/01/26 22:03:25, wtc wrote:
> 
> Nit: for all other protocols => for older versions ?

Done.

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:359: flow_control_ = false;
On 2012/01/26 22:03:25, wtc wrote:
> 
> Nit: you can just set flow_control_ to the logical expression:
> 
>   flow_control_ = (use_flow_control_ != SpdySession::kDisableFlowControl)

Done.

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:365: flow_control_ = true;
On 2012/01/26 22:03:25, wtc wrote:
> 
> Similarly, you can just say
>   flow_control_ = (use_flow_control_ == SpdySession::kEnableFlowControl);

Done.

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/9252029/diff/15012/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:56: kUninitialized = 0,
On 2012/01/26 22:03:25, wtc wrote:
> 
> IMPORTANT: kUninitialized => kFlowControlUninitialized
> 
> otherwise it is not clear what is uninitialized when you
> see the SpdySession::kUninitialized enum constant.
> 
> I think the original name kFlowControlBasedOnNPN is better.
> It just needs to be documented.

Reverted to kFlowControlBasedOnNPN. Thanks wtc.
Done.

http://codereview.chromium.org/9252029/diff/15012/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/9252029/diff/15012/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:189: * length-prefixed). For example:
"\008http/1.1\006spdy/2". */
On 2012/01/26 22:03:25, wtc wrote:
> 
> BUG: please undo this change.  \0dd is in octal notation, so
> \010 is 8.

Done.

[wtc, 2012-01-27 01:30:44]

Patch Set 10 LGTM.

[commit-bot: I haz the power, 2012-01-27 01:36:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/9252029/20020

[commit-bot: I haz the power, 2012-01-27 03:51:21]

Change committed as 119360