Mac: Write gfx::ImageFamily using CGImageDestination.

chrome/browser/web_applications/web_app_mac.mm

Index: chrome/browser/web_applications/web_app_mac.mm
diff --git a/chrome/browser/web_applications/web_app_mac.mm b/chrome/browser/web_applications/web_app_mac.mm
index 710a6259e02d1ca6aa8479ed03c3fca6bfb5ad72..c88022189bacd0e9b72e279337e805fb41c9a59d 100644
--- a/chrome/browser/web_applications/web_app_mac.mm
+++ b/chrome/browser/web_applications/web_app_mac.mm
@@ -4,7 +4,6 @@
 
 #import "chrome/browser/web_applications/web_app_mac.h"
 
-#import <Carbon/Carbon.h>
 #import <Cocoa/Cocoa.h>
 #include <stdint.h>
 
@@ -54,6 +53,7 @@
 #import "skia/ext/skia_utils_mac.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "third_party/skia/include/utils/mac/SkCGUtils.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image_family.h"
@@ -93,53 +93,43 @@ class Latch : public base::RefCountedThreadSafe<
   DISALLOW_COPY_AND_ASSIGN(Latch);
 };
 
-class ScopedCarbonHandle {
+class MacIconWriter {
  public:
-  ScopedCarbonHandle(size_t initial_size) : handle_(NewHandle(initial_size)) {
-    DCHECK(handle_);
-    DCHECK_EQ(noErr, MemError());
+  explicit MacIconWriter(size_t image_count)

[Matt Giuca, 2016/10/11 02:36:50]

This is mostly gibberish to me. Maybe add a Mac person to review this bit?

[tapted, 2016/10/12 02:04:13]

Done.

+      : data_([[NSMutableData alloc] initWithCapacity:0]) {
+    image_destination_.reset(CGImageDestinationCreateWithData(
+        base::mac::NSToCFCast(data_), kUTTypeAppleICNS, image_count, nullptr));
+    DCHECK(image_destination_);
   }
-  ~ScopedCarbonHandle() { DisposeHandle(handle_); }
+  ~MacIconWriter() {}
 
-  Handle Get() { return handle_; }
-  char* Data() { return *handle_; }
-  size_t HandleSize() const { return GetHandleSize(handle_); }
-
-  IconFamilyHandle GetAsIconFamilyHandle() {
-    return reinterpret_cast<IconFamilyHandle>(handle_);
+  bool WriteDataToFile(const base::FilePath& path) {
+    if (CGImageDestinationFinalize(image_destination_)) {
+      return
+          [data_ writeToFile:base::mac::FilePathToNSString(path) atomically:NO];
+    }
+    NOTREACHED() << "CGImageDestinationFinalize failed.";
+    return false;
   }
 
-  bool WriteDataToFile(const base::FilePath& path) {
-    NSData* data = [NSData dataWithBytes:Data()
-                                  length:HandleSize()];
-    return [data writeToFile:base::mac::FilePathToNSString(path)
-                  atomically:NO];
+  void AddIcon(const gfx::Image& image) {
+    base::ScopedCFTypeRef<CGImageRef> cg_image(SkCreateCGImageRefWithColorspace(
+        *image.ToSkBitmap(), base::mac::GetSRGBColorSpace()));
+    CGImageDestinationAddImage(image_destination_, cg_image, nullptr);
   }
 
  private:
-  Handle handle_;
-};
+  base::scoped_nsobject<NSMutableData> data_;
+  base::ScopedCFTypeRef<CGImageDestinationRef> image_destination_;
 
-void ConvertSkiaToARGB(const SkBitmap& bitmap, ScopedCarbonHandle* handle) {
-  CHECK_EQ(4u * bitmap.width() * bitmap.height(), handle->HandleSize());
-
-  char* argb = handle->Data();
-  SkAutoLockPixels lock(bitmap);
-  for (int y = 0; y < bitmap.height(); ++y) {
-    for (int x = 0; x < bitmap.width(); ++x) {
-      SkColor pixel = bitmap.getColor(x, y);
-      argb[0] = SkColorGetA(pixel);
-      argb[1] = SkColorGetR(pixel);
-      argb[2] = SkColorGetG(pixel);
-      argb[3] = SkColorGetB(pixel);
-      argb += 4;
-    }
-  }
-}
+  DISALLOW_COPY_AND_ASSIGN(MacIconWriter);
+};
 
 // Adds |image| to |icon_family|. Returns true on success, false on failure.

[Matt Giuca, 2016/10/11 02:36:50]

Wrong comment.

[tapted, 2016/10/12 02:04:13]

Done.

-bool AddGfxImageToIconFamily(IconFamilyHandle icon_family,
-                             const gfx::Image& image) {
+bool IsImageValidForIcon(const gfx::Image& image) {
+  if (image.IsEmpty())
+    return false;
+
   // When called via ShowCreateChromeAppShortcutsDialog the ImageFamily will
   // have all the representations desired here for mac, from the kDesiredSizes
   // array in web_app.cc.
@@ -149,35 +139,16 @@ bool AddGfxImageToIconFamily(IconFamilyHandle icon_family,
     return false;
   }
 
-  OSType icon_type;
   switch (bitmap.width()) {
     case 512:
-      icon_type = kIconServices512PixelDataARGB;
-      break;
     case 256:
-      icon_type = kIconServices256PixelDataARGB;
-      break;
     case 128:
-      icon_type = kIconServices128PixelDataARGB;
-      break;
     case 48:
-      icon_type = kIconServices48PixelDataARGB;
-      break;
     case 32:
-      icon_type = kIconServices32PixelDataARGB;
-      break;
     case 16:
-      icon_type = kIconServices16PixelDataARGB;
-      break;
-    default:
-      return false;
+      return true;
   }
-
-  ScopedCarbonHandle raw_data(bitmap.getSize());
-  ConvertSkiaToARGB(bitmap, &raw_data);
-  OSErr result = SetIconFamilyData(icon_family, icon_type, raw_data.Get());
-  DCHECK_EQ(noErr, result);
-  return result == noErr;
+  return false;
 }
 
 bool AppShimsDisabledForTest() {
@@ -976,24 +947,24 @@ bool WebAppShortcutCreator::UpdateIcon(const base::FilePath& app_path) const {
   if (info_->favicon.empty())
     return true;
 
-  ScopedCarbonHandle icon_family(0);
-  bool image_added = false;
+  size_t image_count = 0;
   for (gfx::ImageFamily::const_iterator it = info_->favicon.begin();
        it != info_->favicon.end(); ++it) {
-    if (it->IsEmpty())
-      continue;
+    if (IsImageValidForIcon(*it))

[Matt Giuca, 2016/10/11 02:36:50]

I think instead of calling this function twice, it would be better to construct
a vector of gfx::Image here during this first loop, then just iterate over it
during the second loop.

[tapted, 2016/10/12 02:04:13]

Done. (and actually it's neater just to have a function this way, rather than
MacIconWriter - all the state is just in the vector instead).

+      ++image_count;
+  }
+  if (image_count == 0)
+    return false;
 
-    // Missing an icon size is not fatal so don't fail if adding the bitmap
-    // doesn't work.
-    if (!AddGfxImageToIconFamily(icon_family.GetAsIconFamilyHandle(), *it))
+  MacIconWriter icon_family(image_count);

[Matt Giuca, 2016/10/11 02:36:50]

Can we rename this to |icon_writer|.

(The name icon_family is confusing with ImageFamily here.)

[tapted, 2016/10/12 02:04:13]

Done (obsoleted).

+  for (gfx::ImageFamily::const_iterator it = info_->favicon.begin();
+       it != info_->favicon.end(); ++it) {
+    if (!IsImageValidForIcon(*it))
       continue;
 
-    image_added = true;
+    icon_family.AddIcon(*it);
   }
 
-  if (!image_added)
-    return false;
-
   base::FilePath resources_path = GetResourcesPath(app_path);
   if (!base::CreateDirectory(resources_path))
     return false;