[MD User Menu] New way to handle supervised user auth error

[MD User Menu] New way to handle supervised user auth error

Background:
The MD user menu shows an error header when sync errors occur. For auth
errors in particular, the header prompts the user to sign in again to
resolve them. However, when the user is a supervised user, signing in is
not an option.

In this CL, we add a way to specifically handle auth errors for
supervised users, which displays a different message in the error header
and with no action button.

Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGaUtrbUNXVDBlR0k/view

BUG=615893
Committed: https://crrev.com/5d85923b15015fff31fa7e187684b6dc559fe7b8
Cr-Commit-Position: refs/heads/master@{#413847}

[Jane, 2016-08-22 17:07:03]

janeliulwq@google.com changed reviewers:
+ maxbogue@chromium.org

[Jane, 2016-08-22 17:07:03]

janeliulwq@google.com changed required reviewers:
+ maxbogue@chromium.org

[Jane, 2016-08-22 17:07:03]

Hi maxbogue@, could you take a look at c/b/sync please, thanks!

[maxbogue, 2016-08-22 19:56:31]

lgtm

[Jane, 2016-08-22 19:58:02]

janeliulwq@google.com changed reviewers:
+ groby@chromium.org, sky@chromium.org

[Jane, 2016-08-22 19:58:02]

janeliulwq@google.com changed required reviewers:
+ groby@chromium.org, sky@chromium.org

[Jane, 2016-08-22 19:58:03]

Hi sky@, could you take a look at c/b/ui/views; and groby@, c/b/ui/cocoa please.
Thanks!

[sky, 2016-08-22 20:43:23]

https://codereview.chromium.org/2267013002/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2267013002/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1571: }
Can you set button_out to null in the else case to make it clear? Otherwise it
isn't clear what the expectations are.

[Jane, 2016-08-22 21:04:11]

Patchset #2 (id:20001) has been deleted

[Jane, 2016-08-22 21:04:58]

Patchset #2 (id:40001) has been deleted

[Jane, 2016-08-22 21:05:15]

https://codereview.chromium.org/2267013002/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2267013002/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1571: }
On 2016/08/22 20:43:22, sky wrote:
> Can you set button_out to null in the else case to make it clear? Otherwise it
> isn't clear what the expectations are.

Done.

[sky, 2016-08-22 23:16:42]

https://codereview.chromium.org/2267013002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2267013002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1577: button_out =
nullptr;
*button_out = nullptr;
A better idea is likely to merge CreateSyncErrorView() and
CreateSyncErrorViewIfNeeded() as the two are so tightly coupled and then it's
easier to see where button_out is reset.

[Jane, 2016-08-23 00:28:07]

https://codereview.chromium.org/2267013002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2267013002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1577: button_out =
nullptr;
On 2016/08/22 23:16:42, sky wrote:
> *button_out = nullptr;
> A better idea is likely to merge CreateSyncErrorView() and
> CreateSyncErrorViewIfNeeded() as the two are so tightly coupled and then it's
> easier to see where button_out is reset.

Done. Do you just mean merging them like this? I couldn't think of a logical way
to move code around so I'm not sure if it's much more clear this way.

[sky, 2016-08-23 13:20:40]

Yes, this is what I meant. LGTM

https://codereview.chromium.org/2267013002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2267013002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1571: *button_out =
nullptr;
The reason I suggested setting *button_to null here previously is to ensure the
value is always set. Now that there is one function it's clear you don't need to
always set button_out as it's no longer a parameter to the function and
expectations are clearer. In other words you can remove this now.

[Jane, 2016-08-23 15:04:40]

Consolidated sync error functions on mac too, PTAL, thanks!

https://codereview.chromium.org/2267013002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2267013002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1571: *button_out =
nullptr;
On 2016/08/23 13:20:39, sky wrote:
> The reason I suggested setting *button_to null here previously is to ensure
the
> value is always set. Now that there is one function it's clear you don't need
to
> always set button_out as it's no longer a parameter to the function and
> expectations are clearer. In other words you can remove this now.

Done. I see, thanks!
(I actually realized that setting *button_out = nullptr would cause a segfault,
because button_out needs to be initialized before *button_out can be set, but
then if button_out initializes to be nullptr, then setting *button_out would
cause a segfault. Thus, I changed L1502 in the switch case to be simply
button_out = nullptr. Let me know if you think there is a better way. Sorry I
didn't test last night.)

[groby-ooo-7-16, 2016-08-23 15:16:49]

https://codereview.chromium.org/2267013002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/2267013002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1895:
DCHECK(buttonAction);
You added a buttonAction = nil case above. Is th DCHECK still appropriate here?

[Jane, 2016-08-23 16:39:39]

https://codereview.chromium.org/2267013002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/2267013002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1895:
DCHECK(buttonAction);
On 2016/08/23 15:16:49, groby wrote:
> You added a buttonAction = nil case above. Is th DCHECK still appropriate
here?

Yes - when a button string is returned, that means a button is needed here,
hence DCHECK that a buttonAction has been set. I added a comment here to
clarify.

[groby-ooo-7-16, 2016-08-23 20:38:58]

Thank you for clarifying - cocoa LGTM as well

[Jane, 2016-08-23 20:41:02]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 20:41:48]

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-08-23 21:37:44]

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

[commit-bot: I haz the power, 2016-08-23 21:37:45]

Dry run: This issue passed the CQ dry run.

[Jane, 2016-08-23 21:38:02]

The CQ bit was checked by janeliulwq@google.com

[Jane, 2016-08-23 21:38:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2267013002/#ps120001
(title: "Added comments")

[commit-bot: I haz the power, 2016-08-23 21:38:58]

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-08-23 21:44:07]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-08-23 21:45:29]

Description was changed from

==========
[MD User Menu] New way to handle supervised user auth error

Background:
The MD user menu shows an error header when sync errors occur. For auth
errors in particular, the header prompts the user to sign in again to
resolve them. However, when the user is a supervised user, signing in is
not an option.

In this CL, we add a way to specifically handle auth errors for
supervised users, which displays a different message in the error header
and with no action button.

Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGaUtrbUNXVDBlR0k/view

BUG=615893
==========

to

==========
[MD User Menu] New way to handle supervised user auth error

Background:
The MD user menu shows an error header when sync errors occur. For auth
errors in particular, the header prompts the user to sign in again to
resolve them. However, when the user is a supervised user, signing in is
not an option.

In this CL, we add a way to specifically handle auth errors for
supervised users, which displays a different message in the error header
and with no action button.

Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGaUtrbUNXVDBlR0k/view

BUG=615893

Committed: https://crrev.com/5d85923b15015fff31fa7e187684b6dc559fe7b8
Cr-Commit-Position: refs/heads/master@{#413847}
==========

[commit-bot: I haz the power, 2016-08-23 21:45:30]

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