[Toolbar] Base toolbar button class with background images for button states

chrome/browser/ui/views/toolbar/reload_button.cc

Index: chrome/browser/ui/views/toolbar/reload_button.cc
diff --git a/chrome/browser/ui/views/toolbar/reload_button.cc b/chrome/browser/ui/views/toolbar/reload_button.cc
index dff40c4cc0df6851469d87a9af7b0ecb3dac099b..d267a8fe2e4bfd9dcc69178b018f8bbcf6f3dd79 100644
--- a/chrome/browser/ui/views/toolbar/reload_button.cc
+++ b/chrome/browser/ui/views/toolbar/reload_button.cc
@@ -23,9 +23,11 @@
 namespace {
 
 const int kReloadImages[] =
-    { IDR_RELOAD, IDR_RELOAD_H, IDR_RELOAD_P, IDR_RELOAD_D };
+    { IDR_RELOAD_INNER, IDR_RELOAD_INNER, IDR_RELOAD_INNER,
+      IDR_RELOAD_INNER_DIMMED };
 
-const int kStopImages[] = { IDR_STOP, IDR_STOP_H, IDR_STOP_P, IDR_STOP_D };
+const int kStopImages[] = { IDR_STOP_INNER, IDR_STOP_INNER, IDR_STOP_INNER,
+                            IDR_STOP_INNER_D };
 
 // Contents of the Reload drop-down menu.
 const int kReloadMenuItems[]  = {
@@ -44,7 +46,7 @@ const char ReloadButton::kViewClassName[] = "ReloadButton";
 
 ReloadButton::ReloadButton(LocationBarView* location_bar,
                            CommandUpdater* command_updater)
-    : ButtonDropDown(this, CreateMenuModel()),
+    : ToolbarButton(CreateMenuModel()),
       location_bar_(location_bar),
       command_updater_(command_updater),
       intended_mode_(MODE_RELOAD),
@@ -58,7 +60,8 @@ ReloadButton::ReloadButton(LocationBarView* location_bar,
 }
 
 ReloadButton::~ReloadButton() {
-}
+};
+

[Peter Kasting, 2013/11/15 03:28:58]

Nit: Extra semicolon and newline

[Greg Billock, 2013/11/15 19:36:50]

Done.

 
 void ReloadButton::ChangeMode(Mode mode, bool force) {
   intended_mode_ = mode;
@@ -110,7 +113,7 @@ void ReloadButton::LoadImages(ui::ThemeProvider* tp) {
 }
 
 void ReloadButton::OnMouseExited(const ui::MouseEvent& event) {
-  ButtonDropDown::OnMouseExited(event);
+  ToolbarButton::OnMouseExited(event);
   if (!IsMenuShowing())
     ChangeMode(intended_mode_, true);
 }
@@ -131,7 +134,7 @@ const char* ReloadButton::GetClassName() const {
 
 void ReloadButton::GetAccessibleState(ui::AccessibleViewState* state) {
   if (menu_enabled_)
-    ButtonDropDown::GetAccessibleState(state);
+    ToolbarButton::GetAccessibleState(state);
   else
     CustomButton::GetAccessibleState(state);
 }
@@ -141,7 +144,7 @@ bool ReloadButton::ShouldShowMenu() {
 }
 
 void ReloadButton::ShowDropDownMenu(ui::MenuSourceType source_type) {
-  ButtonDropDown::ShowDropDownMenu(source_type);  // Blocks.
+  ToolbarButton::ShowDropDownMenu(source_type);  // Blocks.
   ChangeMode(intended_mode_, true);
 }
 
@@ -251,8 +254,11 @@ void ReloadButton::ChangeModeInternal(Mode mode) {
   if (visible_mode_ == mode)
     return;
 
-  for (size_t i = 0; i < STATE_COUNT; ++i)
+  for (size_t i = 0; i < STATE_COUNT; ++i) {
     std::swap(images_[i], alternate_images_[i]);

[Peter Kasting, 2013/11/15 03:28:58]

Nit: It seems like at the least we could name these arrays better
(|reload_images_| and |stop_images_|?) and avoid the swap() call.

I'd think we could do even better than this, but I can't actually figure out
from just staring at things how ReloadButton actually paints the selected image
set before your patch.

[Greg Billock, 2013/11/15 19:36:50]

It's in the guts of ButtonDropDown > ImageButton, and the existing use isn't
super-pretty -- there's a variable overload of a private superclass member. The
works. I agree this will improve with LabelButton.

+    SetImage((views::Button::ButtonState)i, images_[i]);

[Peter Kasting, 2013/11/15 03:28:58]

Nit: Don't use C-style casts

[Greg Billock, 2013/11/15 19:36:50]

Done.

+  }
+
   visible_mode_ = mode;
   SchedulePaint();
 }