Pass Device ID in the oauth2/token request. Keep Device ID in local state on Chrome OS.

Implemented sending Device ID to LSO in "oauth2/token".
The only place where Device ID is stored on Chrome OS is the local state from now on.

Order of events:
1) User logs in. There are two options:
  a) User athenticated through GAIA. We read known user device ID from local state if it exists or generate a new one if it doesn't. Then we save the device ID in a user context.
  b) Otherwise, we don't store device ID in user context.
2) If it is GAIA_WITHOUT_SAML-mode, we send device ID to LSO in "oauth2/token" request (OAuth2TokenInitializer class). If it is not, we'll send it in "o/oauth2/programmatic_auth" request on step (5).
3) After NotifyUserLoggedIn() call, we store the device ID from the user context (if it is set) to local state. Then we start to create/load a profile for the user.
4) In constructor of ChromeSinginClient we migrate device id from kGoogleServicesSigninScopedDeviceId pref to the local state, if a value in the local state is empty.
5) We request tokens with "o/oauth2/programmatic_auth" if we haven't done that on step (2). Old code for sending device ID works here, because it relies on ChromeSigninClient which returns device ID set on step (4).

BUG=486044, 486136
Committed: https://crrev.com/a912deca5f760681835e4247225c1c83b656d0b5
Cr-Commit-Position: refs/heads/master@{#330207}

[dzhioev (left Google), 2015-05-12 06:11:07]

The CQ bit was checked by dzhioev@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-12 06:11:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138143002/1

[commit-bot: I haz the power, 2015-05-12 07:57:33]

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

[commit-bot: I haz the power, 2015-05-12 07:57:33]

Dry run: This issue passed the CQ dry run.

[dzhioev (left Google), 2015-05-12 22:09:18]

The CQ bit was checked by dzhioev@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-12 22:10:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138143002/20001

[dzhioev (left Google), 2015-05-12 22:48:55]

dzhioev@chromium.org changed reviewers:
+ achuith@chromium.org, pavely@chromium.org

[dzhioev (left Google), 2015-05-12 22:48:56]

Hello, PTAL.

Tests to follow, I'm working on them.

[commit-bot: I haz the power, 2015-05-12 22:57:41]

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

[commit-bot: I haz the power, 2015-05-12 22:57:42]

Dry run: This issue passed the CQ dry run.

[achuithb, 2015-05-12 23:25:00]

achuith@chromium.org changed reviewers:
+ xiyuan@chromium.org

[achuithb, 2015-05-12 23:25:02]

Where does step 5 happen?"o/oauth2/programmatic_auth" part.

Most of the following are just nits.

I'm only familiar with about 50% of this flow. I'm going to add Xiyuan to take a
look as well.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:1126: const
UserContext& user_context_wo_device_id) {
Maybe call this user_context, and use uc for the stack variable?

we have a few other places where we do something similar to filling in some
state of user_context, and we don't have variable names like this in those
places.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:1128: std::string
known_device_id =
I'd suggest merging the variables known_device_id and new_device_id into
device_id. I think it would simplify the logic below.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc:25:
NOTREACHED() << "Device ID is not set";
I assume you're doing this in preference to a DCHECK because you want the
logging in release mode?

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:61: // Need to move device ID from
the old location to the new one, if it has not
Should this code be retired after a few milestones?

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:63: std::string legacy_device_id =
nit: const

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:65: user_manager::UserManager*
user_manager = user_manager::UserManager::Get();
nit: could potentially use auto here since the type is clear - up to you.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:159: std::string
signin_scoped_device_id =
nit: const

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:595: // Kept for
future use.
What does this comment mean?

https://codereview.chromium.org/1138143002/diff/20001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:1151: std::string known_device_id =
GetKnownUserDeviceId(user_id);
nit: const

https://codereview.chromium.org/1138143002/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:1152: if (!known_device_id.empty()
&& device_id != known_device_id) {
Should this just be a DCHECK?

https://codereview.chromium.org/1138143002/diff/20001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:360: if (!device_id.empty())
nit: Believe you need {} here?

[pavely, 2015-05-13 00:05:52]

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:427: if
(!user_context_.GetDeviceId().empty())
nit: Consider using user_context instead of user_context_? From reading just
this function it is not clear where user_context got copied into user_context_
(I know it happened in CreateUserSession)

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:69: }
Could you add DCHECK that fires when both device_ids nonempty and don't match?

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:162: LOG(ERROR) << "Device ID is
not set for user.";
How device_id will be set for users that went straight from M36 to M44? They
wouldn't have device_id set in Preferences in cryptohome and so it will be empty
here as well.
Do we care about such case?
One solution would be to say that such users will have to sign out and sign in
again and by doing so they will go through codepath that generates new
device_id.

[dzhioev (left Google), 2015-05-13 00:25:18]

Step 5 happens when we follow regular non-auth_code authentication path. See
OAuth2LoginManager::FetchOAuth2Tokens()
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:1126: const
UserContext& user_context_wo_device_id) {
On 2015/05/12 23:25:01, achuithb wrote:
> Maybe call this user_context, and use uc for the stack variable?
> 
> we have a few other places where we do something similar to filling in some
> state of user_context, and we don't have variable names like this in those
> places.

'user_context' variable is used 5-6 times in this method. I prefer not to change
its name to keep history clear. I can rename it, if you insist.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:1128: std::string
known_device_id =
On 2015/05/12 23:25:01, achuithb wrote:
> I'd suggest merging the variables known_device_id and new_device_id into
> device_id. I think it would simplify the logic below.
> 

Done.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc:25:
NOTREACHED() << "Device ID is not set";
On 2015/05/12 23:25:01, achuithb wrote:
> I assume you're doing this in preference to a DCHECK because you want the
> logging in release mode?

Yes, on Chrome OS NOTREACHED writes logs in release mode and crashes in Debug
mode. That is what I want, but I don't know another simple way to do this.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:61: // Need to move device ID from
the old location to the new one, if it has not
On 2015/05/12 23:25:01, achuithb wrote:
> Should this code be retired after a few milestones?

I believe it shouldn't. We can't be sure that every existing user has signed in
at least once, no matter how much time passed.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:63: std::string legacy_device_id =
On 2015/05/12 23:25:01, achuithb wrote:
> nit: const

Done.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:65: user_manager::UserManager*
user_manager = user_manager::UserManager::Get();
On 2015/05/12 23:25:01, achuithb wrote:
> nit: could potentially use auto here since the type is clear - up to you.

Sure

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:159: std::string
signin_scoped_device_id =
On 2015/05/12 23:25:01, achuithb wrote:
> nit: const

Done.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:595: // Kept for
future use.
On 2015/05/12 23:25:01, achuithb wrote:
> What does this comment mean?
I believe we are going to make whitelist checking here.
I'm OK with removing this method though, it can be returned back easily when
needed. Removed it.

https://codereview.chromium.org/1138143002/diff/20001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:1151: std::string known_device_id =
GetKnownUserDeviceId(user_id);
On 2015/05/12 23:25:01, achuithb wrote:
> nit: const

Done.

https://codereview.chromium.org/1138143002/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:1152: if (!known_device_id.empty()
&& device_id != known_device_id) {
On 2015/05/12 23:25:01, achuithb wrote:
> Should this just be a DCHECK?
Want this to be logged in Release mode.

https://codereview.chromium.org/1138143002/diff/20001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.cc:360: if (!device_id.empty())
On 2015/05/12 23:25:01, achuithb wrote:
> nit: Believe you need {} here?

Done.

[xiyuan, 2015-05-13 01:18:02]

LGTM with nits

Please wait for other reviewers to sign off as well.

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:427: if
(!user_context_.GetDeviceId().empty())
nit: {} since the branch is more than one line.

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc:25:
NOTREACHED() << "Device ID is not set";
NOTREACHED() does not do much for ChromeOS. Most of the time, we don't have
access to chrome logs for failures. If we don't allow empty device id, we should
CHECK or LOG(FATAL) to get crash report.

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:162: LOG(ERROR) << "Device ID is
not set for user.";
nit: LOG_IF(ERROR, signin_scoped_device_id.empty()) << ...

https://codereview.chromium.org/1138143002/diff/40001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/components/user_manager...
components/user_manager/user_manager_base.cc:1153: NOTREACHED() << "Trying to
change device ID for known user.";
nit: LOG(WARNING)/LOG(ERROR) or LOG(FATAL) to get a crash report.

[dzhioev (left Google), 2015-05-13 20:24:22]

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:427: if
(!user_context_.GetDeviceId().empty())
On 2015/05/13 00:05:52, pavely wrote:
> nit: Consider using user_context instead of user_context_? From reading just
> this function it is not clear where user_context got copied into user_context_
> (I know it happened in CreateUserSession)
Done.

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:69: }
On 2015/05/13 00:05:52, pavely wrote:
> Could you add DCHECK that fires when both device_ids nonempty and don't match?

Unfortunately such thing can happen, and I don't know how to prevent it. It is
an edge case of migration process. This happens if user uses GAIA for the first
sign in after upgrade to M44:
1) User chooses "Add User" and inputs credentials of existing user.
2) User is forced to use GAIA because his token is invalid or user pods are not
displayed (there is a special preference for that).

As I said, this is an edge case. Will it break anything (expect the fact that
new device_id will be issued)? We can detect this situation after sign in and
react somehow (for example revoke old device id, if it possible).

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:162: LOG(ERROR) << "Device ID is
not set for user.";
On 2015/05/13 00:05:52, pavely wrote:
> How device_id will be set for users that went straight from M36 to M44? They
> wouldn't have device_id set in Preferences in cryptohome and so it will be
empty
> here as well.
> Do we care about such case?
> One solution would be to say that such users will have to sign out and sign in
> again and by doing so they will go through codepath that generates new
> device_id.
Oh, I missed this case.
We can catch this by checking if both KnownUserDeviceID and
kGoogleServicesSigninScopedDeviceId are empty in ChromeSingninClient
constructor. Done.

[pavely, 2015-05-13 22:05:33]

lgtm

[Nikita (slow), 2015-05-14 06:29:03]

nkostylev@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Nikita (slow), 2015-05-14 06:29:03]

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:69: }
On 2015/05/13 20:24:22, dzhioev wrote:
> On 2015/05/13 00:05:52, pavely wrote:
> > Could you add DCHECK that fires when both device_ids nonempty and don't
match?
> 
> Unfortunately such thing can happen, and I don't know how to prevent it. It is
> an edge case of migration process. This happens if user uses GAIA for the
first
> sign in after upgrade to M44:
> 1) User chooses "Add User" and inputs credentials of existing user.
> 2) User is forced to use GAIA because his token is invalid or user pods are
not
> displayed (there is a special preference for that).
> 
> As I said, this is an edge case. Will it break anything (expect the fact that
> new device_id will be issued)? We can detect this situation after sign in and
> react somehow (for example revoke old device id, if it possible).
> 
> 

This is an expected edge case of a migration flow with M44. See
go/cros-auth-design, flow 1, step 32 (specifically red line).

[Nikita (slow), 2015-05-14 06:34:14]

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/20001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:69: }
On 2015/05/14 06:29:03, Nikita wrote:
> On 2015/05/13 20:24:22, dzhioev wrote:
> > On 2015/05/13 00:05:52, pavely wrote:
> > > Could you add DCHECK that fires when both device_ids nonempty and don't
> match?
> > 
> > Unfortunately such thing can happen, and I don't know how to prevent it. It
is
> > an edge case of migration process. This happens if user uses GAIA for the
> first
> > sign in after upgrade to M44:
> > 1) User chooses "Add User" and inputs credentials of existing user.
> > 2) User is forced to use GAIA because his token is invalid or user pods are
> not
> > displayed (there is a special preference for that).
> > 
> > As I said, this is an edge case. Will it break anything (expect the fact
that
> > new device_id will be issued)? We can detect this situation after sign in
and
> > react somehow (for example revoke old device id, if it possible).
> > 
> > 
> 
> This is an expected edge case of a migration flow with M44. See
> go/cros-auth-design, flow 1, step 32 (specifically red line).

After such edge case happens and newly created device ID is passed to GAIA, same
device ID needs to used by sync: part of
https://code.google.com/p/chromium/issues/detail?id=486136

[dzhioev (left Google), 2015-05-14 23:50:52]

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:427: if
(!user_context_.GetDeviceId().empty())
On 2015/05/13 01:18:02, xiyuan wrote:
> nit: {} since the branch is more than one line.

Done.

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc:25:
NOTREACHED() << "Device ID is not set";
On 2015/05/13 01:18:02, xiyuan wrote:
> NOTREACHED() does not do much for ChromeOS. Most of the time, we don't have
> access to chrome logs for failures. If we don't allow empty device id, we
should
> CHECK or LOG(FATAL) to get crash report.

That should never happen in theory. But it is not a critical error, that is why
don't want use CHECK here.
NOTREACHED() will fail in Debug tests. In Release it'll produce error message.
That is what I want.

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:162: LOG(ERROR) << "Device ID is
not set for user.";
On 2015/05/13 01:18:02, xiyuan wrote:
> nit: LOG_IF(ERROR, signin_scoped_device_id.empty()) << ...

Done.

https://codereview.chromium.org/1138143002/diff/40001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1138143002/diff/40001/components/user_manager...
components/user_manager/user_manager_base.cc:1153: NOTREACHED() << "Trying to
change device ID for known user.";
On 2015/05/13 01:18:02, xiyuan wrote:
> nit: LOG(WARNING)/LOG(ERROR) or LOG(FATAL) to get a crash report.

Ditto.

[dzhioev (left Google), 2015-05-14 23:54:59]

Achuith, do you have more to say?

I'm still working on tests. They happened to be pretty complex and required some
changes in FakeGaia. When I'll finish with them, I'll send them in a separate
CL.

[achuithb, 2015-05-14 23:55:51]

On 2015/05/14 23:54:59, dzhioev wrote:
> Achuith, do you have more to say?
> 
> I'm still working on tests. They happened to be pretty complex and required
some
> changes in FakeGaia. When I'll finish with them, I'll send them in a separate
> CL.

lgtm.

Please land tests separately. They are lower risk and should be taken into the
branch.

[dzhioev (left Google), 2015-05-15 00:00:24]

dzhioev@chromium.org changed reviewers:
+ rogerta@chromium.org

[dzhioev (left Google), 2015-05-15 00:00:25]

I realized that this CL is still missing OWNERS LGTM for
google_apis/gaia/gaia_auth_fetcher.* and
chrome/browser/signin/chrome_signin_client.*.

Roger, PTAL.

[Nikita (slow), 2015-05-15 03:52:47]

The CQ bit was checked by nkostylev@chromium.org to run a CQ dry run

[Nikita (slow), 2015-05-15 03:52:48]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/1138143002/#ps80001
(title: "Xiyuan comments.")

[commit-bot: I haz the power, 2015-05-15 03:53:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138143002/80001

[dzhioev (left Google), 2015-05-15 04:56:17]

FYI: here is a CL with tests https://codereview.chromium.org/1141163002/

[commit-bot: I haz the power, 2015-05-15 06:21:14]

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

[commit-bot: I haz the power, 2015-05-15 06:21:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Roger Tawa OOO till Jul 10th, 2015-05-15 13:50:24]

Looks pretty much OK, but this code is cros specific when it should also support
desktop.  Due to time constraints, I'm willing to L G T M if there is a
commitment to refactor this code as mentioned in my comments before as soon as
M44 branches.  Please open a crbug and attach to this CL before committing.  
Thanks.

https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:81: std::string());
Why is this cros specific?  Desktop needs this too.

https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:112: std::string
ChromeSigninClient::GenerateSigninScopedDeviceID(
nit: please add "// static" comment

https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:180: #endif
This has to be done for desktop too.  Desktop's profile lock feature needs this
for when we need to reauth outside the context of a profile.

[Roger Tawa OOO till Jul 10th, 2015-05-15 16:39:12]

On 2015/05/15 13:50:24, Roger Tawa wrote:
> Looks pretty much OK, but this code is cros specific when it should also
support
> desktop.  Due to time constraints, I'm willing to L G T M if there is a
> commitment to refactor this code as mentioned in my comments before as soon as
> M44 branches.  Please open a crbug and attach to this CL before committing.  
> Thanks.
> 
>
https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
> File chrome/browser/signin/chrome_signin_client.cc (right):
> 
>
https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
> chrome/browser/signin/chrome_signin_client.cc:81: std::string());
> Why is this cros specific?  Desktop needs this too.
> 
>
https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
> chrome/browser/signin/chrome_signin_client.cc:112: std::string
> ChromeSigninClient::GenerateSigninScopedDeviceID(
> nit: please add "// static" comment
> 
>
https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
> chrome/browser/signin/chrome_signin_client.cc:180: #endif
> This has to be done for desktop too.  Desktop's profile lock feature needs
this
> for when we need to reauth outside the context of a profile.

Chatter with Pavel offline.  He committed to refactoring this code as soon as
M44 branches.  I opened this bug to track:

https://code.google.com/p/chromium/issues/detail?id=488517

lgtm with my previous nit.

[dzhioev (left Google), 2015-05-15 20:05:39]

Thanks everybody

https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/1138143002/diff/80001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:112: std::string
ChromeSigninClient::GenerateSigninScopedDeviceID(
On 2015/05/15 13:50:23, Roger Tawa wrote:
> nit: please add "// static" comment

Done.

[dzhioev (left Google), 2015-05-15 20:05:44]

The CQ bit was checked by dzhioev@chromium.org

[dzhioev (left Google), 2015-05-15 20:05:45]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
achuith@chromium.org, rogerta@chromium.org, pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/1138143002/#ps100001
(title: "Added comment.")

[commit-bot: I haz the power, 2015-05-15 20:07:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138143002/100001

[commit-bot: I haz the power, 2015-05-15 21:53:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-18 11:28:00]

Patchset 6 (id:??) landed as
https://crrev.com/a912deca5f760681835e4247225c1c83b656d0b5
Cr-Commit-Position: refs/heads/master@{#330207}