Break out base functionality of UserCloudPolicyManager.

Break out base functionality of UserCloudPolicyManager.

This change introduces a new base class, CloudPolicyManager, that
contains the initialization/shutdown and policy serving logic which will
be common to user-level and Chrome OS device-level policy.

BUG=chromium:108928
TEST=unit tests
TBR=ben@chromium.org


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

[Mattias Nissler (ping if slow), 2012-09-06 19:23:19]

Another review coming here :)

[Joao da Silva, 2012-09-07 12:44:13]

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_manager.cc (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.cc:12: #include
"chrome/browser/policy/device_management_service.h"
nit: not used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.cc:22: store_->AddObserver(this);
Swap the order of these 2, since this assumes Load() doesn't trigger synchronous
notifications.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_manager.h (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.h:23: class DeviceManagementService;
nit: not used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.h:66: // Starts a refresh scheduler
in case none is running yet. |local_stat| must
nit: |local_state|

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service.h (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service.h:14: #include
"base/memory/scoped_ptr.h"
nit: not used anymore

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
File chrome/browser/policy/user_cloud_policy_manager.cc (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:7: #include <string>
nit: should be in .h

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:9: #include
"base/command_line.h"
nit: not used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:14: #include
"chrome/browser/policy/cloud_policy_service.h"
Nit: these 4 aren't used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:17: #include
"chrome/common/chrome_switches.h"
Nit: these 2 aren't used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:24: #endif
nit: these 3 aren't used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:40: #endif
These constants aren't used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:116: return
cloud_policy_client()->is_registered();
The first line should test cloud_policy_client().

Suggestion:

return cloud_policy_client() && cloud_policy_client()->is_registered();

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:120:
DCHECK(cloud_policy_service()) << "Callers must invoke Initialize() first";
DCHECK(cloud_policy_client())

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:138: CloudPolicyClient*
client) {
DCHECK_EQ(cloud_policy_client(), client) ?

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.h:21: class CloudPolicyService;
nit: these 2 are not used

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.h:36: // Creates a
UserCloudPolicyService instance associated with the passed
Creates a UserCloudPolicyManager?

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.h:64: void RegisterClient(const
std::string& access_token);
#include <string>

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
File chrome/browser/policy/user_cloud_policy_manager_unittest.cc (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager_unittest.cc:12: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
nit: first include

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

Ready for another round, the try bots should be much greener as well.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_manager.cc (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.cc:12: #include
"chrome/browser/policy/device_management_service.h"
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: not used

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.cc:22: store_->AddObserver(this);
On 2012/09/07 12:44:13, Joao da Silva wrote:
> Swap the order of these 2, since this assumes Load() doesn't trigger
synchronous
> notifications.

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_manager.h (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.h:23: class DeviceManagementService;
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: not used

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_manager.h:66: // Starts a refresh scheduler
in case none is running yet. |local_stat| must
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: |local_state|

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service.h (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service.h:14: #include
"base/memory/scoped_ptr.h"
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: not used anymore

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
File chrome/browser/policy/user_cloud_policy_manager.cc (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:7: #include <string>
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: should be in .h

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:9: #include
"base/command_line.h"
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: not used

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:14: #include
"chrome/browser/policy/cloud_policy_service.h"
On 2012/09/07 12:44:13, Joao da Silva wrote:
> Nit: these 4 aren't used

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:17: #include
"chrome/common/chrome_switches.h"
On 2012/09/07 12:44:13, Joao da Silva wrote:
> Nit: these 2 aren't used

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:24: #endif
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: these 3 aren't used

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:40: #endif
On 2012/09/07 12:44:13, Joao da Silva wrote:
> These constants aren't used

True. We probably missed removing all this cruft when landing the CL Drew took
over from me.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:116: return
cloud_policy_client()->is_registered();
On 2012/09/07 12:44:13, Joao da Silva wrote:
> The first line should test cloud_policy_client().
> 
> Suggestion:
> 
> return cloud_policy_client() && cloud_policy_client()->is_registered();

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:120:
DCHECK(cloud_policy_service()) << "Callers must invoke Initialize() first";
On 2012/09/07 12:44:13, Joao da Silva wrote:
> DCHECK(cloud_policy_client())

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.cc:138: CloudPolicyClient*
client) {
On 2012/09/07 12:44:13, Joao da Silva wrote:
> DCHECK_EQ(cloud_policy_client(), client) ?

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.h:21: class CloudPolicyService;
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: these 2 are not used

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.h:36: // Creates a
UserCloudPolicyService instance associated with the passed
On 2012/09/07 12:44:13, Joao da Silva wrote:
> Creates a UserCloudPolicyManager?

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager.h:64: void RegisterClient(const
std::string& access_token);
On 2012/09/07 12:44:13, Joao da Silva wrote:
> #include <string>

Done.

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
File chrome/browser/policy/user_cloud_policy_manager_unittest.cc (right):

http://codereview.chromium.org/10919131/diff/1/chrome/browser/policy/user_clo...
chrome/browser/policy/user_cloud_policy_manager_unittest.cc:12: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
On 2012/09/07 12:44:13, Joao da Silva wrote:
> nit: first include

Done.

[Joao da Silva, 2012-09-07 14:38:14]

lgtm

[commit-bot: I haz the power, 2012-09-10 12:11:35]

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

[commit-bot: I haz the power, 2012-09-10 12:11:40]

Presubmit check for 10919131-13001 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.3s to calculate.

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

+ben to TBR for gyp changes.

[commit-bot: I haz the power, 2012-09-10 12:53:29]

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

[commit-bot: I haz the power, 2012-09-10 12:53:33]

Presubmit check for 10919131-13001 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.3s to calculate.

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

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

[commit-bot: I haz the power, 2012-09-10 15:35:08]

Try job failure for 10919131-13001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

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

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

[commit-bot: I haz the power, 2012-09-10 17:56:49]

Change committed as 155763

[Lei Zhang, 2012-11-05 20:11:00]

https://chromiumcodereview.appspot.com/10919131/diff/13001/chrome/browser/pol...
File chrome/browser/policy/cloud_policy_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10919131/diff/13001/chrome/browser/pol...
chrome/browser/policy/cloud_policy_manager_unittest.cc:75:
Mock::VerifyAndClearExpectations(store_);
Umm, this does not look right...

[Mattias Nissler (ping if slow), 2012-11-06 09:13:28]

https://chromiumcodereview.appspot.com/10919131/diff/13001/chrome/browser/pol...
File chrome/browser/policy/cloud_policy_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10919131/diff/13001/chrome/browser/pol...
chrome/browser/policy/cloud_policy_manager_unittest.cc:75:
Mock::VerifyAndClearExpectations(store_);
On 2012/11/05 20:11:00, Lei Zhang wrote:
> Umm, this does not look right...

Indeed, thanks for pointing out.

CL with the fix is here: http://codereview.chromium.org/11361120/