Add account_type into AccountId

Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Committed: https://crrev.com/fb06787846a2f824cfe48ab4118e000e89249564
Cr-Original-Commit-Position: refs/heads/master@{#440152}
Cr-Commit-Position: refs/heads/master@{#440412}

[Roman Sorokin (ftl), 2016-11-25 14:33:47]

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

[commit-bot: I haz the power, 2016-11-25 14:34:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2016-11-25 14:34:08]

rsorokin@chromium.org changed reviewers:
+ alemate@chromium.org

[Roman Sorokin (ftl), 2016-11-25 14:34:09]

Alex, PTAL

[commit-bot: I haz the power, 2016-11-25 15:29:52]

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

[commit-bot: I haz the power, 2016-11-25 15:29:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2016-11-27 08:41:54]

lgtm with nits.

Thank you for this CL!

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:118: return
std::string(kKeyGaiaIdPrefix) + gaia_id_;
nit: split this into several ifs.

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:121: else
nit: Probably you don't need final "else".

https://codereview.chromium.org/2529103002/diff/1/components/user_manager/kno...
File components/user_manager/known_user.cc (right):

https://codereview.chromium.org/2529103002/diff/1/components/user_manager/kno...
components/user_manager/known_user.cc:294: return (gaia_id.empty()
nit: no else

[Alexander Alekseev, 2016-11-27 08:48:45]

alemate@chromium.org changed reviewers:
+ atwilson@chromium.org, stevenjb@chromium.org

[Alexander Alekseev, 2016-11-27 08:48:46]

+stevenjb@ for:
  c/b/ui/webui/options/chromeos/user_image_source.cc

+atwilson@ for:
  components/signin/core/account_id/account_id.h

[Alexander Alekseev, 2016-11-27 08:51:29]

Also, please correct CL description.
This is not "temporary" as we are not going to "unroll" this CL any time in the
future. It is also correct for the current code.

We will add more code here, so it will change in the future. So I'd say "this is
a preparation for crbug.com/668130 .

[stevenjb, 2016-11-28 17:53:04]

c/b/ui/webui RS LGTM

[Andrew T Wilson (Slow), 2016-11-29 11:05:36]

Mostly, I don't think you should use strings as the account_type enum without a
strong justification (which isn't here).

Also, it's a code smell to add a widely-unused parameter to GetAccountId() - if
in general people don't care about the account type when calling GetAccountId()
then create a separate function for those few cases where we do care.

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:903:
user_manager::known_user::UpdateAccountType(user_context_.GetAccountId());
Why are you passing the account ID here as the account type? This is the problem
with defining account_type as a string - it's just too easy to pass in a random
string to these APIs.

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right):

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:180: // user has
already signed in.
Document what account_type is.

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:183: const
std::string& account_type) const;
Seems like most places are just passing an empty string as account_type. Should
we define a separate GetAccountIdWithType() or something like that, and leave
GetAccountId() alone? Would greatly shrink this patch.

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:60: : gaia_id_(gaia_id),
user_email_(user_email), account_type_(account_type) {}
Validate account_type here to make sure it's a valid value.

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:122: LOG(FATAL) << "Unknown
account type " << GetAccountType();
Agreed, just make this NOTREACHED() << "Unknown account type " <<
GetAccountType()

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.h:22: static const std::string
kGoogle;
Why are we defining account types as strings and not as an enum? It's less
type-safe this way (see my previous comment about passing a gaia_id as the
account type?)

[Alexander Alekseev, 2016-11-29 21:44:00]

On 2016/11/29 11:05:36, Andrew T Wilson (Slow) wrote:
> Mostly, I don't think you should use strings as the account_type enum without
a
> strong justification (which isn't here).

You should talk to Roger about this as it was his idea.
You are the two owners of this code ;)

> Also, it's a code smell to add a widely-unused parameter to GetAccountId() -
if
> in general people don't care about the account type when calling
GetAccountId()
> then create a separate function for those few cases where we do care.

Do you mean known_user::GetAccountId ? It is meant to be "here is everything I
know about the user, give me the real Account ID".
So this doesn't state if user cares about account type. This is just a part of
"all the information I know".

[Alexander Alekseev, 2016-11-30 04:18:42]

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right):

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:183: const
std::string& account_type) const;
On 2016/11/29 11:05:36, Andrew T Wilson (Slow) wrote:
> Seems like most places are just passing an empty string as account_type.
Should
> we define a separate GetAccountIdWithType() or something like that, and leave
> GetAccountId() alone? Would greatly shrink this patch.

Yes, it will. But I believe that people should always supply all the known
information. So I'd prefer the longer version.

On the other hand, call to "GetAccountId" is temporary for most of this patch
and will go away after most of chrome code will use it as user identifier.

[Roman Sorokin (ftl), 2016-12-01 16:27:16]

Description was changed from

==========
Add account_type into AccountId

Also add Active Directory type account
This CL is a temporary solution for Active Directory accounts.
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none
==========

to

==========
Add account_type into AccountId

Also add Active Directory type account

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none
==========

[Roman Sorokin (ftl), 2016-12-02 11:12:31]

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

[commit-bot: I haz the power, 2016-12-02 11:12:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2016-12-02 11:13:11]

Alex, Drew, PTAL

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:903:
user_manager::known_user::UpdateAccountType(user_context_.GetAccountId());
On 2016/11/29 11:05:36, Andrew T Wilson (Slow) wrote:
> Why are you passing the account ID here as the account type? This is the
problem
> with defining account_type as a string - it's just too easy to pass in a
random
> string to these APIs.

GetAccountId returns AccountId class. Actually that call means
UpdateAccountType(user_context_.GetAccountId(),
user_context_.GetAccountId().GetAccountType()).

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right):

https://codereview.chromium.org/2529103002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:180: // user has
already signed in.
On 2016/11/29 11:05:36, Andrew T Wilson (Slow) wrote:
> Document what account_type is.

Switched to enum class

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:60: : gaia_id_(gaia_id),
user_email_(user_email), account_type_(account_type) {}
On 2016/11/29 11:05:36, Andrew T Wilson (Slow) wrote:
> Validate account_type here to make sure it's a valid value.

Done.

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:118: return
std::string(kKeyGaiaIdPrefix) + gaia_id_;
On 2016/11/27 08:41:54, Alexander Alekseev wrote:
> nit: split this into several ifs.

Done.

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:121: else
On 2016/11/27 08:41:54, Alexander Alekseev wrote:
> nit: Probably you don't need final "else".

Done.

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.cc:122: LOG(FATAL) << "Unknown
account type " << GetAccountType();
On 2016/11/29 11:05:36, Andrew T Wilson (Slow) wrote:
> Agreed, just make this NOTREACHED() << "Unknown account type " <<
> GetAccountType()

Done.

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2529103002/diff/1/components/signin/core/acco...
components/signin/core/account_id/account_id.h:22: static const std::string
kGoogle;
On 2016/11/29 11:05:36, Andrew T Wilson (Slow) wrote:
> Why are we defining account types as strings and not as an enum? It's less
> type-safe this way (see my previous comment about passing a gaia_id as the
> account type?)

Switched to enum class AccountType

https://codereview.chromium.org/2529103002/diff/1/components/user_manager/kno...
File components/user_manager/known_user.cc (right):

https://codereview.chromium.org/2529103002/diff/1/components/user_manager/kno...
components/user_manager/known_user.cc:294: return (gaia_id.empty()
On 2016/11/27 08:41:54, Alexander Alekseev wrote:
> nit: no else

Done.

[commit-bot: I haz the power, 2016-12-02 12:35:49]

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

[commit-bot: I haz the power, 2016-12-02 12:35:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roman Sorokin (ftl), 2016-12-02 15:24:32]

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

[commit-bot: I haz the power, 2016-12-02 15:24:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 16:55:50]

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

[commit-bot: I haz the power, 2016-12-02 16:55:51]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2016-12-03 04:34:35]

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

[Alexander Alekseev, 2016-12-03 04:34:36]

Roger, please take a look at changing AccountId type from string to enum (what
atwilson@ proposed).

I am OK with that, but what do you think?
I believe it is easy to change it later to string if we need.

[Roger Tawa OOO till Jul 10th, 2016-12-05 16:34:32]

Hi Alexander,

Yes I think it makes sense to use an enum.  A few comments below.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:122: return
std::string(kKeyAdIdPrefix) + gaia_id_;
Will there be a later pass to change |gaia_id_| to something more generic?

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:124: return std::string();
Nit: use a switch() statement.  Also line 173.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:193: }
Since these are new methods, maybe not use "Gaia" in the name?  So something
like:

FromUserEmailIdAccountType()
FromIdAccountType()

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:194: 
These two new methods should have "// static" comment.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:235: account_type =
StringToAccountType(account_type_string);
I think you should do some error checking here since the serialized source is
external.  Probably OK not to do error checking at line 199 above.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.h:36: const AccountType&
GetAccountType() const;
Is there reason to keep this as a const &?  I would suggest removing the
reference from here and all methods below.

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

https://codereview.chromium.org/2529103002/diff/40001/components/user_manager...
components/user_manager/user_manager_base.cc:494: email, std::string() /*
gaia_id */, AccountType::UNKNOWN);
It's unclear to me why the type is unknown.  If this is for legacy support,
shouldn't the type be google?

If you keep a two-arg overload of GetAccountId() that internally calls the
three-arg overload with AccountType::UNKNOWN, I think you could reduce the size
of this CL by half.

[Alexander Alekseev, 2016-12-06 02:21:14]

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:122: return
std::string(kKeyAdIdPrefix) + gaia_id_;
On 2016/12/05 16:34:31, Roger Tawa wrote:
> Will there be a later pass to change |gaia_id_| to something more generic?

Uuuups, I missed this. Thank you.

I thought of having other account types extend AccountId and make particular
ones have only special "account-specific" fiedls.

For now I think we should add another member like "string uuid_" (or what it
actually is), and check that AD accounts have empty gaia_id_ .

WDYT?

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:193: }
On 2016/12/05 16:34:31, Roger Tawa wrote:
> Since these are new methods, maybe not use "Gaia" in the name?  So something
> like:
> 
> FromUserEmailIdAccountType()
> FromIdAccountType()

Or, probably, we should create AccountId::ADFromUserEmailUUID(), and
AccountId::GoogleFromUserEmailGaiaId(). WDYT?

To somplify this Cl we can add AccountId::ADFromUserEmailUUID() and TODO at
AccountId::FromUserEmailGaiaId() to rename it to
AccountId::GoogleFromUserEmailGaiaId() .

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:235: account_type =
StringToAccountType(account_type_string);
On 2016/12/05 16:34:31, Roger Tawa wrote:
> I think you should do some error checking here since the serialized source is
> external.  Probably OK not to do error checking at line 199 above.

Totally agree.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.h:36: const AccountType&
GetAccountType() const;
On 2016/12/05 16:34:31, Roger Tawa wrote:
> Is there reason to keep this as a const &?  I would suggest removing the
> reference from here and all methods below.

The reason was "it's better to return a reference to string".
If we switch from const & to copy here (which is probably better for enum), it
will later be more difficult to switch back to string if we ever need. (I
believe you had ideas why we could need string here).

But as we do not see this transition in any foreseeable future, let's remove '&'
here and switch to copy enum.

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

https://codereview.chromium.org/2529103002/diff/40001/components/user_manager...
components/user_manager/user_manager_base.cc:494: email, std::string() /*
gaia_id */, AccountType::UNKNOWN);
On 2016/12/05 16:34:32, Roger Tawa wrote:
> It's unclear to me why the type is unknown.  If this is for legacy support,
> shouldn't the type be google?
> 
> If you keep a two-arg overload of GetAccountId() that internally calls the
> three-arg overload with AccountType::UNKNOWN, I think you could reduce the
size
> of this CL by half.

"Known_user::GetAccountId()" is a bit tricky. This is to convert different
"local" account ids to real AccountId. For example, if we receive user e-mail
from Extension, we know only e-mail, but not account type or gaia id.

So the idea is to supply "All the information we know about the user" to this
method and it will resolve to real AccountId.

This is actually temporary method for the most of chrome code (until full
migration is done), and then we can look at what we need for the "external" API
left.

So I'd have all known user data as call parameters here.

[Roger Tawa OOO till Jul 10th, 2016-12-07 22:09:37]

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:122: return
std::string(kKeyAdIdPrefix) + gaia_id_;
On 2016/12/06 02:21:14, Alexander Alekseev wrote:
> On 2016/12/05 16:34:31, Roger Tawa wrote:
> > Will there be a later pass to change |gaia_id_| to something more generic?
> 
> Uuuups, I missed this. Thank you.
> 
> I thought of having other account types extend AccountId and make particular
> ones have only special "account-specific" fiedls.
> 
> For now I think we should add another member like "string uuid_" (or what it
> actually is), and check that AD accounts have empty gaia_id_ .
> 
> WDYT?

Since |gaia_id_| is a private member, seems easier to just rename it to like
|id_| and then store the appropriate id there.  Not sure adding a new member is
easier.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:193: }
On 2016/12/06 02:21:14, Alexander Alekseev wrote:
> On 2016/12/05 16:34:31, Roger Tawa wrote:
> > Since these are new methods, maybe not use "Gaia" in the name?  So something
> > like:
> > 
> > FromUserEmailIdAccountType()
> > FromIdAccountType()
> 
> Or, probably, we should create AccountId::ADFromUserEmailUUID(), and
> AccountId::GoogleFromUserEmailGaiaId(). WDYT?
> 
> To somplify this Cl we can add AccountId::ADFromUserEmailUUID() and TODO at
> AccountId::FromUserEmailGaiaId() to rename it to
> AccountId::GoogleFromUserEmailGaiaId() .

Acknowledged.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.h:36: const AccountType&
GetAccountType() const;
On 2016/12/06 02:21:14, Alexander Alekseev wrote:
> On 2016/12/05 16:34:31, Roger Tawa wrote:
> > Is there reason to keep this as a const &?  I would suggest removing the
> > reference from here and all methods below.
> 
> The reason was "it's better to return a reference to string".
> If we switch from const & to copy here (which is probably better for enum), it
> will later be more difficult to switch back to string if we ever need. (I
> believe you had ideas why we could need string here).
> 
> But as we do not see this transition in any foreseeable future, let's remove
'&'
> here and switch to copy enum.

My 2 cents: given C++11 move semantics and return value optimizations and copy
elision and rvalue references, I don't think returning a reference is
necessarily better even for strings.

[Roman Sorokin (ftl), 2016-12-13 13:41:51]

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

[commit-bot: I haz the power, 2016-12-13 13:42:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2016-12-13 13:45:34]

Description was changed from

==========
Add account_type into AccountId

Also add Active Directory type account

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none
==========

to

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.
 

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none
==========

[Roman Sorokin (ftl), 2016-12-13 13:45:59]

Alex, Roger, PTAL. Thanks!

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:124: return std::string();
On 2016/12/05 16:34:31, Roger Tawa wrote:
> Nit: use a switch() statement.  Also line 173.

Done.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:194: 
On 2016/12/05 16:34:31, Roger Tawa wrote:
> These two new methods should have "// static" comment.

Done.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:235: account_type =
StringToAccountType(account_type_string);
On 2016/12/05 16:34:31, Roger Tawa wrote:
> I think you should do some error checking here since the serialized source is
> external.  Probably OK not to do error checking at line 199 above.

done

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.h:36: const AccountType&
GetAccountType() const;
On 2016/12/05 16:34:31, Roger Tawa wrote:
> Is there reason to keep this as a const &?  I would suggest removing the
> reference from here and all methods below.

Done.

https://codereview.chromium.org/2529103002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.h:36: const AccountType&
GetAccountType() const;
On 2016/12/06 02:21:14, Alexander Alekseev wrote:
> On 2016/12/05 16:34:31, Roger Tawa wrote:
> > Is there reason to keep this as a const &?  I would suggest removing the
> > reference from here and all methods below.
> 
> The reason was "it's better to return a reference to string".
> If we switch from const & to copy here (which is probably better for enum), it
> will later be more difficult to switch back to string if we ever need. (I
> believe you had ideas why we could need string here).
> 
> But as we do not see this transition in any foreseeable future, let's remove
'&'
> here and switch to copy enum.

Done.

[commit-bot: I haz the power, 2016-12-13 14:05:23]

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

[commit-bot: I haz the power, 2016-12-13 14:05:24]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Roman Sorokin (ftl), 2016-12-14 15:48:22]

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

[commit-bot: I haz the power, 2016-12-14 15:48:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 16:54:11]

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

[commit-bot: I haz the power, 2016-12-14 16:54:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roman Sorokin (ftl), 2016-12-14 17:35:40]

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

[commit-bot: I haz the power, 2016-12-14 17:36:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 18:24:43]

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

[commit-bot: I haz the power, 2016-12-14 18:24:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roman Sorokin (ftl), 2016-12-15 10:02:52]

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

[commit-bot: I haz the power, 2016-12-15 10:03:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 11:00:50]

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

[commit-bot: I haz the power, 2016-12-15 11:00:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roman Sorokin (ftl), 2016-12-15 12:37:06]

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

[commit-bot: I haz the power, 2016-12-15 12:37:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 14:57:24]

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

[commit-bot: I haz the power, 2016-12-15 14:57:25]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2016-12-15 19:03:24]

LGTM with a nit

https://codereview.chromium.org/2529103002/diff/140001/components/signin/core...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/140001/components/signin/core...
components/signin/core/account_id/account_id.cc:71: if (account_type_ ==
AccountType::UNKNOWN ||
This behavior is complex enough you really need to comment why this works this
way.

[Roger Tawa OOO till Jul 10th, 2016-12-16 21:14:11]

lgtm

[Roman Sorokin (ftl), 2016-12-21 16:51:43]

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

[commit-bot: I haz the power, 2016-12-21 16:52:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2016-12-21 17:15:04]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2016-12-21 17:15:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, stevenjb@chromium.org, atwilson@chromium.org,
rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/2529103002/#ps180001
(title: "Fix GetCryptohomeId for platform accounts")

[Roman Sorokin (ftl), 2016-12-21 17:15:21]

https://codereview.chromium.org/2529103002/diff/140001/components/signin/core...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/2529103002/diff/140001/components/signin/core...
components/signin/core/account_id/account_id.cc:71: if (account_type_ ==
AccountType::UNKNOWN ||
On 2016/12/15 19:03:24, Andrew T Wilson (Slow) wrote:
> This behavior is complex enough you really need to comment why this works this
> way.

Done.

[commit-bot: I haz the power, 2016-12-21 17:15:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 18:11:04]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482340504628770,
"parent_rev": "f911b5f536d5fd914751f2edb3e93892feaa1509", "commit_rev":
"ab24b22af0fb89076c9aa944fb0cd8dabb60ad0d"}

[commit-bot: I haz the power, 2016-12-21 18:11:33]

Description was changed from

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.
 

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none
==========

to

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.
 

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Review-Url: https://codereview.chromium.org/2529103002
==========

[commit-bot: I haz the power, 2016-12-21 18:11:36]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-21 18:13:31]

Description was changed from

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.
 

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Review-Url: https://codereview.chromium.org/2529103002
==========

to

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Cr-Commit-Position: refs/heads/master@{#440152}
==========

[commit-bot: I haz the power, 2016-12-21 18:13:33]

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

[hcarmona, 2016-12-21 19:14:18]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2593133002/ by hcarmona@chromium.org.

The reason for reverting is: Causing build failure:
https://uberchromegw.corp.google.com/i/chromium/builders/Linux%20x64/builds/3....

[Roman Sorokin (ftl), 2016-12-22 10:58:12]

Description was changed from

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Cr-Commit-Position: refs/heads/master@{#440152}
==========

to

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Cr-Commit-Position: refs/heads/master@{#440152}
==========

[Roman Sorokin (ftl), 2016-12-22 10:58:29]

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

[commit-bot: I haz the power, 2016-12-22 10:58:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 11:13:08]

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

[commit-bot: I haz the power, 2016-12-22 11:13:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Thiemo Nagel, 2016-12-22 11:33:32]

Note that this CL also breaks guest login for Chromad:

[16385:16385:1222/123245.438983:FATAL:account_id.cc(314)] Check failed: false.
Unknown account type
#0 0x7fee6dcab78e base::debug::StackTrace::StackTrace()
#1 0x7fee6dccd4ca logging::LogMessage::~LogMessage()
#2 0x7fee682ee6f7 AccountId::Deserialize()
#3 0x7fee6818fc49 ash::SessionController::AddUserSession()
#4 0x7fee6818fb8e ash::SessionController::UpdateUserSession()
#5 0x7fee682d9dbf ash::mojom::SessionControllerStubDispatch::Accept()
#6 0x7fee6e1d41ad mojo::InterfaceEndpointClient::HandleValidatedMessage()
#7 0x7fee6e1d3a36 mojo::FilterChain::Accept()
#8 0x7fee6e1d510b mojo::InterfaceEndpointClient::HandleIncomingMessage()
#9 0x7fee6e1dbaf6 mojo::internal::MultiplexRouter::ProcessIncomingMessage()
#10 0x7fee6e1db49d mojo::internal::MultiplexRouter::Accept()
#11 0x7fee6e1d3a36 mojo::FilterChain::Accept()
#12 0x7fee6e1cf97e mojo::Connector::ReadSingleMessage()
#13 0x7fee6e1cfef1 mojo::Connector::OnHandleReadyInternal()
#14 0x7fee6e1b5304 mojo::Watcher::OnHandleReady()
#15 0x7fee6e1b5401
_ZN4base8internal13FunctorTraitsIMN4mojo7WatcherEFvjEvE6InvokeIRKNS_7WeakPtrIS3_EEJRKjEEEvS5_OT_DpOT0_
#16 0x7fee6dcac34e base::debug::TaskAnnotator::RunTask()
#17 0x7fee6dcda51d base::MessageLoop::RunTask()
#18 0x7fee6dcdae35 base::MessageLoop::DoWork()
#19 0x7fee6dcdcbe9 base::MessagePumpGlib::Run()
#20 0x7fee6dcda297 base::MessageLoop::RunHandler()
#21 0x7fee6dd0cf0f base::RunLoop::Run()
#22 0x7fee6f1f68d9 ChromeBrowserMainParts::MainMessageLoopRun()
#23 0x7fee6b57ee19 content::BrowserMainLoop::RunMainMessageLoopParts()
#24 0x7fee6b58234a content::BrowserMainRunnerImpl::Run()
#25 0x7fee6b579eae content::BrowserMain()
#26 0x7fee6bcf3a3c content::ContentMainRunnerImpl::Run()
#27 0x7fee6bcf2530 content::ContentMain()
#28 0x7fee6e925ab0 ChromeMain
#29 0x7fee63a04f45 __libc_start_main
#30 0x7fee6e925911 <unknown>

[Roman Sorokin (ftl), 2016-12-22 12:45:46]

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

[commit-bot: I haz the power, 2016-12-22 12:45:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2016-12-22 13:40:02]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2016-12-22 13:40:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, stevenjb@chromium.org, atwilson@chromium.org,
rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/2529103002/#ps240001
(title: "Fix MultiUserWindowManagerChromeOSTest.*")

[commit-bot: I haz the power, 2016-12-22 13:40:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 14:24:49]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482414002284590,
"parent_rev": "593a63c1dc44a5dbfe3ddf6dc46509a7e8fee977", "commit_rev":
"94965f6d9e080c3762bebb35b4a8f2140ff4bc1d"}

[commit-bot: I haz the power, 2016-12-22 14:25:18]

Description was changed from

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Cr-Commit-Position: refs/heads/master@{#440152}
==========

to

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Cr-Commit-Position: refs/heads/master@{#440152}
Review-Url: https://codereview.chromium.org/2529103002
==========

[commit-bot: I haz the power, 2016-12-22 14:25:19]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-12-22 14:28:45]

Description was changed from

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Cr-Commit-Position: refs/heads/master@{#440152}
Review-Url: https://codereview.chromium.org/2529103002
==========

to

==========
Add account_type into AccountId

- Add Active Directory type account
- Rename gaia_id_ to id_.
- Add |AcccountType| param to user_manager::known_user::GetAccountId
- Add |AdFromUserEmailObjGuid| |AdFromUserEmail| functions to create Active
Directory account ids.

This CL a preparation for crbug.com/668130
Should be fixed after migration (see crbug.com/668130)

BUG=656992
TEST=none

Committed: https://crrev.com/d297d94d7922ac0ebd6ca1f3db3fff6db3af24b9
Committed: https://crrev.com/fb06787846a2f824cfe48ab4118e000e89249564
Cr-Original-Commit-Position: refs/heads/master@{#440152}
Cr-Commit-Position: refs/heads/master@{#440412}
==========

[commit-bot: I haz the power, 2016-12-22 14:28:47]

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