MD User Manager: Display error message when all profiles are locked.

MD User Manager: Display error message when all profiles are locked.

BUG=563722
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4834f4ff69fb3b75879b744c9838906f95a73083
Cr-Commit-Position: refs/heads/master@{#397147}

[Moe, 2016-05-27 21:57:18]

Description was changed from

==========
MD User Manager: Display error message when all profiles are locked.

BUG=563722
==========

to

==========
MD User Manager: Display error message when all profiles are locked.

BUG=563722
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-05-27 22:05:58]

mahmadi@chromium.org changed reviewers:
+ anthonyvd@chromium.org, dpapad@chromium.org

[Moe, 2016-05-27 22:05:59]

Hi, Please review this CL.

behavior: https://vid.me/lbQi

anthonyvd@ c/b/profiles/
dpapad@ c/b/resources/
rogerta@ c/b/ui/webui/signin/

[dpapad, 2016-05-27 23:59:43]

https://codereview.chromium.org/2025433002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13092: +          <message
name="IDS_USER_MANAGER_GO_GUEST_PROFILES_LOCKED_ERROR" desc="Error Message
Displayed when trying to browser as guest while all profiles are locked.">
Nit here and below
s/Error Message Displayed/Error message displayed

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profiles_state.cc:269: if (!entry->IsSigninRequired() &&
Can we hide the complexity of determining whether a profile is locked inside
ProfileAttributeEntry?

bool ProfileAttributeEntry::IsLocked() {
  ...
}

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/error_dialog.html (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/error_dialog.html:49: <div
id="backdrop">
Should we be using <paper-dialog> or re-using <settings-dialog>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...),
instead of implementing a dialog from scratch here?

For example the <settings-dialog> already comes styled and has an 'x' button at
the top right. See example at http://imgur.com/a/7Bree

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:488:
"cr.ui.UserManager.showErrorDialog",
Makes me sad that we are calling JS methods by name from C++, now that we have a
better alternative. Can't we simply fire a WebUI event (examples
https://code.google.com/p/chromium/codesearch#search/&q=cr.webuiListenerCallb...

In addition to that, is there a reason for md_user_manager not using the
"browser proxy" pattern used by settings? Is this C++ file shared between old
and new user manager?

Asking because the new "browser proxy" pattern makes it so much easier to write
tests (among other benefits). Speaking of tests, are you planning to update
tests to cover the logic introduced in this CL?

[anthonyvd, 2016-05-30 14:09:28]

c/b/profiles/* lgtm

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profiles_state.cc:269: if (!entry->IsSigninRequired() &&
On 2016/05/27 at 23:59:42, dpapad wrote:
> Can we hide the complexity of determining whether a profile is locked inside
ProfileAttributeEntry?
> 
> bool ProfileAttributeEntry::IsLocked() {
>   ...
> }

Semantically, that's what IsSigninRequired is doing. However, this code also
needs to check that profiles aren't child/supervised profiles (which are never
locked) so really it's checking IsNonChildLockedProfile() which is a little too
specific to this use case to belong in ProfileAttributesEntry imo.

[Moe, 2016-05-30 18:33:06]

+rogerta@ Please review c/b/ui/webui/signin/

https://codereview.chromium.org/2025433002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13092: +          <message
name="IDS_USER_MANAGER_GO_GUEST_PROFILES_LOCKED_ERROR" desc="Error Message
Displayed when trying to browser as guest while all profiles are locked.">
On 2016/05/27 23:59:42, dpapad wrote:
> Nit here and below
> s/Error Message Displayed/Error message displayed

Done.

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/error_dialog.html (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/error_dialog.html:49: <div
id="backdrop">
On 2016/05/27 23:59:42, dpapad wrote:
> Should we be using <paper-dialog> or re-using <settings-dialog>
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...),
> instead of implementing a dialog from scratch here?
> 
> For example the <settings-dialog> already comes styled and has an 'x' button
at
> the top right. See example at http://imgur.com/a/7Bree

Ah, i didn't know about those other dialog implementations! This is the second
modal dialog in the user manager. Instead of implementing this one in a
different way or redo them both in this CL, I prefer to "duplicate"
implementation of the other one and refactor them both in a follow up CL
re-using <paper-dialog> or <settings-dialog>. I left a todo here which I will
follow up on this week. WDYT?

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:488:
"cr.ui.UserManager.showErrorDialog",
On 2016/05/27 23:59:42, dpapad wrote:
> Makes me sad that we are calling JS methods by name from C++, now that we have
a
> better alternative. Can't we simply fire a WebUI event (examples
>
https://code.google.com/p/chromium/codesearch#search/&q=cr.webuiListenerCallb...
> 
> In addition to that, is there a reason for md_user_manager not using the
> "browser proxy" pattern used by settings? Is this C++ file shared between old
> and new user manager?
> 
> Asking because the new "browser proxy" pattern makes it so much easier to
write
> tests (among other benefits). Speaking of tests, are you planning to update
> tests to cover the logic introduced in this CL?

My bad. I do use WebUI events and the browser proxy pattern in the MD User
Manager. I was just not using them properly in this CL.

1) I went ahead added a function to the browser proxy API that returns a promise
which is resolved/rejected depending on whether all profiles are locked. That
would make it a lot easier to write tests (and yes, I will be sending a follow
up CL to test this feature!)

2) Unfortunately It's not always possible to use the browser proxy in the
user_manager.js b/c it includes some js code that don't follow the new way of
making browser calls (e.g., HandleRemoveUser())

[Moe, 2016-05-30 18:33:24]

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

[Moe, 2016-05-30 18:33:25]

+rogerta@ Please review c/b/ui/webui/signin/

[Roger Tawa OOO till Jul 10th, 2016-05-30 19:03:45]

lgtm

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:274: break;
Nit: maybe just return false?

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:500: std::string
webui_callback_id_;
Should remove trailing _ since this is a local var.

[Moe, 2016-05-30 20:21:18]

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:274: break;
On 2016/05/30 19:03:45, Roger Tawa wrote:
> Nit: maybe just return false?

Done.

[Moe, 2016-05-30 20:21:41]

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/2025433002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:500: std::string
webui_callback_id_;
On 2016/05/30 19:03:45, Roger Tawa wrote:
> Should remove trailing _ since this is a local var.

Done.

[dpapad, 2016-05-31 19:11:59]

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/error_dialog.html (right):

https://codereview.chromium.org/2025433002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/error_dialog.html:49: <div
id="backdrop">

> Ah, i didn't know about those other dialog implementations! This is the second
modal dialog in the user manager. Instead of implementing this one in a
different way or redo them both in this CL, I prefer to "duplicate"
implementation of the other one and refactor them both in a follow up CL
re-using <paper-dialog> or <settings-dialog>. I left a todo here which I will
follow up on this week. WDYT?

SG.

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/profile_browser_proxy.js (right):

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:128:
isAtLeastOneProfileUnlocked: function() {
The convention for asynchronous code behavior is the following

Async     -> Sync equivalent
----------------------------
Resolving -> Returning a value.
Rejecting -> Throwing an exception.

Imagine that isAtLeastOneProfileUnlocked() was a synchronous method. Would you
expect it to either return true, or throw an error (instead of returning false)?

Rejecting a Promise should indicate that something went wrong, it should not be
used as a way of communicating a valid result. I suggest changing this method to
always resolve with false/true for the reasons mentioned above.

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:69:
cr.addWebUIListener('show-error-dialog', cr.ui.UserManager.showErrorDialog);
Should this listener be removed at some point? Is there any way the 'initialize'
method is executed twice and we end up with two or more listeners?

Polymer code solves this issue by using WebUIListenerBehavior which
automatically removes listeners within the "detached" lifecyle method. See
example at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

[Moe, 2016-05-31 20:53:05]

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/profile_browser_proxy.js (right):

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:128:
isAtLeastOneProfileUnlocked: function() {
On 2016/05/31 19:11:59, dpapad wrote:
> The convention for asynchronous code behavior is the following
> 
> Async     -> Sync equivalent
> ----------------------------
> Resolving -> Returning a value.
> Rejecting -> Throwing an exception.
> 
> Imagine that isAtLeastOneProfileUnlocked() was a synchronous method. Would you
> expect it to either return true, or throw an error (instead of returning
false)?
> 
> Rejecting a Promise should indicate that something went wrong, it should not
be
> used as a way of communicating a valid result. I suggest changing this method
to
> always resolve with false/true for the reasons mentioned above.

Makes sense. Done.

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:69:
cr.addWebUIListener('show-error-dialog', cr.ui.UserManager.showErrorDialog);
On 2016/05/31 19:11:59, dpapad wrote:
> Should this listener be removed at some point? Is there any way the
'initialize'
> method is executed twice and we end up with two or more listeners?
> 
> Polymer code solves this issue by using WebUIListenerBehavior which
> automatically removes listeners within the "detached" lifecyle method. See
> example at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

user_manager.js is executed in the top level html and initialize() is called on
DOMContentLoaded which is expected to be called only once during the lifetime of
chrome://md-user-manager/. Since we expect the listener to be around as long as
the page lives can we not bother with detaching it at all?

similarly, if the component, once attached, is meant to stick around during the
lifetime of the page do we have to use WebUIListenerBehavior to detach the
listeners?

[dpapad, 2016-05-31 21:05:51]

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/2025433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:69:
cr.addWebUIListener('show-error-dialog', cr.ui.UserManager.showErrorDialog);
> user_manager.js is executed in the top level html and initialize() is called
on DOMContentLoaded which is expected to be called only once during the lifetime
of chrome://md-user-manager/. Since we expect the listener to be around as long
as the page lives can we not bother with detaching it at all?
> 
> similarly, if the component, once attached, is meant to stick around during
the lifetime of the page do we have to use WebUIListenerBehavior to detach the
listeners?

If it is guaranteed that initialized() is called only once, then it is fine to
never remove the listener. Perhaps adding a very short comment can save future
readers some time.
// Listener is purposefully active for the lifetime of this page.


Regarding Polymer components, it is OK to never remove it if the component is
never detached, but it also does not hurt to always use the
WebUIListenerBehavior. It makes it more robust to future optimizations where a
component is removed from the DOM when not visible. For example in md_settings,
which is a fairly large page, several things are removed from the DOM when not
needed. This is especially true for listeners registered by dialogs.

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:55:
cr.webUIListenerCallback(
The original intent of cr.webUIListenerCallback is to send events from C++ to
JS. Can you open the dialog more explicitly here instead of using a "webUI
event" from JS to JS? How about

UserManager.showErrorDialog(this.i18n('BrowseAsGuestAllProfilesLockedError'));

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:74:
cr.webUIListenerCallback(
Same here.

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/profile_browser_proxy.js (right):

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:126: * @return
{Promise}
The type annotation can be more specific, and also the entire comment can be
shortened as follows.

/**
 * @return {!Promise<boolean>} Whether all (non-supervised and non-child)
 *     profiles are locked.
 */

[Moe, 2016-05-31 22:14:05]

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:55:
cr.webUIListenerCallback(
On 2016/05/31 21:05:51, dpapad wrote:
> The original intent of cr.webUIListenerCallback is to send events from C++ to
> JS. Can you open the dialog more explicitly here instead of using a "webUI
> event" from JS to JS? How about
> 
> UserManager.showErrorDialog(this.i18n('BrowseAsGuestAllProfilesLockedError'));

b/c user_manager.js includes a bunch of legacy js code we don't closure compile
it. Closure compiler won't be happy with this call. I'll go with
document.querySelector()

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:74:
cr.webUIListenerCallback(
On 2016/05/31 21:05:51, dpapad wrote:
> Same here.

Done.

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/profile_browser_proxy.js (right):

https://codereview.chromium.org/2025433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:126: * @return
{Promise}
On 2016/05/31 21:05:51, dpapad wrote:
> The type annotation can be more specific, and also the entire comment can be
> shortened as follows.
> 
> /**
>  * @return {!Promise<boolean>} Whether all (non-supervised and non-child)
>  *     profiles are locked.
>  */

Done.

[dpapad, 2016-05-31 23:14:46]

lgtm

https://codereview.chromium.org/2025433002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/2025433002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:860:
localized_strings->SetString("AddProfileAllProfilesLockedError",
Nit: All other string IDs is this file start with a lowercase letter. Let's be
consistent with surrounding code.

[Moe, 2016-06-01 12:48:42]

https://codereview.chromium.org/2025433002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/2025433002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:860:
localized_strings->SetString("AddProfileAllProfilesLockedError",
On 2016/05/31 23:14:46, dpapad wrote:
> Nit: All other string IDs is this file start with a lowercase letter. Let's be
> consistent with surrounding code.

Done.

[Moe, 2016-06-01 14:07:42]

On 2016/06/01 12:48:42, Moe wrote:
>
https://codereview.chromium.org/2025433002/diff/80001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):
> 
>
https://codereview.chromium.org/2025433002/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:860:
> localized_strings->SetString("AddProfileAllProfilesLockedError",
> On 2016/05/31 23:14:46, dpapad wrote:
> > Nit: All other string IDs is this file start with a lowercase letter. Let's
be
> > consistent with surrounding code.
> 
> Done.

Thank y'all.

[Moe, 2016-06-01 14:07:48]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-06-01 14:07:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org, rogerta@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2025433002/#ps100001
(title: "Addressed comment")

[commit-bot: I haz the power, 2016-06-01 14:08:03]

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

[commit-bot: I haz the power, 2016-06-01 15:16:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-01 15:17:51]

Description was changed from

==========
MD User Manager: Display error message when all profiles are locked.

BUG=563722
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD User Manager: Display error message when all profiles are locked.

BUG=563722
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4834f4ff69fb3b75879b744c9838906f95a73083
Cr-Commit-Position: refs/heads/master@{#397147}
==========

[commit-bot: I haz the power, 2016-06-01 15:17:52]

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