Introduce some util code for drawing vector assets.

ui/views/controls/image_view.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_IMAGE_VIEW_H_
 #define UI_VIEWS_CONTROLS_IMAGE_VIEW_H_
 
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/vector_icons.h"
 #include "ui/views/view.h"
 
 namespace gfx {
 class Canvas;
 }
 
 namespace views {
 
 class Painter;
 
@@ skipping 26 matching lines @@
 
   // Set the image that should be displayed from a pointer. Reset the image
   // if the pointer is NULL. The pointer contents is copied in the receiver's
   // image.
   void SetImage(const gfx::ImageSkia* image_skia);
 
   // Returns the image currently displayed or NULL of none is currently set.
   // The returned image is still owned by the ImageView.
   const gfx::ImageSkia& GetImage();
 
+  // Tells the view to draw a monochrome vector image identified by |id| using
+  // |color|. Since the size of the view and the image will be set to

[Peter Kasting, 2015/07/01 22:26:58]

Nit: I think you want to remove "Since" (sentence fragment otherwise)

[Evan Stade, 2015/07/02 00:08:06]

Done.

+  // |image_size|.
+  void SetVectorIcon(gfx::VectorIconId id,
+                     SkColor color,
+                     const gfx::Size& image_size);
+
   // Set the desired image size for the receiving ImageView.
   void SetImageSize(const gfx::Size& image_size);
 
-  // Return the preferred size for the receiving view. Returns false if the
-  // preferred size is not defined, which means that the view uses the image
-  // size.
-  bool GetImageSize(gfx::Size* image_size) const;
-
   // Returns the actual bounds of the visible image inside the view.
   gfx::Rect GetImageBounds() const;
 
   // Reset the image size to the current image dimensions.
   void ResetImageSize();
 
   // Set / Get the horizontal alignment.
   void SetHorizontalAlignment(Alignment ha);
   Alignment GetHorizontalAlignment() const;
 
@@ skipping 16 matching lines @@
   void OnPaint(gfx::Canvas* canvas) override;
   void GetAccessibleState(ui::AXViewState* state) override;
   const char* GetClassName() const override;
   bool GetTooltipText(const gfx::Point& p,
                       base::string16* tooltip) const override;
   bool CanProcessEventsWithinSubtree() const override;
 
  private:
   void OnPaintImage(gfx::Canvas* canvas);
 
+  void OnPaintVectorIcon(gfx::Canvas* canvas);
+
   // Returns true if |img| is the same as the last image we painted. This is
   // intended to be a quick check, not exhaustive. In other words it's possible
   // for this to return false even though the images are in fact equal.
   bool IsImageEqual(const gfx::ImageSkia& img) const;
 
   // Compute the image origin given the desired size and the receiver alignment
   // properties.
   gfx::Point ComputeImageOrigin(const gfx::Size& image_size) const;
 
   // Whether the image size is set.
   bool image_size_set_;
 
   // The actual image size.
   gfx::Size image_size_;
 
   // The underlying image.
   gfx::ImageSkia image_;
 
+  // The ID of the vector icon that should be drawn, or gfx::VECTOR_ICON_NONE.
+  // This is drawn in addition to |image_|, but in most cases you probably want
+  // one or the other and not both.
+  gfx::VectorIconId vector_id_;

[Peter Kasting, 2015/07/01 22:26:58]

Does it make sense to make bitmap and vector image views sibling classes that
mostly inherit a common base, instead of putting both pieces of functionality
into one class?

[Evan Stade, 2015/07/02 00:08:06]

I'm not a huge fan of classes that hold implementation but can't themselves be
instantiated; also, that would require updating a lot of places that use
ImageView, which I think would be fairly disruptive for little benefit. I did
consider making a subclass of ImageView called VectorImageView, but there's so
little actual functionality being added here that I didn't think it worth it.

side note: I was actually thinking that at some point in the future we might
want both things to be drawn. For example, there are content settings icons for
every content type, and then exact copies with an [X] drawn on top. The [X]
could potentially be vectorized.

[Peter Kasting, 2015/07/02 00:23:53]

That latter wouldn't be a concern if the bitmap class was ImageView, the base
ImageViewBase, and the vector class VectorImageView.

It's hard to know how to weigh the different tradeoffs here, but I tend to
dislike classes that conglomerate multiple distinct kinds of functionality/APIs.
 In such cases, it's often easy to remove all the uses of a certain thing later
on, and not realize the functionality is dead, because there are still plenty of
references to the class name.  If you have a VectorImageView and we remove all
instantiations, that's easier to see/codesearch.

Glomming the functionality together also makes it hard for the reader to know
what bits are only needed by bitmaps, what by vectors, and what by both.  The
best we can do is add comments to all the relevant members and functions. 
Splitting into distinct classes makes it easy to see what's shared and what's
not.
That's reasonable but I don't see why we couldn't do that with two images at the
callsite.  It'd likely just be a couple lines of code.  Whereas combining these
into one class that can draw both raises other questions: why is the vector
always on top of the bitmap -- what if we want it below?  Why is vector + bitmap
one image, but vector + vector and bitmap + bitmap both two?

[Evan Stade, 2015/07/02 00:58:24]

Like GetImageSize, which was a public function but not actually being used
anywhere. But I don't think it's actually less likely if there's a separate
VectorImageView class. If you remove a single use of VectorImageView, and you're
not being particularly thorough, what will force you to realize that it was the
last use of VectorImgaeView?
I don't think there's much support with the stock layout managers for stacking
sibling views on top of each other, so I think it would be more than a couple
lines. But yea, this was just a side note and until we have an actual use case
probably not worth considering.
Well, I still dislike this sort of inheritance for sharing code, and I think the
amount of code added here is very small (and hence easy to understand, with or
without comments). How strongly do you feel about this topic?

[Peter Kasting, 2015/07/02 08:06:11]

Having added and then ripped out tons of different functionality in the omnibox
over time, it's always been easier when it's separated into different classes. 
It's just much easier to know you can throw a whole class away from one search
than to individually try and look at every API and data member to see if they're
still used.  No, this doesn't prevent forgetting to remove things entirely; it's
about making the actual removal process easier.

In any case, I think that's a side benefit; the clarity benefit to people
reading the code while it's in the product is the main win.
So use composition.  I don't have a preference.
Which means separate classes would be extremely simple and easy to read.
I think it should be done, but I feel a lot more strongly about the separate
files for each icon proposal.  That one is pretty much a "this has to happen".

+
+  // The color to use when drawing the vector icon.
+  SkColor vector_color_;
+
   // Horizontal alignment.
   Alignment horiz_alignment_;
 
   // Vertical alignment.
   Alignment vert_alignment_;
 
   // The current tooltip text.
   base::string16 tooltip_text_;
 
   // A flag controlling hit test handling for interactivity.
   bool interactive_;
 
   // Scale last painted at.
   float last_paint_scale_;
 
   // Address of bytes we last painted. This is used only for comparison, so its
   // safe to cache.
   void* last_painted_bitmap_pixels_;
 
   scoped_ptr<views::Painter> focus_painter_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageView);
 };
 
 }  // namespace views
 
 #endif  // UI_VIEWS_CONTROLS_IMAGE_VIEW_H_