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

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

Index: chrome/browser/ui/views/toolbar/toolbar_button.h
diff --git a/chrome/browser/ui/views/toolbar/button_dropdown.h b/chrome/browser/ui/views/toolbar/toolbar_button.h
similarity index 61%
copy from chrome/browser/ui/views/toolbar/button_dropdown.h
copy to chrome/browser/ui/views/toolbar/toolbar_button.h
index e49ab5437f9f4b82a4784385c1173a52ae97d1ca..178e799c3e45e2ef198828f3fc7a34999cd4a1d9 100644
--- a/chrome/browser/ui/views/toolbar/button_dropdown.h
+++ b/chrome/browser/ui/views/toolbar/toolbar_button.h
@@ -1,13 +1,13 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright 2012 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_TOOLBAR_BUTTON_DROPDOWN_H_
-#define CHROME_BROWSER_UI_VIEWS_TOOLBAR_BUTTON_DROPDOWN_H_
+#ifndef CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_BUTTON_H_
+#define CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_BUTTON_H_
 
-#include "base/memory/weak_ptr.h"
 #include "ui/views/context_menu_controller.h"
-#include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/button/label_button.h"
 
 namespace ui {
 class MenuModel;
@@ -17,43 +17,42 @@ namespace views {
 class MenuRunner;
 }
 
-////////////////////////////////////////////////////////////////////////////////
-//
-// ButtonDropDown
-//
-// A button class that when pressed (and held) or pressed (and drag down) will
-// display a menu
-//
-////////////////////////////////////////////////////////////////////////////////
-class ButtonDropDown : public views::ImageButton,
-                       public views::ContextMenuController {
+// This class provides basic drawing and mouse-over behavior for buttons
+// appearing in the toolbar.
+// See ButtonDropDown.

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

I don't understand why we're saying "See ButtonDropDown" but this class does not
inherit from or compose with ButtonDropDown.

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

This was stale. Deleting.

+class ToolbarButton : public views::LabelButton,
+                      public views::ButtonListener,
+                      public views::ContextMenuController {
  public:
-  // The button's class name.
-  static const char kViewClassName[];
+  // Takes ownership of the |model|, which can be null if no menu
+  // is to be shown.
+  ToolbarButton(views::ButtonListener* listener, ui::MenuModel* model);
+  virtual ~ToolbarButton();
 
-  // Takes ownership of the |model|.
-  ButtonDropDown(views::ButtonListener* listener, ui::MenuModel* model);
-  virtual ~ButtonDropDown();
+  // Set up basic behavior. Should be called by any subclasses.

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

Nit: When?

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

Done.

+  void Init();

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

Nit: Place all overrides together and all non-overrides together.

My normal ordering for this is overrides above non-overrides.

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

Done. I like Init() type methods right after the constructor/destructor, but
I'll move it if you want them all together in this order.

 
-  // If menu is currently pending for long press - stop it.
-  void ClearPendingMenu();
+  // views::ButtonListener. Default implementation does nothing.

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

Why do we need to subclass ButtonListener at all?  It doesn't seem like any
ToolbarButtons want to be listeners for themselves.

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

It's something we'll want for the site chip, but agreed it doesn't really belong
here.

+  virtual void ButtonPressed(views::Button* sender,
+                             const ui::Event& event) OVERRIDE;
 
-  // Indicates if menu is currently showing.
+  // Methods for handling ButtonDropDown-style menus.
+  void ClearPendingMenu();
   bool IsMenuShowing() const;
 
   // Overridden from views::View

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

Nit: While here, fix these comments to just look like: "views::View:" (2 places)

Also, in this case, we should be saying "views::LabelButton:" instead, since
that's our actual direct parent class.

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

Done.

   virtual bool OnMousePressed(const ui::MouseEvent& event) OVERRIDE;
   virtual bool OnMouseDragged(const ui::MouseEvent& event) OVERRIDE;
   virtual void OnMouseReleased(const ui::MouseEvent& event) OVERRIDE;
-  virtual const char* GetClassName() const OVERRIDE;
   // Showing the drop down results in a MouseCaptureLost, we need to ignore it.
   virtual void OnMouseCaptureLost() OVERRIDE {}

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

Nit: While here, don't define virtual function bodies in a .h file

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

Done.

   virtual void OnMouseExited(const ui::MouseEvent& event) OVERRIDE;
   virtual void OnGestureEvent(ui::GestureEvent* event) OVERRIDE;
   virtual void GetAccessibleState(ui::AccessibleViewState* state) OVERRIDE;
+  virtual gfx::Size GetPreferredSize();

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

Nit: This should be at the top of the list instead of the bottom, to match the
declaration order in view.h

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

Done.

 
   // Overridden from views::ContextMenuController
-  virtual void ShowContextMenuForView(views::View* source,
+  virtual void ShowContextMenuForView(View* source,
                                       const gfx::Point& point,
                                       ui::MenuSourceType source_type) OVERRIDE;
 
@@ -84,9 +83,9 @@ class ButtonDropDown : public views::ImageButton,
   scoped_ptr<views::MenuRunner> menu_runner_;
 
   // A factory for tasks that show the dropdown context menu for the button.
-  base::WeakPtrFactory<ButtonDropDown> show_menu_factory_;
+  base::WeakPtrFactory<ToolbarButton> show_menu_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(ButtonDropDown);
+  DISALLOW_COPY_AND_ASSIGN(ToolbarButton);
 };
 
-#endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_BUTTON_DROPDOWN_H_
+#endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_BUTTON_H_