Skips the processing of images larger then 88x88 in favicons.

Skips the processing of images larger then 88x88 in favicons.

Workaround a crash in CGContextDrawImage when decoding favicon larger
than 88x88 pixels.

BUG=435068
Committed: https://crrev.com/b04ed021c5d4a1678d9799ddf5816975072fbb39
Cr-Commit-Position: refs/heads/master@{#317561}

[sdefresne, 2015-02-19 15:56:15]

sdefresne@chromium.org changed reviewers:
+ junov@chromium.org

[sdefresne, 2015-02-19 15:56:16]

Please take a look.

[Justin Novosad, 2015-02-19 16:10:44]

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios.mm
File skia/ext/skia_utils_ios.mm (right):

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios.mm#n...
skia/ext/skia_utils_ios.mm:95: // in .ico files that are larger then 88x88
(http://crbug.com/435068).
then -> than

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios.mm#n...
skia/ext/skia_utils_ios.mm:114: (size.width < 88 && size.height < 88)) {
Shouldn't the conditions be '<=' instead of '<'?  If not, the your comments and
variable name are wrong (skip_images_88x88_or_larger?)

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios_unit...
File skia/ext/skia_utils_ios_unittest.mm (right):

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios_unit...
skia/ext/skia_utils_ios_unittest.mm:745:
gfx::ImageDataToSkBitmaps(StringToNSData(kIco88x88EncodedData)));
If the icon is 88x88, then it is not "greater than 88x88"

[sdefresne, 2015-02-19 16:46:02]

PTAL

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios.mm
File skia/ext/skia_utils_ios.mm (right):

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios.mm#n...
skia/ext/skia_utils_ios.mm:95: // in .ico files that are larger then 88x88
(http://crbug.com/435068).
On 2015/02/19 at 16:10:44, junov wrote:
> then -> than

Done.

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios.mm#n...
skia/ext/skia_utils_ios.mm:114: (size.width < 88 && size.height < 88)) {
On 2015/02/19 at 16:10:44, junov wrote:
> Shouldn't the conditions be '<=' instead of '<'?  If not, the your comments
and variable name are wrong (skip_images_88x88_or_larger?)

That's correct. Changed to use <=.

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios_unit...
File skia/ext/skia_utils_ios_unittest.mm (right):

https://codereview.chromium.org/923623003/diff/1/skia/ext/skia_utils_ios_unit...
skia/ext/skia_utils_ios_unittest.mm:745:
gfx::ImageDataToSkBitmaps(StringToNSData(kIco88x88EncodedData)));
On 2015/02/19 at 16:10:44, junov wrote:
> If the icon is 88x88, then it is not "greater than 88x88"

Ack, changed the name of the test.

[Justin Novosad, 2015-02-19 18:11:37]

The bug referenced by this change already is marked as fixed and verified.  So
what is going on here?

the logic and the naming are consistent in patch set 2, but you did the opposite
of what I was expecting: 88x88 is excluded. Is this really what is intended?
Just to be clear, size 88x88 is *not* supported on iOS 8.1.1?

[sdefresne, 2015-02-20 09:25:37]

On 2015/02/19 at 18:11:37, junov wrote:
> The bug referenced by this change already is marked as fixed and verified.  So
what is going on here?
> 
> the logic and the naming are consistent in patch set 2, but you did the
opposite of what I was expecting: 88x88 is excluded. Is this really what is
intended? Just to be clear, size 88x88 is *not* supported on iOS 8.1.1?

The code has been developed downstream (Chrome on iOS code is developed in a
private repository that contains some fork of Chromium code). This CL move the
fix to Chromium upstream repository in an effort to upstream all our forks.

Regarding the naming, 88x88 size is *not* supported on iOS 8.1.1. The
inconsistency you pointed was due to my mistake when cleaning up the code for
upstreaming (the code was made in a hurry after the release of iOS 8.1.1 and
quite a hack).

[Justin Novosad, 2015-02-20 21:58:15]

lgtm

[sdefresne, 2015-02-23 10:17:46]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-02-23 10:18:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923623003/20001

[commit-bot: I haz the power, 2015-02-23 10:36:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-23 10:37:20]

Patchset 2 (id:??) landed as
https://crrev.com/b04ed021c5d4a1678d9799ddf5816975072fbb39
Cr-Commit-Position: refs/heads/master@{#317561}