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

ui/gfx/image/image_skia_unittest.cc

 // 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.
 
 #include "ui/gfx/image/image_skia.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/layout.h"
 #include "ui/gfx/image/image_skia_rep.h"
@@ skipping 155 matching lines @@
 
 #endif  // OS_MACOSX
 
 TEST(ImageSkiaTest, GetBitmap) {
   ImageSkia image_skia(new DynamicSource(Size(100, 200)), Size(100, 200));
   const SkBitmap* bitmap = image_skia.bitmap();
   EXPECT_NE(static_cast<SkBitmap*>(NULL), bitmap);
   EXPECT_FALSE(bitmap->isNull());
 }
 
+TEST(ImageSkiaTest, GetBitmapFromEmpty) {
+  // Create an image with 1 representation, remove it, and make sure that
+  // doesn't crash a later bitmap retrieval. We retrieve from a copy, since
+  // RemoveRepresentation could null out the ImageSkia it's called on, but not
+  // the copy.
+  ImageSkia empty_image(ImageSkiaRep(Size(100, 200), ui::SCALE_FACTOR_100P));
+  ImageSkia empty_image_copy(empty_image);

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

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]

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]

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".

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

Done, thanks.

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

+  empty_image.RemoveRepresentation(ui::SCALE_FACTOR_100P);
+
+  const SkBitmap* bitmap = empty_image_copy.bitmap();
+  ASSERT_NE(static_cast<SkBitmap*>(NULL), bitmap);
+  EXPECT_TRUE(bitmap->isNull());
+  EXPECT_TRUE(bitmap->empty());
+}
+
+TEST(ImageSkiaTest, OperatorBitmapFromSource) {
+  ImageSkia image_skia(new DynamicSource(Size(100, 200)), Size(100, 200));
+  // ImageSkia should use the source to create the bitmap.
+  const SkBitmap& bitmap = image_skia;
+  ASSERT_NE(static_cast<SkBitmap*>(NULL), &bitmap);
+  EXPECT_FALSE(bitmap.isNull());
+}
+
 }  // namespace gfx