ChromeOS: Show the new POD RE-AUTH ICON when users needs to re-auth

ChromeOS: Show the new POD RE-AUTH ICON when users needs to re-auth

BUG=469985
TEST=manual

Committed: https://crrev.com/a8bc02408c6431a0a75deb1abb0e9e54663b63c6
Cr-Commit-Position: refs/heads/master@{#338266}

[Alexander Alekseev, 2015-06-19 08:35:57]

alemate@chromium.org changed reviewers:
+ antrim@chromium.org, dzhioev@chromium.org

[Alexander Alekseev, 2015-06-19 08:35:57]

Please review:

dzhioev@ : ui/login/account_picker/*

antrim@: chrome/browser/chromeos/login/signin/*

[dzhioev (left Google), 2015-06-22 23:51:25]

Please verify, that this CL doesn't brake pod's layout in desktop user manager.

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:409:
CheckNextUserStatus("");
std::string()

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:409:
CheckNextUserStatus("");
In my opinion this implementation is too cryptic. Can't we launch status check
for every user in a single loop? Something like:

for (const User& user: users) {
  if (token_handle_util_->HasToken(user_id))
    token_handle_util_->CheckToken(user_id, <OnUserStatusChecked>);
}

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signin/token_handle_util.h (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signin/token_handle_util.h:55: // there is already
token check request for this token, and nother one is not
nit: nother -> another

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
File ui/login/account_picker/user_pod_row.css (right):

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
ui/login/account_picker/user_pod_row.css:153: .name-box {
You need to rename this class in
chrome/browser/resources/user_manager/user_manager.css as well.

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
ui/login/account_picker/user_pod_row.js:1012:
this.reauthWarningElement.style.display = 'inline-flex';
Why not this.reauthWarningElementHidden = this.isAuthTypeOnlineSignIn? Or even
better, implement setter for |isAuthTypeOnlineSignIn| which shows/hides the icon
automatically.

[dzhioev (left Google), 2015-06-23 00:02:09]

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
File ui/login/account_picker/user_pod_template.html (right):

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
ui/login/account_picker/user_pod_template.html:18: <core-icon icon="warning"
class="reauth-warning"></core-icon>
Note: if https://codereview.chromium.org/1179323005/ landed before this CL,
<core-icon> would need to be renamed to <iron-icon>.

[Denis Kuznetsov (DE-MUC), 2015-06-25 18:43:43]

comments + please merge with recent TokenHandle changes.

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:409:
CheckNextUserStatus("");
On 2015/06/22 23:51:24, dzhioev wrote:
> In my opinion this implementation is too cryptic. Can't we launch status check
> for every user in a single loop? Something like:
> 
> for (const User& user: users) {
>   if (token_handle_util_->HasToken(user_id))
>     token_handle_util_->CheckToken(user_id, <OnUserStatusChecked>);
> }
>     

Some magic constant that is not explained in method documentation is a bad
approach. Please either introduce additional method, explicit loop or special
constant for such behavior.

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.h:129: bool
CheckUserStatusImpl(const std::string& user_id, bool check_all_users);
This approach does not give any guarantees.
Please use following (common async) approach:
CheckUserStatus(UserID userid, Callback<void(UserID, bool)> check_callback);

[dzhioev (left Google), 2015-06-25 22:19:36]

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:409:
CheckNextUserStatus("");
On 2015/06/25 18:43:42, Denis Kuznetsov wrote:
> On 2015/06/22 23:51:24, dzhioev wrote:
> > In my opinion this implementation is too cryptic. Can't we launch status
check
> > for every user in a single loop? Something like:
> > 
> > for (const User& user: users) {
> >   if (token_handle_util_->HasToken(user_id))
> >     token_handle_util_->CheckToken(user_id, <OnUserStatusChecked>);
> > }
> >     
> 
> Some magic constant that is not explained in method documentation is a bad
> approach. Please either introduce additional method, explicit loop or special
> constant for such behavior. 

Which constants are you talking about?

[Alexander Alekseev, 2015-06-30 20:43:39]

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:409:
CheckNextUserStatus("");
On 2015/06/22 23:51:24, dzhioev wrote:
> std::string()

Done.

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:409:
CheckNextUserStatus("");
On 2015/06/22 23:51:24, dzhioev wrote:
> In my opinion this implementation is too cryptic. Can't we launch status check
> for every user in a single loop? Something like:
> 
> for (const User& user: users) {
>   if (token_handle_util_->HasToken(user_id))
>     token_handle_util_->CheckToken(user_id, <OnUserStatusChecked>);
> }
>     

Done.

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.h:129: bool
CheckUserStatusImpl(const std::string& user_id, bool check_all_users);
On 2015/06/25 18:43:42, Denis Kuznetsov wrote:
> This approach does not give any guarantees.
> Please use following (common async) approach:
> CheckUserStatus(UserID userid, Callback<void(UserID, bool)> check_callback);

Common approach will not work here, as TokenHandle doesn't support several
simultaneous requests for one user.
But I've changed "check all" implementation.

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signin/token_handle_util.h (right):

https://codereview.chromium.org/1191273002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signin/token_handle_util.h:55: // there is already
token check request for this token, and nother one is not
On 2015/06/22 23:51:24, dzhioev wrote:
> nit: nother -> another

Done.

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
File ui/login/account_picker/user_pod_row.css (right):

https://codereview.chromium.org/1191273002/diff/1/ui/login/account_picker/use...
ui/login/account_picker/user_pod_row.css:153: .name-box {
On 2015/06/22 23:51:24, dzhioev wrote:
> You need to rename this class in
> chrome/browser/resources/user_manager/user_manager.css as well.

Done.

[Alexander Alekseev, 2015-07-01 21:00:56]

PTAL

[dzhioev (left Google), 2015-07-01 21:38:27]

https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/user_manager/user_manager.css (right):

https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/user_manager/user_manager.css:154: .pod .name-box
.reauth-warning {
I beleive you can remove this. "display: none" from user_pod_row.css is enough.

https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/user_manager/user_manager.css:159: .pod .name {
nit: remove this

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.css (right):

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:164: color: #ffc107;
Can you please use #f4b400 (Google Yellow 500) instead? We already use this
color for the warning icon in OOBE.

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
File ui/login/account_picker/user_pod_template.html (right):

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
ui/login/account_picker/user_pod_template.html:1: <link rel="import"
href="chrome://resources/polymer/core-icons/core-icons.html">
This file doesn't exist in repository anymore. Use
"chrome://resources/polymer/v1_0/iron-icons/iron-icons.html" and <iron-icon>

[Alexander Alekseev, 2015-07-02 18:53:40]

https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/user_manager/user_manager.css (right):

https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/user_manager/user_manager.css:154: .pod .name-box
.reauth-warning {
On 2015/07/01 21:38:26, dzhioev wrote:
> I beleive you can remove this. "display: none" from user_pod_row.css is
enough.

Done.

https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/user_manager/user_manager.css:159: .pod .name {
On 2015/07/01 21:38:26, dzhioev wrote:
> nit: remove this

Done.

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.css (right):

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:164: color: #ffc107;
On 2015/07/01 21:38:26, dzhioev wrote:
> Can you please use #f4b400 (Google Yellow 500) instead? We already use this
> color for the warning icon in OOBE.

Done.

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
File ui/login/account_picker/user_pod_template.html (right):

https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
ui/login/account_picker/user_pod_template.html:1: <link rel="import"
href="chrome://resources/polymer/core-icons/core-icons.html">
On 2015/07/01 21:38:26, dzhioev wrote:
> This file doesn't exist in repository anymore. Use
> "chrome://resources/polymer/v1_0/iron-icons/iron-icons.html" and <iron-icon> 

Done.

[dzhioev (left Google), 2015-07-06 20:51:00]

On 2015/07/02 18:53:40, Alexander Alekseev wrote:
>
https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/user_manager/user_manager.css (right):
> 
>
https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/user_manager/user_manager.css:154: .pod .name-box
> .reauth-warning {
> On 2015/07/01 21:38:26, dzhioev wrote:
> > I beleive you can remove this. "display: none" from user_pod_row.css is
> enough.
> 
> Done.
> 
>
https://codereview.chromium.org/1191273002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/user_manager/user_manager.css:159: .pod .name {
> On 2015/07/01 21:38:26, dzhioev wrote:
> > nit: remove this
> 
> Done.
> 
>
https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
> File ui/login/account_picker/user_pod_row.css (right):
> 
>
https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
> ui/login/account_picker/user_pod_row.css:164: color: #ffc107;
> On 2015/07/01 21:38:26, dzhioev wrote:
> > Can you please use #f4b400 (Google Yellow 500) instead? We already use this
> > color for the warning icon in OOBE.
> 
> Done.
> 
>
https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
> File ui/login/account_picker/user_pod_template.html (right):
> 
>
https://codereview.chromium.org/1191273002/diff/60001/ui/login/account_picker...
> ui/login/account_picker/user_pod_template.html:1: <link rel="import"
> href="chrome://resources/polymer/core-icons/core-icons.html">
> On 2015/07/01 21:38:26, dzhioev wrote:
> > This file doesn't exist in repository anymore. Use
> > "chrome://resources/polymer/v1_0/iron-icons/iron-icons.html" and <iron-icon>

> 
> Done.

UI LGTM

[Alexander Alekseev, 2015-07-06 22:37:15]

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

[commit-bot: I haz the power, 2015-07-06 22:39:36]

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

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

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

[commit-bot: I haz the power, 2015-07-07 00:09:15]

Dry run: This issue passed the CQ dry run.

[Denis Kuznetsov (DE-MUC), 2015-07-07 13:00:48]

On 2015/07/07 00:09:15, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

lgtm, but please check with PMs that it is actually needed to perform check for
all users upon boot - this can lead to backend overload.
Note, that there can be significant # of users on school devices (and we limit #
of user pods to 18).

[Denis Kuznetsov (DE-MUC), 2015-07-07 16:09:18]

On 2015/07/07 13:00:48, Denis Kuznetsov wrote:
> On 2015/07/07 00:09:15, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> lgtm, but please check with PMs that it is actually needed to perform check
for
> all users upon boot - this can lead to backend overload.
> Note, that there can be significant # of users on school devices (and we limit
#
> of user pods to 18).

not lgtm (see https://code.google.com/p/chromium/issues/detail?id=469985#c19).
You should only run check for selected pod.

[Alexander Alekseev, 2015-07-09 04:31:16]

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

[Alexander Alekseev, 2015-07-09 04:31:17]

The patchset sent to the CQ was uploaded after l-g-t-m from dzhioev@chromium.org
Link to the patchset: https://codereview.chromium.org/1191273002/#ps120001
(title: "Update after review.")

[commit-bot: I haz the power, 2015-07-09 04:31:37]

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

[Alexander Alekseev, 2015-07-09 04:32:05]

PTAL.

I've replaced signin-button with warning triangle.

[commit-bot: I haz the power, 2015-07-09 05:47:21]

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

[commit-bot: I haz the power, 2015-07-09 05:47:22]

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

(exceeded global retry quota)

[Denis Kuznetsov (DE-MUC), 2015-07-09 06:52:48]

lgtm

https://codereview.chromium.org/1191273002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/1191273002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.h:59: // Returns
true if user check has started possible, otherwise it returns
Please rephrase, I can not understand what method does based on current
documentation. Is it that check is possible, or that check has been started?

[Denis Kuznetsov (DE-MUC), 2015-07-09 06:52:48]

lgtm

https://codereview.chromium.org/1191273002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/1191273002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.h:59: // Returns
true if user check has started possible, otherwise it returns
Please rephrase, I can not understand what method does based on current
documentation. Is it that check is possible, or that check has been started?

[Alexander Alekseev, 2015-07-09 07:19:54]

On 2015/07/09 06:52:48, Denis Kuznetsov wrote:
> lgtm
> 
>
https://codereview.chromium.org/1191273002/diff/100001/chrome/browser/chromeo...
> File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):
> 
>
https://codereview.chromium.org/1191273002/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/login/screens/user_selection_screen.h:59: // Returns
> true if user check has started possible, otherwise it returns
> Please rephrase, I can not understand what method does based on current
> documentation. Is it that check is possible, or that check has been started?

user_selection_screen.h is not modified in the last CL...

[dzhioev (left Google), 2015-07-09 23:21:13]

LGTM

[Alexander Alekseev, 2015-07-09 23:24:38]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-07-09 23:26:36]

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

[commit-bot: I haz the power, 2015-07-09 23:39:46]

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

[commit-bot: I haz the power, 2015-07-09 23:39:48]

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-07-09 23:45:30]

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

[Alexander Alekseev, 2015-07-09 23:45:31]

+xiyuan

xiyuan, please review
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc

[xiyuan, 2015-07-10 01:14:18]

user_manager_screen_handler LGTM

[Alexander Alekseev, 2015-07-10 08:24:54]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-07-10 08:25:24]

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

[commit-bot: I haz the power, 2015-07-10 09:27:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-10 09:28:53]

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