Update "Predict network actions" UI setting.

Update "Predict network actions" UI setting.

* Update chrome://settings Privacy section to change int kNetworkPredictionOptions
instead of bool kNetworkPredictionEnabled.
* Introduce code to migrate user setting (and only user setting).
* Per UI review, keep UI unchanged.
* Note that policy handler for these two preferences has already landed, see https://crrev.com/431493003.

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

[Bence, 2014-07-30 02:54:08]

PTAL at the following files.  Thank you.

Ilya: tools/metrics/actions/actions.xml
Yoyo: chrome/browser/extensions/api/preference/preference_api.cc
Steven: chrome/browser/{resources,ui}/*

[stevenjb, 2014-07-30 18:03:29]

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.html:363: </div -->
Elim commented out code before committing.

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:14: UNSET: 3
Unused?

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:369: var value =
event.target.options[event.target.selectedIndex].value;
nit: assert value <= NEVER

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:1905:
'setNetworkPredictionVisibility',
Do we really need two API calls for this? It looks like we always call both?

https://codereview.chromium.org/421653006/diff/1/tools/metrics/actions/action...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/421653006/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:8747: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
Please list yourself as the owner of these metrics.

[Ilya Sherman, 2014-07-30 20:35:16]

https://codereview.chromium.org/421653006/diff/1/tools/metrics/actions/action...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/421653006/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:8748: <description>Please enter the
description of the metric.</description>
And please add descriptions for the metrics.

[Yoyo Zhou, 2014-07-30 20:35:54]

preference_api rubber stamp LGTM

[Bence, 2014-08-01 14:36:47]

Thank you all for your great suggestions.  The UI folks requested that we do not
implement the dropdown menu, see relevant thread on chrome-ui-review.  I
modified the CL accordingly.

FYI I will deal with fixing the failing PolicyPrefIndicatorTest browsertest as
soon as the related CL lands: https://crrev.com/431493003.

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.html:363: </div -->
On 2014/07/30 18:03:28, stevenjb wrote:
> Elim commented out code before committing.

Moot:  UI folks told us to get rid of dropdown menu entirely.

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:14: UNSET: 3
On 2014/07/30 18:03:29, stevenjb wrote:
> Unused?

Done.

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:369: var value =
event.target.options[event.target.selectedIndex].value;
On 2014/07/30 18:03:29, stevenjb wrote:
> nit: assert value <= NEVER

Moot: this function is removed as a result of UI review.

https://codereview.chromium.org/421653006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:1905:
'setNetworkPredictionVisibility',
On 2014/07/30 18:03:29, stevenjb wrote:
> Do we really need two API calls for this? It looks like we always call both?

Moot: one function is removed as the result of UI review.  FYI
setNetworkPredictionVisibility was only called when the Settings page was
displayed, setNetworkPredictionValue is called upon every value change.

https://codereview.chromium.org/421653006/diff/1/tools/metrics/actions/action...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/421653006/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:8747: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
On 2014/07/30 18:03:29, stevenjb wrote:
> Please list yourself as the owner of these metrics.

Moot: removed per UI review.

https://codereview.chromium.org/421653006/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:8748: <description>Please enter the
description of the metric.</description>
On 2014/07/30 20:35:16, Ilya Sherman wrote:
> And please add descriptions for the metrics.

Moot: removed per UI review.

[Bence, 2014-08-01 18:41:36]

Joao: please review policy_prefs_browsertest.cc.
Ilya, Steven: PTAL.

[Ilya Sherman, 2014-08-01 18:43:43]

On 2014/08/01 18:41:36, Bence wrote:
> Joao: please review policy_prefs_browsertest.cc.
> Ilya, Steven: PTAL.

What did you want me to take a look at?

[Bence, 2014-08-01 18:46:30]

On 2014/08/01 18:43:43, Ilya Sherman wrote:
> On 2014/08/01 18:41:36, Bence wrote:
> > Joao: please review policy_prefs_browsertest.cc.
> > Ilya, Steven: PTAL.
> 
> What did you want me to take a look at?

Oops, sorry about that.  I reverted that change, so there's nothing for you to
do.  My apologies.

[Joao da Silva, 2014-08-01 21:06:18]

+bartfab who originally wrote that test

policy_prefs_browsertest.cc lgtm given that the "fix" is meant to be removed :-)

[bartfab (slow), 2014-08-04 16:44:13]

https://codereview.chromium.org/421653006/diff/60001/chrome/browser/policy/po...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/421653006/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_prefs_browsertest.cc:727: //
kNetworkPredictionOptions, setting a user value.
User values should be set by the user. The migration you described sounds
dangerous. It means that policy-supplied values end up getting stored as if they
had been set by the user. Here are some of the problems that can lead to:

* Any controlled setting indicator keyed by the pref will not show up the
enterprise icon, because the value will look like it was set by the user, not by
policy.
* The user will be able to change the policy-supplied value.
* How about the following scenario: The admin sets the policy to a value now,
the migration code copies this to a user value. Later, after the migration code
has been removed, the admin unsets the policy value. Since there is no migration
code anymore, the value that had been copied stays.

[Bence, 2014-08-04 20:35:33]

Bartosz:  This is an excellent point you bring up about setting user
preferences.  I modified BrowserOptionsHandler::MigrateNetworkPredictionOptions
such that it only sets the new user preference if the old one was actually set
by the user.  This makes the hack unnecessary for the tests.  Please feel free
to take a look at the change, but note that your lgtm is no longer required. 
Thank you for your observation.

https://codereview.chromium.org/421653006/diff/60001/chrome/browser/policy/po...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/421653006/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_prefs_browsertest.cc:727: //
kNetworkPredictionOptions, setting a user value.
On 2014/08/04 16:44:13, bartfab wrote:
> User values should be set by the user. The migration you described sounds
> dangerous. It means that policy-supplied values end up getting stored as if
they
> had been set by the user. Here are some of the problems that can lead to:
> 
> * Any controlled setting indicator keyed by the pref will not show up the
> enterprise icon, because the value will look like it was set by the user, not
by
> policy.
> * The user will be able to change the policy-supplied value.
> * How about the following scenario: The admin sets the policy to a value now,
> the migration code copies this to a user value. Later, after the migration
code
> has been removed, the admin unsets the policy value. Since there is no
migration
> code anymore, the value that had been copied stays.

Done.

[stevenjb, 2014-08-05 15:51:56]

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1517: checkbox.checked =
!(NetworkPredictionOptions.DEFAULT ==
DEFAULT != NEVER

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1520: checkbox.checked =
!(pref.value == NetworkPredictionOptions.NEVER);
value != NEVER

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1665:
pref_service->FindPreference(prefs::kNetworkPredictionEnabled)
nit: Use a local to avoid extra lookup. Also maybe use early exits to simplify
code, e.g. if (..GetInteger... != UNSET) return;

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1693:
pref_service->GetInteger(prefs::kNetworkPredictionOptions));
Use network_prediction_options->GetValue()->GetAsInteger() instead of doing an
extra lookup here.

[bartfab (slow), 2014-08-05 17:27:08]

On 2014/08/04 20:35:33, Bence wrote:
> Bartosz:  This is an excellent point you bring up about setting user
> preferences.  I modified
BrowserOptionsHandler::MigrateNetworkPredictionOptions
> such that it only sets the new user preference if the old one was actually set
> by the user.  This makes the hack unnecessary for the tests.  Please feel free
> to take a look at the change, but note that your lgtm is no longer required. 
> Thank you for your observation.

Thanks for looking into this. I am not sure your new solution covers all the
cases quite yet. Is your goal to migrate users settings from old pref to new
pref? If so, you are on the right track - you should make sure to only copy
*user* values though (the value that GetBoolean() returns may be coming from
policy, even if HasUserSetting() is |true|, because policy shadows any user
values). Or is your goal to (also) make sure that when the policy is set, two
different prefs change their values. In that case, the right place to make
changes is onfiguration_policy_handler_list_factory.cc, which is the file that
translates policies to prefs. The simple handlers translate 1:1, from a single
policy to a single pref. But you can have more complex handlers that affect
multiple prefs.

> 
>
https://codereview.chromium.org/421653006/diff/60001/chrome/browser/policy/po...
> File chrome/browser/policy/policy_prefs_browsertest.cc (right):
> 
>
https://codereview.chromium.org/421653006/diff/60001/chrome/browser/policy/po...
> chrome/browser/policy/policy_prefs_browsertest.cc:727: //
> kNetworkPredictionOptions, setting a user value.
> On 2014/08/04 16:44:13, bartfab wrote:
> > User values should be set by the user. The migration you described sounds
> > dangerous. It means that policy-supplied values end up getting stored as if
> they
> > had been set by the user. Here are some of the problems that can lead to:
> > 
> > * Any controlled setting indicator keyed by the pref will not show up the
> > enterprise icon, because the value will look like it was set by the user,
not
> by
> > policy.
> > * The user will be able to change the policy-supplied value.
> > * How about the following scenario: The admin sets the policy to a value
now,
> > the migration code copies this to a user value. Later, after the migration
> code
> > has been removed, the admin unsets the policy value. Since there is no
> migration
> > code anymore, the value that had been copied stays.
> 
> Done.

[Bence, 2014-08-06 19:38:17]

battre:  Please review base/prefs/*, chrome/browser/net/prediction_options.*,
and chrome/browser/prefs/browser_prefs.cc.

bartfab:  Thank you for your excellent point.  I would want to migrate the user
preference only.  That is, set a user value for the new preference if and only
if there is a user provided value for the old preference and no user value for
the new preference.  I couldn't find existing accessors for this, so I wrote
them in base/prefs/.

FYI, relevant policies were already changed in a separate CL that has landed. 
In particular, the old policy still works without any change required on the
policy administrator side.  Also, a new policy has been added.  See
https://crrev.com/431493003.

Note that I moved calling of the migration code to
chrome/browser/prefs/browser_prefs.cc.  The reason is that this gets called upon
browser startup, whereas BrowserOptionsHandler::InitializePage() only gets
executed when the user opens the Settings page.

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1517: checkbox.checked =
!(NetworkPredictionOptions.DEFAULT ==
On 2014/08/05 15:51:56, stevenjb wrote:
> DEFAULT != NEVER
> 

Done.

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1520: checkbox.checked =
!(pref.value == NetworkPredictionOptions.NEVER);
On 2014/08/05 15:51:56, stevenjb wrote:
> value != NEVER

Done.

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1665:
pref_service->FindPreference(prefs::kNetworkPredictionEnabled)
On 2014/08/05 15:51:56, stevenjb wrote:
> nit: Use a local to avoid extra lookup. Also maybe use early exits to simplify
> code, e.g. if (..GetInteger... != UNSET) return;
> 

Done.

https://codereview.chromium.org/421653006/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1693:
pref_service->GetInteger(prefs::kNetworkPredictionOptions));
On 2014/08/05 15:51:56, stevenjb wrote:
> Use network_prediction_options->GetValue()->GetAsInteger() instead of doing an
> extra lookup here.

Done.

[stevenjb, 2014-08-06 23:18:05]

browser_options* lgtm

[battre, 2014-08-07 07:56:52]

Bernhard, do you see a way to do the migration without opening up the
PrefService? As I don't, the LGTM to me.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
File chrome/browser/net/prediction_options.cc (right):

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
chrome/browser/net/prediction_options.cc:67: bool value;
please initialize.

[Bernhard Bauer, 2014-08-07 08:32:18]

There is already PrefService::GetUserPrefValue(). We don't need to add a new
method to Preference.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
File chrome/browser/net/prediction_options.cc (right):

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
chrome/browser/net/prediction_options.cc:56: if
(network_prediction_options->HasUserSetting())
You could directly use pref_service->IsUserModifiablePreference(...).

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1676:
DCHECK(network_prediction_options->GetValue()->GetAsInteger(&value));
You could use pref_service->GetInteger(...) here.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1680:
dict.SetBoolean("disabled", !network_prediction_options->IsUserModifiable());
And pref_service->IsUserModifiablePreference(...) here.

[bartfab (slow), 2014-08-07 12:17:06]

Thanks for clarifying. The migration logic looks correct to me now.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
File chrome/browser/net/prediction_options.cc (right):

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
chrome/browser/net/prediction_options.cc:56: if
(network_prediction_options->HasUserSetting())
On 2014/08/07 08:32:17, Bernhard Bauer wrote:
> You could directly use pref_service->IsUserModifiablePreference(...).

HasUserSetting() and IsUserModifiablePreference() are not the same thing at all.
The former tells you whether there is a user-set value, even if that value may
be currently shadowed by policy. The latter tells you whether policy is
currently in charge, irrespective of whether there is a user-set value or not.

The current code is correct - if there is a user-set value already, do nothing.

[Bernhard Bauer, 2014-08-07 12:24:12]

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
File chrome/browser/net/prediction_options.cc (right):

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
chrome/browser/net/prediction_options.cc:56: if
(network_prediction_options->HasUserSetting())
On 2014/08/07 12:17:06, bartfab wrote:
> On 2014/08/07 08:32:17, Bernhard Bauer wrote:
> > You could directly use pref_service->IsUserModifiablePreference(...).
> 
> HasUserSetting() and IsUserModifiablePreference() are not the same thing at
all.
> The former tells you whether there is a user-set value, even if that value may
> be currently shadowed by policy. The latter tells you whether policy is
> currently in charge, irrespective of whether there is a user-set value or not.
> 
> The current code is correct - if there is a user-set value already, do
nothing.

Urr, I meant pref_service->GetUserPrefValue().

[Bence, 2014-08-07 13:50:02]

battre and/or Bernhard: PTAL.

Bernhard: thank you for bringing PrefService::GetUserPrefValue to my attention. 
I reverted my changes in base/prefs accordingly.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
File chrome/browser/net/prediction_options.cc (right):

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
chrome/browser/net/prediction_options.cc:56: if
(network_prediction_options->HasUserSetting())
On 2014/08/07 12:24:12, Bernhard Bauer wrote:
> On 2014/08/07 12:17:06, bartfab wrote:
> > On 2014/08/07 08:32:17, Bernhard Bauer wrote:
> > > You could directly use pref_service->IsUserModifiablePreference(...).
> > 
> > HasUserSetting() and IsUserModifiablePreference() are not the same thing at
> all.
> > The former tells you whether there is a user-set value, even if that value
may
> > be currently shadowed by policy. The latter tells you whether policy is
> > currently in charge, irrespective of whether there is a user-set value or
not.
> > 
> > The current code is correct - if there is a user-set value already, do
> nothing.
> 
> Urr, I meant pref_service->GetUserPrefValue().

Done.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/net/pred...
chrome/browser/net/prediction_options.cc:67: bool value;
On 2014/08/07 07:56:52, battre wrote:
> please initialize.

Done.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1676:
DCHECK(network_prediction_options->GetValue()->GetAsInteger(&value));
On 2014/08/07 08:32:18, Bernhard Bauer wrote:
> You could use pref_service->GetInteger(...) here.

Done.

https://codereview.chromium.org/421653006/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1680:
dict.SetBoolean("disabled", !network_prediction_options->IsUserModifiable());
On 2014/08/07 08:32:18, Bernhard Bauer wrote:
> And pref_service->IsUserModifiablePreference(...) here.

Done.

[Bernhard Bauer, 2014-08-07 13:52:26]

Yay! LGTM

[battre, 2014-08-08 07:20:06]

chrome/browser/net/prediction_options.* LGTM as well

[Bence, 2014-08-08 12:18:33]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-08-08 12:18:52]

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

[commit-bot: I haz the power, 2014-08-08 14:11:16]

Change committed as 288352