Avoid crashing when an ImageSkia has no representations and is converted to SkBitmap.

Avoid crashing when an ImageSkia has no representations and is converted to SkBitmap.


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

[Robert Sesek, 2012-08-14 14:42:05]

Is there a BUG= for this?

[pkotwicz, 2012-08-14 14:44:24]

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
File ui/gfx/image/image_skia.h (right):

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
ui/gfx/image/image_skia.h:116: const SkBitmap* bitmap() const { return &operator
SkBitmap&(); }
Can you actually make the operator use ImageSkia::bitmap().
We want to get rid of the operator pretty fast, but bitmap() will stay.

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
File ui/gfx/image/image_skia_unittest.cc (right):

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
ui/gfx/image/image_skia_unittest.cc:182: ImageSkia
empty_image_copy(empty_image);
I don't think it should make a difference as to whether you do the tests on
|empty_image| or |empty_image_copy|.
ImageSkias has refcounted ImageSkiaStorage so the data of both should be
identical

[Jeffrey Yasskin, 2012-08-14 17:08:28]

I didn't file a bug for this. Do I need to?

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
File ui/gfx/image/image_skia.h (right):

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
ui/gfx/image/image_skia.h:116: const SkBitmap* bitmap() const { return &operator
SkBitmap&(); }
On 2012/08/14 14:44:24, pkotwicz wrote:
> Can you actually make the operator use ImageSkia::bitmap().
> We want to get rid of the operator pretty fast, but bitmap() will stay.

This is the direction that makes const-ness work. When you remove the operator,
it should be straightforward to rename the implementation back.

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
File ui/gfx/image/image_skia_unittest.cc (right):

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
ui/gfx/image/image_skia_unittest.cc:182: ImageSkia
empty_image_copy(empty_image);
On 2012/08/14 14:44:24, pkotwicz wrote:
> I don't think it should make a difference as to whether you do the tests on
> |empty_image| or |empty_image_copy|.
> ImageSkias has refcounted ImageSkiaStorage so the data of both should be
> identical

Imagine that RemoveRepresentation checked if it had removed the last image, and
if so, NULLed out its pointer to the ImageSkiaStorage. Then the isNull check in
empty_image.bitmap() would return true, causing the original implementation to
return an empty bitmap. However, empty_image_copy would still have a non-NULL
pointer to the now-empty ImageSkiaStorage, and the isNull check inside
empty_image_copy.bitmap() would still return false.

Is there some better wording in the comment I wrote 2 lines up that would make
this clearer?

[pkotwicz, 2012-08-14 17:41:15]

http://codereview.chromium.org/10828296/diff/2001/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10828296/diff/2001/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:182: ImageSkia
empty_image_copy(empty_image);
I follow your reasoning. My personal opinion is that
ImageSkia::RemoveRepresentation() should make ImageSkia::isNull()==true for all
ImageSkia pointing to the ImageSkiaStorage whose last element was removed. This
would require more work than what you are suggesting.
However, I can see why this test is useful as is. Maybe make the comment "Check
that ImageSkia::bitmap() returns a valid SkBitmap pointer for the image and all
its copies".

[pkotwicz, 2012-08-14 17:43:04]

LGTM with NITS

[Jeffrey Yasskin, 2012-08-14 19:14:15]

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
File ui/gfx/image/image_skia_unittest.cc (right):

https://chromiumcodereview.appspot.com/10828296/diff/2001/ui/gfx/image/image_...
ui/gfx/image/image_skia_unittest.cc:182: ImageSkia
empty_image_copy(empty_image);
On 2012/08/14 17:41:15, pkotwicz wrote:
> I follow your reasoning. My personal opinion is that
> ImageSkia::RemoveRepresentation() should make ImageSkia::isNull()==true for
all
> ImageSkia pointing to the ImageSkiaStorage whose last element was removed.
This
> would require more work than what you are suggesting.
> However, I can see why this test is useful as is. Maybe make the comment
"Check
> that ImageSkia::bitmap() returns a valid SkBitmap pointer for the image and
all
> its copies".

Done, thanks.

I think I still need rsesek to LGTM to satisfy the owners files.

[Robert Sesek, 2012-08-14 19:16:37]

LGTM

[commit-bot: I haz the power, 2012-08-14 19:20:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10828296/6002

[commit-bot: I haz the power, 2012-08-14 21:37:24]

Change committed as 151570