Load policy before signin completes.

Load policy before signin completes.

Updated SigninManager to call into UserPolicySigninService to register for policy and fetch policy as appropriate.

Refactored UserPolicySigninService to expose an external API to register for policy.

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

[Andrew T Wilson (Slow), 2013-02-07 14:58:33]

Joao, PTAL at browser/policy
Roger, PTAL at browser/signin

[Andrew T Wilson (Slow), 2013-02-07 16:52:40]

On 2013/02/07 14:58:33, Andrew T Wilson wrote:
> Joao, PTAL at browser/policy
> Roger, PTAL at browser/signin

Uploaded a test fix - please take a look!

[Roger Tawa OOO till Jul 10th, 2013-02-07 17:47:17]

lgtm for signin.  Any tests worth writing?

[Joao da Silva, 2013-02-07 23:01:25]

lgtm with nits, and a suggestion to remove a callback between 2 methods (and
just let the 1st kick the 2nd; your call).

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

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:76: //
CloudPolicyClient::Observer implemnetation.
*implementation

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:79: virtual void
OnClientError(policy::CloudPolicyClient* client);
OVERRIDE on these 3

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:137:
std::vector<std::string> scopes;
#include <vector>

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:150: DLOG(WARNING) << "Could
not fetch access token for"
space before the closing "

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:187: // CloudPolicyClient
and make requests from DMServer.
from -> to?

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:295: }
At this point, why not kick off the fetch from this class instead of the jump
through the signin_manager?

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:480: if
(registration_helper_.get())
if (registration_helper_) also works now

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

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:48: typedef
base::Callback<void(scoped_ptr<CloudPolicyClient>)>
#include "base/callback.h"

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:59: // Returns a DMToken for
fetching policy for a user. The
I guess this passes a CloudPolicyClient to the callback, not a dmtoken?

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:124:
base::WeakPtrFactory<UserPolicySigninService> weak_factory_;
This is not used anymore.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:501:
base::Unretained(this)));
These 6 or 8 lines are quite common, move to a helper method?

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:20: #include
"chrome/browser/policy/user_policy_signin_service_factory.h"
#ifdef ENABLE_CONFIGURATION_POLICY

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:694: void
SigninManager::OnRegisteredForPolicy(
Does this have to exist? policy_service->RegisterPolicyClient() could also start
the fetch, if the registration succeeded; otherwise it immediately calls back to
CompleteSigninAfterPolicyLoad().

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:712: 
remove extra newline

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:240: // fails, |policy_token| will be
empty.
|policy_token| -> |client|?

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:245: void OnPolicyFetchComplete(bool
success);
These 2 methods #ifdefd

[Andrew T Wilson (Slow), 2013-02-08 10:48:07]

Thanks for the review!

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

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:76: //
CloudPolicyClient::Observer implemnetation.
On 2013/02/07 23:01:25, Joao da Silva wrote:
> *implementation

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:79: virtual void
OnClientError(policy::CloudPolicyClient* client);
On 2013/02/07 23:01:25, Joao da Silva wrote:
> OVERRIDE on these 3

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:137:
std::vector<std::string> scopes;
On 2013/02/07 23:01:25, Joao da Silva wrote:
> #include <vector>

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:150: DLOG(WARNING) << "Could
not fetch access token for"
On 2013/02/07 23:01:25, Joao da Silva wrote:
> space before the closing "

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:187: // CloudPolicyClient
and make requests from DMServer.
On 2013/02/07 23:01:25, Joao da Silva wrote:
> from -> to?

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:295: }
On 2013/02/07 23:01:25, Joao da Silva wrote:
> At this point, why not kick off the fetch from this class instead of the jump
> through the signin_manager?
Because ultimately I want signin UI to be driven from the signin code, not from
the policy code. Eventually we'll probably add other cases where we create a new
profile for signin so the logic should be in that class.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:480: if
(registration_helper_.get())
On 2013/02/07 23:01:25, Joao da Silva wrote:
> if (registration_helper_) also works now

Done.

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

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:48: typedef
base::Callback<void(scoped_ptr<CloudPolicyClient>)>
On 2013/02/07 23:01:25, Joao da Silva wrote:
> #include "base/callback.h"

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:59: // Returns a DMToken for
fetching policy for a user. The
On 2013/02/07 23:01:25, Joao da Silva wrote:
> I guess this passes a CloudPolicyClient to the callback, not a dmtoken?

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:124:
base::WeakPtrFactory<UserPolicySigninService> weak_factory_;
On 2013/02/07 23:01:25, Joao da Silva wrote:
> This is not used anymore.

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:501:
base::Unretained(this)));
On 2013/02/07 23:01:25, Joao da Silva wrote:
> These 6 or 8 lines are quite common, move to a helper method?

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:20: #include
"chrome/browser/policy/user_policy_signin_service_factory.h"
On 2013/02/07 23:01:25, Joao da Silva wrote:
> #ifdef ENABLE_CONFIGURATION_POLICY

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:694: void
SigninManager::OnRegisteredForPolicy(
On 2013/02/07 23:01:25, Joao da Silva wrote:
> Does this have to exist? policy_service->RegisterPolicyClient() could also
start
> the fetch, if the registration succeeded; otherwise it immediately calls back
to
> CompleteSigninAfterPolicyLoad().

Yeah, per our previous conversation we want to put the logic and UI for creating
a profile out here.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:712: 
On 2013/02/07 23:01:25, Joao da Silva wrote:
> remove extra newline

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (right):

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:240: // fails, |policy_token| will be
empty.
On 2013/02/07 23:01:25, Joao da Silva wrote:
> |policy_token| -> |client|?

Done.

https://codereview.chromium.org/12220060/diff/1013/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:245: void OnPolicyFetchComplete(bool
success);
On 2013/02/07 23:01:25, Joao da Silva wrote:
> These 2 methods #ifdefd

Done.

[commit-bot: I haz the power, 2013-02-08 10:48:18]

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

[commit-bot: I haz the power, 2013-02-08 12:53:51]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...