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

chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm

Index: chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm
diff --git a/chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm b/chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm
index ef503f002245ebf0682c24d027402f3e92851b1e..7ca9ed8e699098250f07cb8ee4c4c40d54f4d9a3 100644
--- a/chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm
+++ b/chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm
@@ -15,6 +15,7 @@
 #include "chrome/browser/profiles/profile_window.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/signin/chrome_signin_helper.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -22,8 +23,10 @@
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller.h"
 #import "chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/signin/core/browser/signin_error_controller.h"
 #include "components/signin/core/common/profile_management_switches.h"
+#include "components/sync_driver/sync_error_controller.h"
 #include "ui/base/cocoa/cocoa_base_utils.h"
 
 // Space between the avatar icon and the avatar menu bubble.
@@ -45,7 +48,9 @@ const CGFloat kMenuXOffsetAdjust = 2.0;
 - (void)updateAvatarButtonAndLayoutParent:(BOOL)layoutParent;
 
 // Displays an error icon if any accounts associated with this profile have an
-// auth error.
+// auth error or sync error.
+- (void)updateAuthErrorStatus:(BOOL)hasAuthError;

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

setXYZ instead of updateXYZ, please?

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

Done.

+- (void)updateSyncErrorStatus:(BOOL)hasSyncError;
 - (void)updateErrorStatus:(BOOL)hasError;
 @end
 
@@ -107,7 +112,7 @@ class ProfileAttributesUpdateObserver
     SigninErrorController* errorController =
         profiles::GetSigninErrorController(profile_);
     if (errorController)
-      [avatarController_ updateErrorStatus:errorController->HasError()];
+      [avatarController_ updateAuthErrorStatus:errorController->HasError()];
   }
 
  private:
@@ -117,6 +122,58 @@ class ProfileAttributesUpdateObserver
   DISALLOW_COPY_AND_ASSIGN(ProfileAttributesUpdateObserver);
 };
 
+class SyncErrorObserver : public SyncErrorController::Observer {

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

Please don't inline code in the class declaration.

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

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

+ public:
+  SyncErrorObserver(Profile* profile, AvatarBaseController* avatarController)
+      : profile_(profile), avatarController_(avatarController) {
+    // Subscribe to sync error changes so that the avatar button can update

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

If you change "so that the avatar" to "so the avatar", it'll fit _exactly_ into
80 chars. :)

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

Done.

+    // itself.
+    SyncErrorController* sync_error_controller =
+        GetSyncErrorControllerIfNeeded(profile_);
+    if (sync_error_controller)
+      sync_error_controller->AddObserver(this);
+  }
+
+  ~SyncErrorObserver() override {
+    SyncErrorController* sync_error_controller =
+        GetSyncErrorControllerIfNeeded(profile_);
+    if (sync_error_controller)
+      sync_error_controller->RemoveObserver(this);
+  }
+
+ private:
+  // SyncErrorController::Observer:
+  void OnErrorChanged() override {
+    ProfileSyncService* sync_service =
+        ProfileSyncServiceFactory::GetForProfile(profile_);
+    bool has_sync_error = false;
+    if (switches::IsMaterialDesignUserMenu() && sync_service) {
+      SyncErrorController* sync_error_controller =
+          sync_service->sync_error_controller();
+      ProfileSyncService::Status status;
+      sync_service->QueryDetailedSyncStatus(&status);
+      has_sync_error =

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

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.

+          (sync_service->HasUnrecoverableError() ||
+           status.sync_protocol_error.action == syncer::UPGRADE_CLIENT ||
+           (sync_error_controller && sync_error_controller->HasError()));
+    }
+    [avatarController_ updateSyncErrorStatus:has_sync_error];
+  }
+
+  SyncErrorController* GetSyncErrorControllerIfNeeded(Profile* profile) {
+    if (!switches::IsMaterialDesignUserMenu())

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

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?

+      return nullptr;
+    ProfileSyncService* sync_service =
+        ProfileSyncServiceFactory::GetForProfile(profile);
+    return sync_service ? sync_service->sync_error_controller() : nullptr;
+  }
+
+  Profile* profile_;
+  AvatarBaseController* avatarController_;
+
+  DISALLOW_COPY_AND_ASSIGN(SyncErrorObserver);
+};
+
 @implementation AvatarBaseController
 
 - (id)initWithBrowser:(Browser*)browser {
@@ -124,6 +181,7 @@ class ProfileAttributesUpdateObserver
     browser_ = browser;
     profileAttributesObserver_.reset(
         new ProfileAttributesUpdateObserver(browser_->profile(), self));
+    syncErrorObserver_.reset(new SyncErrorObserver(browser_->profile(), self));
   }
   return self;
 }
@@ -255,6 +313,16 @@ class ProfileAttributesUpdateObserver
   NOTREACHED();
 }
 
+- (void)updateAuthErrorStatus:(BOOL)hasAuthError {
+  hasAuthError_ = hasAuthError;
+  [self updateErrorStatus:(hasAuthError_ || hasSyncError_)];
+}
+
+- (void)updateSyncErrorStatus:(BOOL)hasSyncError {
+  hasSyncError_ = hasSyncError;
+  [self updateErrorStatus:(hasAuthError_ || hasSyncError_)];
+}
+
 - (void)updateErrorStatus:(BOOL)hasError {
 }