Add VectorIconButton functionality to ImageButton.

ui/views/controls/button/vector_icon_image_button_observer.cc

Index: ui/views/controls/button/vector_icon_image_button_observer.cc
diff --git a/ui/views/controls/button/vector_icon_image_button_observer.cc b/ui/views/controls/button/vector_icon_image_button_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..19ad45bda78acd11abc652547e8a05a3051e6e09
--- /dev/null
+++ b/ui/views/controls/button/vector_icon_image_button_observer.cc
@@ -0,0 +1,60 @@
+// Copyright 2017 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 "ui/views/controls/button/vector_icon_image_button_observer.h"
+
+#include "ui/gfx/color_utils.h"
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/views/animation/ink_drop_host_view.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/button/custom_button.h"
+#include "ui/views/controls/button/image_button.h"
+#include "ui/views/painter.h"
+#include "ui/views/view.h"
+#include "ui/views/views_delegate.h"
+
+namespace views {
+
+ImageButton* CreateDefaultVectorIconButton(
+    const gfx::VectorIcon& icon,

[bruthig, 2017/03/13 15:19:49]

nit: |icon| doesn't appear to be required/used.

+    VectorIconImageButtonObserver* observer,
+    ButtonListener* listener) {
+  ImageButton* button = new ImageButton(listener);
+  button->AddObserver(observer);
+  button->SetInkDropMode(InkDropHostView::InkDropMode::ON);
+  button->set_has_ink_drop_action_on_click(true);
+  button->SetImageAlignment(ImageButton::ALIGN_CENTER,
+                            ImageButton::ALIGN_MIDDLE);
+  button->SetFocusPainter(nullptr);
+  button->SetPadding(ViewsDelegate::GetInstance()->GetButtonMargins());
+  return button;
+};
+
+void VectorIconImageButtonObserver::OnThemeChanged(View* view) {
+  UpdateButtonImages(view);
+}
+
+void VectorIconImageButtonObserver::OnNativeThemeChanged(
+    View* view,
+    const ui::NativeTheme* theme) {
+  UpdateButtonImages(view);
+}
+
+void VectorIconImageButtonObserver::UpdateButtonImages(View* view) {

[bruthig, 2017/03/13 15:19:49]

I'm not sure if this is already on your radar or not but I want to call it out.

We should make sure that an icon will properly be set even if OnThemeChanged()
or OnNativeThemeChanged() is not called.  And ideally we would not add any
additional requirements on to the clients to call 'Init()' or something like
that.

I believe this scenario will happen if the View/Button that the observer is
being attached to, is already a in the View tree before the observer is
attached.

[Bret, 2017/03/15 00:07:13]

I fixed this by giving the observer an Attach method that adds itself as an
observer and then updates the icon.

[sky, 2017/03/15 15:05:11]

You shouldn't need this. OnNativeThemeChanged() is always called when the view
becomes parented to a widget.

[bruthig, 2017/03/15 15:53:20]

What if the View is parented to a Widget before the
VectorIconImageButtonObserver is attached?  This may not happen currently
because the ImageButton is being created in the CreateDefaultVectorIconButton()
factory function.  But eventually if we have different effects following this
pattern (e.g. InkDropEffect) then it doesn't make sense for them all to go
through these factory functions that ensure this.  Relying on factory functions
to support this is similar to relying on concrete class constructors to wire
everything together which is one contributing factor to the overgrowth of the
Button hierarchy.

[sky, 2017/03/15 17:01:54]

You're suggesting the function take a View? In that case it could call
view->GetWidget() and if non-null call OnNativeThemeChanged() directly.
I'm not sure I follow. Bret suggested adding another function explicitly for the
initial call. That doesn't seem necessary given OnNativeThemeChanged() covers
this. Maybe I'm missing something?

[bruthig, 2017/03/15 21:41:21]

disclaimer: I'm not clear on what 'function' you mean here or where exactly
Bret's original Attach() would live.

I wasn't orignally suggesting a solution I was just requesting that it be
possible to attach the observer after the button had been parented to a Widget. 
I think it is limiting to require the button to be parented to a Widget before
the observers are added.

sky@, this may be what you are suggesting but thinking about it now, I would
probably explore adding a ViewObserver::AttachedToView(View* observed_view)
method that is called when observers are added to a View.  Then the
VectorIconImageButtonObserver could set an image and use the theme if it's
available.

[Bret, 2017/03/15 23:39:19]

I think bruthig@ is right, that you could hypothetically create an image button,
add it to the hierarchy, and THEN add this observer. Then the image wouldn't get
set from the vector icon. Unless you mean adding an observer itself triggers
OnNativeThemeChanged? But I don't see that code.

I moved AddObserver inside an "Attach" method on the observer that updates the
images too. But the ViewObserver::AttachedToView idea sounds reasonable to me,
since then you don't have to call a different method (Attach vs AddObserver) to
wire everything up correctly.

[sky, 2017/03/15 23:48:04]

You could make that argument for any observer. I think whoever is attaching the
observer to the view should know if they need to make additional calls before
attaching the observer. I don't think we should add functions like
ObserverAttached and ObserverDetached.

+  const gfx::VectorIcon& icon = GetVectorIcon(view);
+  SkColor icon_color =
+      color_utils::DeriveDefaultIconColor(GetVectorIconColor());
+  ImageButton* button = static_cast<ImageButton*>(view);
+  SkColor disabled_color = SkColorSetA(icon_color, 0xff / 2);
+  button->SetImage(CustomButton::STATE_NORMAL,
+                   gfx::CreateVectorIcon(icon, icon_color));
+  button->SetImage(CustomButton::STATE_DISABLED,
+                   gfx::CreateVectorIcon(icon, disabled_color));
+  button->set_ink_drop_base_color(icon_color);
+}
+
+SkColor VectorIconImageButtonObserver::GetVectorIconColor() const {
+  return SK_ColorBLACK;
+}
+}