Add VectorIconButton functionality to ImageButton.

ui/views/controls/button/vector_icon_image_button_observer.h

+// 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.
+
+#ifndef UI_VIEWS_VECTOR_ICON_IMAGE_BUTTON_OBSERVER_H_
+#define UI_VIEWS_VECTOR_ICON_IMAGE_BUTTON_OBSERVER_H_
+
+#include "base/macros.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/vector_icon_types.h"
+#include "ui/views/view_observer.h"
+
+namespace ui {
+class NativeTheme;
+}
+
+namespace views {
+
+class ButtonListener;
+class ImageButton;
+class View;
+
+class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver {

[sky, 2017/03/13 16:21:57]

How about naming this VectorIconImageUpdater or something? I suggest that as the
ViewObserver part of this class is an implementation detail. The main reason for
this class is getting the vector icon and color.

Also, add a description.

[Evan Stade, 2017/03/15 15:04:02]

with ViewObserver I'm no longer sure this interface is all that helpful. It
doesn't cut down on the number of overrides needed. Maybe the owners of the
vector icon buttons should just be ViewObservers.

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

I think the value of this class will grow as it gets enhanced.  The current
value of this class is that it derives a disabled icon color and applies it
depending on the observed view's state.  I don't know why we would want to
duplicate that logic in multiple vector icon owners.

As for naming, I propose SimpleVectorIconImageUpdater.  I imagine we will have
fancier image updater use cases in the future, also be based on vector icons,
that won't make sense to bloat this class with.

[Evan Stade, 2017/03/15 16:59:25]

I see. I think this is breaking the Chromium inheritance rules against multiple
inheritance though. Only one base class can have non-trivial/no-op
functionality.

[Peter Kasting, 2017/03/15 18:03:02]

FWIW, our UI code defies that rule in 8 billion places.

Arguably, we could do better, by holding a lot more things by value instead of
pointer (move constructors make this palatable), using composition when we can,
using templates and inner classes to implement functionality instead of virtual
inheritance -- the Sean Parent talk I linked in a Chromium-dev thread recently
talks a lot about this stuff in the last third of the talk.

But I don't know how much appetite there is to make that change broadly (no one
has talked about doing it, not even me), and I'm worried it's hard to get there
piecemeal.  And when the pattern in the UI code is to use multiple inheritance
to do this, it seems less bad to also do so here?

Anyway, not really arguing hard for one point of view.

[Peter Kasting, 2017/03/15 18:16:45]

Evan sez: we've fixed a lot of these and are trying not to add more.

I am on board with this sentiment.

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

The FindBarViewButtonObserver found in find_bar_view.h only inherits from
VectorIconImageButtonObserver.  Not sure where we are violating the multiple
inheritance restriction.

To be honest I'd rather see a concrete re-usable VectorIconUpdater that maps
view pointers to vector icons instead of having a specialized class like
FindBarViewButtonObserver.

Ex:

FindBarView::FindBarView(...) {
...
  VectorIconUpdater* icon_updater = new VectorIconUpdator();

  icon_updator->SetThemColorId(ui::NativeTheme::kColorId_TextfieldDefaultColor);

  icon_updater->AddImage(find_previous_button_, ui::kCaretUpIcon);
  icon_updater->AddImage(find_next_button_, ui::kCaretDownIcon);
  icon_updater->AddImage(close_button_, ui::kCloseIcon);

  AddObserver(icon_updater);
...
}

[Evan Stade, 2017/03/15 22:46:20]

That's kind of an elaborate way to get around multiple inheritance. Are all six
current VectorIconButtonDelegate implementers going to create a private class
like this, e.g. DownloadItemViewButtonObserver?

I guess I'm struggling to see how defining and owning a thunk-like class that
overrides a half-interface, half-class base class which in turn overrides
another related interface is a win over having VectorIconButton. If we really
don't want VectorIconButton, I think we should just be folding the functionality
into ImageButton, i.e. ImageButton::SetVectorIcon(gfx::VectorIcon, SkColor)

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

To bruthig's suggestion, I thought about doing something where the observer
would have a map of button to icon, but since there's a variety of ways to fetch
the icon color on update subclasses are probably still necessary.

To estade's suggestion, I added ImageButton::SetVectorIcon in patchset 1, if you
want to see how that worked out. The icon color needs to update on a theme
change, so we can't just set-and-forget the icon color.

+ public:
+  VectorIconImageButtonObserver() = default;
+
+  // ViewObserver:
+  void OnThemeChanged(View* view) override;
+  void OnNativeThemeChanged(View* view, const ui::NativeTheme* theme) override;
+
+ private:
+  void UpdateButtonImages(View* view);
+  virtual const gfx::VectorIcon& GetVectorIcon(View* view) const = 0;

[sky, 2017/03/13 16:21:57]

Having these two functions private certainly works, but IMO is confusing. How
about moving these two to the protected section and moving the ViewObserver
implementation to the private section?

+  virtual SkColor GetVectorIconColor() const;

[sky, 2017/03/13 16:21:57]

Add descriptions of these functions.

+
+  DISALLOW_COPY_AND_ASSIGN(VectorIconImageButtonObserver);
+};
+
+VIEWS_EXPORT ImageButton* CreateDefaultVectorIconButton(

[sky, 2017/03/13 16:21:57]

Please add a comment. Also, make this return a unique_ptr so ownership is clear.
Also, 'default' is a little unclear here. How about
CreateImageButtonWithVectorIcon?

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

The name is good. I'm not sure how unique_ptr interacts with the child
view/parent view relationship, it seems to me like it wouldn't work. And it'd be
silly to make all callers call release() on the pointer.

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

Calling release() still makes ownership clear. But I'm ok without unique_ptr
here.

+    const gfx::VectorIcon& icon,
+    VectorIconImageButtonObserver* observer,

[sky, 2017/03/13 16:21:57]

WDYT of making the button own the observer? I realize that wouldn't work with
how you have things in find_bar, but it's an easy change and I suspect less
error prone. Do you think it's going to be common to want to share the observer?

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

I wanted to avoid giving ImageButton knowledge of this class, but it does make
the ownership pattern easier. I could extract a base class, "ImageUpdater" that
calls UpdateButtonImages when appropriate and make VectorIconImageUpdater a
subclass, that way ImageButton remains ignorant of everything
vector-icon-related. But I don't see any other places that would want to make
their own ImageUpdater so I'm worried it would be a wasted base class.

On the other hand most of the current VectorIconButton instances are close
buttons with the same icon, so we could have a global "CloseIconImageUpdater"
for all of them if we can share observers. I'm not sure how compelling that is.

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

Sorry for not being clear. I was suggesting ownership is handled by overriding
OnViewIsDeleting() and deleting this.

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

I did this and it works well.

+    ButtonListener* listener);
+}
+
+#endif  // UI_VIEWS_VECTOR_ICON_IMAGE_BUTTON_OBSERVER_H_