Apply default policy values also on non-ChromeOS

Apply default policy values also on non-ChromeOS

BUG=640950

[Marc Treib, 2016-08-25 14:37:19]

Description was changed from

==========
Apply default policy values also on non-ChromeOS

BUG=
==========

to

==========
Apply default policy values also on non-ChromeOS

BUG=595713
==========

[Marc Treib, 2016-08-25 15:32:54]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 15:33:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2016-08-25 15:34:41]

treib@chromium.org changed reviewers:
+ atwilson@chromium.org

[Marc Treib, 2016-08-25 15:34:42]

PTAL - is this what you had in mind?
Thanks!

[Marc Treib, 2016-08-25 15:48:00]

Description was changed from

==========
Apply default policy values also on non-ChromeOS

BUG=595713
==========

to

==========
Apply default policy values also on non-ChromeOS

BUG=640950
==========

[Marc Treib, 2016-08-25 15:52:07]

https://codereview.chromium.org/2275963004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/2275963004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.cc:116:
SetEnterpriseUsersDefaults(policy_map);
This doesn't quite match the "spec" in the bug: I've moved it to
CloudPolicyManager rather than UserCloudPolicyManager, because the latter is
used only on non-ChromeOS platforms as far as I can tell.

[commit-bot: I haz the power, 2016-08-25 16:11:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 16:11:40]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Andrew T Wilson (Slow), 2016-08-26 07:56:50]

I think your test failures will go away if you move the GetChromePolicy()
override down to UCPM + UCPMChromeOS instead of in the base CloudPolicyManager
class (since CloudPolicyManager is used for device policy also which should not
have enterprise overrides).

https://codereview.chromium.org/2275963004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/2275963004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.cc:116:
SetEnterpriseUsersDefaults(policy_map);
On 2016/08/25 15:52:07, Marc Treib wrote:
> This doesn't quite match the "spec" in the bug: I've moved it to
> CloudPolicyManager rather than UserCloudPolicyManager, because the latter is
> used only on non-ChromeOS platforms as far as I can tell.

I'm a bit concerned about this because it will now populate those policies in
device policy, when we really only want them populated in user policy.

I think it really does belong in UserCloudPolicyManager, and it's gross that
UserCloudPolicyManagerChromeOS doesn't derive from UserCloudPolicyManager, but
that just means you'll need to put it in two places (both UCPM and
UCPMChromeOS). On the bright side, it makes this patch smaller :D

[Marc Treib, 2016-08-26 08:41:25]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 08:41:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 09:12:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 09:12:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marc Treib, 2016-08-26 09:19:19]

Thanks! I moved it into the two UCPMs, now the tests seem to be happy, but
there's still a linker error on Windows... I'll investigate.

https://codereview.chromium.org/2275963004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/2275963004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.cc:116:
SetEnterpriseUsersDefaults(policy_map);
On 2016/08/26 07:56:50, Andrew T Wilson (Slow) wrote:
> On 2016/08/25 15:52:07, Marc Treib wrote:
> > This doesn't quite match the "spec" in the bug: I've moved it to
> > CloudPolicyManager rather than UserCloudPolicyManager, because the latter is
> > used only on non-ChromeOS platforms as far as I can tell.
> 
> I'm a bit concerned about this because it will now populate those policies in
> device policy, when we really only want them populated in user policy.
> 
> I think it really does belong in UserCloudPolicyManager, and it's gross that
> UserCloudPolicyManagerChromeOS doesn't derive from UserCloudPolicyManager, but
> that just means you'll need to put it in two places (both UCPM and
> UCPMChromeOS). On the bright side, it makes this patch smaller :D

Yup, this code duplication is what I was trying to avoid... oh well. It's now in
both UserCloudPolicyManager[ChromeOS].