[Android WebView] Get rid of secondary HttpNetworkSession

[Android WebView] Get rid of secondary HttpNetworkSession

Set up all the required Http parameters via URLRequestContextBuilder.

BUG=556689
Committed: https://crrev.com/f2188baab5bc2948eb2a8900bad041387e393c12
Cr-Commit-Position: refs/heads/master@{#360159}

[mnaganov (inactive), 2015-11-17 01:50:34]

mnaganov@chromium.org changed reviewers:
+ sgurun@chromium.org

[mnaganov (inactive), 2015-11-17 01:50:35]

Selim can you please check that I haven't missed anything here :)

[sgurun-gerrit only, 2015-11-17 08:19:12]

I don't see any issues. let's see what the network guys say

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
android_webview/browser/net/aw_url_request_context_getter.cc:215:
builder.SetSpdyAndQuicEnabled(true, true);
This makes me realize that... Quic is probably already enabled -- will verify
tomorrow.

[mnaganov (inactive), 2015-11-17 16:54:03]

mnaganov@chromium.org changed reviewers:
+ mmenke@chromium.org

[mnaganov (inactive), 2015-11-17 16:54:03]

Hi Matt,

Can you please review the change, thanks!

[mmenke, 2015-11-17 17:22:46]

This looks like a really nice improvement to me.  Should note in the description
that this also enables HTTP2 for webview (Note that we were just enabling SPDY31
before).

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
android_webview/browser/net/aw_url_request_context_getter.cc:214:
ApplyCmdlineOverridesToNetworkSessionParams(&network_session_params);
Bug:  This isn't doing anything.

You can set these using
URLRequestContextBuilder::set_http_network_session_params, though (Which takes
an URLRequestContextBuilder::NetworkSessionParams object).  It must be called
before builder.SetSpdyAndQuicEnabled, since they both touch the same internal
object.

https://codereview.chromium.org/1448193002/diff/1/net/url_request/url_request...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/1448193002/diff/1/net/url_request/url_request...
net/url_request/url_request_context_builder.h:57: DISK_SIMPLE,
Should have comments here.  Not sure we want to keep this parameter, long term. 
Think it's fine for now, though.

[sgurun-gerrit only, 2015-11-17 17:58:03]

On 2015/11/17 17:22:46, mmenke wrote:
> This looks like a really nice improvement to me.  Should note in the
description
> that this also enables HTTP2 for webview (Note that we were just enabling
SPDY31
> before).

Very good point. I saw it yesterday night but forgot mentioning. How stable is
it? should we hold off?

> 
>
https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
> File android_webview/browser/net/aw_url_request_context_getter.cc (right):
> 
>
https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
> android_webview/browser/net/aw_url_request_context_getter.cc:214:
> ApplyCmdlineOverridesToNetworkSessionParams(&network_session_params);
> Bug:  This isn't doing anything.
> 
> You can set these using
> URLRequestContextBuilder::set_http_network_session_params, though (Which takes
> an URLRequestContextBuilder::NetworkSessionParams object).  It must be called
> before builder.SetSpdyAndQuicEnabled, since they both touch the same internal
> object.
> 
>
https://codereview.chromium.org/1448193002/diff/1/net/url_request/url_request...
> File net/url_request/url_request_context_builder.h (right):
> 
>
https://codereview.chromium.org/1448193002/diff/1/net/url_request/url_request...
> net/url_request/url_request_context_builder.h:57: DISK_SIMPLE,
> Should have comments here.  Not sure we want to keep this parameter, long
term. 
> Think it's fine for now, though.

[sgurun-gerrit only, 2015-11-17 18:06:31]

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
android_webview/browser/net/aw_url_request_context_getter.cc:215:
builder.SetSpdyAndQuicEnabled(true, true);
On 2015/11/17 08:19:12, sgurun wrote:
> This makes me realize that... Quic is probably already enabled -- will verify
> tomorrow.

also as mmenke pointed out this enables http2, which is enabled only using field
trial experiments in chrome as far as I can tell. We don't have such support so
we should not do this, in my opinion. Of course let's see what Matt has to say
about it.

[mmenke, 2015-11-17 18:11:14]

On 2015/11/17 17:58:03, sgurun wrote:
> On 2015/11/17 17:22:46, mmenke wrote:
> > This looks like a really nice improvement to me.  Should note in the
> description
> > that this also enables HTTP2 for webview (Note that we were just enabling
> SPDY31
> > before).
> 
> Very good point. I saw it yesterday night but forgot mentioning. How stable is
> it? should we hold off?

Think it's past time to enable it.

It's enabled by default on desktop, and the HTTP2 code is basically the SPDY31
code.  We have run into some SPDY31 sites that are HTTP2 intolerant, and some
proxies with issues with one or the other, but HTTP2 is enabled by default on
trunk, and has been for a while.  The plan is to disable SPDY31 sometime next
year (Q1 or Q2?), and remove support entirely.

I'm less confident about whether or not we want QUIC, actually - it's a source
of more crashes (Or has been in the last 4 months.  All the ones I'm aware of
have been fixed b now, though), and is still under development.  I'm not really
pushing against enabling it - it already was enabled before, after all, and some
third parties are already experimenting with it.

[sgurun-gerrit only, 2015-11-17 18:25:20]

On 2015/11/17 18:11:14, mmenke wrote:
> On 2015/11/17 17:58:03, sgurun wrote:
> > On 2015/11/17 17:22:46, mmenke wrote:
> > > This looks like a really nice improvement to me.  Should note in the
> > description
> > > that this also enables HTTP2 for webview (Note that we were just enabling
> > SPDY31
> > > before).
> > 
> > Very good point. I saw it yesterday night but forgot mentioning. How stable
is
> > it? should we hold off?
> 
> Think it's past time to enable it.
> 
> It's enabled by default on desktop, and the HTTP2 code is basically the SPDY31
> code.  We have run into some SPDY31 sites that are HTTP2 intolerant, and some
> proxies with issues with one or the other, but HTTP2 is enabled by default on
> trunk, and has been for a while.  The plan is to disable SPDY31 sometime next
> year (Q1 or Q2?), and remove support entirely.
> 
> I'm less confident about whether or not we want QUIC, actually - it's a source
> of more crashes (Or has been in the last 4 months.  All the ones I'm aware of
> have been fixed b now, though), and is still under development.  I'm not
really
> pushing against enabling it - it already was enabled before, after all, and
some
> third parties are already experimenting with it.

ok sounds good.

[mnaganov (inactive), 2015-11-17 19:01:39]

As Selim has pointed out, NextProtosSpdy31() that we were using, enables Quic.
With the new code, in addition to Spdy31 we also enable Http2 which Matt says is
a good thing to do.

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
android_webview/browser/net/aw_url_request_context_getter.cc:214:
ApplyCmdlineOverridesToNetworkSessionParams(&network_session_params);
On 2015/11/17 17:22:45, mmenke wrote:
> Bug:  This isn't doing anything.
> 
> You can set these using
> URLRequestContextBuilder::set_http_network_session_params, though (Which takes
> an URLRequestContextBuilder::NetworkSessionParams object).  It must be called
> before builder.SetSpdyAndQuicEnabled, since they both touch the same internal
> object.

*facepalm* Right, I knew I must call it, but somehow forgot to do that. Fixed.

https://codereview.chromium.org/1448193002/diff/1/android_webview/browser/net...
android_webview/browser/net/aw_url_request_context_getter.cc:215:
builder.SetSpdyAndQuicEnabled(true, true);
On 2015/11/17 18:06:31, sgurun wrote:
> On 2015/11/17 08:19:12, sgurun wrote:
> > This makes me realize that... Quic is probably already enabled -- will
verify
> > tomorrow.
> 
> also as mmenke pointed out this enables http2, which is enabled only using
field
> trial experiments in chrome as far as I can tell. We don't have such support
so
> we should not do this, in my opinion. Of course let's see what Matt has to say
> about it.

I confused use_alternative_services with Quic. The original code was setting the
former (in PopulateNetworkSessionParams).

https://codereview.chromium.org/1448193002/diff/1/net/url_request/url_request...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/1448193002/diff/1/net/url_request/url_request...
net/url_request/url_request_context_builder.h:57: DISK_SIMPLE,
On 2015/11/17 17:22:45, mmenke wrote:
> Should have comments here.  Not sure we want to keep this parameter, long
term. 
> Think it's fine for now, though.

Done.

[mmenke, 2015-11-17 19:32:44]

LGTM!

[mnaganov (inactive), 2015-11-17 20:14:03]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2015-11-17 20:14:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1448193002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1448193002/40001

[commit-bot: I haz the power, 2015-11-17 20:59:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-17 21:00:25]

Patchset 3 (id:??) landed as
https://crrev.com/f2188baab5bc2948eb2a8900bad041387e393c12
Cr-Commit-Position: refs/heads/master@{#360159}