SPDY - disable spdy/2.1 (flow control) by default

SPDY - by default disable spdy/2.1 (flow control). It
can be enabled with command line flag --flow-control

R=willchan
TEST=network unittests
BUG=116167
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124794

[ramant (doing other things), 2012-02-29 20:09:07]

Hi willchan,
  Tested this code with and without --flow-control flag and verfified (from
net-internals) that chrome select spdy/2.1 when --flow-control flag is set.

  Implemented what Wan-Teh has suggested in the following CL about how to
support various versions of alternate protocol.

  http://codereview.chromium.org/9390027/

  Would appreciate your comments.

thanks much,
raman

[willchan no longer on Chromium, 2012-02-29 20:15:27]

http://codereview.chromium.org/9516009/diff/9002/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9516009/diff/9002/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:283:
HttpStreamFactory::set_highest_supported_alternate_protocol(
Using a global like this messes up the test dependencies. What if tests executed
in a different order? Nothing is undoing this global mutation. You should
probably scope this somehow.

Also note that in general I prefer not using globals like this. I'm fine in this
case, because theoretically we're going to kill off support for old versions in
the not too distant future. If we had to support this long-term, I'd ask you to
do the work to pass the parameters into the constructor.

[ramant (doing other things), 2012-03-01 03:51:59]

Hi willchan,
  Many many thanks for your comments. All network unittests call
ResetStaticSettingsToInit before each test. Added setting to default value (ala
other statics).

   HttpStreamFactory::ResetStaticSettingsToInit

  Changed the name of the method/variable to supported_spdy_version and set it
to the corresponding values as the SPDY version that is being tested.

  Would appreciate your comments.

thanks,
raman

http://codereview.chromium.org/9516009/diff/9002/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9516009/diff/9002/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:283:
HttpStreamFactory::set_highest_supported_alternate_protocol(
On 2012/02/29 20:15:27, willchan wrote:
> Using a global like this messes up the test dependencies. What if tests
executed
> in a different order? Nothing is undoing this global mutation. You should
> probably scope this somehow.
> 
> Also note that in general I prefer not using globals like this. I'm fine in
this
> case, because theoretically we're going to kill off support for old versions
in
> the not too distant future. If we had to support this long-term, I'd ask you
to
> do the work to pass the parameters into the constructor.

Done.

[Ryan Hamilton, 2012-03-01 18:56:20]

http://codereview.chromium.org/9516009/diff/17004/net/http/http_stream_factor...
File net/http/http_stream_factory.cc (right):

http://codereview.chromium.org/9516009/diff/17004/net/http/http_stream_factor...
net/http/http_stream_factory.cc:92: if (protocol != supported_spdy_version_ ||
As we discussed offline, this approach only allows us to speak a single version
at a time.  It would prevent us from speaking 2 or 2.1 depending on what the
server advertises.

We discussed having HttpStreamFactory maintain an array of booleans indexed by
NPN_foo.  If array[NPN_foo] is true, then that version is enabled, otherwise it
is not.  This would be simple, efficient, and flexible.

[willchan no longer on Chromium, 2012-03-01 18:57:54]

http://codereview.chromium.org/9516009/diff/17004/net/http/http_stream_factor...
File net/http/http_stream_factory.cc (right):

http://codereview.chromium.org/9516009/diff/17004/net/http/http_stream_factor...
net/http/http_stream_factory.cc:92: if (protocol != supported_spdy_version_ ||
On 2012/03/01 18:56:20, Ryan Hamilton wrote:
> As we discussed offline, this approach only allows us to speak a single
version
> at a time.  It would prevent us from speaking 2 or 2.1 depending on what the
> server advertises.
> 
> We discussed having HttpStreamFactory maintain an array of booleans indexed by
> NPN_foo.  If array[NPN_foo] is true, then that version is enabled, otherwise
it
> is not.  This would be simple, efficient, and flexible.

Whew, good catch! I didn't notice this. This is important, since most other SPDY
websites won't advertise 2.1.

[ramant (doing other things), 2012-03-02 00:46:34]

Thanks very much Ryan for suggesting adding support for multiple SPDY protocols
to HttpStreamFactory.

Hi willchan,
  We said whatever is passed to HttpStreamFactory::SetNextProtos is the list of
supported SPDY protocols. In "Alternate-Protocol:" headers, if they send us a
spdy protocol we don't support (for example browser is running with SPDY/2 and
they send us SPDY/2.1 in "Alternate-Protocol:" header), then we won't add that
host_port pair as the SPDY/2.1 supported protocol. Hope that is ok.

thanks,
raman

[Ryan Hamilton, 2012-03-02 16:46:52]

http://codereview.chromium.org/9516009/diff/25014/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9516009/diff/25014/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:6498: "foo", "bar", "http/1.1",
"spdy/2", "spdy/2.1", NULL));
I'm not sure that I understand the significance of the values here.  Do we need
foo and bar?  If yes, do we need the others?

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
File net/http/http_stream_factory.cc (right):

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.cc:60: for (int i = NPN_SPDY_1; i <
NUM_ALTERNATE_PROTOCOLS; ++i)
Start at 0 just to be simple.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.cc:88: for (int i = NPN_SPDY_2; i <
NUM_ALTERNATE_PROTOCOLS; ++i) {
Please start at 0.  If we want to avoid SPDY_1, either set the boolean to false,
or else remove it from the enum definition.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.cc:162: for (uint32 i = 0; i < value.size(); ++i) {
Let's not hard-code the strings again, please.  How about something like:

for (uint32 i = 0; i < value.size(); ++i) {
  for (int p = 0; p < NUM_ALTERNATE_PROTOCOLS; ++p) {
    if (values[i] == kAlternateProtocolStrings[p]) {
       .....
    }
  }
}

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factory.h
File net/http/http_stream_factory.h (right):

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.h:18: #include "net/http/http_server_properties.h"
Is this import needed?

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.h:233: // Sets the next protocol negotiation value
used during the SSL handshake.
Please update comment. I believe this vector is use to negotiate a protocol via
SSL NPN, as well as via HTTP Alternate-Protocol.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.h:274: static bool
supported_protocols_[NUM_ALTERNATE_PROTOCOLS];
nit: I think I would call this enabled_protocols_, because Chrome "supports" all
of them, but some might be disabled via command line flags.

[ramant (doing other things), 2012-03-02 18:40:40]

http://codereview.chromium.org/9516009/diff/25014/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9516009/diff/25014/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:6498: "foo", "bar", "http/1.1",
"spdy/2", "spdy/2.1", NULL));
On 2012/03/02 16:46:53, Ryan Hamilton wrote:
> I'm not sure that I understand the significance of the values here.  Do we
need
> foo and bar?  If yes, do we need the others?

We definitely need spdy/2 and/or spdy/2.1 (I am not sure about "foo" and "bar").
Removed "foo" and "bar".

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
File net/http/http_stream_factory.cc (right):

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.cc:60: for (int i = NPN_SPDY_1; i <
NUM_ALTERNATE_PROTOCOLS; ++i)
On 2012/03/02 16:46:53, Ryan Hamilton wrote:
> Start at 0 just to be simple.

Done.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.cc:88: for (int i = NPN_SPDY_2; i <
NUM_ALTERNATE_PROTOCOLS; ++i) {
On 2012/03/02 16:46:53, Ryan Hamilton wrote:
> Please start at 0.  If we want to avoid SPDY_1, either set the boolean to
false,
> or else remove it from the enum definition.

Done.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.cc:162: for (uint32 i = 0; i < value.size(); ++i) {
On 2012/03/02 16:46:53, Ryan Hamilton wrote:
> Let's not hard-code the strings again, please.  How about something like:
> 
> for (uint32 i = 0; i < value.size(); ++i) {
>   for (int p = 0; p < NUM_ALTERNATE_PROTOCOLS; ++p) {
>     if (values[i] == kAlternateProtocolStrings[p]) {
>        .....
>     }
>   }
> }

As we had discussed, added a TODO and issue 116575. kAlternateProtocolStrings
have different strings from next_protos strings, thus wasn't able to use
kAlternateProtocolStrings.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factory.h
File net/http/http_stream_factory.h (right):

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.h:18: #include "net/http/http_server_properties.h"
On 2012/03/02 16:46:53, Ryan Hamilton wrote:
> Is this import needed?

It is needed because of NUM_ALTERNATE_PROTOCOLS.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.h:233: // Sets the next protocol negotiation value
used during the SSL handshake.
On 2012/03/02 16:46:53, Ryan Hamilton wrote:
> Please update comment. I believe this vector is use to negotiate a protocol
via
> SSL NPN, as well as via HTTP Alternate-Protocol.

Done.

http://codereview.chromium.org/9516009/diff/25014/net/http/http_stream_factor...
net/http/http_stream_factory.h:274: static bool
supported_protocols_[NUM_ALTERNATE_PROTOCOLS];
On 2012/03/02 16:46:53, Ryan Hamilton wrote:
> nit: I think I would call this enabled_protocols_, because Chrome "supports"
all
> of them, but some might be disabled via command line flags.

Done.

[Ryan Hamilton, 2012-03-02 22:03:28]

lgtm

[commit-bot: I haz the power, 2012-03-02 22:05:27]

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

[commit-bot: I haz the power, 2012-03-03 00:43:16]

Change committed as 124794