Make gfx::Image's NSImage constructor retain the incoming image.

The Mac convention for ObjC objects is that if a callee needs to
have a reference, it should take the reference itself. The
current behavior of gfx::Image's NSImage constructor is surprising
and relying on explaining it in the comment is inadequate.

Note that gfx::Image's UIImage constructor had a similar change
made to it in 2a1f622d.

BUG=904645

Change-Id: I4dab00c3c7f3ff2f73e17520562d728b3a2e52aa
Reviewed-on: https://chromium-review.googlesource.com/c/1335957Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608566}

chrome/browser/icon_loader_mac.mm

@@ -35,7 +35,7 @@ void IconLoader::ReadIcon() {
 
   if (icon_size_ == ALL) {
     // The NSImage already has all sizes.
-    image = std::make_unique<gfx::Image>([icon retain]);
+    image = std::make_unique<gfx::Image>(icon);
   } else {
     NSSize size = NSZeroSize;
     switch (icon_size_) {

ui/base/resource/resource_bundle_mac.mm

@@ -133,7 +133,7 @@ gfx::Image& ResourceBundle::GetNativeImageNamed(int resource_id) {
       return GetEmptyImage();
     }
 
-    image = gfx::Image(ns_image.release());
+    image = gfx::Image(ns_image);
   }
 
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

ui/gfx/image/image.cc

@@ -177,7 +177,8 @@ class ImageRepCocoaTouch : public ImageRep {
   explicit ImageRepCocoaTouch(UIImage* image)
       : ImageRep(Image::kImageRepCocoaTouch),
         image_(image) {
-    CHECK(image);
+    CHECK(image_);
+    base::mac::NSObjectRetain(image_);
   }
 
   ~ImageRepCocoaTouch() override {
@@ -204,7 +205,8 @@ class ImageRepCocoa : public ImageRep {
   explicit ImageRepCocoa(NSImage* image)
       : ImageRep(Image::kImageRepCocoa),
         image_(image) {
-    CHECK(image);
+    CHECK(image_);
+    base::mac::NSObjectRetain(image_);
   }
 
   ~ImageRepCocoa() override {
@@ -351,7 +353,6 @@ Image::Image(const ImageSkia& image) {
 #if defined(OS_IOS)
 Image::Image(UIImage* image) {
   if (image) {
-    base::mac::NSObjectRetain(image);
     storage_ = new internal::ImageStorage(Image::kImageRepCocoaTouch);
     AddRepresentation(std::make_unique<internal::ImageRepCocoaTouch>(image));
   }
@@ -463,7 +464,6 @@ UIImage* Image::ToUIImage() const {
         const internal::ImageRepSkia* skia_rep =
             GetRepresentation(kImageRepSkia, true)->AsImageRepSkia();
         UIImage* image = UIImageFromImageSkia(*skia_rep->image());
-        base::mac::NSObjectRetain(image);
         scoped_rep.reset(new internal::ImageRepCocoaTouch(image));
         break;
       }
@@ -496,7 +496,6 @@ NSImage* Image::ToNSImage() const {
             GetRepresentation(kImageRepSkia, true)->AsImageRepSkia();
         NSImage* image = NSImageFromImageSkiaWithColorSpace(*skia_rep->image(),
             default_representation_color_space);
-        base::mac::NSObjectRetain(image);
         scoped_rep.reset(new internal::ImageRepCocoa(image));
         break;
       }

ui/gfx/image/image.h

@@ -71,9 +71,7 @@ class GFX_EXPORT Image {
   // Retains |image|.
   explicit Image(UIImage* image);
 #elif defined(OS_MACOSX)
-  // Does not retain |image|; expects to take ownership.
-  // A single NSImage object can contain multiple bitmaps so there's no reason
-  // to pass a vector of these.
+  // Retains |image|.
   explicit Image(NSImage* image);
 #endif
 

ui/gfx/image/image_mac_unittest.mm

@@ -101,7 +101,7 @@ TEST_F(ImageMacTest, MultiResolutionNSImageToImageSkia) {
   [ns_image addRepresentation:ns_image_rep1];
   [ns_image addRepresentation:ns_image_rep2];
 
-  gfx::Image image(ns_image.release());
+  gfx::Image image(ns_image);
 
   EXPECT_EQ(1u, image.RepresentationCount());
 
@@ -132,7 +132,7 @@ TEST_F(ImageMacTest, UnalignedMultiResolutionNSImageToImageSkia) {
       [[NSImage alloc] initWithSize:NSMakeSize(kWidth1x, kHeight1x)]);
   [ns_image addRepresentation:ns_image_rep4];
 
-  gfx::Image image(ns_image.release());
+  gfx::Image image(ns_image);
 
   EXPECT_EQ(1u, image.RepresentationCount());
 

ui/gfx/image/image_unittest_util.cc

@@ -19,11 +19,9 @@
 #include "ui/gfx/image/image_skia.h"
 
 #if defined(OS_IOS)
-#include "base/mac/foundation_util.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "skia/ext/skia_utils_ios.h"
 #elif defined(OS_MACOSX)
-#include "base/mac/foundation_util.h"
 #include "base/mac/mac_util.h"
 #include "skia/ext/skia_utils_mac.h"
 #endif
@@ -222,12 +220,10 @@ PlatformImage CreatePlatformImage() {
       CGColorSpaceCreateDeviceRGB());
   UIImage* image =
       skia::SkBitmapToUIImageWithColorSpace(bitmap, scale, color_space);
-  base::mac::NSObjectRetain(image);
   return image;
 #elif defined(OS_MACOSX)
   NSImage* image = skia::SkBitmapToNSImageWithColorSpace(
       bitmap, base::mac::GetGenericRGBColorSpace());
-  base::mac::NSObjectRetain(image);
   return image;
 #else
   return gfx::ImageSkia::CreateFrom1xBitmap(bitmap);
@@ -258,7 +254,7 @@ gfx::Image CopyViaPlatformType(const gfx::Image& image) {
 #if defined(OS_IOS)
   return gfx::Image(image.ToUIImage());
 #elif defined(OS_MACOSX)
-  return gfx::Image(image.CopyNSImage());
+  return gfx::Image(image.ToNSImage());
 #else
   return gfx::Image(image.AsImageSkia());
 #endif

ui/message_center/cocoa/notification_controller_unittest.mm

@@ -128,7 +128,7 @@ TEST_F(NotificationControllerTest, BasicLayout) {
           ASCIIToUTF16("Jonathan and 5 others"), gfx::Image(), base::string16(),
           GURL(), DummyNotifierId(), message_center::RichNotificationData(),
           NULL));
-  gfx::Image testIcon([TestIcon() retain]);
+  gfx::Image testIcon(TestIcon());
   notification->set_icon(testIcon);
   notification->set_small_image(testIcon);
 
@@ -241,7 +241,7 @@ TEST_F(NotificationControllerTest, Update) {
   EXPECT_FALSE([[controller smallImageView] image]);
 
   // Update the icon.
-  gfx::Image testIcon([TestIcon() retain]);
+  gfx::Image testIcon(TestIcon());
   notification->set_icon(testIcon);
   notification->set_small_image(testIcon);
   [controller updateNotification:notification.get()];
@@ -284,7 +284,7 @@ TEST_F(NotificationControllerTest, Image) {
           GURL(), DummyNotifierId(), message_center::RichNotificationData(),
           NULL));
   NSImage* image = [NSImage imageNamed:NSImageNameFolder];
-  notification->set_image(gfx::Image([image retain]));
+  notification->set_image(gfx::Image(image));
 
   MockMessageCenter message_center;
 

ui/message_center/cocoa/popup_collection_unittest.mm

@@ -235,8 +235,8 @@ TEST_F(PopupCollectionTest, UpdateIconAndBody) {
   MCNotificationController* controller =
       [[popups objectAtIndex:1] notificationController];
   EXPECT_FALSE([[controller iconView] image]);
-  center_->SetNotificationIcon("2",
-      gfx::Image([[NSImage imageNamed:NSImageNameUser] retain]));
+  center_->SetNotificationIcon(
+      "2", gfx::Image([NSImage imageNamed:NSImageNameUser]));
   WaitForAnimationEnded();
   EXPECT_TRUE([[controller iconView] image]);
 

ui/snapshot/snapshot_mac.mm

@@ -50,9 +50,9 @@ bool GrabViewSnapshot(gfx::NativeView native_view,
   if (CGImageGetWidth(windowSnapshot) <= 0)
     return false;
 
-  NSImage* nsImage =
-      [[NSImage alloc] initWithCGImage:windowSnapshot size:NSZeroSize];
-  *image = gfx::Image(nsImage);
+  *image =
+      gfx::Image([[[NSImage alloc] initWithCGImage:windowSnapshot
+                                              size:NSZeroSize] autorelease]);
   return true;
 }