gfx::Image, ImageFamily: Add and remove copying/moving methods.

Image:
- Adds move constructor/assignment. Not technically required as Image is
  cheaply copyable, but this allows more efficient moving without
  incrementing/decrementing the refcount. Also safer for multi-threaded
  usage.
- Removes SwapRepresentations. Was unused, and superseded by std::move.

ImageFamily:
- Removes copy constructor/assignment. ImageFamily can be fairly
  heavyweight, so it's best to move it rather than copying. Also it is
  heavily used across threads and accidental ImageFamily copying is
  responsible for data races (https://crbug.com/749342).
- Adds Clone method, an explicit copy operator for use if necessary.
- Adds move constructor/assignment. This should be used where possible.

BUG=600237,749342

Change-Id: I00482c8440d62edb8d9fb216c3de65bf4102d153
Reviewed-on: https://chromium-review.googlesource.com/588033Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490704}

chrome/browser/web_applications/web_app.cc

@@ -116,7 +116,12 @@ void OnImageLoaded(std::unique_ptr<web_app::ShortcutInfo> shortcut_info,
     image_skia.MakeThreadSafe();
     shortcut_info->favicon.Add(gfx::Image(image_skia));
   } else {
-    shortcut_info->favicon = image_family;
+    // TODO(mgiuca): Move |image_family| instead of copying it. This copy
+    // results in a refcount of 2 for the duration of the callback, leading to a
+    // data race decrementing the refcount. This requires a change to the
+    // ImageLoaderImageFamilyCallback interface (to pass by value).
+    // https://crbug.com/749342.
+    shortcut_info->favicon = image_family.Clone();
   }
 
   callback.Run(std::move(shortcut_info));

extensions/browser/image_loader_unittest.cc

@@ -51,7 +51,7 @@ class ImageLoaderTest : public ExtensionsTest {
     image_loaded_count_++;
     if (quit_in_image_loaded_)
       base::RunLoop::QuitCurrentWhenIdleDeprecated();
-    image_family_ = image_family;
+    image_family_ = image_family.Clone();
   }
 
   void WaitForImageLoad() {

ui/gfx/image/image.cc

@@ -521,16 +521,15 @@ Image::Image(NSImage* image) {
 }
 #endif
 
-Image::Image(const Image& other) : storage_(other.storage_) {
-}
+Image::Image(const Image& other) = default;
 
-Image& Image::operator=(const Image& other) {
-  storage_ = other.storage_;
-  return *this;
-}
+Image::Image(Image&& other) = default;
 
-Image::~Image() {
-}
+Image& Image::operator=(const Image& other) = default;
+
+Image& Image::operator=(Image&& other) = default;
+
+Image::~Image() {}
 
 // static
 Image Image::CreateFrom1xBitmap(const SkBitmap& bitmap) {
@@ -803,10 +802,6 @@ gfx::Size Image::Size() const {
   return GetRepresentation(DefaultRepresentationType(), true)->Size();
 }
 
-void Image::SwapRepresentations(Image* other) {
-  storage_.swap(other->storage_);
-}
-
 #if defined(OS_MACOSX)  && !defined(OS_IOS)
 void Image::SetSourceColorSpace(CGColorSpaceRef color_space) {
   if (storage())

ui/gfx/image/image.h

@@ -84,9 +84,17 @@ class GFX_EXPORT Image {
   // Initializes a new Image by AddRef()ing |other|'s internal storage.
   Image(const Image& other);
 
+  // Moves a reference from |other| to the new image without changing the
+  // reference count.
+  Image(Image&& other);
+
   // Copies a reference to |other|'s storage.
   Image& operator=(const Image& other);
 
+  // Moves a reference from |other|'s storage without changing the reference
+  // count.
+  Image& operator=(Image&& other);
+
   // Deletes the image and, if the only owner of the storage, all of its cached
   // representations.
   ~Image();
@@ -166,9 +174,6 @@ class GFX_EXPORT Image {
   int Height() const;
   gfx::Size Size() const;
 
-  // Swaps this image's internal representations with |other|.
-  void SwapRepresentations(gfx::Image* other);
-
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   // Set the default representation's color space. This is used for converting
   // to NSImage. This is used to compensate for PNGCodec not writing or reading

ui/gfx/image/image_family.cc

@@ -25,8 +25,17 @@ ImageFamily::const_iterator::const_iterator(
 ImageFamily::const_iterator::~const_iterator() {}
 
 ImageFamily::ImageFamily() {}
+ImageFamily::ImageFamily(ImageFamily&& other) = default;
 ImageFamily::~ImageFamily() {}
 
+ImageFamily& ImageFamily::operator=(ImageFamily&& other) = default;
+
+ImageFamily ImageFamily::Clone() const {
+  ImageFamily clone;
+  clone.map_ = map_;
+  return clone;
+}
+
 void ImageFamily::Add(const gfx::Image& image) {
   gfx::Size size = image.Size();
   if (size.IsEmpty()) {

ui/gfx/image/image_family.h

@@ -98,8 +98,11 @@ class GFX_EXPORT ImageFamily {
   };
 
   ImageFamily();
+  ImageFamily(ImageFamily&& other);
   ~ImageFamily();
 
+  ImageFamily& operator=(ImageFamily&& other);
+
   // Gets an iterator to the first image.
   const_iterator begin() const { return const_iterator(map_.begin()); }
   // Gets an iterator to one after the last image.
@@ -111,6 +114,10 @@ class GFX_EXPORT ImageFamily {
   // Removes all images from the family.
   void clear() { return map_.clear(); }
 
+  // Creates a shallow copy of the family. The Images inside share their backing
+  // store with the original Images.
+  ImageFamily Clone() const;
+
   // Adds an image to the family. If another image is already present at the
   // same size, it will be overwritten.
   void Add(const gfx::Image& image);
@@ -165,6 +172,12 @@ class GFX_EXPORT ImageFamily {
 
   // Map from (aspect ratio, width) to image.
   std::map<MapKey, gfx::Image> map_;
+
+  // Even though the Images in the family are copyable (reference-counted), the
+  // family itself should not be implicitly copied, as it would result in a
+  // shallow clone of the entire map and updates to many reference counts.
+  // ImageFamily can be explicitly Clone()d, but std::move is preferred.
+  DISALLOW_COPY_AND_ASSIGN(ImageFamily);
 };
 
 }  // namespace gfx

ui/gfx/image/image_family_unittest.cc

@@ -69,6 +69,26 @@ TEST_F(ImageFamilyTest, Clear) {
   EXPECT_TRUE(image_family_.empty());
 }
 
+TEST_F(ImageFamilyTest, MoveConstructor) {
+  gfx::ImageFamily family(std::move(image_family_));
+  EXPECT_TRUE(image_family_.empty());
+  EXPECT_FALSE(family.empty());
+}
+
+TEST_F(ImageFamilyTest, MoveAssignment) {
+  gfx::ImageFamily family;
+  EXPECT_TRUE(family.empty());
+  family = std::move(image_family_);
+  EXPECT_TRUE(image_family_.empty());
+  EXPECT_FALSE(family.empty());
+}
+
+TEST_F(ImageFamilyTest, Clone) {
+  gfx::ImageFamily family = image_family_.Clone();
+  EXPECT_FALSE(image_family_.empty());
+  EXPECT_FALSE(family.empty());
+}
+
 // Tests iteration over an ImageFamily.
 TEST_F(ImageFamilyTest, Iteration) {
   gfx::ImageFamily::const_iterator it = image_family_.begin();

ui/gfx/image/image_unittest.cc

@@ -50,26 +50,6 @@ TEST_F(ImageTest, EmptyImage) {
   EXPECT_TRUE(image.IsEmpty());
   EXPECT_EQ(0, image.Width());
   EXPECT_EQ(0, image.Height());
-
-  // Test the copy constructor.
-  gfx::Image imageCopy(image);
-  EXPECT_TRUE(imageCopy.IsEmpty());
-  EXPECT_EQ(0, imageCopy.Width());
-  EXPECT_EQ(0, imageCopy.Height());
-
-  // Test calling SwapRepresentations() with an empty image.
-  gfx::Image image2(gt::CreateImageSkia(25, 25));
-  EXPECT_FALSE(image2.IsEmpty());
-  EXPECT_EQ(25, image2.Width());
-  EXPECT_EQ(25, image2.Height());
-
-  image.SwapRepresentations(&image2);
-  EXPECT_FALSE(image.IsEmpty());
-  EXPECT_EQ(25, image.Width());
-  EXPECT_EQ(25, image.Height());
-  EXPECT_TRUE(image2.IsEmpty());
-  EXPECT_EQ(0, image2.Width());
-  EXPECT_EQ(0, image2.Height());
 }
 
 // Test constructing a gfx::Image from an empty PlatformImage.
@@ -590,61 +570,72 @@ TEST_F(ImageTest, SkBitmapConversionPreservesTransparency) {
   }
 }
 
-TEST_F(ImageTest, SwapRepresentations) {
+TEST_F(ImageTest, Copy) {
   const size_t kRepCount = kUsesSkiaNatively ? 1U : 2U;
 
   gfx::Image image1(gt::CreateImageSkia(25, 25));
-  const gfx::ImageSkia* image_skia1 = image1.ToImageSkia();
+  EXPECT_EQ(25, image1.Width());
+  EXPECT_EQ(25, image1.Height());
+  gfx::Image image2(image1);
+  EXPECT_EQ(25, image2.Width());
+  EXPECT_EQ(25, image2.Height());
+
   EXPECT_EQ(1U, image1.RepresentationCount());
+  EXPECT_EQ(1U, image2.RepresentationCount());
+  EXPECT_EQ(image1.ToImageSkia(), image2.ToImageSkia());
 
-  gfx::Image image2(gt::CreatePlatformImage());
-  const gfx::ImageSkia* image_skia2 = image2.ToImageSkia();
-  gt::PlatformImage platform_image = gt::ToPlatformType(image2);
+  EXPECT_TRUE(gt::IsPlatformImageValid(gt::ToPlatformType(image2)));
   EXPECT_EQ(kRepCount, image2.RepresentationCount());
+  EXPECT_EQ(kRepCount, image1.RepresentationCount());
+}
 
-  image1.SwapRepresentations(&image2);
+TEST_F(ImageTest, Assign) {
+  gfx::Image image1(gt::CreatePlatformImage());
+  EXPECT_EQ(25, image1.Width());
+  EXPECT_EQ(25, image1.Height());
+  // Assignment must be on a separate line to the declaration in order to test
+  // assignment operator (instead of copy constructor).
+  gfx::Image image2;
+  image2 = image1;
+  EXPECT_EQ(25, image2.Width());
+  EXPECT_EQ(25, image2.Height());
 
-  EXPECT_EQ(image_skia2, image1.ToImageSkia());
-  EXPECT_TRUE(gt::PlatformImagesEqual(platform_image,
-                                      gt::ToPlatformType(image1)));
-  EXPECT_EQ(image_skia1, image2.ToImageSkia());
-  EXPECT_EQ(kRepCount, image1.RepresentationCount());
+  EXPECT_EQ(1U, image1.RepresentationCount());
   EXPECT_EQ(1U, image2.RepresentationCount());
+  EXPECT_EQ(image1.ToSkBitmap(), image2.ToSkBitmap());
 }
 
-TEST_F(ImageTest, Copy) {
+TEST_F(ImageTest, Move) {
   const size_t kRepCount = kUsesSkiaNatively ? 1U : 2U;
 
   gfx::Image image1(gt::CreateImageSkia(25, 25));
   EXPECT_EQ(25, image1.Width());
   EXPECT_EQ(25, image1.Height());
-  gfx::Image image2(image1);
+  gfx::Image image2(std::move(image1));
   EXPECT_EQ(25, image2.Width());
   EXPECT_EQ(25, image2.Height());
 
-  EXPECT_EQ(1U, image1.RepresentationCount());
+  EXPECT_EQ(0U, image1.RepresentationCount());
   EXPECT_EQ(1U, image2.RepresentationCount());
-  EXPECT_EQ(image1.ToImageSkia(), image2.ToImageSkia());
 
   EXPECT_TRUE(gt::IsPlatformImageValid(gt::ToPlatformType(image2)));
+  EXPECT_EQ(0U, image1.RepresentationCount());
   EXPECT_EQ(kRepCount, image2.RepresentationCount());
-  EXPECT_EQ(kRepCount, image1.RepresentationCount());
 }
 
-TEST_F(ImageTest, Assign) {
+TEST_F(ImageTest, MoveAssign) {
   gfx::Image image1(gt::CreatePlatformImage());
   EXPECT_EQ(25, image1.Width());
   EXPECT_EQ(25, image1.Height());
   // Assignment must be on a separate line to the declaration in order to test
-  // assignment operator (instead of copy constructor).
+  // move assignment operator (instead of move constructor).
   gfx::Image image2;
-  image2 = image1;
+  image2 = std::move(image1);
   EXPECT_EQ(25, image2.Width());
   EXPECT_EQ(25, image2.Height());
 
-  EXPECT_EQ(1U, image1.RepresentationCount());
+  EXPECT_EQ(0U, image1.RepresentationCount());
   EXPECT_EQ(1U, image2.RepresentationCount());
-  EXPECT_EQ(image1.ToSkBitmap(), image2.ToSkBitmap());
 }
 
 TEST_F(ImageTest, MultiResolutionImageSkia) {