Revamped signin/sync error surfacing on desktop user menu

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

 // 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.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_PROFILES_NEW_AVATAR_BUTTON_H_
 #define CHROME_BROWSER_UI_VIEWS_PROFILES_NEW_AVATAR_BUTTON_H_
 
 #include "base/macros.h"
 #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;
 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,
                   Profile* profile);
   ~NewAvatarButton() override;
 
   // Views::LabelButton
   bool OnMousePressed(const ui::MouseEvent& event) override;
   void OnMouseReleased(const ui::MouseEvent& event) override;
 
   // Views
   void OnGestureEvent(ui::GestureEvent* event) override;
 
  private:
   friend class ProfileChooserViewExtensionsTest;
 
   // ProfileAttributesStorage::Observer:
   void OnProfileAdded(const base::FilePath& profile_path) override;
   void OnProfileWasRemoved(const base::FilePath& profile_path,
                            const base::string16& profile_name) override;
   void OnProfileNameChanged(const base::FilePath& profile_path,
                             const base::string16& old_profile_name) override;
   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
   // have to update the icon/text of the button.
   void Update();
 
   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.
   gfx::ImageSkia generic_avatar_;
 
   // This is used to check if the bubble was showing during the mouse pressed
   // event. If this is true then the mouse released event is ignored to prevent
   // the bubble from reshowing.
   bool suppress_mouse_released_action_;
 
   DISALLOW_COPY_AND_ASSIGN(NewAvatarButton);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_PROFILES_NEW_AVATAR_BUTTON_H_