Refactor the avatar button/icon class

chrome/browser/ui/views/profiles/avatar_menu_button.cc

Index: chrome/browser/ui/views/profiles/avatar_menu_button.cc
diff --git a/chrome/browser/ui/views/profiles/avatar_menu_button.cc b/chrome/browser/ui/views/profiles/avatar_menu_button.cc
index 3100a621347f5472fc4cd93069aa26796d068af4..da035709efbe892cc45562eead6c475ed0f022c9 100644
--- a/chrome/browser/ui/views/profiles/avatar_menu_button.cc
+++ b/chrome/browser/ui/views/profiles/avatar_menu_button.cc
@@ -35,8 +35,8 @@ static inline int Round(double x) {
 const char AvatarMenuButton::kViewClassName[] = "AvatarMenuButton";
 
 AvatarMenuButton::AvatarMenuButton(Browser* browser, bool disabled)
-    : MenuButton(NULL, base::string16(), this, false),
-      browser_(browser),
+    : AvatarBaseButton(browser),
+      MenuButton(NULL, base::string16(), this, false),
       disabled_(disabled),
       is_rectangle_(false),
       old_height_(0),
@@ -46,6 +46,8 @@ AvatarMenuButton::AvatarMenuButton(Browser* browser, bool disabled)
 
   SetEventTargeter(
       scoped_ptr<views::ViewTargeter>(new views::ViewTargeter(this)));
+
+  Update();

[msw, 2015/04/01 01:30:17]

Is this necessary if it wasn't being performed on construction previously?

[yao, 2015/04/10 00:43:32]

Yes, Update just redraws the button. 
when this is being constructed, the button needs to be drew for the first time. 

 }
 
 AvatarMenuButton::~AvatarMenuButton() {
@@ -144,6 +146,30 @@ bool AvatarMenuButton::GetAvatarImages(Profile* profile,
   return true;
 }
 
+void AvatarMenuButton::Update() {
+  // The browser can be null in tests.
+  if (!browser())
+    return;
+
+  gfx::Image avatar;
+  gfx::Image taskbar_badge_avatar;
+  bool is_rectangle = false;
+
+  // Update the avatar button in the window frame and the taskbar overlay.
+  bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu();

[msw, 2015/04/01 01:30:16]

nit: const

[msw, 2015/04/01 01:30:16]

The old value was true if the button existed (browser_non_client_frame_view.cc
had "bool should_show_avatar_menu = avatar_button_ ||
AvatarMenu::ShouldShowAvatarMenu();"), do you know why that was the case? It
seems like the button would always be enabled if it existed, but now it might
exist but not be enabled, is this intentional?

[yao, 2015/04/10 00:43:32]

Done.

+
+  if (!AvatarMenuButton::GetAvatarImages(
+          browser()->profile(), should_show_avatar_menu, &avatar,
+          &taskbar_badge_avatar, &is_rectangle)) {
+    return;

[msw, 2015/04/01 01:30:16]

Why does this early return? Shouldn't the button still be enabled or disabled
regardless of the ability to get the avatar images?

[yao, 2015/04/10 00:43:32]

I don't quite know exactly why here either. but this function was previously
called in BrowserNonClientFrameView::UpdateAvatarInfo, which is triggered
whenever ProfileInfoCacheObserver observes something. 

Now AvatarMenuButton observes ProfileInfoCacheObserver itself, I just moved the
same code here. 

This way the button will behave exactly the same as before. 

[msw, 2015/04/16 22:53:34]

That's mostly okay for a refactoring, but if code looks wrong it's worth
investigating/fixing.

+  }
+
+  // Disable the menu when we should not show the menu.
+  if (!AvatarMenu::ShouldShowAvatarMenu())

[msw, 2015/04/01 01:30:16]

nit: use the cached |should_show_avatar_menu| value here.

[yao, 2015/04/10 00:43:32]

Done.

+    this->SetEnabled(false);

[msw, 2015/04/01 01:30:16]

nit: remove "this->"

[msw, 2015/04/01 01:30:17]

Should this button ever be enabled, and if so, what calls SetEnabled(true)?
Should this be unconditionally called as SetEnabled(should_show_avatar_menu);

[yao, 2015/04/10 00:43:32]

Done.

+  this->SetAvatarIcon(avatar, is_rectangle);

[msw, 2015/04/01 01:30:17]

ditto nit: remove "this->"

[yao, 2015/04/10 00:43:32]

Done.

+}
+
 // views::ViewTargeterDelegate:
 bool AvatarMenuButton::DoesIntersectRect(const views::View* target,
                                          const gfx::Rect& rect) const {
@@ -156,5 +182,5 @@ bool AvatarMenuButton::DoesIntersectRect(const views::View* target,
 void AvatarMenuButton::OnMenuButtonClicked(views::View* source,
                                            const gfx::Point& point) {
   if (!disabled_)
-    chrome::ShowAvatarMenu(browser_);
+    chrome::ShowAvatarMenu(browser());
 }