Add VectorIconButton functionality to ImageButton.

ui/views/controls/button/image_button.h

Index: ui/views/controls/button/image_button.h
diff --git a/ui/views/controls/button/image_button.h b/ui/views/controls/button/image_button.h
index eba2a835a40eff5f07a628558b2fe1fd5ad48b16..00af163f2f9f73d8289f6b052933fee2fd17f884 100644
--- a/ui/views/controls/button/image_button.h
+++ b/ui/views/controls/button/image_button.h
@@ -13,10 +13,24 @@
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/controls/button/custom_button.h"
 
+namespace gfx {
+struct VectorIcon;
+}
+
 namespace views {
 
 class Painter;
 
+// A ButtonListener that also provides padding around the image and a color if
+// the image is a vector icon.
+class VIEWS_EXPORT ImageButtonDelegate : public ButtonListener {

[bruthig, 2017/03/09 05:04:42]

IMO I don't think coupling the click listener to the image properties provider
is the right way to go. I'm pretty sure there are some use cases where the
button pressed listener is and should be a completely different class than the
image property provider.

I'm not sure if this is in line with sky@'s suggestion or not but I've thought
about this a bit in the past and I was thinking about adding an ImageProvider
class that the ImageButton can accept.  The ImageProvider could then be extended
by VectorImageProvider, BitmapImageProvider, MyOwnCustomPaintedImageProvider,
etc.  Something along the lines of:

class ImageProvider {
  unique_ptr<ImageSkia> GetImage(const Size&);
}

And then if you wanted to get super fancy you could have the ImageProvider have
observers defined like below that ImageButton could inherit:

class ImageProviderObserver{
  virtual void ImageChanged();
}

This would enable a few things such as:

- The concrete ImageProvider could own and manage animations
- We could have a small library of general ImageProviders that can be re-used. 
This would support composability which is the primary driving motivation to my
planned button refactor.

sky@, WDYT?

[Bret, 2017/03/09 22:32:37]

In practice I haven't seen any cases where it seems like the listener and color
provider should be different classes. Most of the time the view's parent just
implements all the necessary methods. I do agree it's conceptually wonky though.

[bruthig, 2017/03/10 14:35:25]

As noted below, this is partly because we have many is-a relationships which IMO
would have been better with a has-a relationship.

But FTR ShelfView and BookmarkBarView are two instances that comes to mind that
implement a ButtonListener::ButtonPressed() that handles more than 1 button.  I
think it is quite reasonable that all of the buttons shouldn't have to have the
same color, which this pattern would encourage.

+ public:
+  virtual SkColor GetVectorIconColor() const;

[sky, 2017/03/09 04:05:02]

Specifying the color should not be a requirement for using a vectoricon.
Additionally as vectoricon support is implemented in terms of ImageSkia I don't
think ImageButton needs first class support for vectoricons. Rather I think
there should be a function (not declared in ImageButton, perhaps in
vector_image_buttn.h) for configuring ImageButton from an vectoricon. Basically
this function contains what you have in UpdateImagesFromVectorIcon. If you want
a factory function it can be in the same file.

As to how to update the image color when the theme changes that should be moved
to ViewObserver. Specifically add OnThemeChanged and OnNativeThemeChanged to
ViewObserver and call them from View.

[Bret, 2017/03/09 22:32:37]

Subclasses aren't required to override GetVectorIconColor, there's a default
"return black" in the cc file. This is how VectorIconButton worked.

To your larger point: basically you want a VectorIconButtonObserver that
subclasses ViewObserver that takes the vector icon and calls SetImage when
appropriate, and then a factory that wires it up, if I understand that right.
That doesn't seem substantively different than just keeping VectorIconButton to
me.

[bruthig, 2017/03/10 14:35:25]

It's different because it decouples the button state handling (aka input event)
logic from the buttons presentation. Thus it should be re-usable across any
ImageButton or ImageButton descendant.

We currently have ~100 Button descendants and one contributing factor is because
many buttons have chosen this is-a relationship instead of the has-a one that
sky@ is suggesting.

In short, an enthusiastic +1 to sky@'s suggestion and sentiment.

+
+ protected:
+  ~ImageButtonDelegate() override {}
+};
+
 // An image button.
 // Note that this type of button is not focusable by default and will not be
 // part of the focus chain, unless in accessibility mode. Call
@@ -37,7 +51,11 @@ class VIEWS_EXPORT ImageButton : public CustomButton {
     ALIGN_BOTTOM
   };
 
-  explicit ImageButton(ButtonListener* listener);
+  static ImageButton* CreateDefaultVectorIconButton(
+      const gfx::VectorIcon& icon,
+      ImageButtonDelegate* delegate);
+
+  explicit ImageButton(ImageButtonDelegate* delegate);
   ~ImageButton() override;
 
   // Returns the image for a given |state|.
@@ -51,6 +69,11 @@ class VIEWS_EXPORT ImageButton : public CustomButton {
   // consolidated.
   virtual void SetImage(ButtonState state, const gfx::ImageSkia& image);
 
+  // Sets a vector icon to use for the button images. Generates images for
+  // STATE_NORMAL and STATE_DISALBED based on
+  // ImageButtonDelegate::GetVectorIconColor.
+  void SetVectorIcon(const gfx::VectorIcon& icon);
+
   // Set the background details.
   void SetBackground(SkColor color,
                      const gfx::ImageSkia* image,
@@ -73,10 +96,14 @@ class VIEWS_EXPORT ImageButton : public CustomButton {
     draw_image_mirrored_ = mirrored;
   }
 
+  void SetPadding(const gfx::Insets& padding);
+
   // Overridden from View:
   gfx::Size GetPreferredSize() const override;
   const char* GetClassName() const override;
   void OnPaint(gfx::Canvas* canvas) override;
+  void OnThemeChanged() override;
+  void OnNativeThemeChanged(const ui::NativeTheme* theme) override;
 
  protected:
   // Overridden from View:
@@ -106,6 +133,12 @@ class VIEWS_EXPORT ImageButton : public CustomButton {
   // Returns the correct position of the image for painting.
   gfx::Point ComputeImagePaintPosition(const gfx::ImageSkia& image);
 
+  void UpdateImagesFromVectorIcon();
+
+  ImageButtonDelegate* delegate_;
+
+  const gfx::VectorIcon* vector_icon_;
+
   // Image alignment.
   HorizontalAlignment h_alignment_;
   VerticalAlignment v_alignment_;
@@ -131,7 +164,7 @@ class VIEWS_EXPORT ImageButton : public CustomButton {
 ////////////////////////////////////////////////////////////////////////////////
 class VIEWS_EXPORT ToggleImageButton : public ImageButton {
  public:
-  explicit ToggleImageButton(ButtonListener* listener);
+  explicit ToggleImageButton(ImageButtonDelegate* delegate);
   ~ToggleImageButton() override;
 
   // Change the toggled state.
@@ -145,11 +178,11 @@ class VIEWS_EXPORT ToggleImageButton : public ImageButton {
   // Set the tooltip text displayed when the button is toggled.
   void SetToggledTooltipText(const base::string16& tooltip);
 
-  // Overridden from ImageButton:
+  // ImageButton:
   const gfx::ImageSkia& GetImage(ButtonState state) const override;
   void SetImage(ButtonState state, const gfx::ImageSkia& image) override;
 
-  // Overridden from View:
+  // View:
   bool GetTooltipText(const gfx::Point& p,
                       base::string16* tooltip) const override;
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;