Split UserCloudPolicyManager implementation.

Split UserCloudPolicyManager implementation.

Create different implementations for desktop Chrome / CrOS, and change
initialization to keep the CrOS version in BrowserPolicyConnector
instead of the Profile.

BUG=chromium:108928
TEST=compiles and passes tests, cloud policy works as before.
TBR=phajdan.jr@chromium.org,ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170742

[Mattias Nissler (ping if slow), 2012-11-21 10:47:18]

Here's my attempt at de-tangling the UserCloudPolicyManager stuff on desktop vs.
cros. Comments welcome.

[Joao da Silva, 2012-11-21 17:06:34]

Looks good to me.

Are we ok with removing support to wait for the initial policy fetch on desktop?

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:55: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
nit: not needed

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:52: #include
"chrome/browser/chromeos/login/user_manager.h"
nit: not used

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:555: 
global_user_cloud_policy_provider_.Init()

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:35: class
UserCloudPolicyManager;
nit: not needed

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:235: ProxyPolicyProvider
global_user_cloud_policy_provider_;
This works but I'd prefer to always create the user_cloud_policy_manager_ early
and plug it into the PolicyService, and initialize it (and its store) later, at
InitializeUserPolicy().

IMO that'd be one less class to understand, and all the providers are obvious at
the creation time of PolicyService. It also follows the pattern that providers
become ready when well, they're ready :-) This mostly means loading caches, and
for this provider it means "user signed in, cryptohome is mounted and caches
have been loaded".

That said, it's a matter of personal preference and that's just my opinion. The
proxy may also come in handy for user cloud policy on the desktop later.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.h:25: // upper layers of the policy
stack. It owns CloudPolicyStore,
The store is not owned anymore

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_store.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:15: class Profile;
nit: not needed

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_manager_chromeos.cc:53:
device_store_(store.Pass()),
Cool :-)

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:13: #include
"chrome/browser/policy/cloud_policy_client.h"
This can be forward declared

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:18: class Profile;
nit: not used

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:30: // false as long as there
hasn't been a successful policy fetch.
Update comment.

This is dropping support to wait for an initial fetch on the desktop, is that
intentional?

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager_chromeos.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_chromeos.h:18: class Profile;
nit: not used

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_chromeos.h:37: virtual void
Initialize(PrefService* local_state,
Why virtual?

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store.h:29: static
scoped_ptr<UserCloudPolicyStore> Create(Profile* profile);
Profile needs to be forward-declared after the declaration at
cloud_policy_store.h is removed

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store.h:32: virtual void
LoadImmediately();
Mention that this is virtual for mocks.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:12: #include
"base/command_line.h"
nit: not used

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:15: #include
"base/path_service.h"
nit: not used

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:23: #include
"chromeos/dbus/dbus_thread_manager.h"
nit: not used

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:96: #include
"chrome/browser/policy/user_cloud_policy_store.h"
These 2 are excluded from gyp when OS_CHROMEOS

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:1352: 'browser/policy/proxy_policy_provider.h',
nit: order

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:2405: ['exclude',
'browser/policy/proxy_policy_provider.h'],
nit: order

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:940:
'browser/policy/proxy_policy_provider_unittest.cc',
nit: order

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:958:
'browser/policy/user_cloud_policy_manager_chromeos_unittest.cc',
nit: order

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1842: ['exclude',
'^browser/policy/mock_user_cloud_policy_store.cc' ],
nit: order

[Andrew T Wilson (Slow), 2012-11-21 17:34:23]

LGTM with some comments that you can address or ignore as you deem appropriate.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.h:79: CloudPolicyStore* store_;
Since we don't own this, and this will likely get freed before this object is
freed (since our derived class will hold a reference to it) we probably should
document the semantics of use (i.e. do not access this from our destructor).

Should probably also clear this out in Shutdown(). OR, just go back to the way
it was where this class owned a pointer to the CloudPolicyStore :)

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_store_chromeos.cc (left):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:61: }
YAY for getting rid of this hack.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:26: // Profiles on the desktop
platforms.
Careful about saying "desktop platforms" as we will probably use it on mobile
chrome eventually :) But saying "non-cros platforms" sounds so hack-y.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:30: // false as long as there
hasn't been a successful policy fetch.
On 2012/11/21 17:06:34, Joao da Silva wrote:
> Update comment.
> 
> This is dropping support to wait for an initial fetch on the desktop, is that
> intentional?

BTW, I'm fine with that for now. We might need to add it back eventually if we
ever build that "create a new enterprise profile on user signin" solution - we
might want to load initial policy a different way in that case, anyway.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:356:
policy::UserCloudPolicyStore::Create(this));
Hmmm. Do we like exposing this code to Profile() as opposed to hiding it off in
a factory method as it was before? I'm OK with this, I guess, but it just feels
like maybe the details of UCPM initialization (dependency on
UserCloudPolicyStore, etc) should live in UCPM::Create(), not here?

[Mattias Nissler (ping if slow), 2012-11-22 20:51:59]

Hey guys, I made a major change to enable treating UserCloudPolicyManager
(mostly) as a ProfileKeyedService. This definitely needs to be run by Profile
wizards, but I'd like to get your input first on whether this is worthwhile.
Personally, I think I like it, because it gets us one step closer to the
ProfileKeyedService world and easily migrates to the proper solution when
PrefService becomes a PSK.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:55: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not needed

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:52: #include
"chrome/browser/chromeos/login/user_manager.h"
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not used

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:555: 
On 2012/11/21 17:06:34, Joao da Silva wrote:
> global_user_cloud_policy_provider_.Init()

Done, good catch.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:35: class
UserCloudPolicyManager;
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not needed

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:235: ProxyPolicyProvider
global_user_cloud_policy_provider_;
On 2012/11/21 17:06:34, Joao da Silva wrote:
> This works but I'd prefer to always create the user_cloud_policy_manager_
early
> and plug it into the PolicyService, and initialize it (and its store) later,
at
> InitializeUserPolicy().
> 
> IMO that'd be one less class to understand, and all the providers are obvious
at
> the creation time of PolicyService. It also follows the pattern that providers
> become ready when well, they're ready :-) This mostly means loading caches,
and
> for this provider it means "user signed in, cryptohome is mounted and caches
> have been loaded".

I disagree strongly on the becoming ready part. If I did what you suggest, the
global PolicyService would only flag itself ready after user policy is
available, which is obviously wrong (e.g. if somebody decides to init the
local_state PrefService asynchronously).

This is also further complicated by the fact that public accounts might have a
different UserCloudPolicyManager implementation, at which point we need to
decide at login time which of the two we're actually going to wire to the global
PolicyService.

> 
> That said, it's a matter of personal preference and that's just my opinion.
The
> proxy may also come in handy for user cloud policy on the desktop later.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.h:25: // upper layers of the policy
stack. It owns CloudPolicyStore,
On 2012/11/21 17:06:34, Joao da Silva wrote:
> The store is not owned anymore

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.h:79: CloudPolicyStore* store_;
On 2012/11/21 17:34:24, Andrew T Wilson wrote:
> Since we don't own this, and this will likely get freed before this object is
> freed (since our derived class will hold a reference to it) we probably should
> document the semantics of use (i.e. do not access this from our destructor).
> 
> Should probably also clear this out in Shutdown(). OR, just go back to the way
> it was where this class owned a pointer to the CloudPolicyStore :)

I changed the code to NULL store_ in Shutdown() and added a DCHECK(!store_) to
the dtor, so we should be well-covered.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_store.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:15: class Profile;
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not needed

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_manager_chromeos.cc:53:
device_store_(store.Pass()),
On 2012/11/21 17:06:34, Joao da Silva wrote:
> Cool :-)

See Drew's comment for the drawbacks ;)

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_store_chromeos.cc (left):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_store_chromeos.cc:61: }
On 2012/11/21 17:34:24, Andrew T Wilson wrote:
> YAY for getting rid of this hack.

At a considerable price, but yes.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:13: #include
"chrome/browser/policy/cloud_policy_client.h"
On 2012/11/21 17:06:34, Joao da Silva wrote:
> This can be forward declared

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:18: class Profile;
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not used

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:26: // Profiles on the desktop
platforms.
On 2012/11/21 17:34:24, Andrew T Wilson wrote:
> Careful about saying "desktop platforms" as we will probably use it on mobile
> chrome eventually :) But saying "non-cros platforms" sounds so hack-y.

For now, it's correct. It's unclear at this point what policy delivery mechanism
we'd choose for mobile platforms.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:30: // false as long as there
hasn't been a successful policy fetch.
On 2012/11/21 17:06:34, Joao da Silva wrote:
> Update comment.
> 
> This is dropping support to wait for an initial fetch on the desktop, is that
> intentional?

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:30: // false as long as there
hasn't been a successful policy fetch.
On 2012/11/21 17:34:24, Andrew T Wilson wrote:
> On 2012/11/21 17:06:34, Joao da Silva wrote:
> > Update comment.
> > 
> > This is dropping support to wait for an initial fetch on the desktop, is
that
> > intentional?
> 
> BTW, I'm fine with that for now. We might need to add it back eventually if we
> ever build that "create a new enterprise profile on user signin" solution - we
> might want to load initial policy a different way in that case, anyway.

Once we need it on desktop too, we can see what the best way to share the code
would be. Currently it's not needed, so I think it's only fair to not have it
here.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager_chromeos.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_chromeos.h:18: class Profile;
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not used

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_chromeos.h:37: virtual void
Initialize(PrefService* local_state,
On 2012/11/21 17:06:34, Joao da Silva wrote:
> Why virtual?

No good reason. Removed.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store.h (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store.h:29: static
scoped_ptr<UserCloudPolicyStore> Create(Profile* profile);
On 2012/11/21 17:06:34, Joao da Silva wrote:
> Profile needs to be forward-declared after the declaration at
> cloud_policy_store.h is removed

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store.h:32: virtual void
LoadImmediately();
On 2012/11/21 17:06:34, Joao da Silva wrote:
> Mention that this is virtual for mocks.

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:12: #include
"base/command_line.h"
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not used

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:15: #include
"base/path_service.h"
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not used

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:23: #include
"chromeos/dbus/dbus_thread_manager.h"
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: not used

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:96: #include
"chrome/browser/policy/user_cloud_policy_store.h"
On 2012/11/21 17:06:34, Joao da Silva wrote:
> These 2 are excluded from gyp when OS_CHROMEOS

Thanks, this comment uncovered a deeper issue: Removing them from the #include
list would mean #ifdef-ing out GetUserCloudPolicyManager(). This actually
propagates pretty far (see TestingProfile) and gets rather awkward.

I've bitten the bullet and rolled UserCloudPolicyManager into something that
looks like a ProfileKeyedService, but with different lifetime management. As a
bonus, that allows us to remove most of the UserCloudPolicyManager references
from the Profile implementations, which is nice. WDYT?

https://codereview.chromium.org/11415094/diff/7001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:356:
policy::UserCloudPolicyStore::Create(this));
On 2012/11/21 17:34:24, Andrew T Wilson wrote:
> Hmmm. Do we like exposing this code to Profile() as opposed to hiding it off
in
> a factory method as it was before? I'm OK with this, I guess, but it just
feels
> like maybe the details of UCPM initialization (dependency on
> UserCloudPolicyStore, etc) should live in UCPM::Create(), not here?

Fair. Changed.

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:1352: 'browser/policy/proxy_policy_provider.h',
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:2405: ['exclude',
'browser/policy/proxy_policy_provider.h'],
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:940:
'browser/policy/proxy_policy_provider_unittest.cc',
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:958:
'browser/policy/user_cloud_policy_manager_chromeos_unittest.cc',
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/11415094/diff/7001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1842: ['exclude',
'^browser/policy/mock_user_cloud_policy_store.cc' ],
On 2012/11/21 17:06:34, Joao da Silva wrote:
> nit: order

Done.

[Joao da Silva, 2012-11-23 10:08:12]

It'd be cool if the policy manager were owned by the factory, but there's no
good way to tell the factory that the profile has just been deleted. AFAICT,
Profiles are owned by ProfileManager and only deleted at ist dtor; it'd have to
tell the factory that the profile is gone, for each profile. Not very cool :-)

Other than that, lgtm.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager.h:25: // upper layers of the policy
stack. It CloudPolicyClient and
nit: It owns

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_manager.h:13: #include
"chrome/browser/policy/cloud_policy_constants.h"
nit: not needed

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_manager_factory.h (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_manager_factory.h:30: // depending on
PrefService once the latter is a ProfileKeyedService.
Mention bugs 131843 and 131844.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service_unittest.cc:71:
testing::Mock::VerifyAndClearExpectations(mock_store_);
nit: testing:: not needed

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (left):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:598: cloud_policy_manager_->Shutdown();
This is still necessary.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:95: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
Not included in gyp when OS_CHROMEOS...

https://codereview.chromium.org/11415094/diff/13001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:1385:
'browser/policy/user_cloud_policy_manager_chromeos.h',
nit: order

https://codereview.chromium.org/11415094/diff/13001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:40: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
nit: not needed

[Andrew T Wilson (Slow), 2012-11-23 15:04:20]

So, converting to a PKS is crazy, but I like it - I'd just like to see us go a
little further in that direction by actually making the UCPMFactory responsible
for creating UCPMs (it *is* a Factory after all).

Looking forward, if we ever make PrefService a PKS, I don't think we want a
strict dependency on UCPM, since I really would like to be able to access the
existing user prefs from the policy layer.

I think ideally we'd either break PrefService into two services, where there's
some kind of UserPrefService that loads and exposes the user prefs, which UCPM
can depend on, and PrefService can then depend on UCPM.

Anyhow, that's for some future refactoring :)

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:208:
user_cloud_policy_provider,
Just out of curiosity, why are we making UCPM PKS-based, but not
ManagedModePolicyProvider? Do we think that's the next step?

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_manager.cc (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.cc:31: DCHECK(!store_);
The only way this can happen is if we fail to invoke Shutdown() before
destroying an object - maybe add a comment to that effect (or add an error msg
to this DCHECK)?

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.h:25: // upper layers of the policy
stack. It CloudPolicyClient and
It -> It owns

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager_factory.h (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_factory.h:26: // is managed by
Profile, Profile startup code invokes InitForProfile()
Is this comment still true?

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_factory.h:57: void
Unregister(Profile* profile, UserCloudPolicyManager* instance);
Is there a reason why creation of UCPMs has to happen outside of this class?
That's not typically how PKSes work. Is it because we need to somehow pass the
LoadImmediately flag from Profile down to UCPM::Create()?

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_factory.h:60: ManagerMap
managers_;
I don't totally understand why we need to do this (have our own managers_ map,
instead of using the normal PKS infrastructure). Because it means we have to
re-implement a bunch of functionality.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:354: cloud_policy_manager_ =
Yeah, this is a little weird (having creation happen here instead of via the PKS
directly). I'm guessing this is because we need to pass
force_immediate_policy_load down? Because maybe we should add a separate API to
trigger either a delayed or immediate load (so no automatic load on creation)
and move creation into UCPMFactory like all other PKSes).

[Mattias Nissler (ping if slow), 2012-11-23 17:36:06]

Thanks for the helpful comments.

Drew: Regarding the PKS dependency graph vs. PolicyService, UCPM and
PrefService, we could indeed break the JsonPrefStore that handles user prefs
persistence out into an independent PKS, so you don't need to depend on
PrefService for low-level prefs accesss.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager.h:25: // upper layers of the policy
stack. It CloudPolicyClient and
On 2012/11/23 10:08:12, Joao da Silva wrote:
> nit: It owns

Done.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_manager.h:13: #include
"chrome/browser/policy/cloud_policy_constants.h"
On 2012/11/23 10:08:12, Joao da Silva wrote:
> nit: not needed

Done.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_manager_factory.h (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_manager_factory.h:30: // depending on
PrefService once the latter is a ProfileKeyedService.
On 2012/11/23 10:08:12, Joao da Silva wrote:
> Mention bugs 131843 and 131844.

Done.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service_unittest.cc:71:
testing::Mock::VerifyAndClearExpectations(mock_store_);
On 2012/11/23 10:08:12, Joao da Silva wrote:
> nit: testing:: not needed

Done.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (left):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:598: cloud_policy_manager_->Shutdown();
On 2012/11/23 10:08:12, Joao da Silva wrote:
> This is still necessary.

Actually not, UserCloudPolicyManagerFactory::Shutdown() takes care of it.

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:95: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
On 2012/11/23 10:08:12, Joao da Silva wrote:
> Not included in gyp when OS_CHROMEOS...

Done.

https://codereview.chromium.org/11415094/diff/13001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:1385:
'browser/policy/user_cloud_policy_manager_chromeos.h',
On 2012/11/23 10:08:12, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/11415094/diff/13001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11415094/diff/13001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:40: #include
"chrome/browser/policy/user_cloud_policy_manager.h"
On 2012/11/23 10:08:12, Joao da Silva wrote:
> nit: not needed

Done.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:208:
user_cloud_policy_provider,
On 2012/11/23 15:04:20, Andrew T Wilson wrote:
> Just out of curiosity, why are we making UCPM PKS-based, but not
> ManagedModePolicyProvider? Do we think that's the next step?

That'd indeed be the right thing to do. Then again, this is mostly
proof-of-concept right now:
- I need to run this by Profile people to see whether they're OK
- Converting ManagedModePolicyProvider should probably be done in a separate CL
- ManagedModePolicyProvider exists on all platforms, so the trigger for my PKS
change (#ifdef hell) doesn't apply.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_manager.cc (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.cc:31: DCHECK(!store_);
On 2012/11/23 15:04:20, Andrew T Wilson wrote:
> The only way this can happen is if we fail to invoke Shutdown() before
> destroying an object - maybe add a comment to that effect (or add an error msg
> to this DCHECK)?

Done.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_manager.h (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_manager.h:25: // upper layers of the policy
stack. It CloudPolicyClient and
On 2012/11/23 15:04:20, Andrew T Wilson wrote:
> It -> It owns

Done.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager_factory.h (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_factory.h:26: // is managed by
Profile, Profile startup code invokes InitForProfile()
On 2012/11/23 15:04:20, Andrew T Wilson wrote:
> Is this comment still true?

Updated.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_factory.h:57: void
Unregister(Profile* profile, UserCloudPolicyManager* instance);
On 2012/11/23 15:04:20, Andrew T Wilson wrote:
> Is there a reason why creation of UCPMs has to happen outside of this class?
> That's not typically how PKSes work. Is it because we need to somehow pass the
> LoadImmediately flag from Profile down to UCPM::Create()?

Good idea, fixed that. We still need the Unregister call though since lifetime
management must happen in Profile.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager_factory.h:60: ManagerMap
managers_;
On 2012/11/23 15:04:20, Andrew T Wilson wrote:
> I don't totally understand why we need to do this (have our own managers_ map,
> instead of using the normal PKS infrastructure). Because it means we have to
> re-implement a bunch of functionality.

ProfileKeyedServiceFactory is owning all the services. This is not the case with
this, and I reckon we wouldn't want to promote this weird case of external
ownership to be a supported feature of the standard infrastructure.

That's something to discuss with a chrome/browser/profiles OWNER though, so good
thing you raised it.

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11415094/diff/6077/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:354: cloud_policy_manager_ =
On 2012/11/23 15:04:20, Andrew T Wilson wrote:
> Yeah, this is a little weird (having creation happen here instead of via the
PKS
> directly). I'm guessing this is because we need to pass
> force_immediate_policy_load down? Because maybe we should add a separate API
to
> trigger either a delayed or immediate load (so no automatic load on creation)
> and move creation into UCPMFactory like all other PKSes).

I've now moved creation to the factory.

[Mattias Nissler (ping if slow), 2012-11-23 17:38:29]

Elliot: Please take a look at profile_impl.{h,cc} and
user_cloud_policy_manager_factory.{h,cc}, check the comment on ownership and
decide whether you'd rather kill me or not ;)

My main motivation for doing this is so I can have
ProfileImpl::cloud_policy_manager_ #ifdef'ed out for platforms that don't need
it without ending up in #ifdef hell.

[Elliot Glaysher, 2012-11-26 19:18:30]

After reading both the comments and the code a few times, I'm still not sure I
understand how this is interacting with the normal PKSF stuff, what with the
factory subclass owning the policies.

[Mattias Nissler (ping if slow), 2012-11-27 11:48:14]

On 2012/11/26 19:18:30, Elliot Glaysher wrote:
> After reading both the comments and the code a few times, I'm still not sure I
> understand how this is interacting with the normal PKSF stuff, what with the
> factory subclass owning the policies.

Let me try to make things clearer:

(a) The simplified dependency graph is like this (yay for ASCII art!)

    UserPolicySigninManager -----> TokenService -----> PrefService
              |                                              |
              |                                              |(*)
              |                                              V
              +----------------------------------> UserCloudPolicyManager

(b) PrefService is not a PKS (yet), because stuff makes Profile->GetPrefs()
calls all over the place and adding DependsOn(PrefServiceFactory::GetInstance())
in the right places is a non-trivial problem. See bugs http://crbug.com/131843
and http://crbug.com/131844. Still, the plan is to eventually make PrefService a
PKS to the best of my knowledge. Alas, PrefService is a regular ProfileImpl
member right now, and all dependencies must be constructed before PrefService
and destroyed after it goes away.

(c) PKS initialization needs to happen after PrefService initialization and
before PrefService destruction, because most of the PKS happily do
profile->GetPrefs().

(d) UserCloudPolicyManager only exists on non-OS_CHROMEOS, and PKS make it very
simple to just not compile something that you don't need. In contrast, adding
#if !defined(OS_CHROMEOS) guards throughout Profile, TestingProfile,
OffTheRecordProfile etc. adds lots of noise.

And now for the conclusion: It seems there's value in making
UserCloudPolicyManager look like a PKS, even if we cannot express the (*) edge
in the dependency graph in terms of ProfileKeyedBaseFactory::DependsOn() (yet,
because PrefService isn't a PKS): We can have UserPolicySigningManager declare
its dependency via DependsOn and  we don't need #ifdef-ed
Profile::GetUserCloudPolicyManager() throughout the Profile classes. On the
negative side, I have to manage lifetime of UserCloudPolicyManager manually so
it outlives PrefService, and that's why I rolled the special PKSFactory
implementation that makes UserCloudPolicyManager looks like a PKS, but with
lifetime controlled by the ProfileImpl.

Does this make more sense? Happy to discuss further.

Thanks,
Mattias

[Elliot Glaysher, 2012-11-27 23:31:22]

https://codereview.chromium.org/11415094/diff/10047/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_manager_factory.cc (right):

https://codereview.chromium.org/11415094/diff/10047/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_manager_factory.cc:59:
manager->Shutdown();
So if I understand correctly, ProfileShutdown is really the only part of the
PKSF system you actively use, since you otherwise manage your own pointers.
Doesn't this still run before the PrefService is destroyed?

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

https://codereview.chromium.org/11415094/diff/10047/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_manager_factory.cc (right):

https://codereview.chromium.org/11415094/diff/10047/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_manager_factory.cc:59:
manager->Shutdown();
On 2012/11/27 23:31:22, Elliot Glaysher wrote:
> So if I understand correctly, ProfileShutdown is really the only part of the
> PKSF system you actively use, since you otherwise manage your own pointers.

Mostly correct. However, I do already have a DependsOn() in
UserPolicySigninManagerFactory, and what is here now can be trivially switched
to full PKSF once we have PrefService as a PKS.

So you're right in that the move to PKSF doesn't buy much at this point; my main
motivation is to avoid #ifdef hell (if UserCloudPolicyManager were a regular
ProfileImpl member and I don't want it on OS_CHROMEOS, I'd have to sprinkle many
more #if !defined(OS_CHROMEOS) over profile.h, profile_impl.{cc,h},
testing_profile.h etc.).

Also, assuming that we're still willing to make PrefService a PKS, this seemed a
reasonable step in towards the ultimate goal.

I know all this is pretty weak justification, so if you say "screw it", I'm fine
with that ;)

> Doesn't this still run before the PrefService is destroyed?

Yes, and that is intentional. UserCloudPolicyManager::Shutdown() turns off some
things that should only run when the Profile is fully initialized, after the
Shutdown() call the thing still feeds policy into PrefService, but it will stop
refreshing it from the cloud for example. I thought that kind of thing was the
actually purpose of ProfileShutdown()?

[Elliot Glaysher, 2012-11-28 18:37:55]

On 2012/11/28 09:47:06, Mattias Nissler wrote:
> Also, assuming that we're still willing to make PrefService a PKS, this seemed
a
> reasonable step in towards the ultimate goal.

I sadly can't work on that. Moving windows and linux to aura is too high a
priority and is going to continue for months.

> I know all this is pretty weak justification, so if you say "screw it", I'm
fine
> with that ;)

How about: you take ownership of moving the PrefService to PKSF and I'll approve
this change?

> 
> > Doesn't this still run before the PrefService is destroyed?
> 
> Yes, and that is intentional. UserCloudPolicyManager::Shutdown() turns off
some
> things that should only run when the Profile is fully initialized, after the
> Shutdown() call the thing still feeds policy into PrefService, but it will
stop
> refreshing it from the cloud for example. I thought that kind of thing was the
> actually purpose of ProfileShutdown()?

It is. I was confused with ProfileDestroyed().

[Mattias Nissler (ping if slow), 2012-11-29 16:00:37]

On 2012/11/28 18:37:55, Elliot Glaysher wrote:
> On 2012/11/28 09:47:06, Mattias Nissler wrote:
> > Also, assuming that we're still willing to make PrefService a PKS, this
seemed
> a
> > reasonable step in towards the ultimate goal.
> 
> I sadly can't work on that. Moving windows and linux to aura is too high a
> priority and is going to continue for months.
> 
> > I know all this is pretty weak justification, so if you say "screw it", I'm
> fine
> > with that ;)
> 
> How about: you take ownership of moving the PrefService to PKSF and I'll
approve
> this change?

I'm not really underbooked either, but I think I'm willing to accept this deal
;) It'll be probably Q1 until I find some cycles to look into this though.

> 
> > 
> > > Doesn't this still run before the PrefService is destroyed?
> > 
> > Yes, and that is intentional. UserCloudPolicyManager::Shutdown() turns off
> some
> > things that should only run when the Profile is fully initialized, after the
> > Shutdown() call the thing still feeds policy into PrefService, but it will
> stop
> > refreshing it from the cloud for example. I thought that kind of thing was
the
> > actually purpose of ProfileShutdown()?
> 
> It is. I was confused with ProfileDestroyed().

[Elliot Glaysher, 2012-11-29 20:57:10]

On 2012/11/29 16:00:37, Mattias Nissler wrote:
> On 2012/11/28 18:37:55, Elliot Glaysher wrote:
> > On 2012/11/28 09:47:06, Mattias Nissler wrote:
> > > Also, assuming that we're still willing to make PrefService a PKS, this
> seemed
> > a
> > > reasonable step in towards the ultimate goal.
> > 
> > I sadly can't work on that. Moving windows and linux to aura is too high a
> > priority and is going to continue for months.
> > 
> > > I know all this is pretty weak justification, so if you say "screw it",
I'm
> > fine
> > > with that ;)
> > 
> > How about: you take ownership of moving the PrefService to PKSF and I'll
> approve
> > this change?
> 
> I'm not really underbooked either, but I think I'm willing to accept this deal
> ;) It'll be probably Q1 until I find some cycles to look into this though.
> 
> > 
> > > 
> > > > Doesn't this still run before the PrefService is destroyed?
> > > 
> > > Yes, and that is intentional. UserCloudPolicyManager::Shutdown() turns off
> > some
> > > things that should only run when the Profile is fully initialized, after
the
> > > Shutdown() call the thing still feeds policy into PrefService, but it will
> > stop
> > > refreshing it from the cloud for example. I thought that kind of thing was
> the
> > > actually purpose of ProfileShutdown()?
> > 
> > It is. I was confused with ProfileDestroyed().

That's fine, but please assign the bugs to yourself. lgtm

[Mattias Nissler (ping if slow), 2012-11-30 15:09:05]

+Nikita for chrome/browser/chromeos/login OWNERS approval.

[Nikita (slow), 2012-11-30 15:11:42]

lgtm

[Mattias Nissler (ping if slow), 2012-11-30 16:53:34]

On 2012/11/29 20:57:10, Elliot Glaysher wrote:
> On 2012/11/29 16:00:37, Mattias Nissler wrote:
> > On 2012/11/28 18:37:55, Elliot Glaysher wrote:
> > > On 2012/11/28 09:47:06, Mattias Nissler wrote:
> > > > Also, assuming that we're still willing to make PrefService a PKS, this
> > seemed
> > > a
> > > > reasonable step in towards the ultimate goal.
> > > 
> > > I sadly can't work on that. Moving windows and linux to aura is too high a
> > > priority and is going to continue for months.
> > > 
> > > > I know all this is pretty weak justification, so if you say "screw it",
> I'm
> > > fine
> > > > with that ;)
> > > 
> > > How about: you take ownership of moving the PrefService to PKSF and I'll
> > approve
> > > this change?
> > 
> > I'm not really underbooked either, but I think I'm willing to accept this
deal
> > ;) It'll be probably Q1 until I find some cycles to look into this though.
> > 
> > > 
> > > > 
> > > > > Doesn't this still run before the PrefService is destroyed?
> > > > 
> > > > Yes, and that is intentional. UserCloudPolicyManager::Shutdown() turns
off
> > > some
> > > > things that should only run when the Profile is fully initialized, after
> the
> > > > Shutdown() call the thing still feeds policy into PrefService, but it
will
> > > stop
> > > > refreshing it from the cloud for example. I thought that kind of thing
was
> > the
> > > > actually purpose of ProfileShutdown()?
> > > 
> > > It is. I was confused with ProfileDestroyed().
> 
> That's fine, but please assign the bugs to yourself. lgtm

Done, thanks!

[commit-bot: I haz the power, 2012-12-03 11:24:46]

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

[commit-bot: I haz the power, 2012-12-03 11:25:11]

Presubmit check for 11415094-8047 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/policy/browser_policy_connector.cc:160
    // call SetPolicyProviderForTesting() again. It is still owned by the test.

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

Presubmit checks took 3.9s to calculate.

[Mattias Nissler (ping if slow), 2012-12-03 11:57:05]

+ phajdan.jr@, ben@ to TBR for chrome/test and gyp changes, respectively.

[commit-bot: I haz the power, 2012-12-03 11:57:27]

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

[commit-bot: I haz the power, 2012-12-03 13:43:17]

Retried try job too often on mac_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 13:57:13]

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

[commit-bot: I haz the power, 2012-12-03 14:38:57]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 15:11:54]

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

[commit-bot: I haz the power, 2012-12-03 15:25:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-03 15:31:46]

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