Implementation of SigninClient::GetSigninScopedDeviceId

Implementation of SigninClient::GetSigninScopedDeviceId

This code stores device id in prefs. Device id needs to be cleared
when user signs out and, just in case, right before user signs in. In
all other cases chrome should keep old id. Code to send id is behind
command line flag for now.

This change covers desktop scenarios, doesn't cover chromeos. 
I'll do that in followup changes.

BUG=382968
R=rogerta@chromium.org

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

[pavely, 2014-06-30 23:31:09]

Roger, could you take a look?

The main goal of this change was to clear device id on signout and right before
signin and not to regenerate it unnecessarily.
Other places where device_id is used are:
- account_reconcilor for mirror.
- inline_login_handler_impl for entering credentials.

Thanks, Pavel.

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:664:
GenerateNewSigninScopedDeviceId(args_.profile);
This is where new id generated before signing in.

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_manager.cc (right):

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_manager.cc:206:
client_->ClearSigninScopedDeviceId();
This is where id is cleared on signout.

[pavely, 2014-07-01 00:23:29]

Also, is there a good existing test that tests sign-in/sign-out scenarios where
I could verify this functionality?

[Roger Tawa OOO till Jul 10th, 2014-07-01 13:42:54]

Looks good Pavel.  Some questions below.

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:664:
GenerateNewSigninScopedDeviceId(args_.profile);
On 2014/06/30 23:31:09, pavely wrote:
> This is where new id generated before signing in.

For mirror, sign in occurs in inline_login_handler_impl.cc.  Should't we call
ClearSigninScopedDeviceId() there when the source !=
signin::SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT && source != signin::SOURCE_REAUTH?

How about cros?  Will you handle that is a separate CL?

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_manager.cc (right):

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_manager.cc:206:
client_->ClearSigninScopedDeviceId();
On 2014/06/30 23:31:09, pavely wrote:
> This is where id is cleared on signout.

I wonder if it would be better to have a SignOut() method in client_ and move
all the pref clearing to there.

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_oauth_helper.cc (right):

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_oauth_helper.cc:23:
switches::kEnableNewProfileManagement)) {
Please use switches::IsNewProfileManagement() from
components/signin/core/common/profile_management_switches.h instead of this
command line switch directly.

Why do we need to check this here?  Did you mean to use the new command line
flag you added?

[pavely, 2014-07-01 21:41:47]

PTAL

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:664:
GenerateNewSigninScopedDeviceId(args_.profile);
Ok, it seems like there are multiple codepaths that request refresh token and no
future-proof way to tell if particular call is new signin or reauth. I'll remove
call to clear device id from here and only leave one in signout codepath.

We'll see, maybe I'll find a better place to clear device_id on signin when i
know more details about how signin/signout works on chromeOS.

BTW, Where signin starts in case of mirror? Where should I start tracing code?

Yes, I'll handle CrOS in separate change.

On 2014/07/01 13:42:54, Roger Tawa wrote:
> On 2014/06/30 23:31:09, pavely wrote:
> > This is where new id generated before signing in.
> 
> For mirror, sign in occurs in inline_login_handler_impl.cc.  Should't we call
> ClearSigninScopedDeviceId() there when the source !=
> signin::SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT && source != signin::SOURCE_REAUTH?
> 
> How about cros?  Will you handle that is a separate CL?

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_manager.cc (right):

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_manager.cc:206:
client_->ClearSigninScopedDeviceId();
I wanted to clear device id right before signing in so separate method is
needed. I removed this code for now, but I'll keep looking if there is a good
place to call it before sign-in.

Also I think knowing to clear kGoogleServicesUsername is signin_manager's
responsibility, signin_client just provides means to do things without
introducing dependency on chrome/browser/... code.

I think it should stay the way it is now.

On 2014/07/01 13:42:54, Roger Tawa wrote:
> On 2014/06/30 23:31:09, pavely wrote:
> > This is where id is cleared on signout.
> 
> I wonder if it would be better to have a SignOut() method in client_ and move
> all the pref clearing to there.

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_oauth_helper.cc (right):

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_oauth_helper.cc:23:
switches::kEnableNewProfileManagement)) {
On 2014/07/01 13:42:54, Roger Tawa wrote:
> Please use switches::IsNewProfileManagement() from
> components/signin/core/common/profile_management_switches.h instead of this
> command line switch directly.
> 
> Why do we need to check this here?  Did you mean to use the new command line
> flag you added?
Sorry, missed the line when copy pasting from switches header. 
The idea is for me to be able to experiment with this code while having the
whole feature disabled by default for everyone until all pieces are ready.
Now that I think about it, the other side effect of this change is pref value in
chrome_signin_client. I'd better put this condition there.

[Roger Tawa OOO till Jul 10th, 2014-07-03 15:07:41]

lgtm

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/362613002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:664:
GenerateNewSigninScopedDeviceId(args_.profile);
On 2014/07/01 21:41:47, pavely wrote:
> Ok, it seems like there are multiple codepaths that request refresh token and
no
> future-proof way to tell if particular call is new signin or reauth. I'll
remove
> call to clear device id from here and only leave one in signout codepath.
> 
> We'll see, maybe I'll find a better place to clear device_id on signin when i
> know more details about how signin/signout works on chromeOS.
> 
> BTW, Where signin starts in case of mirror? Where should I start tracing code?
> 
> Yes, I'll handle CrOS in separate change.
> 
> On 2014/07/01 13:42:54, Roger Tawa wrote:
> > On 2014/06/30 23:31:09, pavely wrote:
> > > This is where new id generated before signing in.
> > 
> > For mirror, sign in occurs in inline_login_handler_impl.cc.  Should't we
call
> > ClearSigninScopedDeviceId() there when the source !=
> > signin::SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT && source != signin::SOURCE_REAUTH?
> > 
> > How about cros?  Will you handle that is a separate CL?
> 

In mirror, start tracing from  InlineLoginHandlerImpl::CompleteLogin(),
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_manager.cc (right):

https://codereview.chromium.org/362613002/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_manager.cc:206:
client_->ClearSigninScopedDeviceId();
On 2014/07/01 21:41:47, pavely wrote:
> I wanted to clear device id right before signing in so separate method is
> needed. I removed this code for now, but I'll keep looking if there is a good
> place to call it before sign-in.
> 
> Also I think knowing to clear kGoogleServicesUsername is signin_manager's
> responsibility, signin_client just provides means to do things without
> introducing dependency on chrome/browser/... code.
> 
> I think it should stay the way it is now.
> 
> On 2014/07/01 13:42:54, Roger Tawa wrote:
> > On 2014/06/30 23:31:09, pavely wrote:
> > > This is where id is cleared on signout.
> > 
> > I wonder if it would be better to have a SignOut() method in client_ and
move
> > all the pref clearing to there.
> 

sgtm

[pavely, 2014-07-07 17:28:32]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-07-07 17:29:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pavely@chromium.org/362613002/20001

[commit-bot: I haz the power, 2014-07-07 19:31:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-07-07 19:44:42]

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

[commit-bot: I haz the power, 2014-07-07 19:44:43]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[pavely, 2014-07-07 20:22:32]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-07-07 20:22:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pavely@chromium.org/362613002/20001

[commit-bot: I haz the power, 2014-07-07 21:34:46]

Change committed as 281589