Add support for Manifest.icons.sizes

Add support for Manifest.icons.sizes

This is re-implementing the HTML icon sizes parsing algorithm
given that it is missing a re-usable place for it. There is
a follow-up bug opened at http://crbug.com/416477

BUG=366145
Committed: https://crrev.com/b847169dd6954fee83c5b89f6ef4699d6ff018f1
Cr-Commit-Position: refs/heads/master@{#296386}

[mlamouri (slow - plz ping), 2014-09-22 15:10:07]

mlamouri@chromium.org changed reviewers:
+ avi@chromium.org, palmer@chromium.org

[Avi (use Gerrit), 2014-09-22 15:15:14]

https://codereview.chromium.org/591073002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/591073002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:175: if (str[0] == '0')
Please check the length to be >= 1 before blindly accessing [0]; |str| is not
guaranteed to not be an empty string.

[mlamouri (slow - plz ping), 2014-09-22 15:31:51]

PTAL

https://codereview.chromium.org/591073002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/591073002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:175: if (str[0] == '0')
On 2014/09/22 15:15:14, Avi wrote:
> Please check the length to be >= 1 before blindly accessing [0]; |str| is not
> guaranteed to not be an empty string.

Done.

[Avi (use Gerrit), 2014-09-22 15:35:26]

LGTM with a test or two more.

https://codereview.chromium.org/591073002/diff/40001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/591073002/diff/40001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:521:
EXPECT_EQ(manifest.icons[0].sizes.size(), 0u);
Perhaps a test for the edge condition that I asked you about? Something with
more than one x, Something with an x that is missing a number on one side, etc?

[mlamouri (slow - plz ping), 2014-09-22 16:26:57]

https://codereview.chromium.org/591073002/diff/40001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/591073002/diff/40001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:521:
EXPECT_EQ(manifest.icons[0].sizes.size(), 0u);
On 2014/09/22 15:35:26, Avi wrote:
> Perhaps a test for the edge condition that I asked you about? Something with
> more than one x, Something with an x that is missing a number on one side,
etc?

Done.

[palmer, 2014-09-22 20:26:59]

https://codereview.chromium.org/591073002/diff/60001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/591073002/diff/60001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:222: 
Check that |width| and |height| have values within some sanity range? Do you do
such a check on the browser side?

[mlamouri (slow - plz ping), 2014-09-22 21:14:56]

PTAL

https://codereview.chromium.org/591073002/diff/60001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/591073002/diff/60001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:222: 
On 2014/09/22 20:26:59, Chromium Palmer wrote:
> Check that |width| and |height| have values within some sanity range? Do you
do
> such a check on the browser side?

I could add a check in the browser process that the value is positive but I'm
not sure there is a real security concern here: the width/height are only used
for information in the sense that consumers of the manifest might check them to
know if the icon is worth downloading. The icon could be a 4M file declared as
"2x2" or a thumbnail declared as a HD image.

In other words, if someone were to inject an icon with a very big width/height,
the consumer of the manifest in the browser process will likely ignore that
icon. If by any chance, it wasn't the case, the browser process would just ask
the renderer process to download the image associated with the entry.

[palmer, 2014-09-23 20:21:58]

LGTM

[mlamouri (slow - plz ping), 2014-09-24 10:38:48]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-24 10:39:37]

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

[commit-bot: I haz the power, 2014-09-24 11:36:41]

Committed patchset #5 (id:80001) as d588e4fc3c2b4b9e71f5e2efa2194f86e2ff9b8f

[commit-bot: I haz the power, 2014-09-24 11:37:22]

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