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

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

Index: chrome/browser/ui/views/toolbar/toolbar_button.cc
diff --git a/chrome/browser/ui/views/toolbar/button_dropdown.cc b/chrome/browser/ui/views/toolbar/toolbar_button.cc
similarity index 66%
copy from chrome/browser/ui/views/toolbar/button_dropdown.cc
copy to chrome/browser/ui/views/toolbar/toolbar_button.cc
index c04f2e83551d34a5a8d3a3f0aab0df38dca2a906..26bfa282179bef88c0e9e8ae3597cf1e6e5b1d37 100644
--- a/chrome/browser/ui/views/toolbar/button_dropdown.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_button.cc
@@ -1,65 +1,69 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 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.
 
-#include "chrome/browser/ui/views/toolbar/button_dropdown.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_button.h"
 
 #include "base/bind.h"
-#include "base/compiler_specific.h"
-#include "base/message_loop/message_loop.h"
-#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "grit/theme_resources.h"
 #include "grit/ui_strings.h"
 #include "ui/base/accessibility/accessible_view_state.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/models/menu_model.h"
 #include "ui/gfx/display.h"
 #include "ui/gfx/screen.h"
+#include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/menu_model_adapter.h"
 #include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/widget/widget.h"
 
-// static
-const char ButtonDropDown::kViewClassName[] =
-    "ui/views/controls/button/ButtonDropDown";
-
 // How long to wait before showing the menu.
+// TODO(gbillock): replace with virtual method.

[Peter Kasting, 2013/11/19 02:28:50]

Why do we need a virtual method?

[Greg Billock, 2013/11/20 00:59:03]

This pollutes the global namespace -- should be a class field/method if it needs
changed (trying to think ahead to the WrenchMenu case which is different).

[Peter Kasting, 2013/11/20 01:10:20]

OK, I'm not looking at the WrenchMenu code.  At least for the current code, this
constant should just be moved into ToolbarButton::OnMousePressed() right above
where it's actually used, which avoids polluting any namespace and is more
compliant with the style guide's "declare things in the narrowest scope
possible" sentiment anyway.

[Greg Billock, 2013/11/20 23:13:23]

Done.

 const int kMenuTimerDelay = 500;
 
-////////////////////////////////////////////////////////////////////////////////
-//
-// ButtonDropDown - constructors, destructors, initialization, cleanup
-//
-////////////////////////////////////////////////////////////////////////////////
-
-ButtonDropDown::ButtonDropDown(views::ButtonListener* listener,
-                               ui::MenuModel* model)
-    : views::ImageButton(listener),
+ToolbarButton::ToolbarButton(views::ButtonListener* listener,
+                             ui::MenuModel* model)
+    : views::LabelButton(listener, string16()),
       model_(model),
       menu_showing_(false),
       y_position_on_lbuttondown_(0),
-      show_menu_factory_(this) {
-  set_context_menu_controller(this);
+      show_menu_factory_(this) {}

[Peter Kasting, 2013/11/19 02:28:50]

Nit: I slightly prefer the original "linebreak between braces" style of this
file (3 places)

[Greg Billock, 2013/11/20 00:59:03]

Done.

+
+ToolbarButton::~ToolbarButton() {}
+
+void ToolbarButton::Init() {
+  set_focusable(true);
+
+  // Provides the hover/pressed style used by buttons in the toolbar.
+  views::LabelButtonBorder* border =
+      new views::LabelButtonBorder(views::Button::STYLE_BUTTON);
+  const int kHoverImages[] = IMAGE_GRID(IDR_TOOLBAR_BUTTON_HOVER);
+  border->SetPainter(false, views::Button::STATE_HOVERED,
+                     views::Painter::CreateImageGridPainter(
+                         kHoverImages));
+  const int kPressedImages[] = IMAGE_GRID(IDR_TOOLBAR_BUTTON_PRESSED);
+  border->SetPainter(false, views::Button::STATE_PRESSED,
+                     views::Painter::CreateImageGridPainter(
+                         kPressedImages));
+  border->SetPainter(false, views::Button::STATE_NORMAL, NULL);
+  border->SetPainter(true, views::Button::STATE_NORMAL, NULL);

[Peter Kasting, 2013/11/19 02:28:50]

Don't we also need to set painters for the other focused states, as well as all
disabled states? Otherwise they'll fall back to the default painters in
LabelButtonBorder().

[Greg Billock, 2013/11/20 00:59:03]

I think the default painter is fine since the toolbar buttons have no border
when disabled/normal. That's what my visual testing supports as well --
comparing buttons beside each other during conversion I couldn't tell them apart
in any state.

I'm not sure about focus states, though. I need to figure out how to get focus
onto the toolbar buttons to test -- but I think we just have default focus
indication for them, since I don't think there's anything particular that we do
for these buttons currently. Does LabelButton need focus setup other than just
set_focusable(true) that ImageButton does by default?

Testing, it looks like focus doesn't move through the toolbar buttons -- it goes
from the web page into the omnibox and back, but not through any of the buttons.

[Peter Kasting, 2013/11/20 01:10:20]

I know, but the default painters will be drawing such a border.  Won't they? 
Else what do the IDR_BUTTON_XXX painters there actually do?  In fact maybe we
should just be using one of the other styles that does less?

Also seems like we should probably clear the insets as well.
Ask dmazzoni.

[Greg Billock, 2013/11/20 23:13:23]

The insets get established by the border. I think they're what we want, since as
nearly as I can tell the borders are identical to the existing image borders. I
looked to see whether the existing LabelButtonBorders turn out to be the same
images, but they don't. That may be fodder for a future refactor, depending on
what UI thinks. For now I think we should keep exactly the same look we
currently have.

Another wrinkle: on Linux Aura, the internal image icons are getting clipped,
presumably because of these insets. Smaller assets ought to clear that up, but
does that mean we should do those assets now before this goes in as is? I'm
leaning towards tearing out the existing button migrations so we can get the
base class checked in and make the site chip happen sooner.
Alt-Shift-T for future reference. This is working fine.

+  set_border(border);
 }
 
-ButtonDropDown::~ButtonDropDown() {
-}
+void ToolbarButton::ButtonPressed(views::Button* sender,
+                                  const ui::Event& event) {}
+
 
-void ButtonDropDown::ClearPendingMenu() {
+void ToolbarButton::ClearPendingMenu() {
   show_menu_factory_.InvalidateWeakPtrs();
 }
 
-bool ButtonDropDown::IsMenuShowing() const {
+bool ToolbarButton::IsMenuShowing() const {
   return menu_showing_;
 }
-
-////////////////////////////////////////////////////////////////////////////////
-//
-// ButtonDropDown - Events
-//
-////////////////////////////////////////////////////////////////////////////////
-
-bool ButtonDropDown::OnMousePressed(const ui::MouseEvent& event) {
+bool ToolbarButton::OnMousePressed(const ui::MouseEvent& event) {
   if (enabled() && ShouldShowMenu() &&
       IsTriggerableEvent(event) && HitTestPoint(event.location())) {
     // Store the y pos of the mouse coordinates so we can use them later to
@@ -70,16 +74,16 @@ bool ButtonDropDown::OnMousePressed(const ui::MouseEvent& event) {
     // Schedule a task that will show the menu.
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
-        base::Bind(&ButtonDropDown::ShowDropDownMenu,
+        base::Bind(&ToolbarButton::ShowDropDownMenu,
                    show_menu_factory_.GetWeakPtr(),
                    ui::GetMenuSourceTypeForEvent(event)),
         base::TimeDelta::FromMilliseconds(kMenuTimerDelay));
   }
-  return ImageButton::OnMousePressed(event);
+  return LabelButton::OnMousePressed(event);
 }
 
-bool ButtonDropDown::OnMouseDragged(const ui::MouseEvent& event) {
-  bool result = views::ImageButton::OnMouseDragged(event);
+bool ToolbarButton::OnMouseDragged(const ui::MouseEvent& event) {
+  bool result = LabelButton::OnMouseDragged(event);
 
   if (show_menu_factory_.HasWeakPtrs()) {
     // If the mouse is dragged to a y position lower than where it was when
@@ -94,21 +98,17 @@ bool ButtonDropDown::OnMouseDragged(const ui::MouseEvent& event) {
   return result;
 }
 
-void ButtonDropDown::OnMouseReleased(const ui::MouseEvent& event) {
+void ToolbarButton::OnMouseReleased(const ui::MouseEvent& event) {
   if (IsTriggerableEvent(event) ||
       (event.IsRightMouseButton() && !HitTestPoint(event.location()))) {
-    views::ImageButton::OnMouseReleased(event);
+    LabelButton::OnMouseReleased(event);
   }
 
   if (IsTriggerableEvent(event))
     show_menu_factory_.InvalidateWeakPtrs();
 }
 
-const char* ButtonDropDown::GetClassName() const {
-  return kViewClassName;
-}
-
-void ButtonDropDown::OnMouseExited(const ui::MouseEvent& event) {
+void ToolbarButton::OnMouseExited(const ui::MouseEvent& event) {
   // Starting a drag results in a MouseExited, we need to ignore it.
   // A right click release triggers an exit event. We want to
   // remain in a PUSHED state until the drop down menu closes.
@@ -116,26 +116,38 @@ void ButtonDropDown::OnMouseExited(const ui::MouseEvent& event) {
     SetState(STATE_NORMAL);
 }
 
-void ButtonDropDown::OnGestureEvent(ui::GestureEvent* event) {
+void ToolbarButton::OnGestureEvent(ui::GestureEvent* event) {
   if (menu_showing_) {
     // While dropdown menu is showing the button should not handle gestures.
     event->StopPropagation();
     return;
   }
 
-  ImageButton::OnGestureEvent(event);
+  LabelButton::OnGestureEvent(event);
 }
 
-void ButtonDropDown::GetAccessibleState(ui::AccessibleViewState* state) {
-  views::CustomButton::GetAccessibleState(state);
+void ToolbarButton::GetAccessibleState(ui::AccessibleViewState* state) {
+  CustomButton::GetAccessibleState(state);
   state->role = ui::AccessibilityTypes::ROLE_BUTTONDROPDOWN;
   state->default_action = l10n_util::GetStringUTF16(IDS_APP_ACCACTION_PRESS);
   state->state = ui::AccessibilityTypes::STATE_HASPOPUP;
 }
 
-void ButtonDropDown::ShowContextMenuForView(View* source,
-                                            const gfx::Point& point,
-                                            ui::MenuSourceType source_type) {
+gfx::Size ToolbarButton::GetPreferredSize() {
+  gfx::Size label_size = label()->GetPreferredSize();
+  gfx::Size icon_size = image()->GetPreferredSize();
+  if (label_size.width() > 0) {
+    return gfx::Size(label_size.width() + icon_size.width() +
+                         LocationBarView::GetItemPadding(),

[Peter Kasting, 2013/11/19 02:28:50]

Nit: Prefer to put this entire first arg on the same line if there's a way to
linebreak that will accommodate it.

[Greg Billock, 2013/11/20 00:59:03]

Done.

+                     icon_size.height());
+  } else {

[Peter Kasting, 2013/11/19 02:28:50]

Nit: No else after return.

If you reverse the conditional, you can avoid {} as well.

[Greg Billock, 2013/11/20 00:59:03]

Done.

+    return icon_size;
+  }
+}
+
+void ToolbarButton::ShowContextMenuForView(View* source,
+                                           const gfx::Point& point,
+                                           ui::MenuSourceType source_type) {
   if (!enabled())
     return;
 
@@ -143,7 +155,7 @@ void ButtonDropDown::ShowContextMenuForView(View* source,
   ShowDropDownMenu(source_type);
 }
 
-bool ButtonDropDown::ShouldEnterPushedState(const ui::Event& event) {
+bool ToolbarButton::ShouldEnterPushedState(const ui::Event& event) {
   // Enter PUSHED state on press with Left or Right mouse button or on taps.
   // Remain in this state while the context menu is open.
   return event.type() == ui::ET_GESTURE_TAP ||
@@ -152,11 +164,11 @@ bool ButtonDropDown::ShouldEnterPushedState(const ui::Event& event) {
              ui::EF_RIGHT_MOUSE_BUTTON) & event.flags()) != 0);
 }
 
-bool ButtonDropDown::ShouldShowMenu() {
-  return true;
+bool ToolbarButton::ShouldShowMenu() {
+  return model_.get() != NULL;
 }
 
-void ButtonDropDown::ShowDropDownMenu(ui::MenuSourceType source_type) {
+void ToolbarButton::ShowDropDownMenu(ui::MenuSourceType source_type) {
   if (!ShouldShowMenu())
     return;
 
@@ -231,4 +243,3 @@ void ButtonDropDown::ShowDropDownMenu(ui::MenuSourceType source_type) {
   if (state_ != STATE_DISABLED)
     SetState(STATE_NORMAL);
 }
-