Add support for adding 256x256 pngs to Windows .ico files.

Add support for adding 256x256 pngs to Windows .ico files.

These are needed to support the "large icon" view under Vista+.

BUG=167277, 163864, 110379
TEST=New unit test.

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

[Alexei Svitkine (slow), 2013-01-02 22:27:28]

Hey Scott,

Could you review this?

Unfortunately there doesn't seem to be a better owner since the code was
originally written in 2008 by idana@ who doesn't work on Chrome anymore and has
hardly been touched since.

[Alexei Svitkine (slow), 2013-01-07 16:45:26]

Ping! Let me know if you have a better reviewer in mind for this. (I added you
since you're an owner here and since the original code was written by someone
who no longer works on Chrome).

[sky, 2013-01-07 17:05:24]

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc#newcod...
ui/gfx/icon_util.cc:300: CHECK_EQ(256, large_bitmap.width());
Why CHECK and not DCHECK?

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc#newcod...
ui/gfx/icon_util.cc:317: DCHECK(!bitmaps.empty());
Is there a reason we can't add large_bitmap to bitmaps here?

[Alexei Svitkine (slow), 2013-01-07 17:51:28]

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc#newcod...
ui/gfx/icon_util.cc:300: CHECK_EQ(256, large_bitmap.width());
On 2013/01/07 17:05:24, sky wrote:
> Why CHECK and not DCHECK?

I could do a DCHECK. I guess I wanted to make sure that this never runs with
|large_bitmap| non-being 256x, since that would create an invalid .ico file.
Another alternative would be to return "false" in that case.

Let me know what you prefer.

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc#newcod...
ui/gfx/icon_util.cc:317: DCHECK(!bitmaps.empty());
On 2013/01/07 17:05:24, sky wrote:
> Is there a reason we can't add large_bitmap to bitmaps here?

Yes, because it has to be special-cased. Non-256x ones are stored in BMP format
in the icon file while the 256x one has be stored as a PNG.

We could add it to the list and then do some additional tracking so that it can
be special-treated later. The problem with that is that
ComputeIconFileBufferSize() has to know about it, so that it doesn't compute
size as if it was a BMP and also the for loop that calls
SetSingleIconImageInformation() shouldn't run on that bitmap either and we'd
still need the special case after that loop to add it.

[sky, 2013-01-07 22:18:56]

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc#newcod...
ui/gfx/icon_util.cc:300: CHECK_EQ(256, large_bitmap.width());
On 2013/01/07 17:51:28, Alexei Svitkine wrote:
> On 2013/01/07 17:05:24, sky wrote:
> > Why CHECK and not DCHECK?
> 
> I could do a DCHECK. I guess I wanted to make sure that this never runs with
> |large_bitmap| non-being 256x, since that would create an invalid .ico file.
> Another alternative would be to return "false" in that case.
> 
> Let me know what you prefer.

See
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
. I think this should be a DCHECK.

[Alexei Svitkine (slow), 2013-01-07 22:25:22]

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/11742007/diff/1010/ui/gfx/icon_util.cc#newcod...
ui/gfx/icon_util.cc:300: CHECK_EQ(256, large_bitmap.width());
On 2013/01/07 22:18:56, sky wrote:
> On 2013/01/07 17:51:28, Alexei Svitkine wrote:
> > On 2013/01/07 17:05:24, sky wrote:
> > > Why CHECK and not DCHECK?
> > 
> > I could do a DCHECK. I guess I wanted to make sure that this never runs with
> > |large_bitmap| non-being 256x, since that would create an invalid .ico file.
> > Another alternative would be to return "false" in that case.
> > 
> > Let me know what you prefer.
> 
> See
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> . I think this should be a DCHECK.

Ah, I didn't realise CHECKs had to meet those criteria. Thanks for pointing me
to the guidelines there! Done.

[sky, 2013-01-08 01:15:02]

LGTM

[commit-bot: I haz the power, 2013-01-08 01:39:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11742007/32001

[commit-bot: I haz the power, 2013-01-08 16:08:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11742007/32001

[commit-bot: I haz the power, 2013-01-08 19:03:00]

Change committed as 175567