Implement policy for kNetworkPredictionOptions.

Implement policy for kNetworkPredictionOptions.

A new policy is added for kNetworkPredictionOptions, a preference that
deprecates kNetworkPredicionEnabled.  The policy handler observes policies for
both, making the change seamless for policy admins.

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

[Bence, 2014-07-30 02:32:45]

Alexei: please review tools/metrics/histograms/histograms.xml.

Joao: please review everything else.

Thank you both.

[Joao da Silva, 2014-07-30 07:57:33]

Looks good, just a couple of minor comments.

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
File chrome/browser/policy/network_prediction_policy_handler.cc (right):

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:10: #include
"chrome/common/content_settings.h"
Not used

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:66:
network_prediction_options->GetAsInteger(&setting);
This may fail, if the policy value isn't an integer. Suggestion:

  int setting;
  if (network_prediction_options &&
network_prediction_options->GetAsInteger(&settings) {
    ...
  }

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:80:
network_prediction_enabled->GetAsBoolean(&setting);
Same here

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:82: // Be
conservative here and set Options to WIFI_ONLY.
The legacy semantic was equivalent to ALWAYS and I think we should keep that. Do
you have a reason to prefer WIFI_ONLY here?

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:123: #   For your editing
convenience: highest ID currently used: 270
Update this ID

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:340: 'caption': '''Preference
value has not been set''',
This value is not needed in the policy, since it's equivalent to not setting the
policy at all. Then the pref defaults to its default value, which is 3.

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:343: 'supported_on':
['chrome.*:8-', 'chrome_os:11-', 'android:30-'],
The version should be 38

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:349: 'example_value': True,
The example value should be one of the int values

[Bence, 2014-07-30 13:19:32]

What are the current/upcoming chrome_os and android versions (for
policy_templates.json)?

PolicyPrefIndicatorTestInstance/PolicyPrefIndicatorTest.CheckPolicyIndicators
fails.  I am working on the UI on a separate CL: https://crrev.com/421653006.

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
File chrome/browser/policy/network_prediction_policy_handler.cc (right):

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:10: #include
"chrome/common/content_settings.h"
On 2014/07/30 07:57:32, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:66:
network_prediction_options->GetAsInteger(&setting);
On 2014/07/30 07:57:32, Joao da Silva wrote:
> This may fail, if the policy value isn't an integer. Suggestion:
> 
>   int setting;
>   if (network_prediction_options &&
> network_prediction_options->GetAsInteger(&settings) {
>     ...
>   }

Done.

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:80:
network_prediction_enabled->GetAsBoolean(&setting);
On 2014/07/30 07:57:32, Joao da Silva wrote:
> Same here

Done.

https://codereview.chromium.org/431493003/diff/1/chrome/browser/policy/networ...
chrome/browser/policy/network_prediction_policy_handler.cc:82: // Be
conservative here and set Options to WIFI_ONLY.
On 2014/07/30 07:57:32, Joao da Silva wrote:
> The legacy semantic was equivalent to ALWAYS and I think we should keep that.
Do
> you have a reason to prefer WIFI_ONLY here?

Good question, sorry it is not clear from comments.  Yes, there is a reason:
some of the activities that are now controlled by the "Predictive network
action" preference used to be hardcoded never to run on cellular network.  For
example, prefetch used to do that, see left side of 
https://codereview.chromium.org/357413004/diff/20001/chrome/browser/prefetch/...
for original.  It was a mistake that I removed that check there, and it is being
reinstated:
https://codereview.chromium.org/419833003/diff/40001/chrome/browser/prefetch/....

Once we deprecate the old Boolean setting, we'll get rid of all this logic, and
allow prefetch on cellular network, but only if the user or the policy
explicitly asks for it using the new preference.  Therefore I suggest that the
old policy gets translated to WIFI_ONLY.

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:123: #   For your editing
convenience: highest ID currently used: 270
On 2014/07/30 07:57:32, Joao da Silva wrote:
> Update this ID

Done.

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:340: 'caption': '''Preference
value has not been set''',
On 2014/07/30 07:57:32, Joao da Silva wrote:
> This value is not needed in the policy, since it's equivalent to not setting
the
> policy at all. Then the pref defaults to its default value, which is 3.

Done.

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:343: 'supported_on':
['chrome.*:8-', 'chrome_os:11-', 'android:30-'],
On 2014/07/30 07:57:32, Joao da Silva wrote:
> The version should be 38

Done.

What are the current/upcoming chrome_os and android versions?

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:349: 'example_value': True,
On 2014/07/30 07:57:32, Joao da Silva wrote:
> The example value should be one of the int values

Done.

[Joao da Silva, 2014-07-30 13:33:32]

lgtm after fixing the remaining comments

https://codereview.chromium.org/431493003/diff/1/chrome/test/data/policy/poli...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/431493003/diff/1/chrome/test/data/policy/poli...
chrome/test/data/policy/policy_test_cases.json:129: { "pref":
"net.network_prediction_options",
Setting this value to the old value should fix the test
("dns_prefetching.enabled").

This will test that setting the "NetworkPredictionOptions" policy shows an icon
in the settings indicating that "this is managed by your administrator" (you can
see a lot of managed settings in your browser if you use Chrome in a corp
workstation).

However, the option to "Use a prediction service to help complete searches etc
etc" option is still mapped to "dns_prefetching.enabled". The test fails because
it's trying to locate the management warning icon next to a control that maps to
this new pref, but none exists.

Setting this "pref" to the old one is the right fix because that's still what
the settings UI is using. Once you update that (see
chrome/browser/resources/options/browser_options.html) then this "pref" here
should be updated too.

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:343: 'supported_on':
['chrome.*:8-', 'chrome_os:11-', 'android:30-'],
On 2014/07/30 13:19:31, Bence wrote:
> On 2014/07/30 07:57:32, Joao da Silva wrote:
> > The version should be 38
> 
> Done.
> 
> What are the current/upcoming chrome_os and android versions?

Same version, they should all be 38.

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
File chrome/browser/policy/network_prediction_policy_handler.cc (right):

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
chrome/browser/policy/network_prediction_policy_handler.cc:63: int intSetting;
Other variables in this file use underscores, so rename this to int_setting for
consistency

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
chrome/browser/policy/network_prediction_policy_handler.cc:78: bool boolSetting;
bool_setting

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
chrome/browser/policy/network_prediction_policy_handler.cc:82: // Be
conservative here and set Options to WIFI_ONLY.
Thanks for explaining the rationale for this. Can you expand this comment to
include that, so that future reviewers of this file also understand that
decision?

https://codereview.chromium.org/431493003/diff/20001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/431493003/diff/20001/components/policy/resour...
components/policy/resources/policy_templates.json:338: 'supported_on':
['chrome.*:38-', 'chrome_os:11-', 'android:30-'],
The first version supported should be 38 on all platforms

[Bence, 2014-07-31 20:41:12]

Alexei: PTAL.

https://codereview.chromium.org/431493003/diff/1/chrome/test/data/policy/poli...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/431493003/diff/1/chrome/test/data/policy/poli...
chrome/test/data/policy/policy_test_cases.json:129: { "pref":
"net.network_prediction_options",
On 2014/07/30 13:33:32, Joao da Silva wrote:
> Setting this value to the old value should fix the test
> ("dns_prefetching.enabled").
> 
> This will test that setting the "NetworkPredictionOptions" policy shows an
icon
> in the settings indicating that "this is managed by your administrator" (you
can
> see a lot of managed settings in your browser if you use Chrome in a corp
> workstation).
> 
> However, the option to "Use a prediction service to help complete searches etc
> etc" option is still mapped to "dns_prefetching.enabled". The test fails
because
> it's trying to locate the management warning icon next to a control that maps
to
> this new pref, but none exists.
> 
> Setting this "pref" to the old one is the right fix because that's still what
> the settings UI is using. Once you update that (see
> chrome/browser/resources/options/browser_options.html) then this "pref" here
> should be updated too.

Done.

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/431493003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:343: 'supported_on':
['chrome.*:8-', 'chrome_os:11-', 'android:30-'],
On 2014/07/30 13:33:32, Joao da Silva wrote:
> On 2014/07/30 13:19:31, Bence wrote:
> > On 2014/07/30 07:57:32, Joao da Silva wrote:
> > > The version should be 38
> > 
> > Done.
> > 
> > What are the current/upcoming chrome_os and android versions?
> 
> Same version, they should all be 38.

Done.

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
File chrome/browser/policy/network_prediction_policy_handler.cc (right):

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
chrome/browser/policy/network_prediction_policy_handler.cc:63: int intSetting;
On 2014/07/30 13:33:32, Joao da Silva wrote:
> Other variables in this file use underscores, so rename this to int_setting
for
> consistency

Done.

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
chrome/browser/policy/network_prediction_policy_handler.cc:78: bool boolSetting;
On 2014/07/30 13:33:32, Joao da Silva wrote:
> bool_setting

Done.

https://codereview.chromium.org/431493003/diff/20001/chrome/browser/policy/ne...
chrome/browser/policy/network_prediction_policy_handler.cc:82: // Be
conservative here and set Options to WIFI_ONLY.
On 2014/07/30 13:33:32, Joao da Silva wrote:
> Thanks for explaining the rationale for this. Can you expand this comment to
> include that, so that future reviewers of this file also understand that
> decision?

Done.

https://codereview.chromium.org/431493003/diff/20001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/431493003/diff/20001/components/policy/resour...
components/policy/resources/policy_templates.json:338: 'supported_on':
['chrome.*:38-', 'chrome_os:11-', 'android:30-'],
On 2014/07/30 13:33:32, Joao da Silva wrote:
> The first version supported should be 38 on all platforms

Done.

Thanks.  I didn't realize version numbers are consistent accross platforms.

[Alexei Svitkine (slow), 2014-07-31 20:43:43]

histograms lgtm

[Bence, 2014-08-01 00:16:22]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-08-01 00:19:59]

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

[commit-bot: I haz the power, 2014-08-01 09:35:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 17:00:46]

Change committed as 287022