Revamped signin/sync error surfacing on desktop user menu

chrome/browser/ui/views/profiles/new_avatar_button.cc

Index: chrome/browser/ui/views/profiles/new_avatar_button.cc
diff --git a/chrome/browser/ui/views/profiles/new_avatar_button.cc b/chrome/browser/ui/views/profiles/new_avatar_button.cc
index 44f2863a2e3afc0489aab0de057b078e25c597aa..fcb75304be9aa91fc35dfcc1ff6f9aa559b04dd6 100644
--- a/chrome/browser/ui/views/profiles/new_avatar_button.cc
+++ b/chrome/browser/ui/views/profiles/new_avatar_button.cc
@@ -12,8 +12,10 @@
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profiles_state.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/views/profiles/avatar_button_delegate.h"
 #include "chrome/browser/ui/views/profiles/profile_chooser_view.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "grit/theme_resources.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -57,7 +59,7 @@ NewAvatarButton::NewAvatarButton(AvatarButtonDelegate* delegate,
     : LabelButton(delegate, base::string16()),
       delegate_(delegate),
       profile_(profile),
-      has_auth_error_(false),
+      has_sync_error_(false),
       suppress_mouse_released_action_(false) {
   set_triggerable_event_flags(
       ui::EF_LEFT_MOUSE_BUTTON | ui::EF_RIGHT_MOUSE_BUTTON);
@@ -106,10 +108,21 @@ NewAvatarButton::NewAvatarButton(AvatarButtonDelegate* delegate,
 
   // Subscribe to authentication error changes so that the avatar button can
   // update itself.  Note that guest mode profiles won't have a token service.
-  SigninErrorController* error = profiles::GetSigninErrorController(profile_);
-  if (error) {
-    error->AddObserver(this);
-    OnErrorChanged();  // This calls Update().
+  SigninErrorController* signin_error_controller =
+      profiles::GetSigninErrorController(profile_);
+  SyncErrorController* sync_error_controller = NULL;

[msw, 2016/07/13 22:31:37]

nit: nullptr

[Jane, 2016/07/14 00:18:57]

Done.

+  ProfileSyncService* sync_service =

[msw, 2016/07/13 22:31:37]

Add a file-local GetSyncErrorController helper instead of repeating this 3x.
(I guess the third time will duplicate the code to get the ProfileSyncService)

[Jane, 2016/07/14 00:18:57]

Done.

+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (switches::IsMaterialDesignUserMenu() && sync_service) {

[msw, 2016/07/13 22:31:37]

nit: curlies not needed

[Jane, 2016/07/14 00:18:57]

Done.

+    sync_error_controller = sync_service->sync_error_controller();
+  }
+
+  if (signin_error_controller || sync_error_controller) {
+    if (signin_error_controller)

[msw, 2016/07/13 22:31:37]

nit: move these two conditionals above the either-or conditional block.
(optionally keep each conditional AddObserver with the respective init)

[Jane, 2016/07/14 00:18:57]

Done.

+      signin_error_controller->AddObserver(this);
+    if (sync_error_controller)
+      sync_error_controller->AddObserver(this);
+    OnErrorChanged();  // This calls Update()
   } else {
     Update();
   }
@@ -119,10 +132,19 @@ NewAvatarButton::NewAvatarButton(AvatarButtonDelegate* delegate,
 NewAvatarButton::~NewAvatarButton() {
   g_browser_process->profile_manager()->
       GetProfileAttributesStorage().RemoveObserver(this);
-  SigninErrorController* error =
+  SigninErrorController* signin_error_controller =
       profiles::GetSigninErrorController(profile_);
-  if (error)
-    error->RemoveObserver(this);
+  if (signin_error_controller)
+    signin_error_controller->RemoveObserver(this);
+
+  ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (switches::IsMaterialDesignUserMenu() && sync_service) {
+    SyncErrorController* sync_error_controller =
+        sync_service->sync_error_controller();
+    if (sync_error_controller)
+      sync_error_controller->RemoveObserver(this);
+  }
 }
 
 bool NewAvatarButton::OnMousePressed(const ui::MouseEvent& event) {
@@ -176,10 +198,25 @@ void NewAvatarButton::OnProfileSupervisedUserIdChanged(
 }
 
 void NewAvatarButton::OnErrorChanged() {
-  // If there is an error, show an warning icon.
-  const SigninErrorController* error =
+  // If there is an signin error, show an warning icon.

[msw, 2016/07/13 22:31:37]

nit: "a signin" and "a warning"

[Jane, 2016/07/14 00:18:57]

Done.

+  const SigninErrorController* signin_error_controller =
       profiles::GetSigninErrorController(profile_);
-  has_auth_error_ = error && error->HasError();
+  has_sync_error_ =
+      signin_error_controller && signin_error_controller->HasError();
+
+  // Also show warning icon for sync error for material design user menu.

[msw, 2016/07/13 22:31:37]

nit: "a warning", "sync errors", and "for the material"

[Jane, 2016/07/14 00:18:57]

Done.

+  ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (switches::IsMaterialDesignUserMenu() && sync_service) {
+    SyncErrorController* sync_error_controller =
+        sync_service->sync_error_controller();
+    ProfileSyncService::Status status;
+    sync_service->QueryDetailedSyncStatus(&status);
+    if (sync_service->HasUnrecoverableError() ||

[msw, 2016/07/13 22:31:37]

nit: has_sync_error_ |= ...

[Jane, 2016/07/14 00:18:57]

Done. Is this what you mean?

+        status.sync_protocol_error.action == syncer::UPGRADE_CLIENT ||
+        (sync_error_controller && sync_error_controller->HasError()))
+      has_sync_error_ = true;
+  }
 
   Update();
 }
@@ -213,7 +250,7 @@ void NewAvatarButton::Update() {
 
   if (use_generic_button) {
     SetImage(views::Button::STATE_NORMAL, generic_avatar_);
-  } else if (has_auth_error_) {
+  } else if (has_sync_error_) {
     if (switches::IsMaterialDesignUserMenu()) {
       SetImage(views::Button::STATE_NORMAL,
                gfx::CreateVectorIcon(gfx::VectorIconId::SYNC_PROBLEM, 13,