Revamped signin/sync error surfacing on desktop user menu

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

Index: chrome/browser/ui/views/profiles/new_avatar_button.h
diff --git a/chrome/browser/ui/views/profiles/new_avatar_button.h b/chrome/browser/ui/views/profiles/new_avatar_button.h
index 0811d6d990a8edcbdd2bdfad2bfb63c5b3cba59e..94830c07508011f65c87d17272c0270df77b8336 100644
--- a/chrome/browser/ui/views/profiles/new_avatar_button.h
+++ b/chrome/browser/ui/views/profiles/new_avatar_button.h
@@ -9,6 +9,7 @@
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/ui/views/profiles/avatar_button_style.h"
 #include "components/signin/core/browser/signin_error_controller.h"
+#include "components/sync_driver/sync_error_controller.h"
 #include "ui/views/controls/button/label_button.h"
 
 class AvatarButtonDelegate;
@@ -17,7 +18,8 @@ class Profile;
 // Avatar button that displays the active profile's name in the caption area.
 class NewAvatarButton : public views::LabelButton,
                         public ProfileAttributesStorage::Observer,
-                        public SigninErrorController::Observer {
+                        public SigninErrorController::Observer,
+                        public SyncErrorController::Observer {
  public:
   NewAvatarButton(AvatarButtonDelegate* delegate,
                   AvatarButtonStyle button_style,
@@ -43,7 +45,7 @@ class NewAvatarButton : public views::LabelButton,
   void OnProfileSupervisedUserIdChanged(
       const base::FilePath& profile_path) override;
 
-  // SigninErrorController::Observer:
+  // SigninErrorController::Observer and SyncErrorController::Observer

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

This is bad practice... maybe rename one or both functions?

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

Do you mean changing the function name "OnErrorChanged()" in
SigninErrorController and/or SyncErrorController? I thought they are by
convention named this way, and also, changing the name would involve changes in
all the observers, right? Do you think it's worth the change?

[msw, 2016/07/14 02:23:33]

Yes, renaming the interfaces' OnErrorChanged was my suggestion.
I think naming collisions (like this) is one of the reasons we discourage
multiple inheritance in our style guide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
The Google C++ style guide is even more strict about inheritance.
I don't see guidance on this particular practice, but I believe it's highly
discouraged.

That said, I can think of two other ways forward:
1) Get a second opinion from someone knowledgable saying this is okay as-is.
2) Use composition instead of inheritance here.

[Jane, 2016/07/14 15:34:37]

I discussed with Roger about this, and he suggested adding two subclasses to
each take one observer, and call the OnErrorChanged() function of the parent
class when there is an error. This is a smaller code change than renaming
functions and solves the multiple inheritance problem.
Let me know if you agree that this is ok!

   void OnErrorChanged() override;
 
   // Called when the profile info cache has changed, which means we might
@@ -53,9 +55,9 @@ class NewAvatarButton : public views::LabelButton,
   AvatarButtonDelegate* delegate_;
   Profile* profile_;
 
-  // Whether the signed in profile has an authentication error. Used to display
-  // an error icon next to the button text.
-  bool has_auth_error_;
+  // Whether the signed in profile has any authentication error or sync error.
+  // Used to display an error icon next to the button text.
+  bool has_sync_error_;
 
   // The icon displayed instead of the profile name in the local profile case.
   // Different assets are used depending on the OS version.