[sync]: add ManagedUserSigninManagerWrapper and pass to ProfileSyncService

[sync]: add ManagedUserSigninManagerWrapper and pass to ProfileSyncService

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

[tim (not reviewing), 2014-02-11 20:43:14]

Here's a shot at implementing what was discussed on the email thread earlier!

Bernhard, Roger, & Pavel - please review everything since this touches 3
different (managed, signin, sync) logical parts of ProfileSyncService.

[Roger Tawa OOO till Jul 10th, 2014-02-11 21:05:36]

I see the "effective" username/account-id as profile level concepts, not
something specific to managed users.  I think I would call this class something
like EffectiveUserInfoService, and move it to chrome\browser\profiles.

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_signin_manager_wrapper.cc (right):

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_signin_manager_wrapper.cc:49: return
original_->GetAuthenticatedUsername();
Please use original_->GetAuthenticatedAccountId()

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_signin_manager_wrapper.h (right):

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_signin_manager_wrapper.h:23: Profile*
profile, SigninManagerBase* original);
When is profile != original->profile() ?  Seems like it would be better to have
only |original| arg.

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:935:
scoped_ptr<ManagedUserSigninManagerWrapper> signin_;
If PSS will own the wrapper, then it seems the ctor should keep its original
args, and the wrapper should be allocated in the ctor itself.  Is there an
advantage to injecting an instance in the ctor?  All prod and test code use the
same implementation.

[tim (not reviewing), 2014-02-11 21:24:13]

Thanks Roger, some responses inline.

As for the profile-level concept aspect... I'm not sure?  I do agree with your
proposal on the thread that down the road moving this to SigninManager makes
sense, but right now the only thing that's happening here is that PSS supports
managed users, so if you use a PSS you should be aware that it can be working
with a real Google Account *or* a managed user. In other words, I can see this
living as part of "primary account management / signinManager et al" or, in the
mean time, a composition of signin and managed mode for things that care about
managed mode. WDYT?

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_signin_manager_wrapper.cc (right):

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_signin_manager_wrapper.cc:49: return
original_->GetAuthenticatedUsername();
On 2014/02/11 21:05:37, Roger Tawa wrote:
> Please use original_->GetAuthenticatedAccountId()

Done.

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_signin_manager_wrapper.h (right):

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_signin_manager_wrapper.h:23: Profile*
profile, SigninManagerBase* original);
On 2014/02/11 21:05:37, Roger Tawa wrote:
> When is profile != original->profile() ?  Seems like it would be better to
have
> only |original| arg.

Good point!  Done.

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/137753012/diff/30001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:935:
scoped_ptr<ManagedUserSigninManagerWrapper> signin_;
On 2014/02/11 21:05:37, Roger Tawa wrote:
> If PSS will own the wrapper, then it seems the ctor should keep its original
> args, and the wrapper should be allocated in the ctor itself.  Is there an
> advantage to injecting an instance in the ctor?  All prod and test code use
the
> same implementation.

I thought about this, but ultimately felt it was better to make the dependency
on managed mode, and the fact that it's not a pure "signin" dependency,
well-known to someone creating a ProfileSyncService. If "signin" was aware of
Managed Users (maybe some day?) then I think a SigninManagerBase would make
sense. Another benefit (unrealized by this patch) might be that it's a little
simpler to test managed mode behavior of PSS this way.

[pavely, 2014-02-11 22:24:17]

lgtm with one comment.

https://codereview.chromium.org/137753012/diff/150001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/137753012/diff/150001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:89: #include
"chrome/browser/managed_mode/managed_user_constants.h"
Do you need to remove this include as well?

[Roger Tawa OOO till Jul 10th, 2014-02-12 14:55:16]

lgtm

Sound good Tim. We can flesh out the profile level ideas when the requirements
are clearer.

[Bernhard Bauer, 2014-02-12 14:59:49]

LGTM!

[tim (not reviewing), 2014-02-12 17:23:43]

The CQ bit was checked by tim@chromium.org

[tim (not reviewing), 2014-02-12 17:23:51]

The CQ bit was unchecked by tim@chromium.org

[tim (not reviewing), 2014-02-12 17:24:41]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-02-12 17:25:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/137753012/150001

[commit-bot: I haz the power, 2014-02-12 20:56:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 20:56:04]

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

[tim (not reviewing), 2014-02-12 21:02:13]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-02-12 21:03:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/137753012/150001

[commit-bot: I haz the power, 2014-02-12 22:44:33]

Change committed as 250814

[jschuh, 2014-02-13 05:27:16]

On 2014/02/12 22:44:33, I haz the power (commit-bot) wrote:
> Change committed as 250814

This appears to have blown up Win64, but I'd prefer not to speculatively revert.
Could you investigate?
http://build.chromium.org/p/chromium.win/builders/Win7%20Sync%20x64/builds/11201

[tim (not reviewing), 2014-02-13 07:06:54]

Shoot. I can't really dig into this right now and it's hard to tell from a
cursory look at the log. I'll revert using rietveld, I'm fine trying this
again tomorrow either way.


On Wed, Feb 12, 2014 at 9:27 PM, <jschuh@chromium.org> wrote:

> On 2014/02/12 22:44:33, I haz the power (commit-bot) wrote:
>
>> Change committed as 250814
>>
>
> This appears to have blown up Win64, but I'd prefer not to speculatively
> revert.
> Could you investigate?
> http://build.chromium.org/p/chromium.win/builders/Win7%
> 20Sync%20x64/builds/11201
>
>
> https://codereview.chromium.org/137753012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tim (not reviewing), 2014-02-13 07:07:03]

A revert of this CL has been created in
https://codereview.chromium.org/162943002/ by tim@chromium.org.

The reason for reverting is: Possible build bustage on Win64..