Mac: Write gfx::ImageFamily using CGImageDestination.

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
Committed: https://crrev.com/92d17cf5d75eec4d20cb9a806b82683f844b5c22
Cr-Commit-Position: refs/heads/master@{#424653}

[tapted, 2016-10-10 07:19:05]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 07:19:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-10-10 07:22:45]

Description was changed from

==========
First attempt

BUG=650799
==========

to

==========
First attempt

BUG=650799, 257010
==========

[tapted, 2016-10-10 07:35:55]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 07:36:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-10-10 07:47:56]

Description was changed from

==========
First attempt

BUG=650799, 257010
==========

to

==========
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
==========

[tapted, 2016-10-10 07:47:56]

tapted@chromium.org changed reviewers:
+ mgiuca@chromium.org

[tapted, 2016-10-10 08:02:58]

Hi Matt, please take a look.

[commit-bot: I haz the power, 2016-10-10 08:04:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-10 08:04:11]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2016-10-11 02:36:51]

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:98: explicit MacIconWriter(size_t
image_count)
This is mostly gibberish to me. Maybe add a Mac person to review this bit?

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:128: // Adds |image| to
|icon_family|. Returns true on success, false on failure.
Wrong comment.

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:953: if
(IsImageValidForIcon(*it))
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.

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:959: MacIconWriter
icon_family(image_count);
Can we rename this to |icon_writer|.

(The name icon_family is confusing with ImageFamily here.)

[tapted, 2016-10-12 01:52:24]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 01:52:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-10-12 02:04:12]

tapted@chromium.org changed reviewers:
+ erikchen@chromium.org

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

+erik - could you sanity check the CG API usage? Thanks!

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:98: explicit MacIconWriter(size_t
image_count)
On 2016/10/11 02:36:50, Matt Giuca wrote:
> This is mostly gibberish to me. Maybe add a Mac person to review this bit?

Done.

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:128: // Adds |image| to
|icon_family|. Returns true on success, false on failure.
On 2016/10/11 02:36:50, Matt Giuca wrote:
> Wrong comment.

Done.

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:953: if
(IsImageValidForIcon(*it))
On 2016/10/11 02:36:50, Matt Giuca wrote:
> 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.

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).

https://codereview.chromium.org/2404793002/diff/60001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:959: MacIconWriter
icon_family(image_count);
On 2016/10/11 02:36:50, Matt Giuca wrote:
> Can we rename this to |icon_writer|.
> 
> (The name icon_family is confusing with ImageFamily here.)

Done (obsoleted).

[Matt Giuca, 2016-10-12 02:06:50]

Nice improvement. lgtm

[erikchen, 2016-10-12 02:18:36]

very nice work! lgtm

[commit-bot: I haz the power, 2016-10-12 02:19:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 02:19:40]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-12 02:21:29]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-10-12 02:21:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 02:27:24]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-10-12 02:27:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-12 02:30:31]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/92d17cf5d75eec4d20cb9a806b82683f844b5c22
Cr-Commit-Position: refs/heads/master@{#424653}
==========

[commit-bot: I haz the power, 2016-10-12 02:30:32]

Patchset 5 (id:??) landed as
https://crrev.com/92d17cf5d75eec4d20cb9a806b82683f844b5c22
Cr-Commit-Position: refs/heads/master@{#424653}