Extend srcset's syntax to include 'w' descriptors

Extend srcset's syntax to include 'w' descriptors

This CL extends srcset syntax to match the picture specification.
It also adds unit tests to the srcset parser.

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

[Yoav Weiss, 2014-04-10 14:16:34]

This CL implements the srcset part of
https://code.google.com/p/chromium/issues/detail?id=357586

[eseidel, 2014-04-10 16:42:19]

https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLPreloadScanner.cpp (left):

https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLPreloadScanner.cpp:179: } else if
(RuntimeEnabledFeatures::srcsetEnabled()
Is srcset disabled by default?  Or will this affect shipping code?

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

https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:48: inline bool foundDescriptor()
No need to mark "inline".  "const" would be appropriate however.

https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:88: inline int resourceWidth() const
inline is not needed.

https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.h:101: int m_resourceWidth;
Can this be negative?

[eseidel, 2014-04-10 16:49:29]

Assuming this does not affect shipping APIs, lgtm.

[Yoav Weiss, 2014-04-10 17:15:46]

On 2014/04/10 16:42:19, eseidel wrote:
>
https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLPreloadScanner.cpp (left):
> 
>
https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
> Source/core/html/parser/HTMLPreloadScanner.cpp:179: } else if
> (RuntimeEnabledFeatures::srcsetEnabled()
> Is srcset disabled by default?  Or will this affect shipping code?

srcset is enabled by default since M34. I should probably get rid of that
runtime flag.

> 
>
https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLSrcsetParser.h (right):
> 
>
https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.h:48: inline bool foundDescriptor()
> No need to mark "inline".  "const" would be appropriate however.
OK.

> 
>
https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.h:88: inline int resourceWidth()
const
> inline is not needed.
OK.
> 
>
https://codereview.chromium.org/233213003/diff/40001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.h:101: int m_resourceWidth;
> Can this be negative?
When the resource width is not set (when no 'w' descriptors are present), this
value is initialized as -1.

[Yoav Weiss, 2014-04-10 17:18:52]

On 2014/04/10 16:49:29, eseidel wrote:
> Assuming this does not affect shipping APIs, lgtm.

This change doesn't affect the 'x' only, shipping srcset. Part of the reason
I've added the unit tests was to verify that.
It does however impact one of the layout tests that checks to see that 'w' is
ignored. After this change, it no longer is. I'll remove that layout test.

[eseidel, 2014-04-10 17:24:22]

It's best if we enable new features all in one go.  Otherwise we might end up
having at least a dev channel with oddly broken srcset, etc. where it respects w
but not fully correctly, etc.

Do you believe this would be safe to deploy to users immediately as is.  i.e. is
"w" done?

[Yoav Weiss, 2014-04-10 18:47:45]

On 2014/04/10 17:24:22, eseidel wrote:
> It's best if we enable new features all in one go.  Otherwise we might end up
> having at least a dev channel with oddly broken srcset, etc. where it respects
w
> but not fully correctly, etc.
> 
> Do you believe this would be safe to deploy to users immediately as is.  i.e.
is
> "w" done?

I believe it is done according to the current spec. We may need to add more
tests to be sure that there aren't any bugs still creeping in there, but all in
all, it looks good.
OTOH, discussions following my implementation experience, as well as Mozilla's
John Schoenick's feedback, may result in spec changes. Specifically,
https://github.com/ResponsiveImagesCG/picture-element/pull/147 may result in the
src value getting ignored when 'w' descriptors are around.

In any case, there's no point in using 'w' descriptors before 'sizes' land, and
vice versa.

So, it might be best to land this later on. 

Or, maybe there's a better flag to land this behind? What does "test" do?

[eseidel, 2014-04-10 20:32:16]

See documentation at the top of the file:
http://blink.lc/blink/tree/Source/platform/RuntimeEnabledFeatures.in

Feel free to put this behind a MediaQuerySizes flag, etc.

[Yoav Weiss, 2014-04-10 21:06:49]

On 2014/04/10 20:32:16, eseidel wrote:
> See documentation at the top of the file:
> http://blink.lc/blink/tree/Source/platform/RuntimeEnabledFeatures.in
> 
> Feel free to put this behind a MediaQuerySizes flag, etc.

This is already behind the PictureSizes flag. I'll catch you on IRC to see if we
want land this now, in a few days or change the flag to test.

[Yoav Weiss, 2014-04-10 21:24:59]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-10 21:25:03]

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

[commit-bot: I haz the power, 2014-04-10 21:56:48]

Change committed as 171307

[stevenjb, 2014-04-10 23:12:53]

I think this has the wrong issue number...