Added UserPolicySigninService::FetchPolicyForSignedInUser().

Added UserPolicySigninService::FetchPolicyForSignedInUser().

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

[Andrew T Wilson (Slow), 2012-12-06 15:10:23]

Mattias: PTAL

[Mattias Nissler (ping if slow), 2012-12-07 15:20:45]

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

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:177: void
SetDeviceManagementServiceForTest(
Please make naming consistent with SetPolicyProviderForTesting.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service.h (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service.h:46: typedef
base::Callback<void(bool)> RefreshPolicyCallback;
typedefs should go right after public: per the style guide.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service_unittest.cc (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service_unittest.cc:93: EXPECT_CALL(*this,
OnPolicyRefresh(_)).Times(0);
Hm, all these violate the "don't mix expectation setup with test code" gmock
rule. I guess I should fix this at some point.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:64: 
remove extra blank line.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:119: 
remove extra blank line.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:126: case
chrome::NOTIFICATION_GOOGLE_SIGNED_OUT:
shouldn't we check that this is actually for the profile we're concerned about?

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:129: case
chrome::NOTIFICATION_PROFILE_ADDED: {
Is this correct? This fires for any profile, not just the one we're managing.

Also, could we just put this code in the ctor? If I'm not mistaken,
UserCloudPolicySigninService is created at profile creation, and all
dependencies are up at that point.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.h (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:75: 
remove extra blank line.

[Andrew T Wilson (Slow), 2012-12-07 17:34:29]

PTAL

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

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:177: void
SetDeviceManagementServiceForTest(
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> Please make naming consistent with SetPolicyProviderForTesting.

Done.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service.h (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service.h:46: typedef
base::Callback<void(bool)> RefreshPolicyCallback;
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> typedefs should go right after public: per the style guide.

Done.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service_unittest.cc (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service_unittest.cc:93: EXPECT_CALL(*this,
OnPolicyRefresh(_)).Times(0);
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> Hm, all these violate the "don't mix expectation setup with test code" gmock
> rule. I guess I should fix this at some point.

What is that rule?

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:64: 
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> remove extra blank line.

Done.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:119: 
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> remove extra blank line.

Done.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:126: case
chrome::NOTIFICATION_GOOGLE_SIGNED_OUT:
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> shouldn't we check that this is actually for the profile we're concerned
about?

When we register for the notification, we specify a source profile.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:129: case
chrome::NOTIFICATION_PROFILE_ADDED: {
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> Is this correct? This fires for any profile, not just the one we're managing.

We are specifying a source profile when registering.

> Also, could we just put this code in the ctor? If I'm not mistaken,
> UserCloudPolicySigninService is created at profile creation, and all
> dependencies are up at that point.

Can't do it yet. UrlFetcher and the like are not ready yet so trying to fetch
tokens crashes. It kind of blows that the Profile isn't actually fully
initialized at service creation time. Profile initialization is pretty crufty
and really needs to get cleaned up (i.e. move *everything* to PKS), but for now
the only way to do anything that uses UrlFetcher is to do it post-PROFILE_ADDED.

I'll add a comment about this.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.h (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:75: 
On 2012/12/07 15:20:45, Mattias Nissler wrote:
> remove extra blank line.

Done.

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

lgtm with nits.

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service_unittest.cc (right):

https://codereview.chromium.org/11444029/diff/9003/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service_unittest.cc:93: EXPECT_CALL(*this,
OnPolicyRefresh(_)).Times(0);
On 2012/12/07 17:34:29, Andrew T Wilson wrote:
> On 2012/12/07 15:20:45, Mattias Nissler wrote:
> > Hm, all these violate the "don't mix expectation setup with test code" gmock
> > rule. I guess I should fix this at some point.
> 
> What is that rule?

From https://code.google.com/p/googlemock/wiki/ForDummies:
Important note: Google Mock requires expectations to be set before the mock
functions are called, otherwise the behavior is undefined. In particular, you
mustn't interleave EXPECT_CALL()s and calls to the mock functions.

It's OK again if you put a Mock::VerifyAndClearExpectations() before you
configure new expectations.

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service.cc:132: // (accessing things
like URLFetchers will crash).
nit: Can you elaborate why it crashes and what the thing is that we haven't
initialized yet? My hope is that we can eventually declare our dependencies via
DependsOn() and move the code to the ctor; knowing what's missing would help.

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service.cc:172:
browser_policy_connector()->device_management_service();
nit: declaring a |connector| local might improve readability.

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_signin_service.h (right):

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service.h:53: typedef
base::Callback<void(bool)> PolicyFetchCallback;
nit: move before ctor

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

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service_unittest.cc:354:
ASSERT_TRUE(fetch_request);
You probably want Mock::VerifyAndClearExpectations(device_management_service_)
here.

[Andrew T Wilson (Slow), 2012-12-10 10:12:40]

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service.cc:132: // (accessing things
like URLFetchers will crash).
On 2012/12/10 09:01:09, Mattias Nissler wrote:
> nit: Can you elaborate why it crashes and what the thing is that we haven't
> initialized yet? My hope is that we can eventually declare our dependencies
via
> DependsOn() and move the code to the ctor; knowing what's missing would help.

Done.

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service.cc:172:
browser_policy_connector()->device_management_service();
On 2012/12/10 09:01:09, Mattias Nissler wrote:
> nit: declaring a |connector| local might improve readability.

Done.

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_signin_service.h (right):

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service.h:53: typedef
base::Callback<void(bool)> PolicyFetchCallback;
On 2012/12/10 09:01:09, Mattias Nissler wrote:
> nit: move before ctor

Done.

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

https://codereview.chromium.org/11444029/diff/22001/chrome/browser/policy/use...
chrome/browser/policy/user_policy_signin_service_unittest.cc:354:
ASSERT_TRUE(fetch_request);
On 2012/12/10 09:01:09, Mattias Nissler wrote:
> You probably want Mock::VerifyAndClearExpectations(device_management_service_)
> here.

Done.

[commit-bot: I haz the power, 2012-12-10 10:12:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/11444029/28002

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

Change committed as 172043