Move pref backing up flags from local state to device settings on ChromeOS.

Move pref backing up flags from local state to device settings on ChromeOS.

BUG=chromium-os:39250
TEST=Existing unit and browser tests should pass.


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

[pastarmovj, 2013-03-05 17:17:55]

@Joao: Please review this CL

@James: Please do OWNER review for chrome/browser/ui/webui.

Thanks!

[Joao da Silva, 2013-03-05 17:46:30]

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1312: // storage on ChormeOS.
*ChromeOS

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1317: experiments_list->Clear();
This is unnecessary.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1329: // in a set. On ChormeOS |prefs| can be NULL
when settings machine level flags.
*ChromeOS, *setting

This function gets flags, doesn't set them.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1359: // Takes a set of enabled lab experiments.
On ChormeOS |prefs| can be NULL when
*ChromeOS

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1360: // settings machine level flags.
*setting

https://codereview.chromium.org/12452003/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider.cc:311:
base::ListValue& flags = static_cast<base::ListValue&>(*value);
Use value->GetAsList (same for kAccountsPrefUsers above)

https://codereview.chromium.org/12452003/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider.cc:682:
AttemptMigration();
What happens if that fails (i.e. there is no private owner key)?

I guess that should return a boolean status, and local_state should only be
cleared if the migration succeeded (same for the UMA sample).

https://codereview.chromium.org/12452003/diff/1/chrome/browser/policy/proto/c...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/policy/proto/c...
chrome/browser/policy/proto/chrome_device_policy.proto:277: optional
StartUpFlagsProto start_up_flags = 23;
This should be added to policy_templates.json too.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/ui/webui/flags...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/ui/webui/flags...
chrome/browser/ui/webui/flags_ui.cc:222: web_ui()->AddMessageHandler(new
FlagsDOMHandler(NULL));
Tracking that |prefs| variable around about_flags.cc isn't easy, and it's not
immediately clear that the NULL won't be used. It's very easy to extend one of
those functions in the future and just dereference the |prefs| argument, and
boom.

Maybe not in this CL, but about_flags.cc could be refactored to use an abstract
interface to read/set flags which is passed from here; the default impl uses a
PrefService, and on ChromeOS it uses another impl backed by CrosSettings. I
think that'd be much cleaner.

[pastarmovj, 2013-03-06 17:28:31]

Hey PTAL when you have time! Thanks!

(Had to re-base to newer trunk version so diffing to the original review version
might be not so useful. Sorry about that)

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1312: // storage on ChormeOS.
On 2013/03/05 17:46:30, Joao da Silva wrote:
> *ChromeOS

Done.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1317: experiments_list->Clear();
On 2013/03/05 17:46:30, Joao da Silva wrote:
> This is unnecessary.

Good catch of typical copy/paste error ;)

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1329: // in a set. On ChormeOS |prefs| can be NULL
when settings machine level flags.
On 2013/03/05 17:46:30, Joao da Silva wrote:
> *ChromeOS, *setting
> 
> This function gets flags, doesn't set them.

Done.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1359: // Takes a set of enabled lab experiments.
On ChormeOS |prefs| can be NULL when
On 2013/03/05 17:46:30, Joao da Silva wrote:
> *ChromeOS

Done.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1360: // settings machine level flags.
On 2013/03/05 17:46:30, Joao da Silva wrote:
> *setting

Done.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider.cc:311:
base::ListValue& flags = static_cast<base::ListValue&>(*value);
On 2013/03/05 17:46:30, Joao da Silva wrote:
> Use value->GetAsList (same for kAccountsPrefUsers above)

Done.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider.cc:682:
AttemptMigration();
On 2013/03/05 17:46:30, Joao da Silva wrote:
> What happens if that fails (i.e. there is no private owner key)?
> 
> I guess that should return a boolean status, and local_state should only be
> cleared if the migration succeeded (same for the UMA sample).

I changed the logic to avoid clearing the pref before the device setting got
set.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/policy/proto/c...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/policy/proto/c...
chrome/browser/policy/proto/chrome_device_policy.proto:277: optional
StartUpFlagsProto start_up_flags = 23;
On 2013/03/05 17:46:30, Joao da Silva wrote:
> This should be added to policy_templates.json too.

Done.

https://codereview.chromium.org/12452003/diff/1/chrome/browser/ui/webui/flags...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12452003/diff/1/chrome/browser/ui/webui/flags...
chrome/browser/ui/webui/flags_ui.cc:222: web_ui()->AddMessageHandler(new
FlagsDOMHandler(NULL));
On 2013/03/05 17:46:30, Joao da Silva wrote:
> Tracking that |prefs| variable around about_flags.cc isn't easy, and it's not
> immediately clear that the NULL won't be used. It's very easy to extend one of
> those functions in the future and just dereference the |prefs| argument, and
> boom.
> 
> Maybe not in this CL, but about_flags.cc could be refactored to use an
abstract
> interface to read/set flags which is passed from here; the default impl uses a
> PrefService, and on ChromeOS it uses another impl backed by CrosSettings. I
> think that'd be much cleaner.

Ack. I'd rather keep this work for a later CL.

[Joao da Silva, 2013-03-07 07:50:07]

I guess I'm cool with this change. 2 things missing:

- decode the new device policy in policy/device_policy_decoder_chromeos.cc so
that is shows up in about:policy

- strongly suggest adding a test in policy_browsertest.cc for this. The test can
use the stub cros settings provider (needs to insert the command line flag for
that), set the device policy, restart the test (using a PRE_ prefix for the 1st
test), and then verify that the relaunched browser has the flags configured in
the policy.

- after doing that test, it should be easy to also test migration. Just set the
flags in local_state, then restart the test (PRE_), and check that they were
migrated; then restart again (PRE_PRE_), and check that local_state was cleared.

https://codereview.chromium.org/12452003/diff/16001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12452003/diff/16001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:313:
value->GetAsList(&flags);
This needs to be tested (same for the whitelist above):

if (value->GetAsList(&flags)) { ... }

https://codereview.chromium.org/12452003/diff/16001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:683:
AttemptMigration();
indent

[pastarmovj, 2013-03-07 13:40:59]

Thanks for the review!

I added the link in the device policy decoder as requested.

Writing the test is gonna be a bit more involved than that and I would like to
track progress on that separately. Here is the bug I already put for that:
https://code.google.com/p/chromium/issues/detail?id=180228.

https://codereview.chromium.org/12452003/diff/16001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12452003/diff/16001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:313:
value->GetAsList(&flags);
On 2013/03/07 07:50:07, Joao da Silva wrote:
> This needs to be tested (same for the whitelist above):
> 
> if (value->GetAsList(&flags)) { ... }

Done.

https://codereview.chromium.org/12452003/diff/16001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:683:
AttemptMigration();
On 2013/03/07 07:50:07, Joao da Silva wrote:
> indent

Done.

[Joao da Silva, 2013-03-07 13:46:21]

lgtm

[pastarmovj, 2013-03-07 15:52:32]

@James: Can I have OWNER's approval for chrome/browser/ui/webui/flags_ui.cc
please :)

[James Hawkins, 2013-03-07 18:32:52]

lgtm

[commit-bot: I haz the power, 2013-03-07 18:57:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/12452003/21001

[commit-bot: I haz the power, 2013-03-08 01:18:06]

Change committed as 186837