Refactored signin/sync error controllers for the avatar button

chrome/browser/signin/avatar_signin_error_controller.h

Index: chrome/browser/signin/avatar_signin_error_controller.h
diff --git a/chrome/browser/signin/avatar_signin_error_controller.h b/chrome/browser/signin/avatar_signin_error_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..d199bd2c16fde94bde617993f1b018f65d2c2cf7
--- /dev/null
+++ b/chrome/browser/signin/avatar_signin_error_controller.h
@@ -0,0 +1,31 @@
+// Copyright (c) 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_SIGNIN_AVATAR_SIGNIN_ERROR_CONTROLLER_H_
+#define CHROME_BROWSER_SIGNIN_AVATAR_SIGNIN_ERROR_CONTROLLER_H_
+
+#include "components/signin/core/browser/signin_error_controller.h"
+
+class Profile;
+
+// Keeps track of auth errors, which should be exposed to the user in the avatar
+// button.
+class AvatarSigninErrorController : public SigninErrorController::Observer {
+ public:
+  explicit AvatarSigninErrorController(Profile* profile);

[sky, 2016/07/27 18:17:30]

This class and usage of it would be more natural if it took a
SigninErrorController::Observer. That way NewAvatarButton could pass itself as
the SigninErrorController::Observer and directly extend
SigninErrorController::Observer rather than creating helper classes.

[Jane, 2016/07/27 20:17:49]

The rationale of not having NewAvatarButton directly extending
SigninErrorController::Observer and SyncErrorController::Observer is because
both observers have OnErrorChange() and that would result in a bad-styled naming
collision. That's why helper classes were created. Although, if
AvatarSigninErrorController and AvatarSyncErrorController can keep their current
implementation, I can add helper functions so that NewAvatarButton can directly
extend AvatarSigninErrorController and AvatarSyncErrorController.
The rationale of having this class inherit from SigninErrorController::Observer
instead of taking one in is so that all the code for adding/removing observer
and detecting error can be kept here and used across linux and mac (more so for
avatar_sync_error_controller.cc).
Apologies if I misunderstood your suggestion, but hopefully this could explain
the current implementation. Let me know how you think this could be improved!

[sky, 2016/07/27 21:42:54]

I missed that the two classes are implementing different interfaces. It seems
unfortunate that both SyncErrorController::Observer and
SigninErrorController::Observer have the exact same function. I an not familiar
with them, is there a reason the names aren't more specific, e.g.
OnSyncErrorChanged and OnSigninErrorChanged? Can they change?

It seems like the two classes you are creating, AvatarSigninErrorController and
AvatarSigninErrorController are purely for the new avatar button. Is that right?
If so, I recommend the following:
. move the code to chrome/browser/ui
. name it better to reflect what it is and used for.
. combine the functionality into a single class that takes a pure virtual
interface with the single method when the status changes. (if this class needs
to use two classes because of the interfaces having the same function name, do
that).
. NewAvatarButton creates the single class and implements the pure virtual
interface.

Does that make sense?

[Jane, 2016/07/29 13:53:46]

Yes, these two classes are for avatar button (linux and mac) only.
There's no good reason why they need to have the more generic name, but when I
first added sync errors to avatar button, we figured that changing the interface
name would involve changing many other places, and it'd be easier to just use
nested classes.
 
Your suggestion makes a lot of sense. I made these changes:
. moved the code to chrome/browser/ui/avatar_button_error_controller
. the functionality is contained inside avatar_button_error_controller, and this
class has two nested class that observes signin error and sync error
respectively
. NewAvatarButton directly inherits from the class

Let me know if I missed anything and if this looks ok!

+  ~AvatarSigninErrorController() override;
+
+  // SigninErrorController::Observer:
+  void OnErrorChanged() override;
+
+  bool has_error() { return has_error_; }
+
+ private:
+  Profile* profile_;
+  bool has_error_;
+
+  DISALLOW_COPY_AND_ASSIGN(AvatarSigninErrorController);
+};
+
+#endif  // CHROME_BROWSER_SIGNIN_AVATAR_SIGNIN_ERROR_CONTROLLER_H_