[Variations] Read the policy from profile prefs if on Android

[Variations] Read the policy from profile prefs if on Android

BUG=342839
TEST=Manually tested with a User Policy file running on an Android build.

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

[Joao da Silva, 2014-01-28 14:19:35]

This looks reasonable to, just a comment inline.

https://codereview.chromium.org/136003008/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/136003008/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:1487:
variations_service->set_policy_pref_service(profile_->GetPrefs());
The line below tests |variations_service|, should this be done after testing?

[Mathieu, 2014-01-28 14:24:46]

Thanks!

https://codereview.chromium.org/136003008/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/136003008/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:1487:
variations_service->set_policy_pref_service(profile_->GetPrefs());
On 2014/01/28 14:19:35, Joao da Silva wrote:
> The line below tests |variations_service|, should this be done after testing?

Sure. Done.

[Mathieu, 2014-02-10 22:02:55]

Hi,

I'm fairly confident this approach will work because I manually tested a User
Policy.

Please have a look at the CL.

Joao: Policy
asvitkine: Variations

[Alexei Svitkine (slow), 2014-02-10 22:12:47]

Looks good overall. Does this need a test, similar to the policy tests we have
on other platforms?

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1504: if (profile_ &&
profile_->GetPrefs())
Under what circumstances would these be NULL? If they can't (which is my guess),
then don't check for them.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:108: // Gets the
restrict parameter from |policy_pref_service_| or from Chrome OS
Nit: remove underscore after variable name, since it's talking about the param,
not the member variable.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:117: parameter =
policy_pref_service->
Nit: If this needs to wrap, wrap after the =

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:315:
registry->RegisterStringPref(
Can you add a comment about this? (that it's not actually a real pref, but
something injected by policy... same for the function above that registers it on
the local state)

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:80: void
set_policy_pref_service(PrefService* service) {
Add a comment.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:125: // end up being
Local State or Profile prefs.
Nit: "may end up being" -> "will be either"

[Mathieu, 2014-02-10 22:41:33]

Thanks!

Joao: How do you think this is best tested? I assume the policy team already
tests the path from Android user policy to prefs? If so, I could modify this
test to register profile prefs on Android:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...

What do you think?

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1504: if (profile_ &&
profile_->GetPrefs())
On 2014/02/10 22:12:48, Alexei Svitkine wrote:
> Under what circumstances would these be NULL? If they can't (which is my
guess),
> then don't check for them.

Done. You're right, I don't see other code checking for them here either.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:108: // Gets the
restrict parameter from |policy_pref_service_| or from Chrome OS
On 2014/02/10 22:12:48, Alexei Svitkine wrote:
> Nit: remove underscore after variable name, since it's talking about the
param,
> not the member variable.

Done.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:117: parameter =
policy_pref_service->
On 2014/02/10 22:12:48, Alexei Svitkine wrote:
> Nit: If this needs to wrap, wrap after the =

Done.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:315:
registry->RegisterStringPref(
On 2014/02/10 22:12:48, Alexei Svitkine wrote:
> Can you add a comment about this? (that it's not actually a real pref, but
> something injected by policy... same for the function above that registers it
on
> the local state)

Done.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:80: void
set_policy_pref_service(PrefService* service) {
On 2014/02/10 22:12:48, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/136003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:125: // end up being
Local State or Profile prefs.
On 2014/02/10 22:12:48, Alexei Svitkine wrote:
> Nit: "may end up being" -> "will be either"

Done.

[Joao da Silva, 2014-02-11 16:19:42]

lgtm, nice work!

I've manually verified that this is indeed working as intended.

As for tests: Android runs the unit_tests, so you can extend
VariationsServiceTest.VariationsURLIsValid with an OS_ANDROID variant that uses
a TestingProfile. We do already have unit_tests verifying that the policy gets
forwarded to the prefs.

I'm fine with landing the test in a separate CL if you want to make sure that
this makes it to M34.

[Joao da Silva, 2014-02-11 16:28:33]

Maybe this is why the tests are crashing, see inline.

https://codereview.chromium.org/136003008/diff/130001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/136003008/diff/130001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:190:
seed_store_(local_state),
You need to initialize policy_pref_service_ here

[Mathieu, 2014-02-11 18:02:46]

Thanks.

Fixed the test. Alexei, have a look?

https://codereview.chromium.org/136003008/diff/130001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/136003008/diff/130001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:190:
seed_store_(local_state),
On 2014/02/11 16:28:33, Joao da Silva wrote:
> You need to initialize policy_pref_service_ here

Done.

[Alexei Svitkine (slow), 2014-02-11 18:10:20]

LGTM. Please file a crbug and set the BUG= on the description.

https://codereview.chromium.org/136003008/diff/250001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/136003008/diff/250001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:82: void
set_policy_pref_service(PrefService* service) {
Add DCHECK() that the arg isn't NULL?

[Mathieu, 2014-02-11 18:19:47]

Thanks!

Both 1-line approvals below:
gab@: browser_prefs.cc
jam@: chrome_browser_main.cc

https://codereview.chromium.org/136003008/diff/250001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/136003008/diff/250001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:82: void
set_policy_pref_service(PrefService* service) {
On 2014/02/11 18:10:26, Alexei Svitkine wrote:
> Add DCHECK() that the arg isn't NULL?

Done.

[gab, 2014-02-11 20:31:46]

c/b/prefs lgtm

https://codereview.chromium.org/136003008/diff/300001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/136003008/diff/300001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:83: DCHECK(service !=
NULL);
nit: 

DCHECK(service);

is sufficient

[Mathieu, 2014-02-11 20:38:02]

Thanks

https://codereview.chromium.org/136003008/diff/300001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/136003008/diff/300001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:83: DCHECK(service !=
NULL);
On 2014/02/11 20:31:46, gab wrote:
> nit: 
> 
> DCHECK(service);
> 
> is sufficient

Done.

[Mathieu, 2014-02-12 16:56:39]

-jam (OOO), +sky

1 line change in chrome_browser_main needs approval, thanks!

[sky, 2014-02-12 18:21:00]

LGTM

[Mathieu, 2014-02-12 18:27:41]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-02-12 18:29:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/136003008/390001

[commit-bot: I haz the power, 2014-02-13 00:17:51]

Change committed as 250859