Poll CanPredictNetworkActions in Predictor class.

Poll CanPredictNetworkActions in Predictor class.

Predictor is modified to call functions CanPredictNetworkActionsIO and
CanPredictNetworkActionsUI to comply with user preferences (and consider
network connection state).  This obsoletes Predictor::EnablePredictor.  Also,
bool members Predictor::preconnect_enabled_ and Predictor::predictor_enabled_
now depend solely on command line switches, therefore they are made const.

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

[Bence, 2014-06-30 19:53:48]

zelidrag: chrome/browser/chromeos/login/login_utils_browsertest.cc.  (I am
obsoleting Predictor::EnablePredictor.)

battre: chrome/browser/net/{net_pref_observer.cc,predictor.cc,predictor.h}

erg: chrome/browser/profiles/profile_impl*.cc

pkasting: chrome/browser/ui/startup/startup_browser_creator_impl.cc.  (I moved
acting on this switch to chrome/browser/profiles/profile_impl.cc:407.)

Thank you all.

[Peter Kasting, 2014-06-30 19:55:28]

LGTM

[Elliot Glaysher, 2014-06-30 22:04:04]

lgtm

[battre, 2014-07-01 08:27:48]

I have a suggestion to integrate predictor_enabled_ and preconnect_enabled_ into
the preferences. What do you think of this idea?

Best regards,
Dominic

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils_browsertest.cc:324: }
nit: remove curlys?

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

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:244:
chrome_browser_net::CanPredictNetworkActionsUI(user_prefs_)) {
This approach looks a little bit strange to me. The PrefService has its own
mechanism to declare a preference that can be overridden by a command line flag.
Can't you use that?

See for example command_line_pref_store.cc -> ApplyBackgroundModeSwitches. You
could check the command line flag and set the prefs::kNetworkPredictionOptions
and prefs::kNetworkPredictionEnabled preferences.

base/prefs/pref_value_store.h has documentation on the precedence of various
pref stores. If you implement this as suggested, policies and extensions could
still override the setting.

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/net/predi...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/net/predi...
chrome/browser/net/predictor.h:511: const bool predictor_enabled_;
This is a little bit strange in my opinion, because even if this is true, the
predictor can be enabled. This is more an overriding flag. It is hard to name
this. Therefore, I have another suggestion below.

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:100: }
nit: remove curlys?

[Bence, 2014-07-01 17:38:33]

On 2014/07/01 08:27:48, battre wrote:
> I have a suggestion to integrate predictor_enabled_ and preconnect_enabled_
into
> the preferences. What do you think of this idea?
> 
> Best regards,
> Dominic
> 

Hi Dominic,

I really like your idea, it is much clearer than what I am trying to do here. 
However, I would like to propose going with this CL as it is for the time being.
 The reason is that https://crbug.com/334602 proposes a single user option,
whereas the current command line switches give more gradual control over
predictive behaviour.  Therefore it is currently not possible to implement the
command line switches through a PrefStore.

Once all platform code is changed and prefs::kNetworkPredictionEnabled is
removed, it will be worth considering consolidating the command line switches
--dns-prefetch-disable and --disable-preconnect into a single one disabling all
predictive network options.  If that decision is made, I agree that the current
predictor_enabled_ and preconnect_enabled_ members should be removed, and that
switch implemented à la CommandLinePrefStore::ApplyBackgroundModeSwitches.

Cheers,

Bence

[Bence, 2014-07-01 17:39:27]

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils_browsertest.cc:324: }
On 2014/07/01 08:27:48, battre wrote:
> nit: remove curlys?

Done.

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/360733005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:100: }
On 2014/07/01 08:27:48, battre wrote:
> nit: remove curlys?

Done.

[battre, 2014-07-02 12:57:42]

I think I have found one bug. Sorry for not noticing this earlier. Also note
that your tests don't compile.

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

https://codereview.chromium.org/360733005/diff/40001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:308:
!chrome_browser_net::CanPredictNetworkActionsUI(user_prefs_) ||
This looks like a bug. We allow calling PreconnectUrlAndSubResources on UI and
IO thread.

[Bence, 2014-07-07 16:35:50]

zelidrag, battre: PTAL.

Dominic, thank you for catching both problems.

Also, I did a rebase and forgot to submit as a separate patch, I'm sorry about
that.

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

https://codereview.chromium.org/360733005/diff/40001/chrome/browser/net/predi...
chrome/browser/net/predictor.cc:308:
!chrome_browser_net::CanPredictNetworkActionsUI(user_prefs_) ||
On 2014/07/02 12:57:41, battre wrote:
> This looks like a bug. We allow calling PreconnectUrlAndSubResources on UI and
> IO thread.

Done.

[Bence, 2014-07-09 20:41:57]

On 2014/07/02 12:57:42, battre wrote:
> I think I have found one bug. Sorry for not noticing this earlier. Also note
> that your tests don't compile.

Hi Dominic,

Could you please help me with the tests?  I have difficulties interpreting the
results.  Did I really break some tests or is it just that they are flaky?

Thanks,

Bence

[battre, 2014-07-10 08:24:24]

On 2014/07/09 20:41:57, bnc wrote:
> On 2014/07/02 12:57:42, battre wrote:
> > I think I have found one bug. Sorry for not noticing this earlier. Also note
> > that your tests don't compile.
> 
> Hi Dominic,
> 
> Could you please help me with the tests?  I have difficulties interpreting the
> results.  Did I really break some tests or is it just that they are flaky?
> 
> Thanks,
> 
> Bence

Hi Bence,

did you see this:
http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...
[ RUN      ] PredictorTest.HSTSRedirectSubresources
[4984:3656:0709/135647:59378551:FATAL:prediction_options.cc(59)] Check failed:
prefs.
[7086/7087] PredictorTest.HSTSRedirectSubresources (CRASHED)

This is the function in which the DCHECK(prefs) fails.

bool CanPredictNetworkActionsUI(PrefService* prefs) {
  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
  DCHECK(prefs);
  return CanPredictNetworkActions(
      prefs->GetInteger(prefs::kNetworkPredictionOptions),
      prefs->GetBoolean(prefs::kNetworkPredictionEnabled));
}

In other words, you are missing a pref service.

Best regards,
Dominic

[battre, 2014-07-10 09:01:59]

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

https://codereview.chromium.org/360733005/diff/100001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:799: prefs::kDnsPrefetchingHostReferralList);
This is actually a pretty serious bug that existed before and should be fixed.
You may not instantiate a ListPrefUpdate and/or use a ListPrefUpdate if you are
on the IO thread. All modifications are directly written into a the memory of
the PrefService. Therefore, this is pretty racy.

CCing original author (rlp) as FYI.

[Bernhard Bauer, 2014-07-10 09:59:02]

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

https://codereview.chromium.org/360733005/diff/100001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:799: prefs::kDnsPrefetchingHostReferralList);
On 2014/07/10 09:01:59, battre wrote:
> This is actually a pretty serious bug that existed before and should be fixed.
> You may not instantiate a ListPrefUpdate and/or use a ListPrefUpdate if you
are
> on the IO thread. All modifications are directly written into a the memory of
> the PrefService. Therefore, this is pretty racy.
> 
> CCing original author (rlp) as FYI.

I think in this particular case, we're okay. The only place where this method
seems to be called is from the ProfileImplIOData::Handle destructor, which
DCHECKs that it's on the UI thread. I suspect the reason why we have the check
below is because this might run in a unit test where everything is mapped to a
single thread.

I'm still in favor of adding those DCHECKs to ScopedUserPrefUpdate though.

[Bence, 2014-07-10 13:39:43]

On 2014/07/10 08:24:24, battre wrote:
> On 2014/07/09 20:41:57, bnc wrote:
> > On 2014/07/02 12:57:42, battre wrote:
> > > I think I have found one bug. Sorry for not noticing this earlier. Also
note
> > > that your tests don't compile.
> > 
> > Hi Dominic,
> > 
> > Could you please help me with the tests?  I have difficulties interpreting
the
> > results.  Did I really break some tests or is it just that they are flaky?
> > 
> > Thanks,
> > 
> > Bence
> 
> Hi Bence,
> 
> did you see this:
>
http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...
> [ RUN      ] PredictorTest.HSTSRedirectSubresources
> [4984:3656:0709/135647:59378551:FATAL:prediction_options.cc(59)] Check failed:
> prefs.
> [7086/7087] PredictorTest.HSTSRedirectSubresources (CRASHED)
> 
> This is the function in which the DCHECK(prefs) fails.
> 
> bool CanPredictNetworkActionsUI(PrefService* prefs) {
>   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
>   DCHECK(prefs);
>   return CanPredictNetworkActions(
>       prefs->GetInteger(prefs::kNetworkPredictionOptions),
>       prefs->GetBoolean(prefs::kNetworkPredictionEnabled));
> }
> 
> In other words, you are missing a pref service.
> 
> Best regards,
> Dominic

Thank you.  It seems like HSTSRedirectSubresources instantiates a Predictor, but
then does not pass it a valid pref service through InitNetworkPredictor.  I
modified Predictor so as not to call CanPredictNetworkActions* with NULL.

[battre, 2014-07-10 14:03:45]

LGTM if the trybots are now happy.

[Bence, 2014-07-16 11:48:14]

On 2014/07/10 14:03:45, battre wrote:
> LGTM if the trybots are now happy.

Dominic: PTAL.  I had to make a few modifications to make sure all tests pass.

[zel, 2014-07-16 15:18:00]

LGTM for chrome/browser/chromeos/ part

[Bence, 2014-07-21 19:12:05]

On 2014/07/16 15:18:00, zel wrote:
> LGTM for chrome/browser/chromeos/ part

zelidrag: PTAL at chrome/browser/chromeos/login/login_utils_browsertest.cc.  I
had to add a command line switch to make sure that Predictor does not need to do
stuff on the mock IO thread.  Predictor::EnablePredictorOnIOThread has been
removed since this test was the only place calling it.

[zel, 2014-07-21 19:21:23]

On 2014/07/21 19:12:05, bnc wrote:
> On 2014/07/16 15:18:00, zel wrote:
> > LGTM for chrome/browser/chromeos/ part
> 
> zelidrag: PTAL at chrome/browser/chromeos/login/login_utils_browsertest.cc.  I
> had to add a command line switch to make sure that Predictor does not need to
do
> stuff on the mock IO thread.  Predictor::EnablePredictorOnIOThread has been
> removed since this test was the only place calling it.

still LGTM

[Bence, 2014-07-21 19:29:11]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-07-21 19:29:54]

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

[commit-bot: I haz the power, 2014-07-21 22:47:34]

Change committed as 284526