Create app shims with 32-bit icons only, as per OSX 10.5 spec.

Create app shims with 32-bit icons only, as per OSX 10.5 spec.

This removes third_party/IconFamily from Chrome, which creates
questionable results when adding icon sizes < 128px. Since OSX 10.5, OSX
hasn't used indexed icons or alpha masks when a 32-bit icon is available
(see notes in OSServices.framework/IconStorage.h).

BUG=241304
TEST=Create an app shim and inspect Resources/app.icns using OSX Preview
-- ensure the icons are correct.
TBR=darin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203050

[tapted, 2013-05-22 08:27:33]

ben, would you like to take a look?

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:33: void ConvertSkiaToARGB(const
SkBitmap& bitmap, char* argb) {
This is similar to a function in skia_utils.h, but that function makes RGBA, not
the ARGB, which IconFamily wants. There's an alternative implementation in Patch
Set 1 which looks more like that function, but it uses more skia internals for
the 'unpremultiply' step. This one relies on SkBitmap::getColor returning an
unpremultiplied value, which I think is neater and more robust.

[tapted, 2013-05-22 23:06:19]

Hi Charles, maybe you have some knowledge in this area. Apple's documentation is
a bit sketchy, but I think I've interpreted things right. More details on
http://crbug.com/241304 . Can you take a look for review?

[benwells, 2013-05-23 00:42:04]

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:83: Handle raw_data =
NewHandle(bitmap.getSize());
I can see that it is guaranteed by the config check above that bitmap.getSize is
right, but this code makes me uneasy. Consider adding a max size argument to
CovertSkiaToARGB, or maybe add a comment.

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:83: Handle raw_data =
NewHandle(bitmap.getSize());
Is there a scoped version of the handle you can use?

[tapted, 2013-05-23 04:03:43]

I've addressed both comments by moving my added ScopedCarbonHandle class up and
making it generic. Then asserting some of the inferred properties of the
bitmap.config() that the algo relies on. See what you think.

There's no pre-existing scoper for Carbon handles. They're not used much in
Chrome -- in autocomplete_text_field_cell.mm (and prior to this in
third_party/IconFamily.m).

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:83: Handle raw_data =
NewHandle(bitmap.getSize());
On 2013/05/23 00:42:04, benwells wrote:
> I can see that it is guaranteed by the config check above that bitmap.getSize
is
> right, but this code makes me uneasy. Consider adding a max size argument to
> CovertSkiaToARGB, or maybe add a comment.

Done.

https://codereview.chromium.org/15650005/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:83: Handle raw_data =
NewHandle(bitmap.getSize());
On 2013/05/23 00:42:04, benwells wrote:
> Is there a scoped version of the handle you can use?

Done.

[benwells, 2013-05-23 04:15:24]

lgtm, but we need someone with more mac know how (e.g. dharcourt) to give lg
too.

https://codereview.chromium.org/15650005/diff/21001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/21001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:61: CHECK_EQ(bitmap.rowBytes() *
bitmap.height(), bitmap.getSize());
Maybe just CHECK_EQ(bitmap.width() * bitmap.height() * 4, handle->HandleSize())?
That is the important thing.

https://codereview.chromium.org/15650005/diff/21001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:67: SkColor pixel =
bitmap.getColor(x, y);  // Unpremultiplies.
Nit: I think the comment is unnecessary and implies knowledge of the SkBitmap
internal data structure.

[tapted, 2013-05-23 04:30:02]

https://codereview.chromium.org/15650005/diff/21001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/21001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:61: CHECK_EQ(bitmap.rowBytes() *
bitmap.height(), bitmap.getSize());
On 2013/05/23 04:15:24, benwells wrote:
> Maybe just CHECK_EQ(bitmap.width() * bitmap.height() * 4,
handle->HandleSize())?
> That is the important thing.

Done.

https://codereview.chromium.org/15650005/diff/21001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:67: SkColor pixel =
bitmap.getColor(x, y);  // Unpremultiplies.
On 2013/05/23 04:15:24, benwells wrote:
> Nit: I think the comment is unnecessary and implies knowledge of the SkBitmap
> internal data structure.

Done.

[dharcourt, 2013-05-24 02:03:32]

LGTM. Fun to see Handles still exist out there. I'm assuming there are reasons
to use a .icns file rather than NSWorkspace's setIcon:forFile:options: method to
set the icon (persistence? multiple sizes? it just works?).

[tapted, 2013-05-24 04:38:38]

+Nico for src/chrome OWNERS - there's a semi-structural change in
chrome_browser.gypi for Mac here.

On 2013/05/24 02:03:32, dharcourt wrote:
> LGTM. Fun to see Handles still exist out there. I'm assuming there are reasons
> to use a .icns file rather than NSWorkspace's setIcon:forFile:options: method
to
> set the icon (persistence? multiple sizes? it just works?).

We need a .icns file to build the .app bundle. I think this is the only API that
Apple provide to help with that, and it was the basis for the technique in
third_party/IconFamily that is made obsolete by this CL.

[Nico, 2013-05-24 05:05:56]

+sail who added IconFamily to the repo

[tapted, 2013-05-28 23:17:04]

sail: ping?

[sail, 2013-05-29 00:31:37]

This looks great. Could you delete third_party/icon_family when this lands?

Also, can you verify that hit testing in the dock works correctly?

https://codereview.chromium.org/15650005/diff/26001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/26001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:35: ScopedCarbonHandle(size_t
initial_size) : handle_(NewHandle(initial_size)) {}
Add a DCHECK(handle_) and DCHECK(!MemError()) here?

https://codereview.chromium.org/15650005/diff/26001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:113: return
SetIconFamilyData(icon_family, icon_type, raw_data.Get()) == noErr;
DCHECK !error here?

[tapted, 2013-05-29 01:37:36]

Updated & retested.

On 2013/05/29 00:31:37, sail wrote:
> This looks great. Could you delete third_party/icon_family when this lands?

Yes - I think there's a deps2git post-commit hook that will pick up the DEPS
change, then gclient sync will get an "icon_family is no longer part of this
checkout" message. Not sure there's a smoother way than that.

Then, I'll reach out to the infrastructure folks to notify that it can
eventually be removed from the mirrors (I think it will need to wait for a
stable channel though).

> Also, can you verify that hit testing in the dock works correctly?

I tried this out in 10.8 and 10.6 - I think it is correct. All my dock icons
respond to a rectangular region that extends from a shared outer point on the
dock, to the screen edge. This includes some of the smaller system icons like
terminal and dashboard. Digging around some more, I get the impression that
Apple quietly removed hit-testing from the OSX UI in 10.5. E.g. all the
hit-testing functions in
http://developer.apple.com/legacy/library/documentation/Carbon/Reference/Icon...
are marked "Deprecated in OS X v10.5. Use Icon Services instead." There doesn't
seem to be a replacement.

https://codereview.chromium.org/15650005/diff/26001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/26001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:35: ScopedCarbonHandle(size_t
initial_size) : handle_(NewHandle(initial_size)) {}
On 2013/05/29 00:31:37, sail wrote:
> Add a DCHECK(handle_) and DCHECK(!MemError()) here?

Done.

https://codereview.chromium.org/15650005/diff/26001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:113: return
SetIconFamilyData(icon_family, icon_type, raw_data.Get()) == noErr;
On 2013/05/29 00:31:37, sail wrote:
> DCHECK !error here?

Done.

[sail, 2013-05-29 04:38:42]

> Yes - I think there's a deps2git post-commit hook that will pick up the DEPS
> change, then gclient sync will get an "icon_family is no longer part of this
> checkout" message. Not sure there's a smoother way than that.

The code is checked in directly (not pulled in through DEPS or anything). You
can just do "git rm" I think.

>  All my dock icons
> respond to a rectangular region that extends from a shared outer point on the
> dock, to the screen edge.

Here's something you could try:
  - use an icon that's circular
  - in Finder switch to icon mode
  - click on the inside of the circle
  - verify that the app is selected
  - in Finder, click on the outside of the circle
  - verify that the app is not selected

[tapted, 2013-05-29 05:35:09]

On 2013/05/29 04:38:42, sail wrote:
> > Yes - I think there's a deps2git post-commit hook that will pick up the DEPS
> > change, then gclient sync will get an "icon_family is no longer part of this
> > checkout" message. Not sure there's a smoother way than that.
> 
> The code is checked in directly (not pulled in through DEPS or anything). You
> can just do "git rm" I think.

Right you are - Done. I've rolled it in here to get some bot exposure. Looks
good, but let me know if the delete should be a follow-up instead.

> 
> >  All my dock icons
> > respond to a rectangular region that extends from a shared outer point on
the
> > dock, to the screen edge.
> 
> Here's something you could try:
>   - use an icon that's circular
>   - in Finder switch to icon mode

Success! Verified end-to-end using the 'note' avatar, and `for x in 256 128 32
16 ; do convert profile_avatar_note.png -resize ${x}x${x}'!' app_list_${x}.png ;
done`.  Confirmed that clicking the 100% transparent parts do not select the
icon in Finder.

[sail, 2013-05-29 14:54:04]

lgtm!

[Nico, 2013-05-29 15:05:39]

lgtm with comment addressed

https://codereview.chromium.org/15650005/diff/46001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/46001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:45: template <class GetAsType>
You only call this ever with a type of IconFamilyHandle. What's the point of
having a template that's mostly just a reinterpret_cast? Do
GetAsIconFamilyHandle() instead. You can then probably use static_cast<>() too.

[tapted, 2013-05-29 23:39:03]

TBR=darin for third_party changes (not adding stuff)

https://codereview.chromium.org/15650005/diff/46001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/15650005/diff/46001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:45: template <class GetAsType>
On 2013/05/29 15:05:40, Nico wrote:
> You only call this ever with a type of IconFamilyHandle. What's the point of
> having a template that's mostly just a reinterpret_cast? Do
> GetAsIconFamilyHandle() instead. You can then probably use static_cast<>()
too.

Good point! Done. (However, it still needs the reinterpret_cast due to the extra
indirection -- "static_cast from 'Handle' (aka 'char **') to 'IconFamilyHandle'
(aka 'IconFamilyResource **') is not allowed")

[commit-bot: I haz the power, 2013-05-29 23:44:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/15650005/55001

[commit-bot: I haz the power, 2013-05-30 03:55:07]

Change committed as 203050