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

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/profiles/avatar_base_controller.h"
 
 #include "base/mac/foundation_util.h"
 #include "base/macros.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_metrics.h"
 #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"
 #import "chrome/browser/ui/cocoa/base_bubble_controller.h"
 #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.
 const CGFloat kMenuYOffsetAdjust = 1.0;
 // Offset needed to align the edge of the avatar bubble with the edge of the
 // avatar button.
 const CGFloat kMenuXOffsetAdjust = 2.0;
 
 @interface AvatarBaseController (Private)
 // Shows the avatar bubble.
 - (IBAction)buttonClicked:(id)sender;
 - (IBAction)buttonRightClicked:(id)sender;
 
 - (void)bubbleWillClose:(NSNotification*)notif;
 
 // Updates the profile name displayed by the avatar button. If |layoutParent| is
 // yes, then the BrowserWindowController is notified to relayout the subviews,
 // as the button needs to be repositioned.
 - (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
 
 class ProfileAttributesUpdateObserver
     : public ProfileAttributesStorage::Observer,
       public SigninErrorController::Observer {
  public:
   ProfileAttributesUpdateObserver(Profile* profile,
                                   AvatarBaseController* avatarController)
       : profile_(profile),
@@ skipping 41 matching lines @@
       const base::FilePath& profile_path) override {
     if (profile_->GetPath() == profile_path)
       [avatarController_ updateAvatarButtonAndLayoutParent:YES];
   }
 
   // SigninErrorController::Observer:
   void OnErrorChanged() override {
     SigninErrorController* errorController =
         profiles::GetSigninErrorController(profile_);
     if (errorController)
-      [avatarController_ updateErrorStatus:errorController->HasError()];
+      [avatarController_ updateAuthErrorStatus:errorController->HasError()];
   }
 
  private:
   Profile* profile_;
   AvatarBaseController* avatarController_;  // Weak; owns this.
 
   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 {
   if ((self = [super init])) {
     browser_ = browser;
     profileAttributesObserver_.reset(
         new ProfileAttributesUpdateObserver(browser_->profile(), self));
+    syncErrorObserver_.reset(new SyncErrorObserver(browser_->profile(), self));
   }
   return self;
 }
 
 - (void)dealloc {
   [self browserWillBeDestroyed];
   [super dealloc];
 }
 
 - (void)browserWillBeDestroyed {
@@ skipping 111 matching lines @@
     [static_cast<BrowserWindowController*>(wc)
         releaseBarVisibilityForOwner:self withAnimation:YES delay:NO];
   }
   menuController_ = nil;
 }
 
 - (void)updateAvatarButtonAndLayoutParent:(BOOL)layoutParent {
   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 {
 }
 
 - (BaseBubbleController*)menuController {
   return menuController_;
 }
 
 @end