Consolidate data storage and notifications in the cloud policy subsystem

Consolidate data storage and notifications in the cloud policy subsystem

Get rid of identity strategies and create a central in-memory data store and notification hub, which is more simple. It's called CloudPolicyDataStore.

BUG=chromium-os:17309
TEST=user and device policies work exactly as before

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

[gfeher, 2011-07-06 09:47:48]

Hi, please start reviewing my amazing change. Please feel free to ask any
questions, on-line or off-line...

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/loggi...
File chrome/browser/policy/logging_work_scheduler.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/loggi...
chrome/browser/policy/logging_work_scheduler.cc:147: callback_.reset(NULL);  //
erase the pointer to the callback
This file is support code for cloud_policy_subsystem_unittest.cc. These changes
are necessary, because canceling tasks were broken in tests, but this breakage
was only uncovered by the rest of this CL.

[pastarmovj, 2011-07-06 12:11:57]

Mostly minor comments. Looks pretty good!

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:17: #include
"chrome/browser/policy/device_management_backend.h"
What does the IWYU say about including the same header both in the .h and the
.cc file?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:27: class CloudPolicyData;
Why do you both include the header and forward declare it here. Please pick one
:)
P.S. I think you'll rather need the include given that this is a base class of
CPC.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:29: class
DeviceManagementBackend;
Ditto. Pick one.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:9: #include
"chrome/browser/policy/cloud_policy_data.h"
I thought you had this already included in the header. does our IWYU policy say
we should repeat header inclusions between headers and source files?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:13: #include
"chrome/browser/policy/device_management_backend.h"
What do you use from the last include?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:103: std::string policy_type_;
If these are initialized only by the constructor they can be const right?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_identity_strategy.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_identity_strategy.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Consider removing as discussed offline.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_identity_strategy.h:31: virtual void
OnPolicyHolderGoingAway() OVERRIDE {}
Consider removing this.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:302:
CombinedTestDesc(403, 400),
I think you can use the constants from the policy backend here instead of
hardcoding the numbers.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:219: // TODO(jkummerow): Reminder:
When we want to feed device-wide settings
Is this todo still relevant?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:14: #include
"chrome/browser/policy/cloud_policy_data.h"
I think you might be able to only Fwd declare this one.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_token_cache.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_token_cache.cc:71:
data_->set_device_id(device_id);
Is it secure to assume data_ is not NULL here always?

[gfeher, 2011-07-06 15:14:20]

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:17: #include
"chrome/browser/policy/device_management_backend.h"
Do it only at one place...

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:27: class CloudPolicyData;
On 2011/07/06 12:11:57, pastarmovj wrote:
> Why do you both include the header and forward declare it here. Please pick
one
> :)
> P.S. I think you'll rather need the include given that this is a base class of
> CPC.

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:29: class
DeviceManagementBackend;
On 2011/07/06 12:11:57, pastarmovj wrote:
> Ditto. Pick one.

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:9: #include
"chrome/browser/policy/cloud_policy_data.h"
On 2011/07/06 12:11:57, pastarmovj wrote:
> I thought you had this already included in the header. does our IWYU policy
say
> we should repeat header inclusions between headers and source files?

This unittest doesn't have a header!

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:13: #include
"chrome/browser/policy/device_management_backend.h"
On 2011/07/06 12:11:57, pastarmovj wrote:
> What do you use from the last include?

I am using, em::DeviceRegisterRequest_Type, but that's actually from another
include. Fixed.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:103: std::string policy_type_;
On 2011/07/06 12:11:57, pastarmovj wrote:
> If these are initialized only by the constructor they can be const right?

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_identity_strategy.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_identity_strategy.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Sorry about this, it somehow sneaked in.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_identity_strategy.h:31: virtual void
OnPolicyHolderGoingAway() OVERRIDE {}
On 2011/07/06 12:11:57, pastarmovj wrote:
> Consider removing this.

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:302:
CombinedTestDesc(403, 400),
Those are actually private variables there, so this change seems to be more
difficult. Anyways, I understand your point, but I am not sure that this would
be a good idea, so let's discuss this sometimes later.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:219: // TODO(jkummerow): Reminder:
When we want to feed device-wide settings
TODO(gfeher): investigate more and discuss with Mattias

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:14: #include
"chrome/browser/policy/cloud_policy_data.h"
On 2011/07/06 12:11:57, pastarmovj wrote:
> I think you might be able to only Fwd declare this one.

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_token_cache.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_token_cache.cc:71:
data_->set_device_id(device_id);
Good point. Done.

[pastarmovj, 2011-07-06 15:36:01]

LGTM.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:302:
CombinedTestDesc(403, 400),
My only reasoning is that these values are known to change (seldom) so it will
be great to make sure tests change along the code. But certainly this can be a
topic of another CL.

On 2011/07/06 15:14:20, gfeher wrote:
> Those are actually private variables there, so this change seems to be more
> difficult. Anyways, I understand your point, but I am not sure that this would
> be a good idea, so let's discuss this sometimes later.

[Joao da Silva, 2011-07-06 16:45:14]

LGTM!

I have some issue with unnecessary headers, please fix them :-)

A test for CloudPolicyData would be nice too, maybe another CL.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:38: #include
"content/common/notification_service.h"
Nit: notification_service.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:10: #include "base/callback.h"
Nit: callback.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:12: #include
"base/message_loop.h"
Nit: message_loop.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:24: static const char*
kNonManagedDomains[] = {
Should this static var be in the anonymous namespace instead?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:32: static bool
CanBeInManagedDomain(const std::string& username) {
Same for this function.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:165: // If we already have a
device token, that must be matching the current
Can that be DCHECK'd?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:11: #include "base/file_path.h"
Nit: file_path.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:14: #include "base/task.h"
Nit: task.h and observer_list.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:15: #include "base/time.h"
Nit: time.h only needed on .cc file

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:23: class TokenService;
Nit: Profile and TokenService not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:59: virtual void
OnError(DeviceManagementBackend::ErrorCode code);
Nit: OVERRIDE on these 2 methods

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:64: virtual void
OnPolicyDataGoingAway();
Nit: OVERRIDE on these 3 methods

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:10: #include
"chrome/browser/policy/device_management_service.h"
Nit: device_management_service.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:13: #include
"chrome/browser/policy/mock_configuration_policy_store.h"
Nit: mock_configuration_policy_store.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:17: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
Nit: device_management_backend.pb.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:28: namespace em =
enterprise_management;
Nit: not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:11: #include
"base/memory/scoped_ptr.h"
Nit: scoped_ptr.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:75: std::string machine_model() const;
What do you think about returning these as const std::string&?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:21: class
CloudPolicyIdentityStrategy;
Nit: CloudPolicyIdentityStrategy is no more!

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:14: #include
"chrome/browser/policy/mock_configuration_policy_store.h"
Nit: mock_configuration_policy_store.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:26: #include
"testing/gmock/include/gmock/gmock.h"
Nit: gmock.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:32: using
::testing::AtLeast;
Nit: AtLeast not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:34: using
::testing::DoAll;
Nit: DoAll not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:37: using
::testing::Return;
Nit: Invoke and Return not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:41: class
CloudPolicySubsystemTest;
Nit: not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:10: #include "base/task.h"
Nit: task.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:17: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
Nit: configuration_policy_pref_store.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:24: #include
"content/browser/browser_thread.h"
Nit: browser_thread.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:195: // All the code pathes should
issue a data_->SetDeviceToken(..., true)
Nit: pathes -> paths

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.h:9: #include <string>
Nit: not needed in header, move to .cc

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.h:19: class DevicePolicyHolder;
What's this?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:11: #include "base/message_loop.h"
Nit: callback.h and message_loop.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:9: #include <string>
Nit: string not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:13: #include "base/task.h"
Nit: observer_list.h and task.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:62: virtual void
OnError(DeviceManagementBackend::ErrorCode code);
Nit: add OVERRIDE to these

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:7: #include
"base/file_util.h"
Nit: file_util.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:10: #include
"chrome/browser/net/gaia/token_service.h"
Nit: token_service.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:12: #include
"chrome/browser/policy/device_management_service.h"
Nit: device_management_service.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:17: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
Nit: device_management_backend.pb.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:20: #include
"chrome/test/testing_profile.h"
Nit: gaia_constants.h and testing_profile.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/loggi...
File chrome/browser/policy/logging_work_scheduler.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/loggi...
chrome/browser/policy/logging_work_scheduler.cc:147: callback_.reset(NULL);  //
erase the pointer to the callback
Nit: comments text: start with capitals, end with a dot.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_token_cache.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_token_cache.h:14: #include
"base/memory/weak_ptr.h"
Nit: weak_ptr.h not used

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_token_cache.h:16: #include
"chrome/browser/policy/proto/device_management_local.pb.h"
Nit: move device_management_local.pb.h to .cc

[gfeher, 2011-07-07 13:51:00]

Woo-hoo, thanks for the comments!
I was not sure if there's enough functionality in CloudPolicyData to test it.
But I am open about writing some basic tests. Let's discuss this when I'm back.

In addition to your feedback, I've reintroduced weak pointers in
UserPolicyTokenCache, and renamed CloudPolicyData to CloudPolicyDataStore. Does
it still LGTY?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:38: #include
"content/common/notification_service.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: notification_service.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:10: #include "base/callback.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: callback.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:12: #include
"base/message_loop.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: message_loop.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:24: static const char*
kNonManagedDomains[] = {
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Should this static var be in the anonymous namespace instead?

Agreed.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:32: static bool
CanBeInManagedDomain(const std::string& username) {
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Same for this function.

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:165: // If we already have a
device token, that must be matching the current
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Can that be DCHECK'd?

I think not. The user name cannot be extracted from the token on the client
side, I believe only the server is able to do that.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:11: #include "base/file_path.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: file_path.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:14: #include "base/task.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: task.h and observer_list.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:15: #include "base/time.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: time.h only needed on .cc file

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:23: class TokenService;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: Profile and TokenService not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:59: virtual void
OnError(DeviceManagementBackend::ErrorCode code);
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: OVERRIDE on these 2 methods

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:64: virtual void
OnPolicyDataGoingAway();
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: OVERRIDE on these 3 methods

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:10: #include
"chrome/browser/policy/device_management_service.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: device_management_service.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:13: #include
"chrome/browser/policy/mock_configuration_policy_store.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: mock_configuration_policy_store.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:17: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: device_management_backend.pb.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:28: namespace em =
enterprise_management;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:11: #include
"base/memory/scoped_ptr.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: scoped_ptr.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:75: std::string machine_model() const;
Well, in theory, CloudPolicyData should outlive all its users, so this should be
safe. On the other hand, I'd prefer not to wire this requirement into the
system, because it seems to be hard to be enforced.
And a random example, let's say I was following it: PrefService::GetString(...)
is also returning copies of strings.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:21: class
CloudPolicyIdentityStrategy;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: CloudPolicyIdentityStrategy is no more!

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:14: #include
"chrome/browser/policy/mock_configuration_policy_store.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: mock_configuration_policy_store.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:26: #include
"testing/gmock/include/gmock/gmock.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: gmock.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:32: using
::testing::AtLeast;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: AtLeast not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:34: using
::testing::DoAll;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: DoAll not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:37: using
::testing::Return;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: Invoke and Return not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:41: class
CloudPolicySubsystemTest;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:302:
CombinedTestDesc(403, 400),
Discussed off-line, I'll create another CL for this issue.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:10: #include "base/task.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: task.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:17: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: configuration_policy_pref_store.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:24: #include
"content/browser/browser_thread.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: browser_thread.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:195: // All the code pathes should
issue a data_->SetDeviceToken(..., true)
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: pathes -> paths

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.h:9: #include <string>
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: not needed in header, move to .cc

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.h:19: class DevicePolicyHolder;
On 2011/07/06 16:45:14, Joao da Silva wrote:
> What's this?

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:9: #include <string>
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: string not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:13: #include "base/task.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: observer_list.h and task.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:62: virtual void
OnError(DeviceManagementBackend::ErrorCode code);
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: add OVERRIDE to these

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:7: #include
"base/file_util.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: file_util.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:10: #include
"chrome/browser/net/gaia/token_service.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: token_service.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:12: #include
"chrome/browser/policy/device_management_service.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: device_management_service.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:20: #include
"chrome/test/testing_profile.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: gaia_constants.h and testing_profile.h not used

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/loggi...
File chrome/browser/policy/logging_work_scheduler.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/loggi...
chrome/browser/policy/logging_work_scheduler.cc:147: callback_.reset(NULL);  //
erase the pointer to the callback
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: comments text: start with capitals, end with a dot.

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_token_cache.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_token_cache.h:14: #include
"base/memory/weak_ptr.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: weak_ptr.h not used

Now that you mention, I actually reintroduce using it! :-)

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_token_cache.h:16: #include
"chrome/browser/policy/proto/device_management_local.pb.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: move device_management_local.pb.h to .cc

Done.

[Joao da Silva, 2011-07-07 16:54:16]

Still LGTM! Have a look at the comments below before committing.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:24: static const char*
kNonManagedDomains[] = {
On 2011/07/07 13:51:00, gfeher wrote:
> On 2011/07/06 16:45:14, Joao da Silva wrote:
> > Should this static var be in the anonymous namespace instead?
> 
> Agreed.
Cool. In that case the static qualifier can be dropped.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:15: #include "base/time.h"
On 2011/07/07 13:51:00, gfeher wrote:
> On 2011/07/06 16:45:14, Joao da Silva wrote:
> > Nit: time.h only needed on .cc file
> 
> Done.
Hmm it seems it's still in the .h, is it required there?

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data.h:75: std::string machine_model() const;
On 2011/07/07 13:51:00, gfeher wrote:
> Well, in theory, CloudPolicyData should outlive all its users, so this should
be
> safe. On the other hand, I'd prefer not to wire this requirement into the
> system, because it seems to be hard to be enforced.
> And a random example, let's say I was following it:
PrefService::GetString(...)
> is also returning copies of strings.
Agree, thanks for making that clear.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:11: #include "base/message_loop.h"
On 2011/07/06 16:45:14, Joao da Silva wrote:
> Nit: callback.h and message_loop.h not used
These seem still not used, am I missing something?

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data_store.cc (right):

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:8: #include
"base/string_util.h"
Nit: string_util.h not used

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:15: #include
"chrome/browser/chromeos/login/user_manager.h"
Nit: cros_library.h, ownership_service.h and user_manager.h not used

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:22: static const char
kMachineInfoSystemHwqual[] = "hardware_class";
Is static needed inside the anonymous namespace?

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:29: namespace em =
enterprise_management;
This is already defined in the .h. Actually, is it OK to define it in the .h?

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data_store.h (right):

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.h:12: #include
"base/memory/weak_ptr.h"
Nit: alphabetic order

[gfeher, 2011-07-08 09:19:14]

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:24: static const char*
kNonManagedDomains[] = {
On 2011/07/07 16:54:16, Joao da Silva wrote:
> On 2011/07/07 13:51:00, gfeher wrote:
> > On 2011/07/06 16:45:14, Joao da Silva wrote:
> > > Should this static var be in the anonymous namespace instead?
> > 
> > Agreed.
> Cool. In that case the static qualifier can be dropped.

Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.h:15: #include "base/time.h"
Sorry. Done.

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/7298012/diff/11001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:11: #include "base/message_loop.h"
On 2011/07/07 16:54:16, Joao da Silva wrote:
> On 2011/07/06 16:45:14, Joao da Silva wrote:
> > Nit: callback.h and message_loop.h not used
> These seem still not used, am I missing something?

Nope. I was missing something.

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data_store.cc (right):

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:8: #include
"base/string_util.h"
On 2011/07/07 16:54:16, Joao da Silva wrote:
> Nit: string_util.h not used

Done.

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:15: #include
"chrome/browser/chromeos/login/user_manager.h"
On 2011/07/07 16:54:16, Joao da Silva wrote:
> Nit: cros_library.h, ownership_service.h and user_manager.h not used

Done.

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:22: static const char
kMachineInfoSystemHwqual[] = "hardware_class";
On 2011/07/07 16:54:16, Joao da Silva wrote:
> Is static needed inside the anonymous namespace?

Done.

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.cc:29: namespace em =
enterprise_management;
It's done in other .h files as well. I guess the only problem is that files
including the .h file also get this definition, even if they don't need it. I
tried to move it to an anonymous namespace but then it conflicted with "em"
definitions from other .h files. So I leave it there for now, and I am not sure
if there's a better solution.

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data_store.h (right):

http://codereview.chromium.org/7298012/diff/12033/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.h:12: #include
"base/memory/weak_ptr.h"
On 2011/07/07 16:54:16, Joao da Silva wrote:
> Nit: alphabetic order

Done.