Makes ImageSkia more like SkBitmap

This patch makes ImageSkia more like SkBitmap. The goal is to make swapping from SkBitmap to ImageSkia easier.

Notable changes:
- ImageSkia can be cheaply copied
- Added extractSubset, will remove after SkBitmaps have been converted to ImageSkia
- Modified API to look more like SkBitmap

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

[pkotwicz, 2012-04-27 02:23:54]

This makes gfx::ImageSkia look more like SkBitmap and will make changing the API
to use ImageSkia instead of SkBitmap easier. (Yes, we will want to change isNull
to IsNull etc once the conversion from SkBitmap to gfx::ImageSkia is complete).

gfx::ImageSkia converts to and from SkBitmap.

[pkotwicz, 2012-04-27 02:24:37]

Also moves the paint functions out of gfx::ImageSkia as to make conversion
easier.

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

I didn't make it very far. Before I continue with this could you outline how you
see all the bits fitting together? Specifically what are we going to get the
ResourceBundle? How will a change in scale be handled?

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:35: ImageSkia(const SkBitmap& bitmap);
explicit

[pkotwicz, 2012-04-27 16:14:32]

This is how I suggest we will handle change in DPI
1) Add a method OnMonitorScaleChanged to aura::MonitorObserver
2) Probably need to make aura::MonitorObserver cross platform (move it out of
the aura package)
3) Make resource bundle inherit from MonitorObserver
4) On OnMonitorScaleFactorChanged, resource bundle will add the appropriate data
packs for the new scale factor
5) ResourceBundle::AddDataPack will be modified so that it iterates over images_
(currently loaded images)
6) new method in gfx::ImageSkia::AddBitmap. AddBitmap will add a bitmap to the
vector of bitmaps contained by gfx::ImageSkia

For the one off cases (cases where we have mutated the bitmap), I expect
views::View to be a MonitorObserver.
Add method views::View::OnMonitorScaleFactorChanged for the view to do the right
thing for all of its exceptions

[pkotwicz, 2012-04-27 16:14:40]

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:35: ImageSkia(const SkBitmap& bitmap);
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 17:57:01]

Instead of this how about this. Add a resource id to ImageSkia. It's
id of the resource, or -1 if not from a resource. When ImageSkia goes
to draw it can detect a scale change and refresh the image. This way
ImageSkia doesn't need to know about multiple images.

  -Scott

On Fri, Apr 27, 2012 at 9:14 AM,  <pkotwicz@chromium.org> wrote:
> This is how I suggest we will handle change in DPI
> 1) Add a method OnMonitorScaleChanged to aura::MonitorObserver
> 2) Probably need to make aura::MonitorObserver cross platform (move it out
> of
> the aura package)
> 3) Make resource bundle inherit from MonitorObserver
> 4) On OnMonitorScaleFactorChanged, resource bundle will add the appropriate
> data
> packs for the new scale factor
> 5) ResourceBundle::AddDataPack will be modified so that it iterates over
> images_
> (currently loaded images)
> 6) new method in gfx::ImageSkia::AddBitmap. AddBitmap will add a bitmap to
> the
> vector of bitmaps contained by gfx::ImageSkia
>
> For the one off cases (cases where we have mutated the bitmap), I expect
> views::View to be a MonitorObserver.
> Add method views::View::OnMonitorScaleFactorChanged for the view to do the
> right
> thing for all of its exceptions
>
> http://codereview.chromium.org/10245003/

[pkotwicz, 2012-04-27 20:19:12]

To paraphrase, ImageSkia will inherit from MonitorObserver. Upon detecting a
scale change, ImageSkia will query the ResourceBundle / ThemeProvider for a
resource at the new resolution.
Is this correct?

[pkotwicz, 2012-04-27 20:23:24]

The reason that I had ImageSkia know about multiple images is that NSImage on
the Mac does. From talking to the Skia team, they want to design an object
native to skia which contains scale factor. They didn't have a preference as to
whether ImageSkia contained one image or multiple images though.

[sky, 2012-04-27 20:38:21]

Close. I don't want ImageSkia to be a MonitorObserver, that's just
tooooooo many MonitorObservers. What I'm saying is that when ImageSkia
is asked to paint is can notice the scale change (because it caches
the scale it fetched the image at and knows the current scale from the
canvas).

  -Scott

On Fri, Apr 27, 2012 at 1:19 PM,  <pkotwicz@chromium.org> wrote:
> To paraphrase, ImageSkia will inherit from MonitorObserver. Upon detecting a
> scale change, ImageSkia will query the ResourceBundle / ThemeProvider for a
> resource at the new resolution.
> Is this correct?
>
> http://codereview.chromium.org/10245003/

[sky, 2012-04-27 20:39:12]

I get why multiple images is nice, but I don't think we need it and
it'll unnecessarily complicate things and add more memory. Our common
case is an ImageSkia is only ever used at one scale.

  -Scott

On Fri, Apr 27, 2012 at 1:23 PM,  <pkotwicz@chromium.org> wrote:
> The reason that I had ImageSkia know about multiple images is that NSImage
> on
> the Mac does. From talking to the Skia team, they want to design an object
> native to skia which contains scale factor. They didn't have a preference as
> to
> whether ImageSkia contained one image or multiple images though.
>
> http://codereview.chromium.org/10245003/

[pkotwicz, 2012-04-27 21:39:57]

I think we will still need a structure which contains multiple images.
There was one inside of gfx::Image and it should probably remain.
Otherwise we will get into issues when running in multi monitor mode. Eg one
monitor in  high res and another monitor in low res.
I can see how it would be nice to replace SkBitmap with something which contains
a single image.
I still need to think as to how the object will be created

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/canvas.cc#newcode269
ui/gfx/canvas.cc:269: if (image.ShouldBuildMipMap())
Can we hide this in ImageSkia? By that I mean could we make asking for an image
build the mip-map for you?

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:43: std::vector<const SkBitmap*> bitmaps_;
Why does this need to be pointers? It would be a lot safer to make it by value.

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:44: gfx::Size size_;
Document what this is.

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:45: bool build_mip_map_;
Shouldn't the mip-map be per-image?

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:131: if (bitmaps[i]->isNull())
Is there a reason to allow null/empty images into the storage?

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:28: class UI_EXPORT ImageSkia {
Document this supports copy semantics and why.

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:35: ImageSkia(const SkBitmap& bitmap);
On 2012/04/27 16:14:41, pkotwicz wrote:
> 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.

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:43: explicit ImageSkia(const SkBitmap* bitmap);
Do we really need this to take ownership? Can't it copy instead?

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:63: operator SkBitmap() const;
Why do you want this?

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:76: bool isNull() const { return storage_ == NULL; }
Do we really need to distinguish between empty() and isNull?

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:85: bool extractSubset(ImageSkia* dst, SkIRect&
subset) const;
ExtractSubset, Make this take a SkIRect*

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

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:76: bool isNull() const { return storage_ == NULL; }
nit: also either is_null() or IsNull(), but not isNull().

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

Changes kind of as requested.

A couple differences:
I would like to be able to introduce gfx::ImageSkia incrementally. I expect it
being difficult to do so but am very open to suggestions.
In writing the current API for gfx::ImageSkia I have written it in such a way
that it will make convrsion of SkBitmap easier. There are a number of functions
which I hope can disappear quickly. (eg the implicit conversion from
gfx::ImageSkia to SkBitmap and vice versa, having both isNull & empty etc, the
fact that isNull is a violation of the chromium style guide)

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:43: std::vector<const SkBitmap*> bitmaps_;
I think this needs to be pointers as long as we have SkBitmap*
Image::ToSkBitmap()

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:131: if (bitmaps[i]->isNull())
Changed semantics such that if bitmap->isNull(), then ImageSkia->isNull().
Unfortunately, we do want to allow gfx::ImageSkia to wrap a null SkBitmap. There
are a number of places where we use gfx::Image to wrap a bitmap which can be
null. The case I know about is in favicon_tab_helper.cc

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:28: class UI_EXPORT ImageSkia {
On 2012/04/27 23:33:58, sky wrote:
> Document this supports copy semantics and why.

Done.

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:35: ImageSkia(const SkBitmap& bitmap);
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?

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:43: explicit ImageSkia(const SkBitmap* bitmap);
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).

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:63: operator SkBitmap() const;
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.

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:85: bool extractSubset(ImageSkia* dst, SkIRect&
subset) const;
This is how the method is defined in the SkBitmap API

[tfarina, 2012-04-30 20:36:48]

Why is ImageSkia replacing SkBitmap? Can we have a bug filed for this with the
reason, and fill the BUG= line?

Thanks!

[sky, 2012-04-30 21:03:32]

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

http://codereview.chromium.org/10245003/diff/6001/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:43: std::vector<const SkBitmap*> bitmaps_;
On 2012/04/30 20:27:07, pkotwicz wrote:
> I think this needs to be pointers as long as we have SkBitmap*
> Image::ToSkBitmap()

Is the long term plan to get rid of ToSkBitmap? If not, can it return a value?

Either way, why does the need for ToSkbitmap returning a pointer imply these
have to be pointers? Couldn't places that need a pointer use & ?

[sky, 2012-04-30 21:07:24]

Also, I'm ok with transitional APIs, but they need to be clearly marked as such
so that reviewers (and anyone looking at the code) knows why it is the way it
is. Use something like:

// TODO(xxx): this is temporary until conversion is done.

[oshima, 2012-04-30 21:10:08]

I have a couple of question I want to ask offline. I'll ping you later.

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

http://codereview.chromium.org/10245003/diff/22002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:134: return isNull() ? 0 : storage_->size().width();
We don't want to exclude the possibility to have images with different size in
DIP and ImageSkia should return the DIP of the image for given density. 

If this is transient and to make transition easier, please add TODO.

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

http://codereview.chromium.org/10245003/diff/22002/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:43: // DIP width and height are set based on
|scale_factor|.
My understanding is that this creates ImageSkia with one bitmap that is desgined
for |scale_factor|. Am I wrong?

[pkotwicz, 2012-05-01 17:50:32]

Marked transitional APIs as such.
I would rather convert the vector of SkBitmap* to SkBitmap in a separate patch.

Did a couple other things that I needed to do eventually:
Deprecated ImageSkia constructor which takes in a vector of pointers and added
ImageSkia::AddBitmapForScale instead
Also deprecated Image constructor which takes in vector of bitmaps
Changed ImageSkia::GetBitmapForScale to return the scale of the returned bitmap
and to use out parameters.
Changed Canvas::GetBitmapToPaint to take in a user defined scale parameter in
addition to the canvas scale.

[sky, 2012-05-01 18:17:48]

On Tue, May 1, 2012 at 10:50 AM,  <pkotwicz@chromium.org> wrote:
> Marked transitional APIs as such.
> I would rather convert the vector of SkBitmap* to SkBitmap in a separate
> patch.

Is there a compelling reason for that? Memory management is simpler
and subsequently the code is easier to review if you use values rather
than pointers.

  -Scott

> Did a couple other things that I needed to do eventually:
> Deprecated ImageSkia constructor which takes in a vector of pointers and
> added
> ImageSkia::AddBitmapForScale instead
> Also deprecated Image constructor which takes in vector of bitmaps
> Changed ImageSkia::GetBitmapForScale to return the scale of the returned
> bitmap
> and to use out parameters.
> Changed Canvas::GetBitmapToPaint to take in a user defined scale parameter
> in
> addition to the canvas scale.
>
> http://codereview.chromium.org/10245003/

[pkotwicz, 2012-05-01 18:58:46]

I think I would have to deprecate ImageSkia::ImageSkia(SkBitmap*) in order to
convert the vector to use values instead of pointers.
The reason is that the constructor which passes in the pointer gives ownership
of that pointer to ImageSkia.
If the vector contains values, we are creating a second reference in caching the
value.
We can decrement the ref count by calling delete on the SkBitmap pointer passed
in.
If the callers are well behaved and do not use the pointer after calling
gfx::Image(SkBitmap*) (which is most likely the case) then there isn't a
problem.
I just do not want to assume that they are well behaved.

[sky, 2012-05-01 19:09:04]

On Tue, May 1, 2012 at 11:58 AM,  <pkotwicz@chromium.org> wrote:
> I think I would have to deprecate ImageSkia::ImageSkia(SkBitmap*) in order
> to
> convert the vector to use values instead of pointers.
> The reason is that the constructor which passes in the pointer gives
> ownership
> of that pointer to ImageSkia.
> If the vector contains values, we are creating a second reference in caching
> the
> value.
> We can decrement the ref count by calling delete on the SkBitmap pointer
> passed
> in.

I like this approach.

> If the callers are well behaved and do not use the pointer after calling
> gfx::Image(SkBitmap*) (which is most likely the case) then there isn't a
> problem.
> I just do not want to assume that they are well behaved.

If the callers aren't well behaved you're going to crash later rather
anyway, right? I'm also OK with a DeleteSoon.

  -Scott

[pkotwicz, 2012-05-04 19:44:05]

Changed vector to be of SkBitmap instead of SkBitmap*
Fixed up unittests

[sky, 2012-05-04 20:12:24]

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

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:33: void set_size(gfx::Size size) { size_ = size; }
const gfx::Size& (same for next line).

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:65: if (MessageLoop::current()) {
Document why you use DeleteSoon.

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:106: if (isNull())
Can you add some error checking here (just for debugging)?
How come it's safe to drop the DIP on the floor if not empty?

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:112: bool ImageSkia::GetBitmapForScale(float
x_scale_factor,
Can this return a const SkBitmap& ?

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

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:29: // ImageSkia supports copy semantics as legacy
from SkBitmap. Ideally, this
This comment isn't right, we want copy semantics.

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:34: // Creates null instance.
null instance -> instance with no images.

[pkotwicz, 2012-05-04 22:35:36]

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

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:106: if (isNull())
Right now we assume that bitmap sizes are relative to scale factor. Thus a
bitmap which is twice as wide as a bitmap of scale factor 1 has scale factor 2.
This will be wrong for scale factors other than two because of rounding errors.
(eg a 1x bitmap 11px wide will 15px wide in it's 1.4x version)
We need the device scale factor for the first bitmap because we don't know what
scale factor it is at. It is conceivable to have an object with only a bitmap at
scale factor 2.

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:112: bool ImageSkia::GetBitmapForScale(float
x_scale_factor,
I am not sure.
What would we return in the case where we have empty()? We could do the same
hack that I am doing for
ImageSkia::operator SkBitmap&() const and return a reference to a static.

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

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:29: // ImageSkia supports copy semantics as legacy
from SkBitmap. Ideally, this
Do you have a suggestion for a good comment? Copy semantics make it easier to
port from SkBitmap. It also hides memory management.

[sky, 2012-05-04 23:41:42]

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

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:106: if (isNull())
On 2012/05/04 22:35:36, pkotwicz wrote:
> Right now we assume that bitmap sizes are relative to scale factor. Thus a
> bitmap which is twice as wide as a bitmap of scale factor 1 has scale factor
2.
> This will be wrong for scale factors other than two because of rounding
errors.
> (eg a 1x bitmap 11px wide will 15px wide in it's 1.4x version)
> We need the device scale factor for the first bitmap because we don't know
what
> scale factor it is at. It is conceivable to have an object with only a bitmap
at
> scale factor 2.

Can you make what you just described a DCHECK? At a minimum the width/height
should be bigger, right?

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:112: bool ImageSkia::GetBitmapForScale(float
x_scale_factor,
On 2012/05/04 22:35:36, pkotwicz wrote:
> I am not sure.
> What would we return in the case where we have empty()? We could do the same
> hack that I am doing for
> ImageSkia::operator SkBitmap&() const and return a reference to a static.

Yes.

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

http://codereview.chromium.org/10245003/diff/30002/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:29: // ImageSkia supports copy semantics as legacy
from SkBitmap. Ideally, this
On 2012/05/04 22:35:36, pkotwicz wrote:
> Do you have a suggestion for a good comment? Copy semantics make it easier to
> port from SkBitmap. It also hides memory management.

I agree. My point is your comment makes it sounds like we're doing this for
legacy reasons, which isn't true. How about:

// ImageSkia is cheap to copy and intentionally supports copy semantics.

[Robert Sesek, 2012-05-05 02:08:34]

Thanks for doing this, btw!

http://codereview.chromium.org/10245003/diff/42003/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/10245003/diff/42003/skia/ext/skia_utils_mac.mm...
skia/ext/skia_utils_mac.mm:260: scoped_nsobject<NSBitmapImageRep> bitmap(
nit: over-indented

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

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image.cc#newc...
ui/gfx/image/image.cc:241: Image::Image(const SkBitmap& bitmap)
Is this ctor going away? If not, you could probably avoid an extra copy in this
case by having ImageRepSkia take a pointer instead of a reference.

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_mac.mm#...
ui/gfx/image/image_mac.mm:19: SkBitmap
bitmap(gfx::NSImageRepToSkBitmap(imageRep, [imageRep size],
Pull [imageRep size] into a local.

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_mac.mm#...
ui/gfx/image/image_mac.mm:41: [image setSize: NSMakeSize(image_skia->width(),
image_skia->height())];
nit: extra space after :

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

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:27: void add_bitmap(const SkBitmap& bitmap) {
naming: AddBitmap

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:70: // Hit in unittests.
It's generally not a good thing to have different implementation semantics from
production and tests. How much stuff breaks if you don't do this? Can't you just
add a MessageLoop member to the fixture?

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:97: std::vector<SkBitmap>& v =
const_cast<std::vector<SkBitmap>&>(bitmaps());
Why do you need to const_cast the vector? Can't you just const_cast the return
value?

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:184: DCHECK(scale_factor > 0.0f);
DCHECK_GT

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

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:26: // Functions which mutate the image should operate
on the SkBitmap returned
nit: s/which/that (restrictive clause)

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:36: // DIP width and height are set based on scale
factor of 1x.
Maybe want to comment what "DIP" means at the class-level.

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:91: bool isNull() const { return storage_ == NULL; }
Won't nit this since it's going away, but style is is_null() or IsNull()

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:101: bool extractSubset(ImageSkia* dst, SkIRect&
subset) const;
Same naming comment

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_unittes...
File ui/gfx/image/image_unittest.cc (right):

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_unittes...
ui/gfx/image/image_unittest.cc:261:
image_skia.AddBitmapForScale(*gt::CreateBitmap(width1x, height1x), 1.0f);
I think this will leak. CreateBitmap returns a pointer the caller owns.

[pkotwicz, 2012-05-08 22:10:14]

Changes as requested

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

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:27: void add_bitmap(const SkBitmap& bitmap) {
On 2012/05/05 02:08:34, rsesek wrote:
> naming: AddBitmap

Done.

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:70: // Hit in unittests.
Will keep this given this constructor is going away in very short notice

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:97: std::vector<SkBitmap>& v =
const_cast<std::vector<SkBitmap>&>(bitmaps());
On 2012/05/05 02:08:34, rsesek wrote:
> Why do you need to const_cast the vector? Can't you just const_cast the return
> value?

Done.

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:184: DCHECK(scale_factor > 0.0f);
On 2012/05/05 02:08:34, rsesek wrote:
> DCHECK_GT

Done.

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

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:26: // Functions which mutate the image should operate
on the SkBitmap returned
On 2012/05/05 02:08:34, rsesek wrote:
> nit: s/which/that (restrictive clause)

Done.

http://codereview.chromium.org/10245003/diff/42003/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:36: // DIP width and height are set based on scale
factor of 1x.
On 2012/05/05 02:08:34, rsesek wrote:
> Maybe want to comment what "DIP" means at the class-level.

Done.

[sky, 2012-05-08 22:38:07]

http://codereview.chromium.org/10245003/diff/30004/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10245003/diff/30004/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:318: const SkBitmap bitmap =
src_bitmaps[i];
const SkBitmap&

http://codereview.chromium.org/10245003/diff/30004/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10245003/diff/30004/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:251: // ImageSkia adds a reference to
bitmap.
Remove this comment as its a bit misleading.

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

http://codereview.chromium.org/10245003/diff/30004/ui/gfx/image/image.h#newco...
ui/gfx/image/image.h:65: // Creates a new image with the default representation.
The object will take
Remove second sentence since it isn't applicable anymore.

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

http://codereview.chromium.org/10245003/diff/30004/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:13: #include "ui/base/ui_export.h"
sort

http://codereview.chromium.org/10245003/diff/30004/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:104: const std::vector<SkBitmap>& bitmaps() const;
Can this return const std::vector<const SkBitmap>? Otherwise folks can mutate,
right?

[Robert Sesek, 2012-05-08 23:30:46]

http://codereview.chromium.org/10245003/diff/30004/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10245003/diff/30004/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:998: const gfx::ImageSkia*
image_to_tint = (it->second)->ToImageSkia();
nit: don't need () around it->second

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

http://codereview.chromium.org/10245003/diff/30004/ui/gfx/image/image.cc#newc...
ui/gfx/image/image.cc:34: ImageSkia NSImageToImageSkia(NSImage* image);
What if this conversion fails?

http://codereview.chromium.org/10245003/diff/30004/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

http://codereview.chromium.org/10245003/diff/30004/ui/gfx/image/image_mac.mm#...
ui/gfx/image/image_mac.mm:20: NSSize imageRepSize = [imageRep size];
naming: in C++ context, use image_rep_size

[pkotwicz, 2012-05-08 23:58:49]

Changes as requested

[sky, 2012-05-09 03:21:13]

LGTM

[Robert Sesek, 2012-05-09 15:34:12]

LGTM % one question

http://codereview.chromium.org/10245003/diff/30009/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (left):

http://codereview.chromium.org/10245003/diff/30009/ui/gfx/image/image.cc#oldc...
ui/gfx/image/image.cc:390:
CHECK(internal::NSImageToSkBitmaps(nsimage_rep->image(), &bitmaps));
Could removing this CHECK leave gfx::Image in an inconsistent state if the
conversion fails?

[commit-bot: I haz the power, 2012-05-09 16:51:39]

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

[commit-bot: I haz the power, 2012-05-09 16:51:56]

Can't apply patch for file ui/base/resource/resource_bundle.cc.
While running patch -p1 --forward --force;
patching file ui/base/resource/resource_bundle.cc
Hunk #1 FAILED at 26.
Hunk #2 FAILED at 241.
Hunk #3 FAILED at 262.
3 out of 3 hunks FAILED -- saving rejects to file
ui/base/resource/resource_bundle.cc.rej

[commit-bot: I haz the power, 2012-05-09 23:48:37]

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

[commit-bot: I haz the power, 2012-05-10 00:15:36]

Try job failure for 10245003-35009 (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-05-10 01:58:01]

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

[commit-bot: I haz the power, 2012-05-10 02:20:21]

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

[commit-bot: I haz the power, 2012-05-10 03:13:56]

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

[commit-bot: I haz the power, 2012-05-10 03:35:47]

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

[commit-bot: I haz the power, 2012-05-10 03:51:27]

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

[commit-bot: I haz the power, 2012-05-10 04:02:07]

Try job failure for 10245003-47010 (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-05-10 04:48:21]

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

[commit-bot: I haz the power, 2012-05-10 13:33:56]

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

[commit-bot: I haz the power, 2012-05-10 15:06:59]

Change committed as 136304

[reed1, 2012-05-11 13:57:09]

We are seeing some leaked bitmaps now

http://code.google.com/p/chromium/issues/detail?id=127717

Don't know what the cause it, but perhaps the reviews of this CL might have some
insight into possible areas to check...

[sky, 2012-05-11 16:31:41]

In the bug you mention this could be either a Skia roll or Peters
changes. Did you try reverting the two to narrow down which is the
culprit?

  -Scott

On Fri, May 11, 2012 at 6:57 AM,  <reed@google.com> wrote:
> We are seeing some leaked bitmaps now
>
> http://code.google.com/p/chromium/issues/detail?id=127717
>
> Don't know what the cause it, but perhaps the reviews of this CL might have
> some
> insight into possible areas to check...
>
> https://chromiumcodereview.appspot.com/10245003/

[commit-bot: I haz the power, 2012-05-16 19:42:33]

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

[commit-bot: I haz the power, 2012-05-16 19:44:12]

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

[commit-bot: I haz the power, 2012-05-16 21:30:18]

Change committed as 137520