Use SizesAttributeParser to get the right srcset resource

Use SizesAttributeParser to get the right srcset resource

This CL connects the sizes parser to the extended srcset parser and makes sure the images are properly fetched and displayed.

BUG=357586
Depends on http://crrev.com/242103008

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

[Yoav Weiss, 2014-04-17 15:07:19]

I still need to add PreloadScanner layout tests and look into the layout test
crashes, but I think this is ready for review.

[Yoav Weiss, 2014-04-18 08:12:47]

On 2014/04/17 15:07:19, Yoav Weiss wrote:
> I still need to add PreloadScanner layout tests and look into the layout test
> crashes, but I think this is ready for review.

I've split https://codereview.chromium.org/242883002/ from this CL, to make it
easier to review.

[eseidel, 2014-04-19 04:18:46]

I think we need more testing per the bugs below:

https://codereview.chromium.org/236713005/diff/160001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.cpp (right):

https://codereview.chromium.org/236713005/diff/160001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.cpp:216: if (string.isNull() ||
string.isEmpty())
you only need isEmpty().  isNull implies isEmpty.

https://codereview.chromium.org/236713005/diff/160001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/236713005/diff/160001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:161: effectiveSize =
SizesAttributeParser::findEffectiveSize(fastGetAttribute(sizesAttr),
MediaValuesCached::create(document()));
This doesn't seem quite right.  If you set src then sizes we'll fail to load
with the new sizes or?

https://codereview.chromium.org/236713005/diff/160001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/236713005/diff/160001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:185: } else if
(RuntimeEnabledFeatures::pictureSizesEnabled() && match(attributeName,
sizesAttr) && m_imgSourceSize < 0) {
it's unsigned < 0 is impossible.

[Yoav Weiss, 2014-04-19 04:46:47]

On 2014/04/19 04:18:46, eseidel wrote:
> I think we need more testing per the bugs below:
> 
>
https://codereview.chromium.org/236713005/diff/160001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryTokenizer.cpp (right):
> 
>
https://codereview.chromium.org/236713005/diff/160001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryTokenizer.cpp:216: if (string.isNull() ||
> string.isEmpty())
> you only need isEmpty().  isNull implies isEmpty.
> 
>
https://codereview.chromium.org/236713005/diff/160001/Source/core/html/HTMLIm...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/236713005/diff/160001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.cpp:161: effectiveSize =
> SizesAttributeParser::findEffectiveSize(fastGetAttribute(sizesAttr),
> MediaValuesCached::create(document()));
> This doesn't seem quite right.  If you set src then sizes we'll fail to load
> with the new sizes or?
> 
>
https://codereview.chromium.org/236713005/diff/160001/Source/core/html/parser...
> File Source/core/html/parser/HTMLPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/236713005/diff/160001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.cpp:185: } else if
> (RuntimeEnabledFeatures::pictureSizesEnabled() && match(attributeName,
> sizesAttr) && m_imgSourceSize < 0) {
> it's unsigned < 0 is impossible.

Bugs indeed. I'll add more JS creation tests, as well as preloader tests.

[eseidel, 2014-04-19 04:54:19]

https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HT...
File LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html (right):

https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html:9:
shouldBeTrue('document.getElementById("simple").clientWidth==(window.innerWidth)');
You wan "shouldBe()" then it will print nice things for you. :)

[Yoav Weiss, 2014-04-20 20:57:46]

On 2014/04/19 04:54:19, eseidel wrote:
>
https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HT...
> File LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html (right):
> 
>
https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HT...
> LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html:9:
>
shouldBeTrue('document.getElementById("simple").clientWidth==(window.innerWidth)');
> You wan "shouldBe()" then it will print nice things for you. :)

I've added preloader and JS creation tests (and fixed some bugs while doing
that).

[eseidel, 2014-04-20 21:27:43]

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/MediaVa...
File Source/core/css/MediaValuesDynamic.h (left):

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/MediaVa...
Source/core/css/MediaValuesDynamic.h:36: 
Spurious.

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.cpp (right):

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.cpp:216: if (string.isNull() ||
string.isEmpty())
isEmpty() is sufficient.

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParser.cpp (right):

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.cpp:38: } else if (type ==
FunctionToken) {
This change has no effect, but OK.  If the earlier places use "return" there is
no need for an "else" after returns.

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParserTest.cpp (right):

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParserTest.cpp:49: {"50vw", 250},
Is this enough testing?  Or is most covered by the layout tests?

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:50: AtomicString currentSrc() const;
This could also be const AtomicString&, but I doubt it matters.

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/parser...
File Source/core/html/parser/HTMLSrcsetParser.cpp (left):

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/parser...
Source/core/html/parser/HTMLSrcsetParser.cpp:219:
imageCandidates.append(ImageCandidate(srcAttribute, 0, srcAttribute.length(),
defaultResult, ImageCandidate::SrcOrigin));
Same thing, I think you have some confusiong with null vs. empty.

null means the impl is set to 0.  Often used to mean something has never been
set at all.

isEmpty means that either it's null, or that its == "".

The String::emptyString() aka String("") isEmpty(), but is not null.

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

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/parser...
Source/core/html/parser/HTMLSrcsetParser.cpp:201: if (!srcAttribute.isNull() &&
!srcAttribute.isEmpty())
!isEmpty should be the same...

[Yoav Weiss, 2014-04-20 21:48:02]

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParserTest.cpp (right):

https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParserTest.cpp:49: {"50vw", 250},
On 2014/04/20 21:27:43, eseidel wrote:
> Is this enough testing?  Or is most covered by the layout tests?

The integration with HTMLImageElement and HTMLPreloadScanner is covered by the
layout tests.
This is simply a test case for the parser itself that I forgot to add earlier.

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:50: AtomicString currentSrc() const;
On 2014/04/20 21:27:43, eseidel wrote:
> This could also be const AtomicString&, but I doubt it matters.

Changed to match the other attribute getters signature. Thanks! :)

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/parser...
File Source/core/html/parser/HTMLSrcsetParser.cpp (left):

https://codereview.chromium.org/236713005/diff/220001/Source/core/html/parser...
Source/core/html/parser/HTMLSrcsetParser.cpp:219:
imageCandidates.append(ImageCandidate(srcAttribute, 0, srcAttribute.length(),
defaultResult, ImageCandidate::SrcOrigin));
On 2014/04/20 21:27:43, eseidel wrote:
> Same thing, I think you have some confusiong with null vs. empty.
> 
> null means the impl is set to 0.  Often used to mean something has never been
> set at all.
> 
> isEmpty means that either it's null, or that its == "".
> 
> The String::emptyString() aka String("") isEmpty(), but is not null.

For some reason I thought isEmpty is not safe to call when String is NULL. Got
it now.

[eseidel, 2014-04-20 22:06:20]

lgtm

https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParser.cpp (left):

https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.cpp:30: if
(!CSSPrimitiveValue::isLength(startToken->unitType()))
I think this previous early-return based version was more readable.

https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParserTest.cpp (right):

https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParserTest.cpp:49: {"50vw", 250},
Do we need to test the empty string or other extreme values?

[Yoav Weiss, 2014-04-20 22:16:48]

On 2014/04/20 22:06:20, eseidel wrote:
> lgtm
> 
>
https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
> File Source/core/css/parser/SizesAttributeParser.cpp (left):
> 
>
https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.cpp:30: if
> (!CSSPrimitiveValue::isLength(startToken->unitType()))
> I think this previous early-return based version was more readable.

Reverted to an early return form & unified two consecutive ifs there into one.
> 
>
https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
> File Source/core/css/parser/SizesAttributeParserTest.cpp (right):
> 
>
https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParserTest.cpp:49: {"50vw", 250},
> Do we need to test the empty string or other extreme values?
Empty string is already tested at line 38. Added a "whitespace only" test.

[Yoav Weiss, 2014-04-20 22:17:15]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-20 22:17:23]

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

[commit-bot: I haz the power, 2014-04-21 01:13:08]

Change committed as 172004