gfx::Image: Made Image objects shallow-copy their representations.

ui/gfx/image/image.cc

Index: ui/gfx/image/image.cc
diff --git a/ui/gfx/image/image.cc b/ui/gfx/image/image.cc
index aa3f2b33e6d32e14de1435b0e3cc622a3d16e3e4..a93c4369c32a9611c91eadc524dcc28d794b97fb 100644
--- a/ui/gfx/image/image.cc
+++ b/ui/gfx/image/image.cc
@@ -181,6 +181,8 @@ class ImageRep {
   // Deletes the associated pixels of an ImageRep.
   virtual ~ImageRep() {}
 
+  virtual std::unique_ptr<ImageRep> Clone() = 0;
+
   // Cast helpers ("fake RTTI").
   ImageRepPNG* AsImageRepPNG() {
     CHECK_EQ(type_, Image::kImageRepPNG);
@@ -224,6 +226,16 @@ class ImageRepPNG : public ImageRep {
         image_png_reps_(image_png_reps) {
   }
 
+  ImageRepPNG(const ImageRepPNG& other)
+      : ImageRep(Image::kImageRepPNG),
+        image_png_reps_(other.image_png_reps_),
+        size_cache_(other.size_cache_ ? new gfx::Size(*other.size_cache_)
+                                      : nullptr) {}
+
+  std::unique_ptr<ImageRep> Clone() override {
+    return base::WrapUnique(new ImageRepPNG(*this));
+  }
+
   ~ImageRepPNG() override {}
 
   int Width() const override { return Size().width(); }
@@ -253,8 +265,6 @@ class ImageRepPNG : public ImageRep {
 
   // Cached to avoid having to parse the raw data multiple times.
   mutable std::unique_ptr<gfx::Size> size_cache_;
-
-  DISALLOW_COPY_AND_ASSIGN(ImageRepPNG);
 };
 
 class ImageRepSkia : public ImageRep {
@@ -265,7 +275,18 @@ class ImageRepSkia : public ImageRep {
         image_(image) {
   }
 
-  ~ImageRepSkia() override {}
+  ImageRepSkia(const ImageRepSkia& other)
+      : ImageRep(Image::kImageRepSkia), image_(new ImageSkia(*other.image_)) {}
+
+  std::unique_ptr<ImageRep> Clone() override {
+    return base::WrapUnique(new ImageRepSkia(*this));
+  }
+
+  ~ImageRepSkia() override {
+    // TODO(mgiuca): Obviously bad, but we just leak the pointer because there
+    // is a lot of code depending on this outliving the Image.
+    image_.release();
+  }
 
   int Width() const override { return image_->width(); }
 
@@ -273,12 +294,16 @@ class ImageRepSkia : public ImageRep {
 
   gfx::Size Size() const override { return image_->size(); }
 
+  // So... this no longer returns a stable pointer because the Image owns the
+  // ImageSkia and if the local image is destroyed, so is the ImageSkia. Major
+  // problem with this approach: any call to ToImageSkia will now require the
+  // immediate Image object to outlive the ImageSkia*. See
+  // ThemeService::GetImageSkiaNamed, which very deeply relies on the ImageSkia*
+  // outliving the immediate Image object there.
   ImageSkia* image() { return image_.get(); }
 
  private:
   std::unique_ptr<ImageSkia> image_;
-
-  DISALLOW_COPY_AND_ASSIGN(ImageRepSkia);
 };
 
 #if defined(OS_IOS)
@@ -290,6 +315,13 @@ class ImageRepCocoaTouch : public ImageRep {
     CHECK(image);
   }
 
+  // TODO(mgiuca): |image_| double-free!!! DO NOT SUBMIT.
+  ImageRepCocoaTouch(const ImageRepCocoaTouch& other) = default;
+
+  std::unique_ptr<ImageRep> Clone() override {
+    return base::WrapUnique(new ImageRepCocoaTouch(*this));
+  }
+
   ~ImageRepCocoaTouch() override {
     base::mac::NSObjectRelease(image_);
     image_ = nil;
@@ -305,8 +337,6 @@ class ImageRepCocoaTouch : public ImageRep {
 
  private:
   UIImage* image_;
-
-  DISALLOW_COPY_AND_ASSIGN(ImageRepCocoaTouch);
 };
 #elif defined(OS_MACOSX)
 class ImageRepCocoa : public ImageRep {
@@ -317,6 +347,13 @@ class ImageRepCocoa : public ImageRep {
     CHECK(image);
   }
 
+  // TODO(mgiuca): |image_| double-free!!! DO NOT SUBMIT.
+  ImageRepCocoa(const ImageRepCocoa& other) = default;
+
+  std::unique_ptr<ImageRep> Clone() override {
+    return base::WrapUnique(new ImageRepCocoa(*this));
+  }
+
   ~ImageRepCocoa() override {
     base::mac::NSObjectRelease(image_);
     image_ = nil;
@@ -332,15 +369,13 @@ class ImageRepCocoa : public ImageRep {
 
  private:
   NSImage* image_;
-
-  DISALLOW_COPY_AND_ASSIGN(ImageRepCocoa);
 };
 #endif  // defined(OS_MACOSX)
 
 // The Storage class acts similarly to the pixels in a SkBitmap: the Image
 // class holds a refptr instance of Storage, which in turn holds all the
 // ImageReps. This way, the Image can be cheaply copied.
-class ImageStorage : public base::RefCounted<ImageStorage> {
+class ImageStorage {
  public:
   ImageStorage(Image::RepresentationType default_type)
       : default_representation_type_(default_type)
@@ -352,6 +387,20 @@ class ImageStorage : public base::RefCounted<ImageStorage> {
   {
   }
 
+  ImageStorage(const ImageStorage& other)
+      : default_representation_type_(other.default_representation_type_)
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+        ,
+        default_representation_color_space_(
+            other.default_representation_color_space_)
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+  {
+    for (const auto& item : other.representations_)
+      representations_.insert(std::make_pair(item.first, item.second->Clone()));
+  }
+
+  ~ImageStorage() {}
+
   Image::RepresentationType default_representation_type() {
     return default_representation_type_;
   }
@@ -369,8 +418,6 @@ class ImageStorage : public base::RefCounted<ImageStorage> {
  private:
   friend class base::RefCounted<ImageStorage>;
 
-  ~ImageStorage() {}
-
   // The type of image that was passed to the constructor. This key will always
   // exist in the |representations_| map.
   Image::RepresentationType default_representation_type_;
@@ -386,8 +433,6 @@ class ImageStorage : public base::RefCounted<ImageStorage> {
   // All the representations of an Image. Size will always be at least one, with
   // more for any converted representations.
   Image::RepresentationMap representations_;
-
-  DISALLOW_COPY_AND_ASSIGN(ImageStorage);
 };
 
 }  // namespace internal
@@ -407,13 +452,13 @@ Image::Image(const std::vector<ImagePNGRep>& image_reps) {
   if (filtered.empty())
     return;
 
-  storage_ = new internal::ImageStorage(Image::kImageRepPNG);
+  storage_.reset(new internal::ImageStorage(Image::kImageRepPNG));
   AddRepresentation(base::WrapUnique(new internal::ImageRepPNG(filtered)));
 }
 
 Image::Image(const ImageSkia& image) {
   if (!image.isNull()) {
-    storage_ = new internal::ImageStorage(Image::kImageRepSkia);
+    storage_.reset(new internal::ImageStorage(Image::kImageRepSkia));
     AddRepresentation(
         base::WrapUnique(new internal::ImageRepSkia(new ImageSkia(image))));
   }
@@ -429,17 +474,19 @@ Image::Image(UIImage* image)
 #elif defined(OS_MACOSX)
 Image::Image(NSImage* image) {
   if (image) {
-    storage_ = new internal::ImageStorage(Image::kImageRepCocoa);
+    storage_.reset(new internal::ImageStorage(Image::kImageRepCocoa));
     AddRepresentation(base::WrapUnique(new internal::ImageRepCocoa(image)));
   }
 }
 #endif
 
-Image::Image(const Image& other) : storage_(other.storage_) {
-}
+Image::Image(const Image& other)
+    : storage_(other.storage_ ? new internal::ImageStorage(*other.storage_)
+                              : nullptr) {}
 
 Image& Image::operator=(const Image& other) {
-  storage_ = other.storage_;
+  storage_.reset(other.storage_ ? new internal::ImageStorage(*other.storage_)
+                                : nullptr);
   return *this;
 }