Expose array of bitmaps contained by gfx::Image similar to NSImage

On the Mac, we populate NSImage from bitmaps in resource bundles. Introduce ImageSkia which contains vector of SkBitmaps. Previously this functionality was within ImageRepSkia. ImageSkia exposes this.

Move gfx::Image::GetSkBitmapAtIndex and gfx::Image::GetNumSkBitmaps to ImageSkia

BUG=122992
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133607

[pkotwicz, 2012-04-14 00:15:34]

This is the first part for properly scaling bitmaps in views in high DPI mode.
The eventual goal is to replace gfx::Canvas::DrawBitmapInt with
gfx::Canvas::DrawImageSkiaInt

[sky, 2012-04-16 14:35:26]

rsesek should review this first.

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.h#newcode102
ui/gfx/image/image.h:102: const ImageSkia* ToImageSkia() const;
Document ownership.

[Robert Sesek, 2012-04-16 16:06:01]

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.cc#newcode391
ui/gfx/image/image.cc:391: rep = new internal::ImageRepSkia(bitmaps);
an internal::ImageRepSkia is still being created with a vector here. Should this
be happening?

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.h#newcode72
ui/gfx/image/image.h:72: 
Should you be able to construct an Image with a ImageSkia? (See comment in
image_skia.h about use cases).

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.h#newcode101
ui/gfx/image/image.h:101: 
nit: no blank line

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.cc#new...
ui/gfx/image/image_skia.cc:45: float smallest_diff = FLT_MAX;
std::numeric_limits

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.cc#new...
ui/gfx/image/image_skia.cc:47: float diff = fabs(bitmaps_[i]->width() -
desired_width) +
std::abs

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:15: // Container for images at various densities.
This should be filled out more (use cases, should this be created directly, if
so, when?). Also, move this to be at the class-level, not at the namespace.

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:21: ImageSkia(const SkBitmap* bitmap);
explicit?

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:29: size_t GetNumberOfBitmaps() const { return
bitmaps_.size(); }
Out-of-line.

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:33: return bitmaps_[index];
Out-of-line.

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:37: bool empty() const { return size_.IsEmpty(); }
This is unclear and confounds IsEmpty() in image.h. Also, out-of-line this.
Naming: IsZeroSized().

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:40: bool isNull() const { return bitmaps_.empty(); };
Naming: IsEmpty() to be consistent with image.h. Out-of-line.

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image_skia.h#newc...
ui/gfx/image/image_skia.h:46: const SkBitmap* bitmap() { return bitmaps_[0]; }
Needs a comment.

[Tom Hudson, 2012-04-18 19:10:36]

Just to make a record of our discussion here, we think you're on the right
architectural track - it's a very common pattern in a 3D graphics engine to have
a container node that has several models at varying resolutions and a function
to select between them given a particular distance / scale /
screen-space-projected-area. That's what you're doing in
ImageSkia::GetBitmapForScaleFactor(). I think reed@ wanted to see drawing lumped
in with selection if there's any more logic for the multiresolution image case
than the simple single-resolution SkBitmap case.

[reed1, 2012-04-18 20:52:47]

As an alternative style for the class...

class MultiBitmap {
public:
    MultiBitmap(const SkBitmap&);
    MultiBitmap(vector<>...., const SkISize& canonicalSize);

    // these return the canonical size. Should they be floats?
    int width() const;
    int height() const;

    void draw(SkCanvas*, SkScalar x, SkScalar y, const SkPaint*);
};

///////

void MultiBitmap::draw(SkCanvas* canvas, SkScalar x, SkScalar y, const SkPaint*
paint) {
    SkRect dstRect = SkMakeXYWH(x, y, width(), height());
    const SkBitmap& bm = find_best_fit_bitmap(list, canvas->getTotalMatrix());
    canvas->drawBitmapRect(bm, NULL, dstRect, paint);
}

[pkotwicz, 2012-04-19 03:15:05]

Changes as requested.
Added Draw methods to ImageSkia as suggested by reed@

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.h#newcode102
ui/gfx/image/image.h:102: const ImageSkia* ToImageSkia() const;
Ownership is documented in unmodified comment above. The ownership of all the
"To" methods is the same. ie ToSkBitmap(), ToNSImage() etc

[reed1, 2012-04-19 12:23:58]

My proposal was probably more defensive : I was suggesting that we don't need to
expose the underlying array of bitmaps at all, but just present an Image that
had a canonical size, and knew how to draw itself.

Do you need to make the getters/iterator public?

[pkotwicz, 2012-04-19 13:48:10]

Currently we are using the getters/iterators for unittests. We use the iterator
to transform each Bitmap which is part of an image.
I am not sure if there is a more elegant way.

One option would be to make ImageRepSkia a friend of ImageSkia and expose
getters and setters in gfx::Image but not in gfx::ImageSkia

[reed1, 2012-04-19 13:50:16]

perhaps separate from this CL, I'd like to understand the "transform the image"
or any other requirements, as Skia considers creating a similar data type. Our
preference would be to make it as opaque as possible.

[pkotwicz, 2012-04-19 18:25:28]

I looked at what transformations exist. It seems as if the requirement is the
following: It should be possible to transform the contained SkBitmaps within the
ImageSkia object and make a new ImageSkia out of the transformed SkBitmaps.
I can add a ImageSkia::bitmaps() method which returns a const pointer to a
vector of the contained SkBitmaps and remove ::GetBitmapAtIndex(),
::GetNumberOfBitmaps()

[pkotwicz, 2012-04-19 18:28:47]

I suggest looking deeper into whether ImageSkia::bitmaps() can be improved in a
separate CL.
reed1, resek do you have any other suggestions?

[reed1, 2012-04-19 18:36:59]

My comments are only related to the interface design, in terms of how closely it
might fit into Skia's core. To that end, perhaps 

ImageSkia copyWithTransform(const SkMatrix&) const;

method might encapsulate that use-case, without having to expose internals.

Outside of these API thoughts, I have no real opinion on the implementation
changes, and so am removing myself from the reviewer line (moving to cc).

[Robert Sesek, 2012-04-19 19:12:48]

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.cc#newcode391
ui/gfx/image/image.cc:391: rep = new internal::ImageRepSkia(bitmaps);
On 2012/04/16 16:06:01, rsesek wrote:
> an internal::ImageRepSkia is still being created with a vector here. Should
this
> be happening?

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:137: float smallest_diff =
std::numeric_limits<int>::max();
<float>?

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:18: // Container for images at various densities.
Didn't address the class comment. This needs to talk about when, if ever, you'd
use this directly.

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:23: ImageSkia(const std::vector<const SkBitmap*>&
bitmaps);
explicit?

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:31: void DrawToCanvasInt(Canvas* canvas, int x, int
y);
Would it make sense to use gfx::Point here?

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:53: int src_x, int src_y, int src_w, int src_h,
And gfx::Rect here?

[pkotwicz, 2012-04-19 21:21:26]

Changes as requested.
Added ImageSkia::bitmaps() method
removed ImageSkia::bitmap(), ImageSkia::GetBitmapCount(),
ImageSkia::GetBitmapAt()
This makes the class a bit more similar to NSImage.

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10086023/diff/1/ui/gfx/image/image.cc#newcode391
ui/gfx/image/image.cc:391: rep = new internal::ImageRepSkia(bitmaps);
Thanks for pointing this out, this should not be happening

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10086023/diff/15001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:31: void DrawToCanvasInt(Canvas* canvas, int x, int
y);
Perhaps. However, the goal is to keep the API as similar as possible to that in
gfx::Canvas::DrawBitmapInt.

[Robert Sesek, 2012-04-20 16:33:14]

LGTM

http://codereview.chromium.org/10086023/diff/18003/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/10086023/diff/18003/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:205:
SkBitmapToImageRep(*bitmaps[i]);
nit: can you join this with the nxt line?

[pkotwicz, 2012-04-20 17:07:56]

sky for OWNERS

[sky, 2012-04-20 19:31:53]

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:17: DCHECK(bitmap);
Is this DCHECK useful? Seems like this would crash if bitmap is NULL since the
member initializer list uses it.

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:45: void ImageSkia::DrawToCanvasInt(gfx::Canvas*
canvas, int x, int y,
nit: each param on its own line (x/y can be grouped on one line if you want).

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:122: float y_scale_factor) const {
Why did you make this return a const SkBitmap when the two places that use it
cast the const away?

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:27: virtual ~ImageSkia();
Do you really need this to be virtual?

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:66: bool IsEmpty() const;
Any reason you didn't inline this?

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:73: const std::vector<const SkBitmap*> bitmaps() const
{ return bitmaps_; }
const std::vector&

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:75: private:
nit: spacing

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:83: bool mip_map_build_pending_;
This is never initialized.

[pkotwicz, 2012-04-20 20:39:54]

Changes as requested

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10086023/diff/18003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:122: float y_scale_factor) const {
To address your issue, I am casting the constness away immediately before
calling buildMipMap()

bitmaps_ is a vector of const SkBitmap*.
If ImageSkia::GetBitmapForScale returns SkBitmap* instead of const SkBitmap*,
cost_cast would have to be in ImageSkia::GetBitmapForScale instead
It is useful for bitmaps_ to be const vector, as bitmaps() should return a
vector of const SkBitmaps
The constructor must take as an argument a const SkBitmap* as the "creator"
should not be able to call SkBitmap::reset() after having created the ImageSkia
object

[sky, 2012-04-20 21:16:15]

LGTM
Please add comment as to why const_cast is safe.

[commit-bot: I haz the power, 2012-04-23 19:04:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10086023/31005

[commit-bot: I haz the power, 2012-04-23 19:18:26]

Try job failure for 10086023-31005 (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-04-23 19:42:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10086023/42002

[commit-bot: I haz the power, 2012-04-23 20:03:53]

Try job failure for 10086023-42002 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-23 20:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10086023/39004

[commit-bot: I haz the power, 2012-04-23 21:06:19]

Try job failure for 10086023-39004 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-23 21:21:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10086023/52001

[commit-bot: I haz the power, 2012-04-23 22:28:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10086023/61002

[commit-bot: I haz the power, 2012-04-23 22:44:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10086023/59003

[commit-bot: I haz the power, 2012-04-24 00:22:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10086023/54003

[commit-bot: I haz the power, 2012-04-24 02:26:18]

Change committed as 133607