Enable SPDY 3.1 and 3.0 in UrlRequestContextBuilder by default

Enable SPDY 3.1 and 3.0 in UrlRequestContextBuilder by default

BUG=372528
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273856

[mmenke, 2014-05-29 16:48:12]

It seems like every time someone's tries to use our network library, they're
surprised when they discover it not using SPDY, sometimes long after deploying
it.  I think we should have it enabled by default when used externally.

Could enable it by default at the HttpNetworkSession::Params layer instead, but
I'm a bit nervous about doing that, primarily from a unit test standpoint.  If
there's consensus that we should do that instead, I'm happy to investigate, when
I have time, but then we'd still want it configurable from the builder, anyways.

Misha:  We'll want to add cronet functions to make this configurable, of course
(And enable QUIC as well), but think that belongs in another CL.

[Ryan Hamilton, 2014-05-29 19:33:43]

On 2014/05/29 16:48:12, mmenke wrote:
> It seems like every time someone's tries to use our network library, they're
> surprised when they discover it not using SPDY, sometimes long after deploying
> it.  I think we should have it enabled by default when used externally.
> 
> Could enable it by default at the HttpNetworkSession::Params layer instead,
but
> I'm a bit nervous about doing that, primarily from a unit test standpoint.  If
> there's consensus that we should do that instead, I'm happy to investigate,
when
> I have time, but then we'd still want it configurable from the builder,
anyways.
> 
> Misha:  We'll want to add cronet functions to make this configurable, of
course
> (And enable QUIC as well), but think that belongs in another CL.

I confess, I'm unfamiliar with the UrlRequestContextBuilder. Can you clarify how
it relates to, say, IOThread, which is the "normal" place that we initialize
SPDY?

[mmenke, 2014-05-29 19:39:05]

On 2014/05/29 19:33:43, Ryan Hamilton wrote:
> On 2014/05/29 16:48:12, mmenke wrote:
> > It seems like every time someone's tries to use our network library, they're
> > surprised when they discover it not using SPDY, sometimes long after
deploying
> > it.  I think we should have it enabled by default when used externally.
> > 
> > Could enable it by default at the HttpNetworkSession::Params layer instead,
> but
> > I'm a bit nervous about doing that, primarily from a unit test standpoint. 
If
> > there's consensus that we should do that instead, I'm happy to investigate,
> when
> > I have time, but then we'd still want it configurable from the builder,
> anyways.
> > 
> > Misha:  We'll want to add cronet functions to make this configurable, of
> course
> > (And enable QUIC as well), but think that belongs in another CL.
> 
> I confess, I'm unfamiliar with the UrlRequestContextBuilder. Can you clarify
how
> it relates to, say, IOThread, which is the "normal" place that we initialize
> SPDY?

Sorry, should have stated that initially.

It's not used by Chrome (May use it on iOS, but I don't believe so).  It exists
solely so third parties can use the network stack, and is also used by Cronet.

[Ryan Hamilton, 2014-05-29 19:51:33]

On 2014/05/29 19:39:05, mmenke wrote:
> On 2014/05/29 19:33:43, Ryan Hamilton wrote:
> > I confess, I'm unfamiliar with the UrlRequestContextBuilder. Can you clarify
how
> > it relates to, say, IOThread, which is the "normal" place that we initialize
> > SPDY?
> 
> Sorry, should have stated that initially.
> 
> It's not used by Chrome (May use it on iOS, but I don't believe so).  It
exists
> solely so third parties can use the network stack, and is also used by Cronet.

Ah, I see. Makes sense. Reviewing...

[Ryan Hamilton, 2014-05-29 19:54:35]

https://codereview.chromium.org/302893003/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/302893003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_context_builder.cc:184:
next_protos.push_back(kProtoSPDY31);
So at some point, we're going to turn QUIC on by default. I'm sure I won't
remember to do it here :> In the least, I'd be inclined to use one of the
existing NextProtos methods from next_protos.h. But I think we really should
work towards having the right defaults. So I think your suggestion of doing this
in HttpNetworkSession::Params is the right way forward. I you want to land this
while you work on that, I'm fine with that approach, but I wouldn't want to see
this code exist as anything other than a bandaid. What do you think?

[mmenke, 2014-05-29 19:58:04]

On 2014/05/29 19:54:35, Ryan Hamilton wrote:
>
https://codereview.chromium.org/302893003/diff/20001/net/url_request/url_requ...
> File net/url_request/url_request_context_builder.cc (right):
> 
>
https://codereview.chromium.org/302893003/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_context_builder.cc:184:
> next_protos.push_back(kProtoSPDY31);
> So at some point, we're going to turn QUIC on by default. I'm sure I won't
> remember to do it here :> In the least, I'd be inclined to use one of the
> existing NextProtos methods from next_protos.h. But I think we really should
> work towards having the right defaults. So I think your suggestion of doing
this
> in HttpNetworkSession::Params is the right way forward. I you want to land
this
> while you work on that, I'm fine with that approach, but I wouldn't want to
see
> this code exist as anything other than a bandaid. What do you think?

Let's land this as a bandaid, longer term, I'm happy to work on updating
HttpNetworkSession::Params, though I'm a bit wary of enabling QUIC in other apps
by default until it has published specs - seems too soon, otherwise.

[mmenke, 2014-05-29 19:59:52]

On 2014/05/29 19:58:04, mmenke wrote:
> On 2014/05/29 19:54:35, Ryan Hamilton wrote:
> >
>
https://codereview.chromium.org/302893003/diff/20001/net/url_request/url_requ...
> > File net/url_request/url_request_context_builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/302893003/diff/20001/net/url_request/url_requ...
> > net/url_request/url_request_context_builder.cc:184:
> > next_protos.push_back(kProtoSPDY31);
> > So at some point, we're going to turn QUIC on by default. I'm sure I won't
> > remember to do it here :> In the least, I'd be inclined to use one of the
> > existing NextProtos methods from next_protos.h. But I think we really should
> > work towards having the right defaults. So I think your suggestion of doing
> this
> > in HttpNetworkSession::Params is the right way forward. I you want to land
> this
> > while you work on that, I'm fine with that approach, but I wouldn't want to
> see
> > this code exist as anything other than a bandaid. What do you think?
> 
> Let's land this as a bandaid, longer term, I'm happy to work on updating
> HttpNetworkSession::Params, though I'm a bit wary of enabling QUIC in other
apps
> by default until it has published specs - seems too soon, otherwise.

Actually, I'm happy to add a function to next_protos.h.

NextProtosDefaults()?

[Ryan Hamilton, 2014-05-29 20:04:13]

On Thu, May 29, 2014 at 12:59 PM, <mmenke@chromium.org> wrote:

> On 2014/05/29 19:58:04, mmenke wrote:
>
>> Let's land this as a bandaid, longer term, I'm happy to work on updating
>> HttpNetworkSession::Params, though I'm a bit wary of enabling QUIC in
>> other
>>
> apps
>
>> by default until it has published specs - seems too soon, otherwise.
>>
>
​SGTM
​

>  Actually, I'm happy to add a function to next_protos.h.
>
> NextProtosDefaults()?
>
> ttps://codereview.chromium.org/302893003/
> <https://codereview.chromium.org/302893003/>
>

​LGTM​

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Hamilton, 2014-05-29 20:07:20]

lgtm

I guess I need to push the "LGTM" button.  Pushing.

[mmenke, 2014-05-30 16:03:48]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-05-30 16:05:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/302893003/60001

[commit-bot: I haz the power, 2014-05-30 16:27:23]

Change committed as 273856