Makes ImageSkia more like SkBitmap

ui/gfx/image/image_skia.h

Index: ui/gfx/image/image_skia.h
diff --git a/ui/gfx/image/image_skia.h b/ui/gfx/image/image_skia.h
index bdfb53f8d89ae941d143d303b88c35a41af76551..729f94f44cd149090ae87ff42a5868ac8de75f9b 100644
--- a/ui/gfx/image/image_skia.h
+++ b/ui/gfx/image/image_skia.h
@@ -9,80 +9,95 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "ui/base/ui_export.h"
 #include "third_party/skia/include/core/SkBitmap.h"
-#include "ui/gfx/canvas.h"
-#include "ui/gfx/size.h"
 
 namespace gfx {
 
+namespace internal {
+class ImageSkiaStorage;
+}  // namespace internal
+
 // Container for the same image at different densities, similar to NSImage.
 // Smallest image is assumed to represent 1x density.
 
-// ImageSkia should be used for caching and drawing images of different
-// densities. It should not be used as an SkBitmap wrapper.
+// ImageSkia should be used whenever possible instead of SkBitmap.
+// Functions which mutate the image should operate on the SkBitmap returned
+// from ImageSkia::GetBitmapForScale, not on ImageSkia.
 class UI_EXPORT ImageSkia {

[sky, 2012/04/27 23:33:58]

Document this supports copy semantics and why.

[pkotwicz, 2012/04/30 20:27:07]

Done.

  public:
+  // Creates null instance.
+  ImageSkia();
+
+  // Adds ref to passed in bitmap.
+  // DIP width and height are set based on scale factor of 1x.
+  ImageSkia(const SkBitmap& bitmap);

[sky, 2012/04/27 15:47:56]

explicit

[pkotwicz, 2012/04/27 16:14:41]

This cannot be explicit. In this case we want it to be non explicit such that
the compiler can convert methods to which SkBitmaps are passed in to
gfx::ImageSkia expected as a parameter. We should change this explicit after the
conversion is done but for the purpose of this CL it should not be explicit.

[sky, 2012/04/27 23:33:58]

I would rather see us introduce this slowly than add explicit.
Also, shouldn't this pick up the default scale rather than 1?

[pkotwicz, 2012/04/30 20:27:07]

I am changing this to take the monitor scale factor for now.
HOWEVER, I intend to remove this constructor once the conversion is done and use
the constructor which uses a scale factor parameter instead.
We can pick a default scale factor as you mentioned from the gfx::Monitor
object. However, in the multi monitor case, we will get an invalid DIP size. The
reason is that we get the DIP width and height from |bitmap.width()| /
scale_factor.
I would prefer to phase in the use of this object slowly. Do you have any
suggestions as to how this can be done?

+
+  // Adds ref to passed in bitmap.
+  // DIP width and height are set based on |scale_factor|.
+  ImageSkia(const SkBitmap& bitmap, float scale_factor);
+
+  // Takes ownership of passed in bitmap.
+  // DIP width and height are set assuming scale factor of 1x.
   explicit ImageSkia(const SkBitmap* bitmap);

[sky, 2012/04/27 23:33:58]

Do we really need this to take ownership? Can't it copy instead?

[pkotwicz, 2012/04/30 20:27:07]

I think we need this as long as gfx::Image(SkBitmap*) exists.
However, it is not difficult to remove that constructor. (I have a patch).

+
+  // Takes ownership of passed in bitmap.
+  // DIP width and height are set assuming smallest bitmap has scale factor of
+  // 1x.
   explicit ImageSkia(const std::vector<const SkBitmap*>& bitmaps);
-  ~ImageSkia();
 
-  // Build mipmap at time of next call to |DrawToCanvasInt|.
-  void BuildMipMap();
+  // Copies a reference to |other|'s storage.
+  ImageSkia(const ImageSkia& other);
 
-  // Draws the image with the origin at the specified location. The upper left
-  // corner of the image is rendered at the specified location.
-  void DrawToCanvasInt(Canvas* canvas, int x, int y);
-
-  // Draws the image with the origin at the specified location, using the
-  // specified paint. The upper left corner of the image is rendered at the
-  // specified location.
-  void DrawToCanvasInt(Canvas* canvas,
-                       int x, int y,
-                       const SkPaint& paint);
-
-  // Draws a portion of the image in the specified location. The src parameters
-  // correspond to the region of the image to draw in the region defined
-  // by the dest coordinates.
-  //
-  // If the width or height of the source differs from that of the destination,
-  // the image will be scaled. When scaling down, it is highly recommended
-  // that you call BuildMipMap() on your image to ensure that it has
-  // a mipmap, which will result in much higher-quality output. Set |filter| to
-  // use filtering for bitmaps, otherwise the nearest-neighbor algorithm is used
-  // for resampling.
-  //
-  // An optional custom SkPaint can be provided.
-  void DrawToCanvasInt(Canvas* canvas,
-                       int src_x, int src_y, int src_w, int src_h,
-                       int dest_x, int dest_y, int dest_w, int dest_h,
-                       bool filter);
-  void DrawToCanvasInt(Canvas* canvas,
-                       int src_x, int src_y, int src_w, int src_h,
-                       int dest_x, int dest_y, int dest_w, int dest_h,
-                       bool filter,
-                       const SkPaint& paint);
-
-  // Returns true if |size_| is empty.
-  bool IsZeroSized() const { return size_.IsEmpty(); }
+  // Copies a reference to |other|'s storage.
+  ImageSkia& operator=(const ImageSkia& other);
 
-  // Width and height of image in DIP coordinate system.
-  int width() const { return size_.width(); }
-  int height() const { return size_.height(); }
+  // Converts from SkBitmap.
+  // Adds ref to passed in bitmap.
+  // DIP width and height are set based on scale factor of 1x.
+  ImageSkia& operator=(const SkBitmap& other);
 
-  // Returns a vector with the SkBitmaps contained in this object.
-  const std::vector<const SkBitmap*>& bitmaps() const { return bitmaps_; }
+  // Converts to SkBitmap.
+  // TODO(pkotwicz): Remove this function.
+  operator SkBitmap() const;

[sky, 2012/04/27 23:33:58]

Why do you want this?

[pkotwicz, 2012/04/30 20:27:07]

I want this temporarily to make the transition to SkBitmap easier.
This will allow changing the callers of SkBitmapOperations to use ImageSkia
while keeping SkBitmapOperations operating on an SkBitmap.

+
+  ~ImageSkia();
 
- private:
   // Returns the bitmap whose density best matches |x_scale_factor| and
   // |y_scale_factor|.
   const SkBitmap* GetBitmapForScale(float x_scale_factor,
                                     float y_scale_factor) const;
 
-  std::vector<const SkBitmap*> bitmaps_;
-  gfx::Size size_;
-  bool mip_map_build_pending_;
+  // Returns true if object is null or |size_| is empty.
+  bool empty() const;
+
+  // Returns true if this is a null object.
+  bool isNull() const { return storage_ == NULL; }

[sky, 2012/04/27 23:33:58]

Do we really need to distinguish between empty() and isNull?

[tfarina, 2012/04/30 00:14:58]

nit: also either is_null() or IsNull(), but not isNull().

+
+  // Width and height of image in DIP coordinate system.
+  int width() const;
+  int height() const;
+
+  // Wrapper function for SkBitmap extractBitmap.
+  // Operates on bitmap at index 0 if available.
+  // TODO(pkotwicz): Remove this function from gfx::ImageSkia.
+  bool extractSubset(ImageSkia* dst, SkIRect& subset) const;

[sky, 2012/04/27 23:33:58]

ExtractSubset, Make this take a SkIRect*

[pkotwicz, 2012/04/30 20:27:07]

This is how the method is defined in the SkBitmap API

+
+  // Build mipmap at time of next call to |DrawToCanvasInt| or |TileInCanvas|.
+  // TODO(pkotwicz): Do something smarter for building mipmap.
+  void BuildMipMap();
+
+  // Returns true if a mip map should be generated when the image is drawn.
+  bool ShouldBuildMipMap() const;
+
+  // Returns a vector with the SkBitmaps contained in this object.
+  const std::vector<const SkBitmap*>& bitmaps() const;
+
+ private:
 
-  DISALLOW_COPY_AND_ASSIGN(ImageSkia);
+  // A refptr so that ImageRepSkia can be copied cheaply.
+  scoped_refptr<internal::ImageSkiaStorage> storage_;
 };
 
 }  // namespace gfx