Add VectorIconButton functionality to ImageButton.

ui/views/controls/button/image_button.h

 // Copyright (c) 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 UI_VIEWS_CONTROLS_BUTTON_IMAGE_BUTTON_H_
 #define UI_VIEWS_CONTROLS_BUTTON_IMAGE_BUTTON_H_
 
 #include <memory>
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "ui/base/layout.h"
 #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
 // SetFocusForPlatform() to make it part of the focus chain.
 class VIEWS_EXPORT ImageButton : public CustomButton {
  public:
   static const char kViewClassName[];
 
   enum HorizontalAlignment {
     ALIGN_LEFT = 0,
     ALIGN_CENTER,
     ALIGN_RIGHT
   };
 
   enum VerticalAlignment {
     ALIGN_TOP = 0,
     ALIGN_MIDDLE,
     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|.
   virtual const gfx::ImageSkia& GetImage(ButtonState state) const;
 
   // Set the image the button should use for the provided state.
   void SetImage(ButtonState state, const gfx::ImageSkia* image);
 
   // As above, but takes a const ref. TODO(estade): all callers should be
   // updated to use this version, and then the implementations can be
   // 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,
                      const gfx::ImageSkia* mask);
 
   // Sets how the image is laid out within the button's bounds.
   void SetImageAlignment(HorizontalAlignment h_align,
                          VerticalAlignment v_align);
 
   void SetFocusPainter(std::unique_ptr<Painter> focus_painter);
 
   // The minimum size of the contents (not including the border). The contents
   // will be at least this size, but may be larger if the image itself is
   // larger.
   const gfx::Size& minimum_image_size() const { return minimum_image_size_; }
   void SetMinimumImageSize(const gfx::Size& size);
 
   // Whether we should draw our images resources horizontally flipped.
   void SetDrawImageMirrored(bool mirrored) {
     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:
   void OnFocus() override;
   void OnBlur() override;
 
   // Returns the image to paint. This is invoked from paint and returns a value
   // from images.
   virtual gfx::ImageSkia GetImageToPaint();
 
   // Updates button background for |scale_factor|.
   void UpdateButtonBackground(ui::ScaleFactor scale_factor);
 
   Painter* focus_painter() { return focus_painter_.get(); }
 
   // The images used to render the different states of this button.
   gfx::ImageSkia images_[STATE_COUNT];
 
   gfx::ImageSkia background_image_;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(ImageButtonTest, Basics);
   FRIEND_TEST_ALL_PREFIXES(ImageButtonTest, ImagePositionWithBorder);
   FRIEND_TEST_ALL_PREFIXES(ImageButtonTest, LeftAlignedMirrored);
   FRIEND_TEST_ALL_PREFIXES(ImageButtonTest, RightAlignedMirrored);
 
   // 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_;
   gfx::Size minimum_image_size_;
 
   // Whether we draw our resources horizontally flipped. This can happen in the
   // linux titlebar, where image resources were designed to be flipped so a
   // small curved corner in the close button designed to fit into the frame
   // resources.
   bool draw_image_mirrored_;
 
   std::unique_ptr<Painter> focus_painter_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageButton);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // ToggleImageButton
 //
 // A toggle-able ImageButton.  It swaps out its graphics when toggled.
 //
 ////////////////////////////////////////////////////////////////////////////////
 class VIEWS_EXPORT ToggleImageButton : public ImageButton {
  public:
-  explicit ToggleImageButton(ButtonListener* listener);
+  explicit ToggleImageButton(ImageButtonDelegate* delegate);
   ~ToggleImageButton() override;
 
   // Change the toggled state.
   void SetToggled(bool toggled);
 
   // Like ImageButton::SetImage(), but to set the graphics used for the
   // "has been toggled" state.  Must be called for each button state
   // before the button is toggled.
   void SetToggledImage(ButtonState state, const gfx::ImageSkia* image);
 
   // 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;
 
  private:
   // The parent class's images_ member is used for the current images,
   // and this array is used to hold the alternative images.
   // We swap between the two when toggling.
   gfx::ImageSkia alternate_images_[STATE_COUNT];
 
   // True if the button is currently toggled.
   bool toggled_;
 
   // The parent class's tooltip_text_ is displayed when not toggled, and
   // this one is shown when toggled.
   base::string16 toggled_tooltip_text_;
 
   DISALLOW_COPY_AND_ASSIGN(ToggleImageButton);
 };
 
 }  // namespace views
 
 #endif  // UI_VIEWS_CONTROLS_BUTTON_IMAGE_BUTTON_H_