Implement new-style CloudPolicyStore for Chrome OS device policy.

Implement new-style CloudPolicyStore for Chrome OS device policy.

BUG=chromium:108928
TEST=Unit tests, new code is not active yet.
TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155703

[Mattias Nissler (ping if slow), 2012-08-28 18:38:10]

Hi guys, here is something to review for both of you :)

[Joao da Silva, 2012-08-28 19:56:57]

lgtm with nits

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/device_settings_service.h:161: void
Store(scoped_ptr<enterprise_management::PolicyFetchResponse> policy,
PolicyFetchResponse needs forward decl

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
File chrome/browser/chromeos/settings/session_manager_operation.h (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/session_manager_operation.h:144:
scoped_ptr<enterprise_management::PolicyFetchResponse> policy);
PolicyFetchResponse needs a forward declaration

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
File chrome/browser/policy/device_cloud_policy_store_chromeos.cc (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:8: #include
"chrome/browser/chromeos/settings/device_settings_service.h"
Already in .h

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:50:
validator.release()->StartValidation();
The scoped_ptr can be skipped for this block but I guess it's a matter of
preference :-)

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:77:
validator.release()->StartValidation();
Same here

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
File chrome/browser/policy/device_cloud_policy_store_chromeos.h (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.h:17: class
DeviceSettingsService;
.h included

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_cache.cc:9: #include <vector>
nit: vector and limits not used

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_cache.cc:106:
scoped_ptr<em::PolicyFetchResponse>(new em::PolicyFetchResponse(policy)),
#include "base/memory/scoped_ptr.h"

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
File chrome/browser/policy/device_policy_decoder_chromeos.cc (left):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_decoder_chromeos.cc:15: #include
"base/logging.h"
There's a LOG(WARNING)

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_decoder_chromeos.cc:17: #include
"base/values.h"
This is used

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_browser.gypi#new...
chrome/chrome_browser.gypi:4639: ['exclude',
'browser/policy/device_policy_cache.cc'],
?

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_tests.gypi#newco...
chrome/chrome_tests.gypi:1438:
'browser/policy/device_cloud_policy_store_chromeos_unittest.cc',
nit: order

[pastarmovj, 2012-08-29 11:39:49]

+1 (a.k.a. lgtm when you clean the tiny miny nits :) )

[Mattias Nissler (ping if slow), 2012-08-31 14:13:18]

Comments fixed.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/device_settings_service.h:161: void
Store(scoped_ptr<enterprise_management::PolicyFetchResponse> policy,
On 2012/08/28 19:56:57, Joao da Silva wrote:
> PolicyFetchResponse needs forward decl

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
File chrome/browser/chromeos/settings/session_manager_operation.h (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/session_manager_operation.h:144:
scoped_ptr<enterprise_management::PolicyFetchResponse> policy);
On 2012/08/28 19:56:57, Joao da Silva wrote:
> PolicyFetchResponse needs a forward declaration

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
File chrome/browser/policy/device_cloud_policy_store_chromeos.cc (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:8: #include
"chrome/browser/chromeos/settings/device_settings_service.h"
On 2012/08/28 19:56:57, Joao da Silva wrote:
> Already in .h

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:50:
validator.release()->StartValidation();
On 2012/08/28 19:56:57, Joao da Silva wrote:
> The scoped_ptr can be skipped for this block but I guess it's a matter of
> preference :-)

Returning the scoped_ptr from CreateValidator clearly conveys ownership
transfer, and this is actually consistent with what is done in
UserCloudPolicyStore.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:77:
validator.release()->StartValidation();
On 2012/08/28 19:56:57, Joao da Silva wrote:
> Same here

Same reply

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
File chrome/browser/policy/device_cloud_policy_store_chromeos.h (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_store_chromeos.h:17: class
DeviceSettingsService;
On 2012/08/28 19:56:57, Joao da Silva wrote:
> .h included

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_cache.cc:9: #include <vector>
On 2012/08/28 19:56:57, Joao da Silva wrote:
> nit: vector and limits not used

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_cache.cc:106:
scoped_ptr<em::PolicyFetchResponse>(new em::PolicyFetchResponse(policy)),
On 2012/08/28 19:56:57, Joao da Silva wrote:
> #include "base/memory/scoped_ptr.h"

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
File chrome/browser/policy/device_policy_decoder_chromeos.cc (left):

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_decoder_chromeos.cc:15: #include
"base/logging.h"
On 2012/08/28 19:56:57, Joao da Silva wrote:
> There's a LOG(WARNING)

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_decoder_chromeos.cc:17: #include
"base/values.h"
On 2012/08/28 19:56:57, Joao da Silva wrote:
> This is used

Done.

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_browser.gypi#new...
chrome/chrome_browser.gypi:4639: ['exclude',
'browser/policy/device_policy_cache.cc'],
On 2012/08/28 19:56:57, Joao da Silva wrote:
> ?

No idea :) Reverted.

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10885015/diff/1/chrome/chrome_tests.gypi#newco...
chrome/chrome_tests.gypi:1438:
'browser/policy/device_cloud_policy_store_chromeos_unittest.cc',
On 2012/08/28 19:56:57, Joao da Silva wrote:
> nit: order

Done.

[Joao da Silva, 2012-09-07 12:45:10]

What's the status of this CL? Do you need another round of reviews?

[Mattias Nissler (ping if slow), 2012-09-07 12:53:51]

On 2012/09/07 12:45:10, Joao da Silva wrote:
> What's the status of this CL? Do you need another round of reviews?

It hasn't really changed and is ready to go. I just waited to see whether things
would work out with the other CLs building on top of this one. All seems good,
so I'll submit this one.

[commit-bot: I haz the power, 2012-09-07 16:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10885015/25002

[commit-bot: I haz the power, 2012-09-07 16:09:16]

Presubmit check for 10885015-25002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.8s to calculate.

[Mattias Nissler (ping if slow), 2012-09-10 07:51:30]

+Ben to TBR for the gyp changes.

[commit-bot: I haz the power, 2012-09-10 07:51:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10885015/25002

[commit-bot: I haz the power, 2012-09-10 11:04:53]

Change committed as 155703