Mac: Write gfx::ImageFamily using CGImageDestination.

chrome/browser/web_applications/web_app_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/web_applications/web_app_mac.h"
 
-#import <Carbon/Carbon.h>
 #import <Cocoa/Cocoa.h>
 #include <stdint.h>
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/mac/foundation_util.h"
@@ skipping 29 matching lines @@
 #include "chrome/grit/generated_resources.h"
 #include "components/crx_file/id_util.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #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"
 
 bool g_app_shims_allow_update_and_launch_in_tests = false;
 
 namespace {
 
 // Launch Services Key to run as an agent app, which doesn't launch in the dock.
 NSString* const kLSUIElement = @"LSUIElement";
@@ skipping 19 matching lines @@
       content::BrowserThread::UI>;
 
   ~Latch() { callback_.Run(); }
   void NoOp() {}
 
   base::Closure callback_;
 
   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_;
+
+  DISALLOW_COPY_AND_ASSIGN(MacIconWriter);
 };
 
-void ConvertSkiaToARGB(const SkBitmap& bitmap, ScopedCarbonHandle* handle) {
-  CHECK_EQ(4u * bitmap.width() * bitmap.height(), handle->HandleSize());
+// 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 IsImageValidForIcon(const gfx::Image& image) {
+  if (image.IsEmpty())
+    return false;
 
-  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;
-    }
-  }
-}
-
-// Adds |image| to |icon_family|. Returns true on success, false on failure.
-bool AddGfxImageToIconFamily(IconFamilyHandle icon_family,
-                             const gfx::Image& image) {
   // When called via ShowCreateChromeAppShortcutsDialog the ImageFamily will
   // have all the representations desired here for mac, from the kDesiredSizes
   // array in web_app.cc.
   SkBitmap bitmap = image.AsBitmap();
   if (bitmap.colorType() != kN32_SkColorType ||
       bitmap.width() != bitmap.height()) {
     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() {
   // Disable app shims in tests because shims created in ~/Applications will not
   // be cleaned up.
   return base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType);
 }
 
 base::FilePath GetWritableApplicationsDirectory() {
   base::FilePath path;
@@ skipping 778 matching lines @@
   NSString* localized_path = base::mac::FilePathToNSString(
       localized_dir.Append("InfoPlist.strings"));
   return [strings_plist writeToFile:localized_path
                          atomically:YES];
 }
 
 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())
+    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;
+
+  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;
 
-    // Missing an icon size is not fatal so don't fail if adding the bitmap
-    // doesn't work.
-    if (!AddGfxImageToIconFamily(icon_family.GetAsIconFamilyHandle(), *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;
 
   return icon_family.WriteDataToFile(resources_path.Append("app.icns"));
 }
 
 bool WebAppShortcutCreator::UpdateInternalBundleIdentifier() const {
   NSString* plist_path = GetPlistPath(GetInternalShortcutPath());
   NSMutableDictionary* plist = ReadPlist(plist_path);
@@ skipping 170 matching lines @@
         BuildShortcutInfoFromBundle(*it);
     WebAppShortcutCreator shortcut_creator(it->DirName(), shortcut_info.get(),
                                            extensions::FileHandlersInfo());
     shortcut_creator.DeleteShortcuts();
   }
 }
 
 }  // namespace internals
 
 }  // namespace web_app