Add HTMLPictureElement-based source selection to HTMLImageElement

Add HTMLPictureElement-based source selection to HTMLImageElement

This CL adds HTMLPictureElement-based source selection algorithm to 
HTMLImageElement and makes sure that the right resource is picked when
HTMLImageElement has an HTMLPictureElement parent.

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

[esprehn, 2014-05-16 23:31:58]

It's nice if you wrap your change description at 80-100 chars.

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:209: bool
HTMLImageElement::getBestFitImageFromPictureParent()
This is pretty strange to start with "get", what does the boolean return mean
too?

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:215: for (Node* child =
parent->firstChild(); child; child = child->nextSibling()) {
You want Traversal<HTMLSourceElement> here.

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:225: if
(source->fastHasAttribute(typeAttr) &&
!supportedImageType(source->fastGetAttribute(typeAttr)))
ditto. Just unconditionally call get and make supportedImageType take empty
strings.

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:228: if
(source->fastHasAttribute(mediaAttr)) {
I'd suggest just calling fastGetAttribute() and looking if it's empty. Doing
both has() and get() just makes this slower.

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:231: if
(!mediaQueryEvaluator.eval(mediaQueries.get()))
Why are you not using document()->mediaQueryMatcher() ?

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:239:
setBestFitURLAndDPRFromImageCandidate(candidate);
Lets just return the candidate instead. This method should be called find*().

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:242: return false;
return 0;

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:291: imageWasModified =
getBestFitImageFromPictureParent();
getters don't start with "get", things that return through out params do.

This should be find*() and should return a candidate instead.

https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:372: // FIXME - Need to absolutize the
returned value.
FIXME: not dash

[Yoav Weiss, 2014-05-17 05:36:08]

On 2014/05/16 23:31:58, esprehn wrote:
> It's nice if you wrap your change description at 80-100 chars.

Sure. will do from now on.
> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:209: bool
> HTMLImageElement::getBestFitImageFromPictureParent()
> This is pretty strange to start with "get", what does the boolean return mean
> too?

Yeah. Didn't mean for that to be a getter.
The boolean returns true if the best fit image was changed.
"findBestImageFromPictureParent"?

> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:215: for (Node* child =
> parent->firstChild(); child; child = child->nextSibling()) {
> You want Traversal<HTMLSourceElement> here.

I need to bail out of the traversal if HTMLImageElement encounters itself when
going over the children.
Is there a way I can do that with Traversal?

> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:225: if
> (source->fastHasAttribute(typeAttr) &&
> !supportedImageType(source->fastGetAttribute(typeAttr)))
> ditto. Just unconditionally call get and make supportedImageType take empty
> strings.
OK.

> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:228: if
> (source->fastHasAttribute(mediaAttr)) {
> I'd suggest just calling fastGetAttribute() and looking if it's empty. Doing
> both has() and get() just makes this slower.
OK.
> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:231: if
> (!mediaQueryEvaluator.eval(mediaQueries.get()))
> Why are you not using document()->mediaQueryMatcher() ?

No particular reason. Will use it.

> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:239:
> setBestFitURLAndDPRFromImageCandidate(candidate);
> Lets just return the candidate instead. This method should be called find*().

OK
> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:242: return false;
> return 0;
> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:291: imageWasModified =
> getBestFitImageFromPictureParent();
> getters don't start with "get", things that return through out params do.
> 
> This should be find*() and should return a candidate instead.

OK
> 
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:372: // FIXME - Need to absolutize the
> returned value.
> FIXME: not dash

OK

[Yoav Weiss, 2014-05-17 05:41:31]

On 2014/05/17 05:36:08, Yoav Weiss wrote:
> On 2014/05/16 23:31:58, esprehn wrote:
> > 
> >
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> > Source/core/html/HTMLImageElement.cpp:242: return false;
> > return 0;
> > 

Since ImageCandidate is not dynamically allocated, I'd have to "return
ImageCandidate()", and check if it's empty on the other end.
Alternatively, I can change ImageCandidate to be dynamically allocated
everywhere.

[Yoav Weiss, 2014-05-17 05:41:31]

On 2014/05/17 05:36:08, Yoav Weiss wrote:
> On 2014/05/16 23:31:58, esprehn wrote:
> > 
> >
>
https://codereview.chromium.org/288033018/diff/40001/Source/core/html/HTMLIma...
> > Source/core/html/HTMLImageElement.cpp:242: return false;
> > return 0;
> > 

Since ImageCandidate is not dynamically allocated, I'd have to "return
ImageCandidate()", and check if it's empty on the other end.
Alternatively, I can change ImageCandidate to be dynamically allocated
everywhere.

[esprehn, 2014-05-20 06:54:05]

lgtm, please fix the tests and nits before landing. You should always use a
doctype. toAtomicString() is also pretty weird. Can we rename it to url()
instead since that's how it's used? You can do that in another patch though.

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLImageElement/image-picture-1x.html (right):

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLImageElement/image-picture-1x.html:1: <html>
Missing doctype.

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLImageElement/image-picture-1x.html:2: <head>
We usually leave off <html>, <head> and <body>.

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLImageElement/image-picture-invalid.html (right):

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLImageElement/image-picture-invalid.html:1: <html>
ditto, the you need a doctype unless your test really needs quirks mode (which
this doesn't)

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLImageElement/image-picture-nested.html (right):

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLImageElement/image-picture-nested.html:1: <html>
ditto

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html (right):

https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html:1: <html>
All of these tests need doctypes

https://codereview.chromium.org/288033018/diff/80001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/288033018/diff/80001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:159: m_bestFitImageURL =
candidate.toAtomicString();
toAtomicString is weird, I think you want url() ?

https://codereview.chromium.org/288033018/diff/80001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:379: // FIXME: Need to absolutize the
returned value.
Is there a bug for this?

[Yoav Weiss, 2014-05-20 07:54:59]

On 2014/05/20 06:54:05, esprehn wrote:
> lgtm, please fix the tests and nits before landing. You should always use a
> doctype. toAtomicString() is also pretty weird. Can we rename it to url()
> instead since that's how it's used? You can do that in another patch though.
> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> File LayoutTests/fast/dom/HTMLImageElement/image-picture-1x.html (right):
> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> LayoutTests/fast/dom/HTMLImageElement/image-picture-1x.html:1: <html>
> Missing doctype.

Added doctype everywhere

> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> LayoutTests/fast/dom/HTMLImageElement/image-picture-1x.html:2: <head>
> We usually leave off <html>, <head> and <body>.
Got rid of them :)

> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> File LayoutTests/fast/dom/HTMLImageElement/image-picture-invalid.html (right):
> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> LayoutTests/fast/dom/HTMLImageElement/image-picture-invalid.html:1: <html>
> ditto, the you need a doctype unless your test really needs quirks mode (which
> this doesn't)
> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> File LayoutTests/fast/dom/HTMLImageElement/image-picture-nested.html (right):
> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> LayoutTests/fast/dom/HTMLImageElement/image-picture-nested.html:1: <html>
> ditto
> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> File LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html (right):
> 
>
https://codereview.chromium.org/288033018/diff/80001/LayoutTests/fast/dom/HTM...
> LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html:1: <html>
> All of these tests need doctypes
> 
>
https://codereview.chromium.org/288033018/diff/80001/Source/core/html/HTMLIma...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/288033018/diff/80001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:159: m_bestFitImageURL =
> candidate.toAtomicString();
> toAtomicString is weird, I think you want url() ?

Renamed
> 
>
https://codereview.chromium.org/288033018/diff/80001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:379: // FIXME: Need to absolutize the
> returned value.
> Is there a bug for this?

Bug for that is at http://code.google.com/p/chromium/issues/detail?id=375148
CL for that is at https://codereview.chromium.org/290803002/

[Yoav Weiss, 2014-05-20 08:02:54]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-05-20 08:03:10]

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

[commit-bot: I haz the power, 2014-05-20 09:07:58]

Change committed as 174354