Refactored signin/sync error controllers for the avatar button

chrome/browser/ui/avatar_button_error_controller.h

Index: chrome/browser/ui/avatar_button_error_controller.h
diff --git a/chrome/browser/ui/avatar_button_error_controller.h b/chrome/browser/ui/avatar_button_error_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..560abd63ebfef4b842dae4a2f74fd8fd4d3b6f0d
--- /dev/null
+++ b/chrome/browser/ui/avatar_button_error_controller.h
@@ -0,0 +1,75 @@
+// Copyright 2016 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.
+
+#ifndef CHROME_BROWSER_UI_AVATAR_BUTTON_ERROR_CONTROLLER_H_
+#define CHROME_BROWSER_UI_AVATAR_BUTTON_ERROR_CONTROLLER_H_
+
+#include "components/signin/core/browser/signin_error_controller.h"
+#include "components/sync_driver/sync_error_controller.h"
+
+class Profile;
+
+// Keeps track of the signin and sync errors that should be exposed to the user
+// in the avatar button.
+class AvatarButtonErrorController {
+ public:
+  explicit AvatarButtonErrorController(Profile* profile);
+  virtual ~AvatarButtonErrorController() {}
+
+  void UpdateSigninError(bool has_signin_error);

[sky, 2016/08/01 15:55:26]

The two Update functions are meant for internal use, right? They should be
private. If you need to friend Avatar*ErrorController.

[Jane, 2016/08/01 19:10:32]

Done.

+  void UpdateSyncError(bool has_sync_error);
+  bool HasAvatarError() { return has_signin_error_ || has_sync_error_; }
+
+  // Explicitly requests an update to the signin/sync errors.
+  void GetAvatarErrorUpdate();
+
+ private:
+  class AvatarSigninErrorController : public SigninErrorController::Observer {

[sky, 2016/08/01 15:55:26]

Please add description. Also, this class is an inner class so that the Avatar
part of the name is redundant. In other words I suggest naming this
SigninErrorControllerObserver and the the other SyncErrorControllerObserver, or
just SigninErrorObserver and SyncErrorObserver, but it's up to you.

[Jane, 2016/08/01 19:10:32]

Done.

+   public:
+    AvatarSigninErrorController(
+        Profile* profile,
+        AvatarButtonErrorController* avatar_button_error_controller);
+    ~AvatarSigninErrorController() override;
+
+    // SigninErrorController::Observer:
+    void OnErrorChanged() override;
+
+   private:
+    Profile* profile_;
+    AvatarButtonErrorController* avatar_button_error_controller_;
+
+    DISALLOW_COPY_AND_ASSIGN(AvatarSigninErrorController);
+  };
+
+  class AvatarSyncErrorController : public SyncErrorController::Observer {
+   public:
+    AvatarSyncErrorController(
+        Profile* profile,
+        AvatarButtonErrorController* avatar_button_error_controller);
+    ~AvatarSyncErrorController() override;
+
+    // SyncErrorController::Observer:
+    void OnErrorChanged() override;
+
+   private:
+    SyncErrorController* GetSyncErrorControllerIfNeeded();
+
+    Profile* profile_;
+    AvatarButtonErrorController* avatar_button_error_controller_;
+
+    DISALLOW_COPY_AND_ASSIGN(AvatarSyncErrorController);
+  };
+
+  virtual void OnAvatarErrorChanged() = 0;

[sky, 2016/08/01 15:55:26]

Does this really work? OnAvatarErrorChanged is in the private section?
None-the-less the pattern you have here is problematic for two reasons:
1. Style guide discourages multiple inheritance of anything but pure virtual
classes, and this class is no longer pure virtual.
2. You don't really want NewAvatarButton to be a AvatarButtonErrorController,
but rather have a AvatarButtonErrorController. In general try to favor
composition over inheritance.

To fix this make AvatarButtonErrorController have an
AvatarButtonErrorControllerDelegate that has the single function
OnAvatarErrorChanged(). NewAvatarButton then has a AvatarButtonErrorController
and is a AvatarButtonErrorControllerDelegate. Sorry if I wasn't clear earlier,
but this is what I meant when I said, "a single class that takes a pure virtual
interface with the single method when the status changes."

[Jane, 2016/08/01 19:10:32]

Ah, I see what you mean! Thanks for the detailed explanation!
Refactored it this way, PTAL.

+
+  AvatarSigninErrorController avatar_signin_error_controller_;
+  AvatarSyncErrorController avatar_sync_error_controller_;
+
+  bool has_signin_error_;
+  bool has_sync_error_;
+
+  DISALLOW_COPY_AND_ASSIGN(AvatarButtonErrorController);
+};
+
+#endif  // CHROME_BROWSER_UI_AVATAR_BUTTON_ERROR_CONTROLLER_H_