Display local signin error without browser and record the path of selected profile in user manager …

Mac: Support UserManager::DisplayErrorMessage(),  ShowSigninDialog() and GetSigninProfilePath().

DisplayErrorMessage() is needed to display the signin error without a browser
window.
ShowSigninDialog() allows fresh signin with the UserManager.
GetSigninProfilePath() returns the path of selected profile after fresh signin.

It was implemented for other platforms in r420713."

TEST=Manual
1) Enable FormceBrowserSignin policy.
2) Launch Chrome and open UserManager.
3) Login dialog should be opened while new profile is added or the last profile is deleted.
4) Local signin error (i.e. email doesn't match RestrictSigninToPattern policy) should be displayed without browser window.

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

[zmin, 2016-10-13 16:12:06]

zmin@chromium.org changed reviewers:
+ anthonyvd@chromium.org, tapted@chromium.org, xiyuan@chromium.org

[zmin, 2016-10-13 16:12:07]

tapted@chromium.org: Please review changes in 
chrome/browser/ui/cocoa/profiles/*

xiyuan@chromium.org: Please review changes in 
chrome/browser/ui/webui/*

[zmin, 2016-10-13 16:18:09]

Description was changed from

==========
Display local signin error without browser and record the path of selected
profile in user manager on Mac.
This is the follow up of
https://crrev.com/c105747d69ec6c66856327ba51765ab6e628e0f3

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

to

==========
Display local signin error without browser and record the path of selected
profile in user manager on Mac.
This is the follow up of
https://crrev.com/c105747d69ec6c66856327ba51765ab6e628e0f3

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

[zmin, 2016-10-13 16:18:10]

zmin@chromium.org changed reviewers:
+ michaelpg@chromium.org
- xiyuan@chromium.org

[zmin, 2016-10-13 16:18:59]

Update due to reviewer OOO:

michaelpg, Can you please review:
chrome/browser/ui/webui/*

[zmin, 2016-10-13 16:58:54]

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

[commit-bot: I haz the power, 2016-10-13 16:59:22]

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-10-13 17:35:11]

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

[commit-bot: I haz the power, 2016-10-13 17:35:12]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-14 15:46:31]

https://codereview.chromium.org/2411033006/diff/1/chrome/browser/ui/cocoa/pro...
File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right):

https://codereview.chromium.org/2411033006/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:514:
instance_->DisplayErrorMessage();
but you just DCHECK'd that instance_ is null...

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

https://codereview.chromium.org/2411033006/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:477: // password
token, the user perform a full online reauth.
grammar: "have the user perform" or "the user must perform" or "the user
performs"

[zmin, 2016-10-14 16:03:21]

https://codereview.chromium.org/2411033006/diff/1/chrome/browser/ui/cocoa/pro...
File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right):

https://codereview.chromium.org/2411033006/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:514:
instance_->DisplayErrorMessage();
On 2016/10/14 15:46:31, michaelpg (OOO M-W) wrote:
> but you just DCHECK'd that instance_ is null...

Oops, it should be "DCHECK(instance);" here. Thanks a lot.

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

https://codereview.chromium.org/2411033006/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:477: // password
token, the user perform a full online reauth.
On 2016/10/14 15:46:31, michaelpg (OOO M-W) wrote:
> grammar: "have the user perform" or "the user must perform" or "the user
> performs"

Done.

[michaelpg, 2016-10-14 16:56:18]

lgtm

[anthonyvd, 2016-10-14 17:05:58]

lgtm

[tapted, 2016-10-17 05:28:20]

There's quite a lot that's wrong with user_manager_mac.mm :/ - I haven't been
reviewing stuff there before, but what you're adding is at least consistent.

lgtm with the anonymous namespace moved, and some CL description tweaks:

 - the first line should be shorter (it's also difficult to read, perhaps
 "Mac: Support UserManager::DisplayErrorMessage() and SetSigninProfilePath().

DisplayErrorMessage() is needed to display the signin error without a browser
window.

SetSigninProfilePath() allows the selected profile to be shown in the
[something] UI.

It was implemented for other platforms in r420713."

 - It needs a TEST= line about how it's tested (either manually, or preferably
with a regression test)

https://codereview.chromium.org/2411033006/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right):

https://codereview.chromium.org/2411033006/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:55: UserManagerMac*
instance_ = nullptr;  // Weak.
eep! This was very confusing - these are not instance variables, but globals.

Let's at least move them into an anonymous namespace... Move line 51 down - you
should be able to put it just before `@interface ReauthDialogWindowController`
around line 173

But really they need to be renamed to g_instance or similar (or not be globals
at all...)

https://codereview.chromium.org/2411033006/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:502: void
UserManager::ShowSigninDialog(content::BrowserContext* browser_context,
It's a bit sad that these look identical to the implementation in
user_manager_view.cc, except |instance_| is a different global :'(

[tapted, 2016-10-17 05:29:45]

On 2016/10/17 05:28:20, tapted wrote:
>  - It needs a TEST= line about how it's tested (either manually, or preferably
> with a regression test)

(and please don't just say "TEST=Manual" - it needs to be the steps you used on
Mac to summon the changed UI, and a description of how it now works, or is
different."

[zmin, 2016-10-17 19:07:08]

Description was changed from

==========
Display local signin error without browser and record the path of selected
profile in user manager on Mac.
This is the follow up of
https://crrev.com/c105747d69ec6c66856327ba51765ab6e628e0f3

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

to

==========
Mac: Support UserManager::DisplayErrorMessage(),  ShowSigninDialog() and
GetSigninProfilePath().

DisplayErrorMessage() is needed to display the signin error without a browser
window.
ShowSigninDialog() allows fresh signin with the UserManager.
GetSigninProfilePath() returns the path of selected profile after fresh signin.

It was implemented for other platforms in r420713."

TEST=Manual
1) Enable FormceBrowserSignin policy.
2) Launch Chrome and open UserManager.
3) Login dialog should be opened while new profile is added or the last profile
is deleted.
4) Local signin error (i.e. email doesn't match RestrictSigninToPattern policy)
should be displayed without browser window.

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

[zmin, 2016-10-17 19:26:22]

Thanks a lot for the review, response inline.

On 2016/10/17 05:28:20, tapted wrote:
> There's quite a lot that's wrong with user_manager_mac.mm :/ - I haven't been
> reviewing stuff there before, but what you're adding is at least consistent.
Sorry for that. I added you because I found that you're the owner of
"chrome/browser/ui/cocoa/" and working on something related to Mac UI. But yes,
this CL simply copies the code from user_manager_view.cc so that the feature
will be also supported for Mac.


> lgtm with the anonymous namespace moved, and some CL description tweaks:
> 
>  - the first line should be shorter (it's also difficult to read, perhaps
>  "Mac: Support UserManager::DisplayErrorMessage() and SetSigninProfilePath().
> 
> DisplayErrorMessage() is needed to display the signin error without a browser
> window.
> 
> SetSigninProfilePath() allows the selected profile to be shown in the
> [something] UI.
> 
> It was implemented for other platforms in r420713."
> 
>  - It needs a TEST= line about how it's tested (either manually, or preferably
> with a regression test)
Done, and I also add the step of manual test.

>
https://codereview.chromium.org/2411033006/diff/20001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right):
> 
>
https://codereview.chromium.org/2411033006/diff/20001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:55: UserManagerMac*
> instance_ = nullptr;  // Weak.
> eep! This was very confusing - these are not instance variables, but globals.
> 
> Let's at least move them into an anonymous namespace... Move line 51 down -
you
> should be able to put it just before `@interface ReauthDialogWindowController`
> around line 173
Done.

> But really they need to be renamed to g_instance or similar (or not be globals
> at all...)
Yes, I agree that instance_ is not a good name but I'll keep it there for this
CL because it's consistent with the same variable in user_manager_view.cc

>
https://codereview.chromium.org/2411033006/diff/20001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:502: void
> UserManager::ShowSigninDialog(content::BrowserContext* browser_context,
> It's a bit sad that these look identical to the implementation in
> user_manager_view.cc, except |instance_| is a different global :'(

The |instance_| in user_manager_view.cc only belongs to the .cc file only like
here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/profiles/user_ma...

[zmin, 2016-10-17 19:33:09]

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

[commit-bot: I haz the power, 2016-10-17 19:33:34]

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-10-17 21:14:11]

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

[commit-bot: I haz the power, 2016-10-17 21:14:12]

Dry run: This issue passed the CQ dry run.

[zmin, 2016-10-17 21:27:10]

The CQ bit was checked by zmin@chromium.org

[zmin, 2016-10-17 21:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
anthonyvd@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2411033006/#ps40001
(title: "tapted's comments")

[commit-bot: I haz the power, 2016-10-17 21:27:40]

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-10-17 21:33:54]

Description was changed from

==========
Mac: Support UserManager::DisplayErrorMessage(),  ShowSigninDialog() and
GetSigninProfilePath().

DisplayErrorMessage() is needed to display the signin error without a browser
window.
ShowSigninDialog() allows fresh signin with the UserManager.
GetSigninProfilePath() returns the path of selected profile after fresh signin.

It was implemented for other platforms in r420713."

TEST=Manual
1) Enable FormceBrowserSignin policy.
2) Launch Chrome and open UserManager.
3) Login dialog should be opened while new profile is added or the last profile
is deleted.
4) Local signin error (i.e. email doesn't match RestrictSigninToPattern policy)
should be displayed without browser window.

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

to

==========
Mac: Support UserManager::DisplayErrorMessage(),  ShowSigninDialog() and
GetSigninProfilePath().

DisplayErrorMessage() is needed to display the signin error without a browser
window.
ShowSigninDialog() allows fresh signin with the UserManager.
GetSigninProfilePath() returns the path of selected profile after fresh signin.

It was implemented for other platforms in r420713."

TEST=Manual
1) Enable FormceBrowserSignin policy.
2) Launch Chrome and open UserManager.
3) Login dialog should be opened while new profile is added or the last profile
is deleted.
4) Local signin error (i.e. email doesn't match RestrictSigninToPattern policy)
should be displayed without browser window.

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

[commit-bot: I haz the power, 2016-10-17 21:33:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-17 21:35:40]

Description was changed from

==========
Mac: Support UserManager::DisplayErrorMessage(),  ShowSigninDialog() and
GetSigninProfilePath().

DisplayErrorMessage() is needed to display the signin error without a browser
window.
ShowSigninDialog() allows fresh signin with the UserManager.
GetSigninProfilePath() returns the path of selected profile after fresh signin.

It was implemented for other platforms in r420713."

TEST=Manual
1) Enable FormceBrowserSignin policy.
2) Launch Chrome and open UserManager.
3) Login dialog should be opened while new profile is added or the last profile
is deleted.
4) Local signin error (i.e. email doesn't match RestrictSigninToPattern policy)
should be displayed without browser window.

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

to

==========
Mac: Support UserManager::DisplayErrorMessage(),  ShowSigninDialog() and
GetSigninProfilePath().

DisplayErrorMessage() is needed to display the signin error without a browser
window.
ShowSigninDialog() allows fresh signin with the UserManager.
GetSigninProfilePath() returns the path of selected profile after fresh signin.

It was implemented for other platforms in r420713."

TEST=Manual
1) Enable FormceBrowserSignin policy.
2) Launch Chrome and open UserManager.
3) Login dialog should be opened while new profile is added or the last profile
is deleted.
4) Local signin error (i.e. email doesn't match RestrictSigninToPattern policy)
should be displayed without browser window.

BUG=642059

Committed: https://crrev.com/119da1a794947d78510019d2d66408b8aff154ce
Cr-Commit-Position: refs/heads/master@{#425781}
==========

[commit-bot: I haz the power, 2016-10-17 21:35:42]

Patchset 3 (id:??) landed as
https://crrev.com/119da1a794947d78510019d2d66408b8aff154ce
Cr-Commit-Position: refs/heads/master@{#425781}