[Mac][MD User Menu] Revamped signin/sync error surfacing UI

[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMkLc/edit#heading=h.o6ftotuznu1h
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot: https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20Sign%20In/user_menu/specs#%2Fspec-3.png (Don't refer to the strings there)
- Project design doc: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnqik/edit?ts=57445a70#heading=h.6xrfx2rucezz

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

[Jane, 2016-07-22 21:34:10]

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

[commit-bot: I haz the power, 2016-07-22 21:34:42]

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

[Jane, 2016-07-22 21:38:21]

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

[Jane, 2016-07-22 21:38:21]

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

[Jane, 2016-07-22 21:38:22]

Hi Rachel, PTAL, thanks!
(Last big CL of the project!)

[Jane, 2016-07-22 21:39:11]

Description was changed from

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGUGtKaFFRMmxBSG8/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

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

to

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

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

[Jane, 2016-07-22 21:41:48]

Description was changed from

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

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

to

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

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

[commit-bot: I haz the power, 2016-07-22 22:12:45]

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

[commit-bot: I haz the power, 2016-07-22 22:12:46]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2016-07-22 23:53:16]

I fear it might not be the last one, sorry :(

The pure UI code LG, but there's so much code shared with the Views version that
I'd really like to see this factored out into a shared class.

Really sorry for adding that extra work, but it'll hopefully pay off in future
updates/bug fixes...

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.h (right):

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.h:38: // Observer that
listens for updates to the SyncErrorController.
Tiny style nit: Empty line above?

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

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:52: -
(void)updateAuthErrorStatus:(BOOL)hasAuthError;
setXYZ instead of updateXYZ, please?

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:125: class
SyncErrorObserver : public SyncErrorController::Observer {
Please don't inline code in the class declaration.

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:129: // Subscribe to
sync error changes so that the avatar button can update
If you change "so that the avatar" to "so the avatar", it'll fit _exactly_ into
80 chars. :)

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:155: has_sync_error =
This code has me a bit confused - it seemed like a lot of code that should be
somewhere on the sync system, not in the UI. So I started digging, and we seem
to have several similar function in different places.

* sync_ui_util has similar but more complex code. 
* profile_chooser_view in Views has code similar to sync_ui_util (so definitely
more complex than what's happening here)
* views/profiles/new_avatar_button.cc duplicates the logic here almost exactly,
except it also checks the signin controller.

I'm not sure if all those differences are intentional - if they are not, it's
probably time to share some of this code. If they are, we should explicitly call
out why this is different from the other places.

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:164: if
(!switches::IsMaterialDesignUserMenu())
Same goes for this, which is completely shared with the views implementation.

In fact, as far as I can tell, it looks like the entire observer could be
shared, with just a callback back to the UI passed in?

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

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:177:
ProfileSyncService* syncService =
Probably shareable as well?

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

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1864: if (service
&& service->HasUnrecoverableError()) {
It seems a ton of this logic is shared with the Views implementation...

[Jane, 2016-08-03 19:50:52]

Patchset #2 (id:20001) has been deleted

[Jane, 2016-08-03 19:57:57]

Thanks for pushing me to do the refactoring - I wrote most of the parallel code
in views myself (sorry, should've referred to them in the desc) and I should've
made the decision to refactor myself.
Either way, I landed a CL that factors out all the error observing and error
classification code (https://codereview.chromium.org/2179283002/), and turned
this CL into a simpler version.
I also addressed your other comments that are still applicable.
PTAL, thanks again! :)

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.h (right):

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.h:38: // Observer that
listens for updates to the SyncErrorController.
On 2016/07/22 23:53:15, groby wrote:
> Tiny style nit: Empty line above?

Done.

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

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:52: -
(void)updateAuthErrorStatus:(BOOL)hasAuthError;
On 2016/07/22 23:53:15, groby wrote:
> setXYZ instead of updateXYZ, please?

Done.

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:125: class
SyncErrorObserver : public SyncErrorController::Observer {
On 2016/07/22 23:53:16, groby wrote:
> Please don't inline code in the class declaration.

Done. I put the declaration separately in avatar_base_controller.h.

https://codereview.chromium.org/2174973002/diff/1/chrome/browser/ui/cocoa/pro...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:129: // Subscribe to
sync error changes so that the avatar button can update
On 2016/07/22 23:53:15, groby wrote:
> If you change "so that the avatar" to "so the avatar", it'll fit _exactly_
into
> 80 chars. :)

Done.

[groby-ooo-7-16, 2016-08-04 01:46:03]

Thank you for the refactoring, and LGTM!

[Jane, 2016-08-04 13:41:35]

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

[commit-bot: I haz the power, 2016-08-04 13: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-04 14:18:30]

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

[commit-bot: I haz the power, 2016-08-04 14:18:31]

Dry run: This issue passed the CQ dry run.

[Jane, 2016-08-04 14:19:32]

The CQ bit was checked by janeliulwq@google.com

[commit-bot: I haz the power, 2016-08-04 14:19:48]

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-04 14:23:35]

Description was changed from

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

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

to

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

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

[commit-bot: I haz the power, 2016-08-04 14:23:36]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-08-04 14:25:56]

Description was changed from

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

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

to

==========
[Mac][MD User Menu] Revamped signin/sync error surfacing UI

Specifically:
1. Created a new UI for error surfacing that sits above the user menu
(to replace the current email badging UI);
2. Funneled all signin- and sync-related errors through this UI:
unrecoverable errors, auth errors, out-of-date-client errors, and
passphrase errors. See the detailed list of errors and their designed
behavior here:
https://docs.google.com/document/d/1jEzY44oMWenzJQUIBf1x8dkkqm2PJbYngvQzeXTMk...
3. Made respective modifications to the avatar button observers so that
the button displays an error icon for these new errors too;
4. Removed the blue tutorial headers for the user menu because the
existing tutorial modes will be etiher no longer applicable in MD or
migrated into another UI (and we are not planning to add new tutorials).

Other useful links:
- Screenshot:
https://drive.google.com/a/google.com/file/d/0B7Fvv7JszRyGYXhRbFJTODBZWTg/view
- Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
(Don't refer to the strings there)
- Project design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

BUG=615893

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

[commit-bot: I haz the power, 2016-08-04 14:25:57]

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