Enable TCP preconnect and DNS preresolve on cellular.

Enable TCP preconnect and DNS preresolve on cellular.

Previously the default setting was to perform TCP preconnect and DNS
preresolution on all networks, and do prefetch and prerender on non cellular
network only.  Changing to kNetworkPredictionOptions has modified this default
behavior, disabling preconnect and preresolve on cellular networks by default
(the default setting is NETWORK_PREDICTION_WIFI_ONLY).  This CL reverts this
change, allowing preconnect and preresolve (but not prefetch and prerender) on
any network if kNetworkPredicitonOptions == NETWORK_PREDICTION_WIFI_ONLY.  In
order to do so, the helper functions CanPredictNetworkActions*() are split up
into new helper functions, one set for preconnect and preresolve, the other
for prefetch and prerender.

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

[cbentzel, 2014-08-08 15:46:10]

mmenke: Sorry to add something more to your plate, but you did the review of
prediction_options.

[mmenke, 2014-08-08 16:05:42]

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
File chrome/browser/net/prediction_options.cc (right):

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.cc:38: bool NetworkPredictionEnabled) {
These should be named network_prediction_options and network_prediction_enabled.
 Same goes for the above function.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.cc:43: // if the user setting is
WIFI_ONLY.
nit:  Shouldn't use we in comments, since it's ambiguous (We the programmers, we
google, we meaning the program, etc).

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.cc:88: 
optional nit:  The two IO thread functions have blank lines here, the two UI
thread ones do not.  I suggest being uniform about the decision, since the
functions are all so similar.  I'm fine either way.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
File chrome/browser/net/prediction_options.h (right):

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.h:40: bool
CanPredictNetworkActionsUI(PrefService* prefs);
I think this name is confusing, giving the preconnect is also predicting network
actions.  Suggest calling these CanPrefetch[IO/UI].

Alternatively, could be more specific with CanPrefetchAndPrerender, and rename
the other CanPreresolveAndPreconnect.  A bit wordier, but clearer.  Only
potential issue is we may use them for other decisions, too.  Anyhow, up to you.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictor...
chrome/browser/net/predictor.h:443: virtual bool CanPreconnectIO();
nit:  All 4 of these should be const.

[Bence, 2014-08-08 20:10:03]

mmenke: net/*, prerender/prerender_manager.cc
bartfab: policy/policy_browsertest.cc
gavinp: prefetch/prefetch.cc

PTAL.  Thank you all.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
File chrome/browser/net/prediction_options.cc (right):

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.cc:38: bool NetworkPredictionEnabled) {
On 2014/08/08 16:05:42, mmenke wrote:
> These should be named network_prediction_options and
network_prediction_enabled.
>  Same goes for the above function.

Done.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.cc:43: // if the user setting is
WIFI_ONLY.
On 2014/08/08 16:05:42, mmenke wrote:
> nit:  Shouldn't use we in comments, since it's ambiguous (We the programmers,
we
> google, we meaning the program, etc).

Done.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.cc:88: 
On 2014/08/08 16:05:42, mmenke wrote:
> optional nit:  The two IO thread functions have blank lines here, the two UI
> thread ones do not.  I suggest being uniform about the decision, since the
> functions are all so similar.  I'm fine either way.

Done.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
File chrome/browser/net/prediction_options.h (right):

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictio...
chrome/browser/net/prediction_options.h:40: bool
CanPredictNetworkActionsUI(PrefService* prefs);
On 2014/08/08 16:05:42, mmenke wrote:
> I think this name is confusing, giving the preconnect is also predicting
network
> actions.  Suggest calling these CanPrefetch[IO/UI].
> 
> Alternatively, could be more specific with CanPrefetchAndPrerender, and rename
> the other CanPreresolveAndPreconnect.  A bit wordier, but clearer.  Only
> potential issue is we may use them for other decisions, too.  Anyhow, up to
you.

Done.

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictor.h
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/443413002/diff/1/chrome/browser/net/predictor...
chrome/browser/net/predictor.h:443: virtual bool CanPreconnectIO();
On 2014/08/08 16:05:42, mmenke wrote:
> nit:  All 4 of these should be const.

Done.

[mmenke, 2014-08-08 20:18:29]

https://codereview.chromium.org/443413002/diff/40001/chrome/browser/net/predi...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/40001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:300: CanonicalizeUrl(url), motivation));
Should we be doing this, if CanPreresolveAndPreconnectUI is false?  Doesn't look
like we have any logic to prevent this from doing resolutions.

[mmenke, 2014-08-08 20:19:03]

Also, should we have regression tests for this?

[bartfab (slow), 2014-08-11 08:29:04]

policy/policy_browsertest.cc LGTM

[Bence, 2014-08-11 20:17:13]

mmenke: PTAL.

https://codereview.chromium.org/443413002/diff/40001/chrome/browser/net/predi...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/40001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:300: CanonicalizeUrl(url), motivation));
On 2014/08/08 20:18:29, mmenke wrote:
> Should we be doing this, if CanPreresolveAndPreconnectUI is false?  Doesn't
look
> like we have any logic to prevent this from doing resolutions.

Very good catch.  If you look at a version before CanPredictNetworkPrediction()
and similar helper functions were introduced:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/net/predictor....
you can see that preconnect was executed iff predictor_enabled AND
preconnect_enabled, whicle preresolution was executed iff predictor_enabled. 
This is not what we want, so let's take advantage of the verbose helper function
names and make behavior consistent.

Done.

[mmenke, 2014-08-12 17:38:10]

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:232: if (!preconnect_enabled_ ||
!CanPreresolveAndPreconnectUI())
I think !preconnect_enabled_ should go down to where preconnectable is?  At
least I don't believe it should disable presresolve as well, if we want to keep
the old behavior.

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:728: return;
nit:  While you're here, mind adding braces?

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:947: bool Predictor::CanPrefetchAndPrerenderUI()
const {
Suggest adding DCHECKs for which thread we're on in these functions.

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:947: bool Predictor::CanPrefetchAndPrerenderUI()
const {
optional:  It's marginally slower, but you might want to merge these into a
single function that both figures out what thread it's on, and then calls it,
and completely get rid of the thread-dependent versions.  we're doing the checks
half the time, anyways, and I don't think we're making calls often enough for
the performance to matter.

[Bence, 2014-08-12 20:21:56]

gavinp, mmenke: PTAL

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:232: if (!preconnect_enabled_ ||
!CanPreresolveAndPreconnectUI())
On 2014/08/12 17:38:10, mmenke wrote:
> I think !preconnect_enabled_ should go down to where preconnectable is?  At
> least I don't believe it should disable presresolve as well, if we want to
keep
> the old behavior.

Done.

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:728: return;
On 2014/08/12 17:38:10, mmenke wrote:
> nit:  While you're here, mind adding braces?

Done.

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:947: bool Predictor::CanPrefetchAndPrerenderUI()
const {
On 2014/08/12 17:38:10, mmenke wrote:
> optional:  It's marginally slower, but you might want to merge these into a
> single function that both figures out what thread it's on, and then calls it,
> and completely get rid of the thread-dependent versions.  we're doing the
checks
> half the time, anyways, and I don't think we're making calls often enough for
> the performance to matter.

Done.

https://codereview.chromium.org/443413002/diff/60001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:947: bool Predictor::CanPrefetchAndPrerenderUI()
const {
On 2014/08/12 17:38:10, mmenke wrote:
> Suggest adding DCHECKs for which thread we're on in these functions.

Done.

[mmenke, 2014-08-12 20:29:11]

One more thing, otherwise, I'm happy.

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:231:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Should we still have the predictor_enabled_ check?

[Bence, 2014-08-12 20:45:29]

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:231:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/08/12 20:29:11, mmenke wrote:
> Should we still have the predictor_enabled_ check?

I don't think it is necessary.  In the current implementation,
predictor_enabled_ is false iff --dns-prefetch-disable is present, and
preconnect_enabled_ is false iff --disable-preconnect is present.  Nothing else
(user preference, policies etc.) changes these members.

[mmenke, 2014-08-12 20:50:08]

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:231:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/08/12 20:45:28, Bence wrote:
> On 2014/08/12 20:29:11, mmenke wrote:
> > Should we still have the predictor_enabled_ check?
> 
> I don't think it is necessary.  In the current implementation,
> predictor_enabled_ is false iff --dns-prefetch-disable is present, and
> preconnect_enabled_ is false iff --disable-preconnect is present.  Nothing
else
> (user preference, policies etc.) changes these members.

But we're still calling Predictor::Resolve in the dns-prefetch-disable case, and
we weren't before.  I do think preconnect without prefetch is a little weird,
but if we have the command line options, we should try and follow them.

[Bence, 2014-08-12 21:15:23]

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:231:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/08/12 20:50:07, mmenke wrote:
> On 2014/08/12 20:45:28, Bence wrote:
> > On 2014/08/12 20:29:11, mmenke wrote:
> > > Should we still have the predictor_enabled_ check?
> > 
> > I don't think it is necessary.  In the current implementation,
> > predictor_enabled_ is false iff --dns-prefetch-disable is present, and
> > preconnect_enabled_ is false iff --disable-preconnect is present.  Nothing
> else
> > (user preference, policies etc.) changes these members.
> 
> But we're still calling Predictor::Resolve in the dns-prefetch-disable case,
and
> we weren't before.  I do think preconnect without prefetch is a little weird,
> but if we have the command line options, we should try and follow them.

You are right.  Note that I also am working on https://crbug.com/396215, which
will help clearing up this issue around command line switches.

Done.

[mmenke, 2014-08-12 21:45:32]

On 2014/08/12 21:15:23, Bence wrote:
>
https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
> File chrome/browser/net/predictor.cc (right):
> 
>
https://codereview.chromium.org/443413002/diff/100001/chrome/browser/net/pred...
> chrome/browser/net/predictor.cc:231:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> On 2014/08/12 20:50:07, mmenke wrote:
> > On 2014/08/12 20:45:28, Bence wrote:
> > > On 2014/08/12 20:29:11, mmenke wrote:
> > > > Should we still have the predictor_enabled_ check?
> > > 
> > > I don't think it is necessary.  In the current implementation,
> > > predictor_enabled_ is false iff --dns-prefetch-disable is present, and
> > > preconnect_enabled_ is false iff --disable-preconnect is present.  Nothing
> > else
> > > (user preference, policies etc.) changes these members.
> > 
> > But we're still calling Predictor::Resolve in the dns-prefetch-disable case,
> and
> > we weren't before.  I do think preconnect without prefetch is a little
weird,
> > but if we have the command line options, we should try and follow them.
> 
> You are right.  Note that I also am working on https://crbug.com/396215, which
> will help clearing up this issue around command line switches.
> 
> Done.

LGTM

[gavinp, 2014-08-12 21:59:46]

LGTM

[Bence, 2014-08-13 14:54:19]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-08-13 14:54:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bnc@chromium.org/443413002/120001

[commit-bot: I haz the power, 2014-08-13 15:54:09]

Committed patchset #7 (120001) as 289302