Make the favicons look visually the same after refreshing

This CL fixes two bugs:
1) Makes the favicons (tab, bookmarks) look the same in the browser UI as they do in the renderer). This fixes a regression (probably by one of my CLs) since https://codereview.chromium.org/6117006
2) Make the favicons in the tab strip look the same after refreshing. The difference is due to the conversions PNG -> NSImage and PNG -> SkBitmap -> NSImage producing visually different NSImages. In particular, the result is different when the input PNG data has no colorspace information specified. Cocoa defaults to the device colorspace when decoding PNG data with no colorspace information. The generic RGB colorspace is used for converting from SkBitmap to NSImage. 

BUG=242877
TEST=Manual, see bug

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

[pkotwicz, 2013-06-12 01:26:19]

rsesek@ can you please take a look at the changes in ui/gfx/image.
I decided against passing the colorspace as an argument for
gfx::Image::AsNSImage() because I would need to change many more callsites. (And
it would be easy to forget passing in the colorspace).

Unfortunately I cannot think of any ways of unittesting this.
- gfx::test::IsEqual() returns true even if the colors are off by 16%. This CL
does make ImageTest.* which involve PNGs need less of an error margin. However,
ImageTest.SkBitmapConversionPreservesOrientation still requires the really large
error margin. (It has to do with NSReadPixel() mutating the NSImage on which it
is called but I do not understand why).
- Cocoa considers any bitmaps with really crazy ICC profiles I give it to be
using the generic RGB colorspace so the retagging is a noop for those bitmaps.
So I cannot test that my CL creates the same resulting SkBitmap via PNG ->
SkBitmap and PNG -> NSImage -> SkBitmap

https://codereview.chromium.org/16370006/diff/1/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

https://codereview.chromium.org/16370006/diff/1/ui/gfx/image/image_mac.mm#new...
ui/gfx/image/image_mac.mm:88: [ns_retagged_image_rep retain];
I am confused why I need to retain |ns_retagged_image_rep|. (I get a crash if I
do not retain it)

[Avi (use Gerrit), 2013-06-12 01:35:29]

https://codereview.chromium.org/16370006/diff/1/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

https://codereview.chromium.org/16370006/diff/1/ui/gfx/image/image_mac.mm#new...
ui/gfx/image/image_mac.mm:83: [[NSColorSpace alloc] initWithCGColorSpace:
color_space]);
drive by: no space after the colon

https://codereview.chromium.org/16370006/diff/1/ui/gfx/image/image_mac.mm#new...
ui/gfx/image/image_mac.mm:86: bitmapImageRepByRetaggingWithColorSpace:
ns_color_space];
drive by: no space after the colon

https://codereview.chromium.org/16370006/diff/1/ui/gfx/image/image_mac.mm#new...
ui/gfx/image/image_mac.mm:88: [ns_retagged_image_rep retain];
On 2013/06/12 01:26:19, pkotwicz wrote:
> I am confused why I need to retain |ns_retagged_image_rep|. (I get a crash if
I
> do not retain it)

It comes from -bitmapImageRepByRetaggingWithColorSpace:, so it is autoreleased,
which means it has an effective retain count of 0. ns_image_rep is a
scoped_nsobject, which assumes that you are giving it a retained object, and
when it goes out of scope it will release it. So if you don't retain
ns_retagged_image_rep here, it gets released when ns_image_rep goes out of
scope, and then is released again when the autorelease pool goes away and you
die.

Style-wise, move the retain into the line below:

ns_image_rep.reset([ns_retagged_image_rep retain]);

[Robert Sesek, 2013-06-12 16:12:24]

On 2013/06/12 01:26:19, pkotwicz wrote:
> rsesek@ can you please take a look at the changes in ui/gfx/image.
> I decided against passing the colorspace as an argument for
> gfx::Image::AsNSImage() because I would need to change many more callsites.
(And
> it would be easy to forget passing in the colorspace).
> 
> Unfortunately I cannot think of any ways of unittesting this.
> - gfx::test::IsEqual() returns true even if the colors are off by 16%. This CL
> does make ImageTest.* which involve PNGs need less of an error margin.
However,
> ImageTest.SkBitmapConversionPreservesOrientation still requires the really
large
> error margin. (It has to do with NSReadPixel() mutating the NSImage on which
it
> is called but I do not understand why).
> - Cocoa considers any bitmaps with really crazy ICC profiles I give it to be
> using the generic RGB colorspace so the retagging is a noop for those bitmaps.
> So I cannot test that my CL creates the same resulting SkBitmap via PNG ->
> SkBitmap and PNG -> NSImage -> SkBitmap

I think unittesting image conversions has proven itself to be more painful than
it's worth. I think the existing tests that roundtrip conversions through
gfx::Image succeed (regardless of color space) are sufficient here.

[Robert Sesek, 2013-06-12 16:12:30]

Oh and LGTM

[pkotwicz, 2013-06-12 20:22:56]

Nico, can you please take a look at the changes to base/mac and chrome/browser ?

[Nico, 2013-06-13 00:12:44]

lgtm with below comment addressed

https://codereview.chromium.org/16370006/diff/8001/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

https://codereview.chromium.org/16370006/diff/8001/ui/gfx/image/image.h#newco...
ui/gfx/image/image.h:32: #include <Carbon/Carbon.h>
Carbon? Does ApplicationServices work instead?

Also, can you say `typedef struct CGColorSpace *CGColorSpaceRef;` and include
the header only in the cc file?

[commit-bot: I haz the power, 2013-06-16 18:39:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16370006/16001

[commit-bot: I haz the power, 2013-06-16 18:55:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-16 19:11:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16370006/16001

[commit-bot: I haz the power, 2013-06-16 19:21:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-17 15:53:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16370006/33001

[commit-bot: I haz the power, 2013-06-17 16:12:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-18 05:32:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16370006/33001

[commit-bot: I haz the power, 2013-06-21 20:11:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16370006/59001

[commit-bot: I haz the power, 2013-06-22 01:55:28]

Change committed as 207990