New issue
Advanced search Search tips

Issue 650799 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 622481



Sign in to add a comment

Prepare chrome/browser/web_applications/web_app_mac.mm for 10.8 deployment target.

Project Member Reported by erikc...@chromium.org, Sep 27 2016

Issue description

web_app_mac.mm uses Carbon handles (!?) for interacting with images/bitmaps, it appears. This seems quite unnecessary/archaic.
 

[24315/38611] OBJCXX obj/chrome/browser/extensions/extensions/web_app_mac.o
../../chrome/browser/web_applications/web_app_mac.mm:98:53: warning: 'NewHandle' is deprecated: first deprecated in macOS 10.8 [-Wdeprecated-declarations]
  ScopedCarbonHandle(size_t initial_size) : handle_(NewHandle(initial_size)) {
                                                    ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/MacMemory.h:451:1: note: 'NewHandle' has been explicitly marked deprecated here
NewHandle(Size byteCount)                                     __OSX_AVAILABLE_BUT_DEPRECATED(__MAC_10_0, __MAC_10_8, __IPHONE_NA, __IPHONE_NA);
^
../../chrome/browser/web_applications/web_app_mac.mm:100:22: warning: 'MemError' is deprecated: first deprecated in macOS 10.8 [-Wdeprecated-declarations]
    DCHECK_EQ(noErr, MemError());
                     ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/MacMemory.h:330:1: note: 'MemError' has been explicitly marked deprecated here
MemError(void)                                                __OSX_AVAILABLE_BUT_DEPRECATED(__MAC_10_0, __MAC_10_8, __IPHONE_NA, __IPHONE_NA);
^
../../chrome/browser/web_applications/web_app_mac.mm:102:27: warning: 'DisposeHandle' is deprecated: first deprecated in macOS 10.8 [-Wdeprecated-declarations]
  ~ScopedCarbonHandle() { DisposeHandle(handle_); }
                          ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/MacMemory.h:1278:1: note: 'DisposeHandle' has been explicitly marked deprecated here
DisposeHandle(Handle h)                                       __OSX_AVAILABLE_BUT_DEPRECATED(__MAC_10_0, __MAC_10_8, __IPHONE_NA, __IPHONE_NA);
^
../../chrome/browser/web_applications/web_app_mac.mm:106:38: warning: 'GetHandleSize' is deprecated: first deprecated in macOS 10.8 [-Wdeprecated-declarations]
  size_t HandleSize() const { return GetHandleSize(handle_); }
                                     ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/MacMemory.h:1356:1: note: 'GetHandleSize' has been explicitly marked deprecated here
GetHandleSize(Handle h)                                       __OSX_AVAILABLE_BUT_DEPRECATED(__MAC_10_0, __MAC_10_8, __IPHONE_NA, __IPHONE_NA);
^
4 warnings generated.
Cc: tapted@chromium.org
So the main API we want access to is

extern OSErr
SetIconFamilyData(
  IconFamilyHandle   iconFamily,
  OSType             iconType,
  Handle             h)                                       AVAILABLE_MAC_OS_X_VERSION_10_0_AND_LATER;


This is _not_ deprecated, but making the `Handle` argument to pass to it is only possible with deprecated APIs. Also the `IconFamilyHandle` argument is just cast<> Handle, which we currently also create, but maybe there's a hacky way to get one using:

extern OSErr
IconRefToIconFamily(
  IconRef             theIconRef,
  IconSelectorValue   whichIcons,
  IconFamilyHandle *  iconFamily)                             AVAILABLE_MAC_OS_X_VERSION_10_0_AND_LATER;



https://developer.apple.com/library/prerelease/content/releasenotes/General/CarbonCoreDeprecations/index.html

says

MacMemory
The MacMemory header file defines many of the functions of the Memory Manager, which is available only for compatibility with legacy apps and API. Among other things, the Memory Manager helped developers set up an app’s memory partition at launch time and helped perform memory-management tasks that became unnecessary in OS X. Except for QuickTime usages, you can use other API instead of Memory Manager API. For example, if you use NewHandle or NewPtr for simple allocations, use malloc instead. If you use handles to contain resizable or editable data, switch to the CFMutableData API (for more information, see CFMutableData Reference). Finally, use memmove instead of BlockMove or BlockMoveData, and use bzero or memset instead of BlockZero.


If we assume IconRefToIconFamily() creates a resizable [IconFamily]Handle buffer, and we can pass something fixed-size created with CFMutableData, malloc(), or similar to SetIconFamilyData() as the `Handle` argument, then we could possibly avoid the deprecated APIs.

But it's more likely that IconRefToIconFamily() and SetIconFamilyData() should really be deprecated too and Apple just forgot (although they did convert them to Swift - weird).


Under "Package Your Icon Resources" in the OSX Human Interface Guidelines at https://developer.apple.com/library/content/documentation/UserExperience/Conceptual/OSXHIGuidelines/Designing.html#//apple_ref/doc/uid/20000957-CH87-SW12 the "new" way to make icons is to create a bunch of PNGs, save them on disk in a particular structure, then

"""
 place them in a folder named icon.iconset.

Use the system-provided tools to convert your .iconset folder into a single .icns file. First, use an image-editing program to output app icons in the PNG format, which preserves your design’s alpha values. Your source art files should use sRGB and retain their color profiles. You don’t need to compress image files because the tools used to package them take care of compression for you.

To create an .icns file, use iconutil in Terminal. Terminal is located in /Applications/Utilities/Terminal. Enter the command iconutil -c icns <iconset filename>, where <iconset filename> is the path to the .iconset folder. You must use iconutil, not Icon Composer, to create high-resolution .icns files. For more information, see Provide High-Resolution Versions of All App Graphics Resources.
"""


Open questions:
 - Can we guarantee that /usr/bin/iconutil exists on all Mac computers?
 - Is it OK to spawn this process? Can we sandbox it? Pass it open file descriptors to avoid race conditions?
 - /usr/bin/iconutil looks like a mixed ObjC/C++ program that just talks to /System/Library/PrivateFrameworks/IconCompiler.framework
  * we could reverse-engineer it the API and call it, but that's probably a lot grosser than just using a deprecated API. Like,
   * -[ICIcns initWithURL:]
   * N * -[ICIconset addResouceWithURL:error:]
   * -[ICIconset writeAsICNSToURL:flags:error:]
 - Alternative: reverse-engineer the .icns format and implement support for it [in Skia?]
  * this probably amounts to re-implementing PrivateFrameworks/IconCompiler.framework
tapted: Based on http://lists.apple.com/archives/cocoa-dev/2012/Nov/msg00326.html, it seems like CGImageDestination might provide the same functionality you need?
https://developer.apple.com/reference/imageio/1666386-cgimagedestination

Would that be usable as a replacement to SetIconFamilyData?
Cc: -tapted@chromium.org
Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)
CGImageDestination does indeed look promising :). I'll investigate!

Comment 5 by tapted@chromium.org, Oct 10 2016

Status: Started (was: Assigned)
Seems to work.. https://codereview.chromium.org/2404793002
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92d17cf5d75eec4d20cb9a806b82683f844b5c22

commit 92d17cf5d75eec4d20cb9a806b82683f844b5c22
Author: tapted <tapted@chromium.org>
Date: Wed Oct 12 02:27:01 2016

Mac: Write gfx::ImageFamily using CGImageDestination.

Currently Mac uses SetIconFamilyData() which relies on deprecated Carbon
APIs.

CGImageDestination provides the same functionality and supports
colorspaces properly as an added bonus.

BUG= 650799 ,  257010 

Review-Url: https://codereview.chromium.org/2404793002
Cr-Commit-Position: refs/heads/master@{#424653}

[modify] https://crrev.com/92d17cf5d75eec4d20cb9a806b82683f844b5c22/chrome/browser/web_applications/web_app_mac.mm

Comment 7 by tapted@chromium.org, Oct 12 2016

Status: Fixed (was: Started)

Sign in to add a comment