Refactor srcset parser to align it with spec changes

Refactor srcset parser to align it with spec changes

The srcset parser spec (http://picture.responsiveimages.org/#parse-srcset-attr )
have recently changed to have better future compatibility.
It will enable us to add future descriptors of the form "descriptor(value, anotherValue)".
It will also ease the envisioned addition of the 'h' descriptor.

BUG=376726
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175080

[Yoav Weiss, 2014-05-26 10:48:28]

Resolved the remaining spec questions, and added some tests, following zcorpan's
advice.
Now it's ready for review.

[eseidel, 2014-05-28 22:24:58]

Overall this looks OK.  I think I need to read this again when I'm done with my
other reviews.

I think we should also try and train-up another reviewer to work with you (who
has better response times than I do).  Maybe Alan Cutter has interest in this
code?

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.h:85: inline bool isComma(CharType
character)
inline is only a compiler hint and only has an effect in .cpp files to my
knowledge.  Shouldn't be needed.  Given that it's also in the other one I guess
we can leave it.

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.cpp:64: if (position > descriptorStart)
{
I'm entertained that you use {} here and not below. :)  I believe officially
both are OK these days, but you might want to pick a style for your file here.
:)

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.cpp:67: descriptorStart = 0;
It's a bit odd that this has a side-effect...

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:45: density = -1.0;
Bleh.  Sentinel values/in-band data.  :(

You might want to make these have real accessors that ASSERT that they're not
called when these values are bad...  You may end up with very strange bugs if
someone grabs these and fails to check foundFoo.

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:50: bool foundDensity() { return
density >= 0; }
I think "hasDensitiy" might be more consistent with other code in blink, but
found can be OK if it matches the callers.  Up to you.

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:51: bool foundWidth() { return
resourceWidth >= 0; }
Can these ever be set to negative numbers intentionally?  What should the
behavior be?  Do we need a setter which ignores negative values?  Ah, the
pit-falls of sentinal values.

[Yoav Weiss, 2014-05-29 05:17:39]

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.h:85: inline bool isComma(CharType
character)
On 2014/05/28 22:24:59, eseidel wrote:
> inline is only a compiler hint and only has an effect in .cpp files to my
> knowledge.  Shouldn't be needed.  Given that it's also in the other one I
guess
> we can leave it.

It's inline here since I also use it as a standalone in HTMLSrcsetParser.cpp.

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.cpp:64: if (position > descriptorStart)
{
On 2014/05/28 22:24:59, eseidel wrote:
> I'm entertained that you use {} here and not below. :)  I believe officially
> both are OK these days, but you might want to pick a style for your file here.
> :)
Leftovers from adding a printf at some point.
OTOH, I didn't know that the style guide now permits it.

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.cpp:67: descriptorStart = 0;
On 2014/05/28 22:24:59, eseidel wrote:
> It's a bit odd that this has a side-effect...

This is why I added "AndReset" to the function name. Do you think it's not
enough?

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:45: density = -1.0;
On 2014/05/28 22:24:59, eseidel wrote:
> Bleh.  Sentinel values/in-band data.  :(
> 
> You might want to make these have real accessors that ASSERT that they're not
> called when these values are bad...  You may end up with very strange bugs if
> someone grabs these and fails to check foundFoo.

OK, I'll turn that into a class with proper getters.

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:50: bool foundDensity() { return
density >= 0; }
On 2014/05/28 22:24:59, eseidel wrote:
> I think "hasDensitiy" might be more consistent with other code in blink, but
> found can be OK if it matches the callers.  Up to you.

Sure

https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:51: bool foundWidth() { return
resourceWidth >= 0; }
On 2014/05/28 22:24:59, eseidel wrote:
> Can these ever be set to negative numbers intentionally?  

No

> What should the
> behavior be?  Do we need a setter which ignores negative values?  

Yeah, I'll add that

> Ah, the
> pit-falls of sentinal values.

[Yoav Weiss, 2014-05-29 06:21:31]

On 2014/05/29 05:17:39, Yoav Weiss wrote:
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLParserIdioms.h (right):
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLParserIdioms.h:85: inline bool isComma(CharType
> character)
> On 2014/05/28 22:24:59, eseidel wrote:
> > inline is only a compiler hint and only has an effect in .cpp files to my
> > knowledge.  Shouldn't be needed.  Given that it's also in the other one I
> guess
> > we can leave it.
> 
> It's inline here since I also use it as a standalone in HTMLSrcsetParser.cpp.
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLSrcsetParser.cpp (right):
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.cpp:64: if (position >
descriptorStart)
> {
> On 2014/05/28 22:24:59, eseidel wrote:
> > I'm entertained that you use {} here and not below. :)  I believe officially
> > both are OK these days, but you might want to pick a style for your file
here.
> > :)
> Leftovers from adding a printf at some point.
> OTOH, I didn't know that the style guide now permits it.
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.cpp:67: descriptorStart = 0;
> On 2014/05/28 22:24:59, eseidel wrote:
> > It's a bit odd that this has a side-effect...
> 
> This is why I added "AndReset" to the function name. Do you think it's not
> enough?
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLSrcsetParser.h (right):
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.h:45: density = -1.0;
> On 2014/05/28 22:24:59, eseidel wrote:
> > Bleh.  Sentinel values/in-band data.  :(
> > 
> > You might want to make these have real accessors that ASSERT that they're
not
> > called when these values are bad...  You may end up with very strange bugs
if
> > someone grabs these and fails to check foundFoo.
> 
> OK, I'll turn that into a class with proper getters.
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.h:50: bool foundDensity() { return
> density >= 0; }
> On 2014/05/28 22:24:59, eseidel wrote:
> > I think "hasDensitiy" might be more consistent with other code in blink, but
> > found can be OK if it matches the callers.  Up to you.
> 
> Sure
> 
>
https://codereview.chromium.org/293423002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.h:51: bool foundWidth() { return
> resourceWidth >= 0; }
> On 2014/05/28 22:24:59, eseidel wrote:
> > Can these ever be set to negative numbers intentionally?  
> 
> No
> 
> > What should the
> > behavior be?  Do we need a setter which ignores negative values?  
> 
> Yeah, I'll add that
> 
> > Ah, the
> > pit-falls of sentinal values.

Turned DescriptorParsingResult into a class with getters/setters.

[eseidel, 2014-05-29 06:59:53]

lgtm

I think this is OK.  It's a big change to digest.

Please see nits below.  Hopefully clusterfuzz will catch any pointer math errors
we've made. :(

https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLParserIdioms.h:93: return isComma(character) ||
isHTMLSpace<CharType>(character);
Why do we explicitly call one template but not the other here?  I suspect the
<CharType> on isHTMLSpace is unecessary?

https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLSrcsetParser.cpp:148: CharType c =
attribute[it->start + it->length - 1];
Should DescriptorToken have a helper for this value?  it->lastIndex()?

https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLSrcsetParser.cpp:153: int resourceWidth =
charactersToInt(attribute + it->start, it->length - 1, &isValid);
Should charactersToInt be a member on DescriptorToken?  it->toInt(attribute,
isValid)?

https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLSrcsetParser.h:38: class DescriptorParsingResult {
Much nicer, thanks.

[Yoav Weiss, 2014-05-29 08:37:54]

On 2014/05/29 06:59:53, eseidel wrote:
> lgtm
> 
> I think this is OK.  It's a big change to digest.
> 
> Please see nits below.  Hopefully clusterfuzz will catch any pointer math
errors
> we've made. :(
> 
>
https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
> File Source/core/html/parser/HTMLParserIdioms.h (right):
> 
>
https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
> Source/core/html/parser/HTMLParserIdioms.h:93: return isComma(character) ||
> isHTMLSpace<CharType>(character);
> Why do we explicitly call one template but not the other here?  I suspect the
> <CharType> on isHTMLSpace is unecessary?

Indeed

> 
>
https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
> File Source/core/html/parser/HTMLSrcsetParser.cpp (right):
> 
>
https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
> Source/core/html/parser/HTMLSrcsetParser.cpp:148: CharType c =
> attribute[it->start + it->length - 1];
> Should DescriptorToken have a helper for this value?  it->lastIndex()?
Sure
> 
>
https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
> Source/core/html/parser/HTMLSrcsetParser.cpp:153: int resourceWidth =
> charactersToInt(attribute + it->start, it->length - 1, &isValid);
> Should charactersToInt be a member on DescriptorToken?  it->toInt(attribute,
> isValid)?

Added toInt and toFloat
> 
>
https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
> File Source/core/html/parser/HTMLSrcsetParser.h (right):
> 
>
https://codereview.chromium.org/293423002/diff/100001/Source/core/html/parser...
> Source/core/html/parser/HTMLSrcsetParser.h:38: class DescriptorParsingResult {
> Much nicer, thanks.

[Yoav Weiss, 2014-05-29 09:30:07]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-05-29 09:31:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/293423002/120001

[Yoav Weiss, 2014-05-29 09:51:01]

The CQ bit was unchecked by yoav@yoav.ws

[Yoav Weiss, 2014-05-29 10:41:15]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-05-29 10:42:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/293423002/140001

[commit-bot: I haz the power, 2014-05-29 11:03:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-05-29 11:28:05]

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

[commit-bot: I haz the power, 2014-05-29 11:28:06]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[Yoav Weiss, 2014-05-29 13:22:31]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-05-29 13:23:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/293423002/160001

[Yoav Weiss, 2014-05-29 19:14:32]

The CQ bit was unchecked by yoav@yoav.ws

[Yoav Weiss, 2014-05-29 19:14:35]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-05-29 19:16:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/293423002/160001

[commit-bot: I haz the power, 2014-05-30 00:39:24]

Change committed as 175080