Improve handling of Skia images by gfx::Image

Removed unnecessary indirection and allocation via std::unique_ptr for
storage of value types in cached image data (both gfx::Size and
gfx::ImageSkia are value types).

Added unit tests checking assumptions and ensuring that we will not
break said assumptions about how image data is cached/image data
duplication is prevented in the future.

Change-Id: I59a70aa1ccb2e00fbc1a69ad92e48374643b6d20
Reviewed-on: https://chromium-review.googlesource.com/c/1471514Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632315}

ui/gfx/image/image.cc

@@ -109,10 +109,8 @@ class ImageRepPNG : public ImageRep {
   ImageRepPNG() : ImageRep(Image::kImageRepPNG) {
   }
 
-  ImageRepPNG(const std::vector<ImagePNGRep>& image_png_reps)
-      : ImageRep(Image::kImageRepPNG),
-        image_png_reps_(image_png_reps) {
-  }
+  explicit ImageRepPNG(const std::vector<ImagePNGRep>& image_png_reps)
+      : ImageRep(Image::kImageRepPNG), image_png_reps_(image_png_reps) {}
 
   ~ImageRepPNG() override {}
 
@@ -125,11 +123,11 @@ class ImageRepPNG : public ImageRep {
     if (!size_cache_) {
       for (auto it = image_reps().begin(); it != image_reps().end(); ++it) {
         if (it->scale == 1.0f) {
-          size_cache_.reset(new gfx::Size(it->Size()));
+          size_cache_ = it->Size();
           return *size_cache_;
         }
       }
-      size_cache_.reset(new gfx::Size);
+      size_cache_ = gfx::Size();
     }
 
     return *size_cache_;
@@ -141,7 +139,7 @@ class ImageRepPNG : public ImageRep {
   std::vector<ImagePNGRep> image_png_reps_;
 
   // Cached to avoid having to parse the raw data multiple times.
-  mutable std::unique_ptr<gfx::Size> size_cache_;
+  mutable base::Optional<gfx::Size> size_cache_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageRepPNG);
 };
@@ -150,23 +148,21 @@ class ImageRepSkia : public ImageRep {
  public:
   // Takes ownership of |image|.
   explicit ImageRepSkia(ImageSkia* image)
-      : ImageRep(Image::kImageRepSkia),
-        image_(image) {
-  }
+      : ImageRep(Image::kImageRepSkia), image_(*image) {}
 
   ~ImageRepSkia() override {}
 
-  int Width() const override { return image_->width(); }
+  int Width() const override { return image_.width(); }
 
-  int Height() const override { return image_->height(); }
+  int Height() const override { return image_.height(); }
 
-  gfx::Size Size() const override { return image_->size(); }
+  gfx::Size Size() const override { return image_.size(); }
 
-  const ImageSkia* image() const { return image_.get(); }
-  ImageSkia* image() { return image_.get(); }
+  const ImageSkia* image() const { return &image_; }
+  ImageSkia* image() { return &image_; }
 
  private:
-  std::unique_ptr<ImageSkia> image_;
+  ImageSkia image_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageRepSkia);
 };
@@ -238,7 +234,7 @@ class ImageRepCocoa : public ImageRep {
 // themselves not threadsafe.
 class ImageStorage : public base::RefCounted<ImageStorage> {
  public:
-  ImageStorage(Image::RepresentationType default_type)
+  explicit ImageStorage(Image::RepresentationType default_type)
       : default_representation_type_(default_type)
 #if defined(OS_MACOSX) && !defined(OS_IOS)
         ,

ui/gfx/image/image_unittest.cc

@@ -4,6 +4,8 @@
 
 #include <stddef.h>
 
+#include <utility>
+
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkCanvas.h"
@@ -575,6 +577,72 @@ TEST_F(ImageTest, MoveAssign) {
   EXPECT_EQ(1U, image2.RepresentationCount());
 }
 
+TEST_F(ImageTest, Copy_PreservesRepresentation) {
+  const gfx::Size kSize1x(25, 25);
+  const gfx::Size kSize2x(50, 50);
+  std::vector<gfx::ImagePNGRep> image_png_reps;
+  image_png_reps.push_back(
+      gfx::ImagePNGRep(gt::CreatePNGBytes(kSize1x.width()), 1.0f));
+  image_png_reps.push_back(
+      gfx::ImagePNGRep(gt::CreatePNGBytes(kSize2x.width()), 2.0f));
+  gfx::Image image(image_png_reps);
+
+  gfx::ImageSkia image_skia = image.AsImageSkia();
+  EXPECT_EQ(kSize1x, image_skia.size());
+  SkISize size = image_skia.GetRepresentation(2.0f).GetBitmap().dimensions();
+  EXPECT_EQ(kSize2x, gfx::Size(size.fWidth, size.fHeight));
+
+  gfx::Image image2(image);
+  gfx::ImageSkia image_skia2 = image2.AsImageSkia();
+  EXPECT_EQ(kSize1x, image_skia2.size());
+  size = image_skia2.GetRepresentation(2.0f).GetBitmap().dimensions();
+  EXPECT_EQ(kSize2x, gfx::Size(size.fWidth, size.fHeight));
+
+  EXPECT_TRUE(image_skia.BackedBySameObjectAs(image_skia2));
+}
+
+TEST_F(ImageTest, Copy_PreventsDuplication) {
+  const gfx::Size kSize1x(25, 25);
+  const gfx::Size kSize2x(50, 50);
+  std::vector<gfx::ImagePNGRep> image_png_reps;
+  image_png_reps.push_back(
+      gfx::ImagePNGRep(gt::CreatePNGBytes(kSize1x.width()), 1.0f));
+  image_png_reps.push_back(
+      gfx::ImagePNGRep(gt::CreatePNGBytes(kSize2x.width()), 2.0f));
+  gfx::Image image(image_png_reps);
+
+  gfx::Image image2(image);
+
+  gfx::ImageSkia image_skia = image.AsImageSkia();
+  EXPECT_EQ(kSize1x, image_skia.size());
+  SkISize size = image_skia.GetRepresentation(2.0f).GetBitmap().dimensions();
+  EXPECT_EQ(kSize2x, gfx::Size(size.fWidth, size.fHeight));
+
+  gfx::ImageSkia image_skia2 = image2.AsImageSkia();
+  EXPECT_EQ(kSize1x, image_skia2.size());
+  size = image_skia2.GetRepresentation(2.0f).GetBitmap().dimensions();
+  EXPECT_EQ(kSize2x, gfx::Size(size.fWidth, size.fHeight));
+
+  EXPECT_TRUE(image_skia.BackedBySameObjectAs(image_skia2));
+}
+
+TEST_F(ImageTest, Copy_PreservesBackingStore) {
+  const gfx::Size kSize1x(25, 25);
+
+  gfx::Image image(gt::CreateImageSkia(kSize1x.width(), kSize1x.height()));
+  gfx::Image image2 = gfx::Image::CreateFrom1xBitmap(image.AsBitmap());
+
+  gfx::ImageSkia image_skia = image.AsImageSkia();
+  gfx::ImageSkia image_skia2 = image2.AsImageSkia();
+
+  // Because we haven't copied the image representation (scale info, etc.) the
+  // new Skia image isn't backed by the same object, but it should still contain
+  // the same bitmap data.
+  EXPECT_FALSE(image_skia2.BackedBySameObjectAs(image_skia));
+  EXPECT_EQ(image_skia.bitmap()->getPixels(),
+            image_skia2.bitmap()->getPixels());
+}
+
 TEST_F(ImageTest, MultiResolutionImageSkia) {
   const int kWidth1x = 10;
   const int kHeight1x = 12;