This CL replaces e-mail with AccountId on user selection screen.

This CL replaces e-mail with AccountId on user selection screen.

This CL replaces e-mail with serialized AccountId on user selection screen.
It also adds simple AccountId Serialize/Deserialize methods suitable for
passing AccountId to JS code.

This is part of transition to AccountId.

BUG=462823, 552034
TEST=manual

Committed: https://crrev.com/1e24fe57a4ca921355bb1506f5a79627647a8c16
Cr-Commit-Position: refs/heads/master@{#360319}

[Alexander Alekseev, 2015-11-11 13:42:31]

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

[commit-bot: I haz the power, 2015-11-11 13:42:38]

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

[Alexander Alekseev, 2015-11-11 13:59:16]

alemate@chromium.org changed reviewers:
+ dzhioev@chromium.org, oshima@chromium.org, rogerta@chromium.org,
stevenjb@chromium.org

[Alexander Alekseev, 2015-11-11 13:59:17]

Please review:

dzhioev@:

c/b/c/login/screens/user_selection_screen.cc
c/b/ui/webui/chromeos/login/gaia_screen_handler.cc
c/b/ui/webui/chromeos/login/signin_screen_handler.cc
c/b/ui/webui/chromeos/login/signin_screen_handler.h
components/user_manager/*

rogerta@:

components/signin/core/account_id/*

oshima@:

ash/test/test_session_state_delegate.cc

stevenjb@:

c/b/ui/webui/options/chromeos/user_image_source.cc

[commit-bot: I haz the power, 2015-11-11 14:07:37]

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

[commit-bot: I haz the power, 2015-11-11 14:07:38]

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

[Alexander Alekseev, 2015-11-11 14:08:08]

Description was changed from

==========
This CL replaces e-mail with AccountId on user selection screen.

This CL replaces e-mail with serialized AccountId on user selection screen.
It also adds simple AccountId Serialize/Deserialize methods suitable for
passing AccountId to JS code.

This is part of transition to AccountId.

BUG=468875
TEST=manual
==========

to

==========
This CL replaces e-mail with AccountId on user selection screen.

This CL replaces e-mail with serialized AccountId on user selection screen.
It also adds simple AccountId Serialize/Deserialize methods suitable for
passing AccountId to JS code.

This is part of transition to AccountId.

BUG=468875, 552034
TEST=manual
==========

[Alexander Alekseev, 2015-11-11 14:39:19]

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

[commit-bot: I haz the power, 2015-11-11 14:39:32]

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

[Alexander Alekseev, 2015-11-11 15:11:28]

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

[commit-bot: I haz the power, 2015-11-11 15:11:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/40001

[commit-bot: I haz the power, 2015-11-11 15:19:43]

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

[commit-bot: I haz the power, 2015-11-11 15:19:43]

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

[stevenjb, 2015-11-11 20:39:23]

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

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:840:
context.gaia_id = gaia_id;
nit: Add {} while you're in here.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:997: const bool
status = AccountId::Deserialize(user_id, &account_id);
"user_id" does not imply a JSON dictionary. Use something like 'account_json' or
'serialized_user'.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/user_image_source.cc (right):

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/user_image_source.cc:29: const
std::string user_id = net::UnescapeURLComponent(
s/user_id/serialized_account_id/

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

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:22: const char kEmailKey[] =
"e";
nit: Use 'gaia' and 'email' even short term.

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:144: }
nit: no {}

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:157: return false;
We shouldn't combine DCHECK and if(). Log an error here instead of the DCHECKs
above.

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

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.h:52: AccountId* out_account_id);
Even if these are unstable, please document that these input and output JSON
dictionaries.

[dzhioev (left Google), 2015-11-12 00:30:11]

I have an idea on how to make passing AccountId to and from JS easy and safe. 

You need is to overload login::ParseValue and login::MakeValue from
components/login/base_screen_handler_utils.h to work with AccountId. The
overloads would be:

bool ParseValue(const base::Value* value, AccountId* out_value);

and 

base::StringValue MakeValue(const AccountId& account_id);

After that you will be able to define C++ handlers that has AccountId arguments
and call JS methods with AccountId as an argument. All the (de)serialization
would happen underneath.

Example for C++ handler:

  // In C++
  void Foo::HandleBar(const AccountID& account_id);

  ...

  AddCallback("bar", &Foo::HandleBar);

  // In JS
  chrome.send('bar', [serialized_account_id]);


Example for CallJS:
  // In C++
  AccountId account_id;
  ...
  CallJS('doFoo', account_id);

  // In JS
  function doFoo(serialized_account_id) {
    ...
  }

[dzhioev (left Google), 2015-11-12 00:46:24]

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:996: AccountId
account_id(EmptyAccountId());
Why default constructor of AccountId doesn't produse 'empty account id'?

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/user_image_source.cc (right):

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/user_image_source.cc:24: //
"chrome://userimage/user@host?key1=value1&...&key_n=value_n",
Update the comment.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/user_image_source.cc:37: LOG(WARNING)
<< "Failed to deserialize '" << user_id << "'";
I believe we shouldn't produce this warning, as long as it perfectly legal for
options page to use the old format.
We can introduce an additional query param to distinguish email requests from
account_id requests, i.e.:

  * chrome://userimage/old_format_email@example.com for email requests,
  * chrome://userimage/<serialized_account_id>?format=account_id for account_id
requests.

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

https://codereview.chromium.org/1440583002/diff/40001/components/user_manager...
components/user_manager/user_manager_base.cc:123: // TODO(antrim): update code
once user id is really a struct.
TODO(alemate)?

[Alexander Alekseev, 2015-11-12 06:52:56]

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

[Alexander Alekseev, 2015-11-12 06:53:04]

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

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:840:
context.gaia_id = gaia_id;
On 2015/11/11 20:39:23, stevenjb wrote:
> nit: Add {} while you're in here.

Done.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:996: AccountId
account_id(EmptyAccountId());
On 2015/11/12 00:46:24, dzhioev wrote:
> Why default constructor of AccountId doesn't produse 'empty account id'?

To force manual construction. We can change this after migration is done, but
for now it is dangerous.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:997: const bool
status = AccountId::Deserialize(user_id, &account_id);
On 2015/11/11 20:39:23, stevenjb wrote:
> "user_id" does not imply a JSON dictionary. Use something like 'account_json'
or
> 'serialized_user'.

Done.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/user_image_source.cc (right):

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/user_image_source.cc:24: //
"chrome://userimage/user@host?key1=value1&...&key_n=value_n",
On 2015/11/12 00:46:24, dzhioev wrote:
> Update the comment.

Done.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/user_image_source.cc:29: const
std::string user_id = net::UnescapeURLComponent(
On 2015/11/11 20:39:23, stevenjb wrote:
> s/user_id/serialized_account_id/

Done.

https://codereview.chromium.org/1440583002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/user_image_source.cc:37: LOG(WARNING)
<< "Failed to deserialize '" << user_id << "'";
On 2015/11/12 00:46:24, dzhioev wrote:
> I believe we shouldn't produce this warning, as long as it perfectly legal for
> options page to use the old format.
> We can introduce an additional query param to distinguish email requests from
> account_id requests, i.e.:
> 
>   * chrome://userimage/old_format_email@example.com for email requests,
>   * chrome://userimage/<serialized_account_id>?format=account_id for
account_id
> requests.

I don't think we should support other ID formats for long. So this is going to
be removed after migration.

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

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:22: const char kEmailKey[] =
"e";
On 2015/11/11 20:39:23, stevenjb wrote:
> nit: Use 'gaia' and 'email' even short term.

Done.

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:144: }
On 2015/11/11 20:39:23, stevenjb wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.cc:157: return false;
On 2015/11/11 20:39:23, stevenjb wrote:
> We shouldn't combine DCHECK and if(). Log an error here instead of the DCHECKs
> above.

Done.

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

https://codereview.chromium.org/1440583002/diff/40001/components/signin/core/...
components/signin/core/account_id/account_id.h:52: AccountId* out_account_id);
On 2015/11/11 20:39:23, stevenjb wrote:
> Even if these are unstable, please document that these input and output JSON
> dictionaries.

Done.

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

https://codereview.chromium.org/1440583002/diff/40001/components/user_manager...
components/user_manager/user_manager_base.cc:123: // TODO(antrim): update code
once user id is really a struct.
On 2015/11/12 00:46:24, dzhioev wrote:
> TODO(alemate)?

Done.

[commit-bot: I haz the power, 2015-11-12 06:53:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/60001

[Alexander Alekseev, 2015-11-12 06:56:33]

On 2015/11/12 00:30:11, dzhioev wrote:
> I have an idea on how to make passing AccountId to and from JS easy and safe. 
> 
> You need is to overload login::ParseValue and login::MakeValue from
> components/login/base_screen_handler_utils.h to work with AccountId. The
> overloads would be:

I've implemented this. Please, take another look.

[commit-bot: I haz the power, 2015-11-12 07:04:12]

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

[commit-bot: I haz the power, 2015-11-12 07:04:12]

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

[Alexander Alekseev, 2015-11-12 12:29:05]

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

[commit-bot: I haz the power, 2015-11-12 12:29:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/100001

[commit-bot: I haz the power, 2015-11-12 12:36:57]

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

[commit-bot: I haz the power, 2015-11-12 12:36:59]

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

[stevenjb, 2015-11-12 22:39:55]

c/b/ui/webui lgtm

[Alexander Alekseev, 2015-11-13 15:10:25]

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

[commit-bot: I haz the power, 2015-11-13 15:10:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/120001

[commit-bot: I haz the power, 2015-11-13 15:23:58]

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

[commit-bot: I haz the power, 2015-11-13 15:23:59]

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

[Alexander Alekseev, 2015-11-13 15:36:30]

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

[commit-bot: I haz the power, 2015-11-13 15:36:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/140001

[commit-bot: I haz the power, 2015-11-13 15:43:44]

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

[commit-bot: I haz the power, 2015-11-13 15:43:45]

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

[oshima, 2015-11-13 15:45:10]

https://codereview.chromium.org/1440583002/diff/120001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/1440583002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc:401:
;
remove this

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:253: return
EmptyAccountId();
can this happen? The caller of this seems to access map using return value, and
it may create new empty value for EmptyAccountId(). Is it expected?

[Alexander Alekseev, 2015-11-13 16:04:11]

https://codereview.chromium.org/1440583002/diff/120001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/1440583002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc:401:
;
On 2015/11/13 15:45:10, oshima wrote:
> remove this

Done.

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:253: return
EmptyAccountId();
On 2015/11/13 15:45:10, oshima wrote:
> can this happen? The caller of this seems to access map using return value,
and
> it may create new empty value for EmptyAccountId(). Is it expected?

This happens for me in tests
(For example, when device owner is not set).

[Alexander Alekseev, 2015-11-13 16:04:11]

https://codereview.chromium.org/1440583002/diff/120001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/1440583002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc:401:
;
On 2015/11/13 15:45:10, oshima wrote:
> remove this

Done.

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:253: return
EmptyAccountId();
On 2015/11/13 15:45:10, oshima wrote:
> can this happen? The caller of this seems to access map using return value,
and
> it may create new empty value for EmptyAccountId(). Is it expected?

This happens for me in tests
(For example, when device owner is not set).

[Alexander Alekseev, 2015-11-13 16:07:38]

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

[commit-bot: I haz the power, 2015-11-13 16:08:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/160001

[commit-bot: I haz the power, 2015-11-13 16:16:10]

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

[commit-bot: I haz the power, 2015-11-13 16:16:10]

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

[dzhioev (left Google), 2015-11-13 23:17:37]

https://codereview.chromium.org/1440583002/diff/100001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/1440583002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc:401:
;
nit: remove ;

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:80: const
AccountId& account_id(GetAccountIdOfKnownUser(broker->user_id()));
nit: such syntax seems weird to me.
Why not const AccountId& account_id = ...; ?

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:109: const
AccountId account_id(GetAccountIdOfKnownUser(broker->user_id()));
Why account_id is not a reference here?

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:261: AccountId
known_account_id(EmptyAccountId());
nit: 'AccountId known_account_id = EmptyAccountId()' seems more natural to me

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:460: const
AccountId& account_id(GetAccountIdOfKnownUser(user_id));
ditto

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:471: const
AccountId& account_id(GetAccountIdOfKnownUser(username));
ditto

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:499: const
AccountId account_id(GetAccountIdOfKnownUser(user_id));
ditto

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:504: const
AccountId account_id(GetAccountIdOfKnownUser(user_id));
ditto

https://codereview.chromium.org/1440583002/diff/160001/components/login/base_...
File components/login/base_screen_handler_utils.h (right):

https://codereview.chromium.org/1440583002/diff/160001/components/login/base_...
components/login/base_screen_handler_utils.h:72: struct
ParsedValueContainer<AccountId> {
Could you please declare a default constructor here and define it in CC file
(the definition would be empty)? Current implementation breaks the rule
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i....

https://codereview.chromium.org/1440583002/diff/160001/components/user_manage...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1440583002/diff/160001/components/user_manage...
components/user_manager/user_manager_base.cc:599: AccountId
account_id(EmptyAccountId());
partial_account_id = ...
account_id = ...

[oshima, 2015-11-13 23:56:47]

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:80: const
AccountId& account_id(GetAccountIdOfKnownUser(broker->user_id()));
don't you have to check if the account_id is valid?

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:109: const
AccountId account_id(GetAccountIdOfKnownUser(broker->user_id()));
same here

[Alexander Alekseev, 2015-11-14 00:26:09]

Description was changed from

==========
This CL replaces e-mail with AccountId on user selection screen.

This CL replaces e-mail with serialized AccountId on user selection screen.
It also adds simple AccountId Serialize/Deserialize methods suitable for
passing AccountId to JS code.

This is part of transition to AccountId.

BUG=468875, 552034
TEST=manual
==========

to

==========
This CL replaces e-mail with AccountId on user selection screen.

This CL replaces e-mail with serialized AccountId on user selection screen.
It also adds simple AccountId Serialize/Deserialize methods suitable for
passing AccountId to JS code.

This is part of transition to AccountId.

BUG=462823, 552034
TEST=manual
==========

[Alexander Alekseev, 2015-11-14 00:27:42]

https://codereview.chromium.org/1440583002/diff/100001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/1440583002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc:401:
;
On 2015/11/13 23:17:37, dzhioev wrote:
> nit: remove ;

Done.

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:80: const
AccountId& account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/13 23:56:47, oshima wrote:
> don't you have to check if the account_id is valid?

Hmmm... We didn't have this check here. So probably we don't need.
(It can be invalid only if broker->user_id() is an empty string, but this should
be checked somewhere else, I think.)

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:80: const
AccountId& account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/13 23:17:37, dzhioev wrote:
> nit: such syntax seems weird to me.
> Why not const AccountId& account_id = ...; ?

Done.

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:109: const
AccountId account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/13 23:17:37, dzhioev wrote:
> Why account_id is not a reference here?

Because it is generated and temporary inside GetKnownAccountIdOfKnownUser().

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:261: AccountId
known_account_id(EmptyAccountId());
On 2015/11/13 23:17:37, dzhioev wrote:
> nit: 'AccountId known_account_id = EmptyAccountId()' seems more natural to me

Done.

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:460: const
AccountId& account_id(GetAccountIdOfKnownUser(user_id));
On 2015/11/13 23:17:37, dzhioev wrote:
> ditto

Done.

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:471: const
AccountId& account_id(GetAccountIdOfKnownUser(username));
On 2015/11/13 23:17:37, dzhioev wrote:
> ditto

Done.

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:499: const
AccountId account_id(GetAccountIdOfKnownUser(user_id));
On 2015/11/13 23:17:37, dzhioev wrote:
> ditto

Done.

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:504: const
AccountId account_id(GetAccountIdOfKnownUser(user_id));
On 2015/11/13 23:17:37, dzhioev wrote:
> ditto

Done.

https://codereview.chromium.org/1440583002/diff/160001/components/login/base_...
File components/login/base_screen_handler_utils.h (right):

https://codereview.chromium.org/1440583002/diff/160001/components/login/base_...
components/login/base_screen_handler_utils.h:72: struct
ParsedValueContainer<AccountId> {
On 2015/11/13 23:17:37, dzhioev wrote:
> Could you please declare a default constructor here and define it in CC file
> (the definition would be empty)? Current implementation breaks the rule
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i....

Done.

https://codereview.chromium.org/1440583002/diff/160001/components/user_manage...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1440583002/diff/160001/components/user_manage...
components/user_manager/user_manager_base.cc:599: AccountId
account_id(EmptyAccountId());
On 2015/11/13 23:17:37, dzhioev wrote:
> partial_account_id = ...
> account_id = ...

Done.

[Alexander Alekseev, 2015-11-14 00:28:29]

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

[commit-bot: I haz the power, 2015-11-14 00:29:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/180001

[commit-bot: I haz the power, 2015-11-14 01:02:05]

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

[commit-bot: I haz the power, 2015-11-14 01:02:06]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[dzhioev (left Google), 2015-11-14 01:21:15]

LGTM

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:109: const
AccountId account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/14 00:27:42, Alexander Alekseev wrote:
> On 2015/11/13 23:17:37, dzhioev wrote:
> > Why account_id is not a reference here?
> 
> Because it is generated and temporary inside GetKnownAccountIdOfKnownUser().

It is perfectly legal to bind const reference to a temporary object (and  you
are already doing that in line 80).
http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-c...

[oshima, 2015-11-14 01:46:38]

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:80: const
AccountId& account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/14 00:27:42, Alexander Alekseev wrote:
> On 2015/11/13 23:56:47, oshima wrote:
> > don't you have to check if the account_id is valid?
> 
> Hmmm... We didn't have this check here. So probably we don't need.
> (It can be invalid only if broker->user_id() is an empty string, but this
should
> be checked somewhere else, I think.)

Isn't it strange to create a value for empty account, and execute
SetPublichSessionDisplayName/SetPublicSessionLocales?

If this shouldn't happen, then shouldn't it DCHECK(account_id.is_valid())?

[Alexander Alekseev, 2015-11-14 07:10:33]

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:80: const
AccountId& account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/14 01:46:38, oshima wrote:
> On 2015/11/14 00:27:42, Alexander Alekseev wrote:
> > On 2015/11/13 23:56:47, oshima wrote:
> > > don't you have to check if the account_id is valid?
> > 
> > Hmmm... We didn't have this check here. So probably we don't need.
> > (It can be invalid only if broker->user_id() is an empty string, but this
> should
> > be checked somewhere else, I think.)
> 
> Isn't it strange to create a value for empty account, and execute
> SetPublichSessionDisplayName/SetPublicSessionLocales?
> 
> If this shouldn't happen, then shouldn't it DCHECK(account_id.is_valid())?

Done.

https://codereview.chromium.org/1440583002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:109: const
AccountId account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/13 23:56:47, oshima wrote:
> same here

Done.

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1440583002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:109: const
AccountId account_id(GetAccountIdOfKnownUser(broker->user_id()));
On 2015/11/14 01:21:15, dzhioev wrote:
> On 2015/11/14 00:27:42, Alexander Alekseev wrote:
> > On 2015/11/13 23:17:37, dzhioev wrote:
> > > Why account_id is not a reference here?
> > 
> > Because it is generated and temporary inside GetKnownAccountIdOfKnownUser().
> 
> It is perfectly legal to bind const reference to a temporary object (and  you
> are already doing that in line 80).
>
http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-c...

Done.

[Alexander Alekseev, 2015-11-14 07:11:06]

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

[commit-bot: I haz the power, 2015-11-14 07:11:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/200001

[commit-bot: I haz the power, 2015-11-14 07:21:59]

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

[commit-bot: I haz the power, 2015-11-14 07:22:00]

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

[Alexander Alekseev, 2015-11-16 10:31:04]

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

[commit-bot: I haz the power, 2015-11-16 10:32:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/220001

[commit-bot: I haz the power, 2015-11-16 10:41:18]

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

[commit-bot: I haz the power, 2015-11-16 10:41:19]

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

[oshima, 2015-11-16 17:48:23]

lgtm, thanks!

[Alexander Alekseev, 2015-11-17 00:38:39]

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

[commit-bot: I haz the power, 2015-11-17 00:39:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/220001

[commit-bot: I haz the power, 2015-11-17 01:09:35]

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

[commit-bot: I haz the power, 2015-11-17 01:09:36]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[Alexander Alekseev, 2015-11-17 01:10:50]

rogerta@, friendly ping.

[Roger Tawa OOO till Jul 10th, 2015-11-18 09:30:26]

lgtm components/signin

Sorry for delay.

[Alexander Alekseev, 2015-11-18 09:31:04]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-11-18 09:31:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, dzhioev@chromium.org
Link to the patchset: https://codereview.chromium.org/1440583002/#ps220001
(title: "Fix build.")

[Alexander Alekseev, 2015-11-18 09:31:35]

On 2015/11/18 09:30:26, Roger Tawa wrote:
> lgtm components/signin
> 
> Sorry for delay.

Thank you!

[commit-bot: I haz the power, 2015-11-18 09:32:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440583002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440583002/220001

[commit-bot: I haz the power, 2015-11-18 10:46:27]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-11-18 10:47:46]

Patchset 12 (id:??) landed as
https://crrev.com/1e24fe57a4ca921355bb1506f5a79627647a8c16
Cr-Commit-Position: refs/heads/master@{#360319}

[hcarmona, 2015-11-18 18:36:59]

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

The reason for reverting is: Suspecting this CL for introducing failures on Mac
10.10 Release (ATI)

First failure:
https://build.chromium.org/p/chromium.gpu/builders/Mac%2010.10%20Release%20%2...

Failing tests:
CastStreamingApiTestWithPixelOutput.EndToEnd
TabCaptureApiPixelTest.EndToEndThroughWebRTC
TabCaptureApiPixelTest.EndToEndWithoutRemoting

Please update tests..