Display error message when user try to open a locked supervised user profile when force-sign-in is …

Display error message when user try to open a locked supervised user profile when force-sign-in is enabled.

BUG=642059
Committed: https://crrev.com/fe95cd62038b43250648d0b108a14a9f4198e530
Cr-Commit-Position: refs/heads/master@{#437668}

[zmin, 2016-12-02 23:41:58]

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

[commit-bot: I haz the power, 2016-12-02 23:42:55]

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

[zmin, 2016-12-02 23:43:49]

zmin@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[zmin, 2016-12-02 23:43:49]

Hi Anthony,

Can you review this CL please?

Owen

[commit-bot: I haz the power, 2016-12-03 00:47:22]

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

[commit-bot: I haz the power, 2016-12-03 00:47:22]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2016-12-05 18:56:37]

Sorry for the delay Owen! Few comments.

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

https://codereview.chromium.org/2552473002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:11249: +        This supervised user has been
disabled by your administrator.
Does the policy disable specific supervised users or all of them. If it disable
all of them, this should probably be something like "Supervised users have been
disabled by your administrator."

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

https://codereview.chromium.org/2552473002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_ui.cc:45: // Sign in is terminated
before profile creation.
I don't think "terminated" is the right word. Probably "completed".

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

https://codereview.chromium.org/2552473002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:479:
UserManager::ShowSigninDialog(browser_context, profile_path);
Why is this showing the reauth dialog before showing the error message?

[zmin, 2016-12-08 20:41:51]

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

[zmin, 2016-12-08 20:42:43]

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

https://codereview.chromium.org/2552473002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:11249: +        This supervised user has been
disabled by your administrator.
On 2016/12/05 18:56:37, anthonyvd wrote:
> Does the policy disable specific supervised users or all of them. If it
disable
> all of them, this should probably be something like "Supervised users have
been
> disabled by your administrator."

Done.

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

https://codereview.chromium.org/2552473002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_error_ui.cc:45: // Sign in is terminated
before profile creation.
On 2016/12/05 18:56:37, anthonyvd wrote:
> I don't think "terminated" is the right word. Probably "completed".

Done.

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

https://codereview.chromium.org/2552473002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:479:
UserManager::ShowSigninDialog(browser_context, profile_path);
On 2016/12/05 18:56:37, anthonyvd wrote:
> Why is this showing the reauth dialog before showing the error message?

Replace it with ShowDialogAndDisplayErrorMessage() and refactor the code.

[commit-bot: I haz the power, 2016-12-08 20:42:58]

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

[zmin, 2016-12-08 20:47:14]

zmin@chromium.org changed reviewers:
+ sky@chromium.org

[zmin, 2016-12-08 20:47:15]

+sky@ for the user manager change.

[sky, 2016-12-08 20:57:49]

https://codereview.chromium.org/2552473002/diff/20001/chrome/browser/ui/user_...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/2552473002/diff/20001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:74: static void HideDialog();
It's confusing that this class has Show(), Hide, ShowSigninDialog and
ShowDialogAndDisplayErrorMessage. If this class is going to control multiple
types of dialogs, then the naming should be clear, e.g. ShowUserManager,
HideUserManager, IsUserManagerShowing... An alternative is to move some of these
into a separate class to make context clear.

[commit-bot: I haz the power, 2016-12-08 21:59:27]

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

[commit-bot: I haz the power, 2016-12-08 21:59:29]

Dry run: This issue passed the CQ dry run.

[zmin, 2016-12-08 22:43:58]

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

[commit-bot: I haz the power, 2016-12-08 22:44:38]

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

[zmin, 2016-12-08 22:45:25]

On 2016/12/08 20:57:49, sky wrote:
>
https://codereview.chromium.org/2552473002/diff/20001/chrome/browser/ui/user_...
> File chrome/browser/ui/user_manager.h (right):
> 
>
https://codereview.chromium.org/2552473002/diff/20001/chrome/browser/ui/user_...
> chrome/browser/ui/user_manager.h:74: static void HideDialog();
> It's confusing that this class has Show(), Hide, ShowSigninDialog and
> ShowDialogAndDisplayErrorMessage. If this class is going to control multiple
> types of dialogs, then the naming should be clear, e.g. ShowUserManager,
> HideUserManager, IsUserManagerShowing... An alternative is to move some of
these
> into a separate class to make context clear.

I have moved all dialog related API to a separate class.

[sky, 2016-12-08 23:40:00]

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:55: DISALLOW_COPY_AND_ASSIGN(UserManager);
Are subclasses of these classes actually created? Can you use
DISALLOW_IMPLICIT_CONSTRUCTORS?

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:59: class UserManagerDialog {
How about a more specific name, UserManagerProfileDialog?

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:113: virtual void CloseDialog() = 0;
Shouldn't the virtual function be protected? It's mildly confusing to have a
function you expect to be overriden in the private section.

[commit-bot: I haz the power, 2016-12-08 23:41:58]

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

[commit-bot: I haz the power, 2016-12-08 23:41:59]

Dry run: This issue passed the CQ dry run.

[zmin, 2016-12-09 00:15:06]

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

[commit-bot: I haz the power, 2016-12-09 00:15:33]

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

[zmin, 2016-12-09 00:16:29]

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:55: DISALLOW_COPY_AND_ASSIGN(UserManager);
On 2016/12/08 23:39:59, sky wrote:
> Are subclasses of these classes actually created? Can you use
> DISALLOW_IMPLICIT_CONSTRUCTORS?

Done.

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:59: class UserManagerDialog {
On 2016/12/08 23:39:59, sky wrote:
> How about a more specific name, UserManagerProfileDialog?

Done. Also changed the delegate's name to UserManagerProfileDialogDelegate

https://codereview.chromium.org/2552473002/diff/40001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:113: virtual void CloseDialog() = 0;
On 2016/12/08 23:39:59, sky wrote:
> Shouldn't the virtual function be protected? It's mildly confusing to have a
> function you expect to be overriden in the private section.

I just copy+paste but I agree private is weird here, changed to protected.

[sky, 2016-12-09 00:24:17]

Thanks! LGTM

[commit-bot: I haz the power, 2016-12-09 01:25:00]

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

[commit-bot: I haz the power, 2016-12-09 01:25:01]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2016-12-09 21:33:49]

lgtm, thanks for the refactor!

[zmin, 2016-12-09 21:40:21]

The CQ bit was checked by zmin@chromium.org

[commit-bot: I haz the power, 2016-12-09 21:40:50]

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-09 22:08:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481319621006140,
"parent_rev": "a2de9ad36ccce80d294db2c2ce8a0f28633c295e", "commit_rev":
"191c4b6cab0a1016f3a7a3ee55e8c94e8b550834"}

[commit-bot: I haz the power, 2016-12-09 22:08:59]

Description was changed from

==========
Display error message when user try to open a locked supervised user profile
when force-sign-in is enabled.

BUG=642059
==========

to

==========
Display error message when user try to open a locked supervised user profile
when force-sign-in is enabled.

BUG=642059

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-12 14:56:53]

Description was changed from

==========
Display error message when user try to open a locked supervised user profile
when force-sign-in is enabled.

BUG=642059

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

to

==========
Display error message when user try to open a locked supervised user profile
when force-sign-in is enabled.

BUG=642059

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

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

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