DeviceCloudPolicyStore should load consumer policies so that other classes may function normally.

DeviceCloudPolicyStore should load consumer policies so that other classes may
function normally.

Other classes that depend on DeviceCloudPolicyStore to work normally include
CloudPolicyManager, CloudPolicyService, and CloudPolicyInvalidator.

PolicyUI, VersionInfoUpdater, SystemTrayDelegateChromeOS, and
PolicyHeaderService also observe on DeviceCloudPolicyStore, but they don't
really use the policy from the store and can work either way.

BUG=chromium:353050
TEST=unit_tests

Committed: https://crrev.com/166c0f0be9bc0d02a6744cfcf2cdd1c073517703
Cr-Commit-Position: refs/heads/master@{#307022}

[davidyu, 2014-12-05 10:43:44]

Patchset #1 (id:1) has been deleted

[davidyu, 2014-12-05 10:44:14]

Patchset #1 (id:20001) has been deleted

[davidyu, 2014-12-05 10:45:07]

davidyu@chromium.org changed reviewers:
+ bartfab@chromium.org, mnissler@chromium.org

[Mattias Nissler (ping if slow), 2014-12-05 12:28:37]

https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:151:
policy_.reset(new em::PolicyData());
Why can't you just copy device_settings_service_->policy_data?

https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:154:
policy_map_.Swap(&new_policy_map);
Instead of swapping in an empty map, you could just call policy_map_.Clear().

https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc
(right):

https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc:82:
device_settings_service_.Store(policy.Pass(), base::Bind(&base::DoNothing));
If the intention here is to test a load (and the function name suggests that),
then this shouldn't call Store(), but just stage the policy to be loaded, i.e.
  device_settings_test_helper_.set_policy_blob(device_policy_.GetBlob());
device_settings_service_.Load();
FlushDeviceSettings();

[Mattias Nissler (ping if slow), 2014-12-05 12:30:05]

On 2014/12/05 12:28:37, Mattias Nissler wrote:
>
https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
> (right):
> 
>
https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:151:
> policy_.reset(new em::PolicyData());
> Why can't you just copy device_settings_service_->policy_data?
> 
>
https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:154:
> policy_map_.Swap(&new_policy_map);
> Instead of swapping in an empty map, you could just call policy_map_.Clear().
> 
>
https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
> File
> chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
>
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc:82:
> device_settings_service_.Store(policy.Pass(), base::Bind(&base::DoNothing));
> If the intention here is to test a load (and the function name suggests that),
> then this shouldn't call Store(), but just stage the policy to be loaded, i.e.
>   device_settings_test_helper_.set_policy_blob(device_policy_.GetBlob());
> device_settings_service_.Load();
> FlushDeviceSettings();

One more thing: I assume you'll be updating chrome://policy to indicate whether
a device is consumer-managed or enterprise managed?

[davidyu, 2014-12-05 13:54:10]

Updated the CL according to our meeting. The policy blob is now loaded, but
PolicyMap is empty for consumer-managed devices.

https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc
(right):

https://codereview.chromium.org/782483002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc:82:
device_settings_service_.Store(policy.Pass(), base::Bind(&base::DoNothing));
On 2014/12/05 12:28:37, Mattias Nissler wrote:
> If the intention here is to test a load (and the function name suggests that),
> then this shouldn't call Store(), but just stage the policy to be loaded, i.e.
>   device_settings_test_helper_.set_policy_blob(device_policy_.GetBlob());
> device_settings_service_.Load();
> FlushDeviceSettings();

Done.

[Mattias Nissler (ping if slow), 2014-12-05 14:02:34]

LGTM (pending green try jobs).

https://codereview.chromium.org/782483002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/782483002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:164: // For
enterprise devices, Once per session, validate internal consistency of
nit: once (lowercase)

[davidyu, 2014-12-05 14:07:41]

The CQ bit was checked by davidyu@chromium.org

[davidyu, 2014-12-05 14:07:56]

The CQ bit was unchecked by davidyu@chromium.org

[davidyu, 2014-12-05 14:08:41]

https://codereview.chromium.org/782483002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/782483002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:164: // For
enterprise devices, Once per session, validate internal consistency of
On 2014/12/05 14:02:34, Mattias Nissler wrote:
> nit: once (lowercase)

Done.

[davidyu, 2014-12-05 14:08:49]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-12-05 14:09:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/782483002/80001

[commit-bot: I haz the power, 2014-12-05 14:50:12]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2014-12-05 14:50:50]

Patchset 3 (id:??) landed as
https://crrev.com/166c0f0be9bc0d02a6744cfcf2cdd1c073517703
Cr-Commit-Position: refs/heads/master@{#307022}