Bug 116963 – Implement `picture` element

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anselm Hannemann]

The `picture` element is a markup pattern that allows developers to declare multiple sources for an image using `media` attributes on `source` elements, similar to the `video` element. This allows authors to specify requirements for when/if image sources are requested/displayed.

The Specification published by the HTML WG is the following:
http://www.w3.org/TR/html-picture-element/

Note that the `picture` element is able to make use of the resolution feature of the `srcset` attribute, but does not strictly require it. The two patterns are complementary, and together fulfill the full list of Use Cases and Requirements for Standardizing Responsive Images as published by the W3C:
http://www.w3.org/TR/2013/WD-respimg-usecases-20130226/

Change description:
Enable a responsive images solution and give developers control over the image resource that is downloaded & displayed using the various resources' media attributes

Changes to API surface:
* The `picture` element will be recognized in HTML and displayed as an image. 
* Its `src` attribute will trigger a resource download. 
* The `src` attribute of the first matching `source` child element with a matching `media` attribute will trigger a resource download, unless `picture`has an `src` attribute.
* The DOM API for the element will resemble that of `img`, with possible minor changes.

Public standards discussion: http://www.w3.org/TR/html-picture-element/

---

Additional information about this:

We ran an open survey, and 42% of the 264 responses (at time of posting this) said that they are making use of “art direction” with their current “responsive images” solution.
This survey is largely comprised of attendees of last week’s Mobilism conference and developers following http://twitter.com/smashingmag (702,117 followers), so I assume it represents a fairly wide cross-section of the developer community. It’s also worth noting that developers heavily favor the `picture` element (or equivalent polyfill).
https://docs.google.com/forms/d/1LIbd9wiM7M_m-rUB85WAiGT-h8QUY-PRJwjiBRiuY4Y/viewanalytics

[Yoav Weiss]

The specification had recently undergone a significant simplification. The latest version can be found in http://picture.responsiveimages.org/

As a result, the above change description is no longer accurate.

A better description of current proposed API surface change:
* The `srcset` attribute will be extended. It would be able include resource dimensions, as an alternative to DPR.
* The `sizes` attribute will be defined, enabling the browser to calculate the appropriately sized resource needed, before layout is calculated.
* The `<picture>` element will be defined and used (with its child `<source>` elements) for the child `<img>`'s resource selection.

According to the new specification, `<img>` remains responsible for displaying the actual image, so previous concerns regarding `<picture>` requiring duplication of img's layout tests were addressed.


Is the WebKit project interested in accepting patches for this feature?

Thanks,
Yoav

P.S. CCing Maciej since he reviewed the spec on WHATWG

[Radar WebKit Bug Importer]

<rdar://problem/19715560>

[erick]

what about  Safari 9 and picture element ?
it works perfectly with Chrome and Firefox  and it is necessary for responsive design

[Dave Hyatt]

Created attachment 266265 [details]
Patch (first cut)

[WebKit Commit Bot]

Attachment 266265 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLImageElement.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit/mac/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit2/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/html/HTMLPictureElement.cpp:51:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/html/HTMLSourceElement.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 266265 [details]
Patch (first cut)

Attachment 266265 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/498384

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html
imported/w3c/web-platform-tests/html/dom/interfaces.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/srcset/select-an-image-source.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html
imported/w3c/html-templates/template-element/template-content.html

[Build Bot]

Created attachment 266278 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266265 [details]
Patch (first cut)

Attachment 266265 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/498406

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html
imported/w3c/web-platform-tests/html/dom/interfaces.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/srcset/select-an-image-source.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html
imported/w3c/html-templates/template-element/template-content.html

[Build Bot]

Created attachment 266280 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266265 [details]
Patch (first cut)

Attachment 266265 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/498394

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/srcset/select-an-image-source.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html
imported/w3c/html-templates/template-element/template-content.html

[Build Bot]

Created attachment 266281 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yoav Weiss]

I believe that in order to properly implement <picture> without double downloads, you would need first to land image async loading (https://bugs.webkit.org/show_bug.cgi?id=134488), which in turn needs to rely on the microtask fix (https://bugs.webkit.org/show_bug.cgi?id=147933) Sam has been working on.

[Simon Pieters (:zcorpan)]

Comment on attachment 266265 [details]
Patch (first cut)

> +picture interfaceName=HTMLPictureElement, JSInterfaceName=HTMLElement

The interface exposed to JS should be HTMLPictureElement per spec. It is relevant for feature-checking (e.g. picturefill uses `!!window.HTMLPictureElement`).

[Dave Hyatt]

Created attachment 266357 [details]
Patch

[Dave Hyatt]

(In reply to comment #13)
> Comment on attachment 266265 [details]
> Patch (first cut)
> 
> > +picture interfaceName=HTMLPictureElement, JSInterfaceName=HTMLElement
> 
> The interface exposed to JS should be HTMLPictureElement per spec. It is
> relevant for feature-checking (e.g. picturefill uses
> `!!window.HTMLPictureElement`).

Ah ok will make this adjustment.

[Darin Adler]

Comment on attachment 266357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266357&action=review

> Source/WebCore/html/HTMLImageElement.cpp:143
> +    Node* parent = parentNode();

Should be auto* here, since parentNode is actually a ContainerNode and we like letting it be the more specific type.

> Source/WebCore/html/HTMLImageElement.cpp:144
> +    if (!parent || !is<HTMLPictureElement>(*parent))

Another way to write this is:

    if (!is<HTMLPictureElement>(parent))

> Source/WebCore/html/HTMLImageElement.cpp:145
> +        return ImageCandidate();

Can use { } here instead of ImageCandidate() if you prefer. I do.

> Source/WebCore/html/HTMLImageElement.cpp:146
> +    for (Node* child = parent->firstChild(); child; child = child->nextSibling()) {

Could put "child != this" in here instead of "child" as the end condition, or "child && child != this" if we are paranoid.

> Source/WebCore/html/HTMLImageElement.cpp:152
> +        String srcset = source.fastGetAttribute(srcsetAttr);

Type here should be auto& instead of String so it can be a const AtomicString&.

> Source/WebCore/html/HTMLImageElement.cpp:153
> +        if (srcset.isEmpty())

Is the check for null (no attribute), empty, or empty after stripping whitespace? Ideally would like test cases that cover these.

> Source/WebCore/html/HTMLImageElement.cpp:155
> +        String type = source.fastGetAttribute(typeAttr).string().stripWhiteSpace();

Should be using stripLeadingAndTrailingHTMLSpaces instead of stripWhiteSpace, because that latter function strips things it shouldn't.

Would be nicer to use StringView for this instead of String, since we are only stripping and truncating, but it probably takes some work.

> Source/WebCore/html/HTMLImageElement.cpp:158
> +        int indexOfSemicolon = type.find(';');
> +        if (indexOfSemicolon >= 0)
> +            type.truncate(indexOfSemicolon);

What about stripping whitespace before the ";"? Seems like we need to do this first, and then strip the whitespace.

> Source/WebCore/html/HTMLImageElement.cpp:160
> +        if (!type.isEmpty() && !MIMETypeRegistry::isSupportedImageMIMEType(type) && type != "image/svg+xml")
> +            continue;

This check of MIME type looks like it’s case sensitive. But MIME types are case folding. We need to either lowercase this or use a case folding comparison function.

> Source/WebCore/html/HTMLImageElement.cpp:162
> +        MediaQueryEvaluator queryEval(document().printing() ? "print" : "screen", document().frame(), computedStyle());

Can we call this evaluator instead of queryEval?

> Source/WebCore/html/HTMLImageElement.cpp:165
> +        RefPtr<MediaQuerySet> media = MediaQuerySet::createAllowingDescriptionSyntax(source.media());
> +        if (!queryEval.eval(media.get()))
> +            continue;

Should be Ref instead of RefPtr, but I’d prefer it as a one liner:

    if (!evaluator.eval(MediaQuerySet::createAllowingDescriptionSyntax(source.media()).ptr()))

> Source/WebCore/html/HTMLImageElement.cpp:171
> +        if (candidate.isEmpty())
> +            continue;
> +        return candidate;

I like it better as:

    if (!candidate.isEmpty())
        return candidate;

> Source/WebCore/html/HTMLImageElement.cpp:173
> +    return ImageCandidate();

Can use { } here instead of ImageCandidate() if you prefer. I do.

> Source/WebCore/html/HTMLImageElement.cpp:183
> +        float sourceSize = parseSizesAttribute(fastGetAttribute(sizesAttr).string(), document().renderView(), document().frame());
> +        candidate = bestFitSourceForImageAttributes(document().deviceScaleFactor(), fastGetAttribute(srcAttr), fastGetAttribute(srcsetAttr), sourceSize);

Annoying that this code is repeated. Can we make a helper function.

> Source/WebCore/html/HTMLImageElement.cpp:308
> +    Node* parent = parentNode();
> +    if (parent && is<HTMLPictureElement>(*parent))
> +        selectImageSource();

I like it better like this:

    if (is<HTMLPictureElement>(parentNode()))
        selectImageSource();

> Source/WebCore/html/HTMLImageElement.h:91
> +    void selectImageSource();

Can this please be private instead of public?

> Source/WebCore/html/HTMLImageElement.idl:-38
> -#if ENABLE_CURRENTSRC
>      readonly attribute DOMString currentSrc;
> -#endif

Is this the right thing to do? Making this unconditional?

> Source/WebCore/html/HTMLPictureElement.cpp:35
> +using namespace HTMLNames;

This isn’t used and should be omitted.

> Source/WebCore/html/HTMLPictureElement.cpp:51
> +    for (HTMLImageElement* imageElement = Traversal<HTMLImageElement>::firstChild(*this); imageElement; imageElement = Traversal<HTMLImageElement>::nextSibling(*imageElement)) {
> +        imageElement->selectImageSource();
> +    }

This should use the iterator:

    for (auto& imageElement : childrenOfType<HTMLImageElement>(*this))
        imageElement.selectImageSource();

> Source/WebCore/html/HTMLSourceElement.cpp:170
> +        Element* parent = parentElement();

Strange to use parentElement here but parentNode elsewhere.

[Build Bot]

Comment on attachment 266357 [details]
Patch

Attachment 266357 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/502253

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html

[Build Bot]

Created attachment 266364 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266357 [details]
Patch

Attachment 266357 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/502256

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html

[Build Bot]

Created attachment 266366 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Dave Hyatt]

Created attachment 266378 [details]
Patch

Incorporate Darin's suggested changes and do what Simon suggested by adding HTMLPictureElement.idl.

[WebKit Commit Bot]

Attachment 266378 [details] did not pass style-queue:


ERROR: LayoutTests/imported/w3c/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Created attachment 266380 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 266378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266378&action=review

> Source/WebCore/html/HTMLImageElement.cpp:141
> +ImageCandidate HTMLImageElement::bestFitSourceFromPictureElement()

Can this be a const method?

> Source/WebCore/html/HTMLPictureElement.cpp:35
> +inline HTMLPictureElement::HTMLPictureElement(const QualifiedName& tagName, Document& document)

Does the 'inline' really help?

> Source/WebCore/html/HTMLSourceElement.cpp:65
> +        if (is<HTMLMediaElement>(*parent))
> +            downcast<HTMLMediaElement>(*parent).sourceWasAdded(this);
> +        else if (is<HTMLPictureElement>(*parent))
> +            downcast<HTMLPictureElement>(*parent).sourcesChanged();

Sad that these are different.

> Source/WebCore/html/HTMLSourceElement.cpp:79
> +        if (is<HTMLMediaElement>(*parent))
> +            downcast<HTMLMediaElement>(*parent).sourceWasRemoved(this);
> +        else if (is<HTMLPictureElement>(*parent))
> +            downcast<HTMLPictureElement>(*parent).sourcesChanged();

Ditto.

[Dean Jackson]

Comment on attachment 266378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266378&action=review

> Source/WebCore/ChangeLog:11
> +        Add HTMLPictureElement.cpp/.h to the project.

Add HTMLPictureElement.* to the project

> Source/WebCore/ChangeLog:39
> +        * html/HTMLPictureElement.cpp: Added.

You need an entry for html/HTMLPictureElement.idl too

> Source/WebCore/html/HTMLImageElement.cpp:146
> +    for (Node* child = parent->firstChild(); child && child != this; child = child->nextSibling()) {

Shame we don't have an iterator for this, like childrenOfNode<parent>, although in this case you have the extra "child != this" condition, so it wouldn't work directly even if it did exist.

> Source/WebCore/html/HTMLImageElement.cpp:160
> +            if (!type.isEmpty() && !MIMETypeRegistry::isSupportedImageMIMEType(type) && type != "image/svg+xml")

Not now, but we should maybe have something in the future that replaces type != "image/svg+xml" with !MIMETypeRegistry::isScalableImageMIMEType(type)

> Source/WebCore/html/HTMLSourceElement.cpp:65
> +        if (is<HTMLMediaElement>(*parent))
> +            downcast<HTMLMediaElement>(*parent).sourceWasAdded(this);
> +        else if (is<HTMLPictureElement>(*parent))
> +            downcast<HTMLPictureElement>(*parent).sourcesChanged();

I wonder if it is worth making these the same name (sourcesChanged)? Doesn't really avoid the downcast though, just looks nicer.

> Source/WebCore/html/HTMLSourceElement.cpp:80
> +    if (parent) {
> +        if (is<HTMLMediaElement>(*parent))
> +            downcast<HTMLMediaElement>(*parent).sourceWasRemoved(this);
> +        else if (is<HTMLPictureElement>(*parent))
> +            downcast<HTMLPictureElement>(*parent).sourcesChanged();
> +    }

Oh, I guess Media really wants to know if it was added or removed, rather than working it out :(

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set-expected.txt:128
> +FAIL <picture><source srcset="data:,b" media="all and !"><img src="data:,a" data-expect="data:,a"></picture> assert_equals: expected "data:,a" but got "data:,b"
> +FAIL <picture><source srcset="data:,b" media="all and (!)"><img src="data:,a" data-expect="data:,a"></picture> assert_equals: expected "data:,a" but got "data:,b"

Should we file bugs for our remaining failures?

[Dean Jackson]

r=me whenever you get it to build.

[Build Bot]

Comment on attachment 266378 [details]
Patch

Attachment 266378 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/503335

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266386 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266378 [details]
Patch

Attachment 266378 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/503398

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266388 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266380 [details]
Patch

Attachment 266380 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/503518

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266389 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266380 [details]
Patch

Attachment 266380 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/503522

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266391 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266380 [details]
Patch

Attachment 266380 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/503538

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266393 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Dave Hyatt]

Created attachment 266399 [details]
Patch

[Dean Jackson]

Comment on attachment 266399 [details]
Patch

r=me based on previous reviews, even though you didn't fix my minor nits in the ChangeLog :)

[Build Bot]

Comment on attachment 266399 [details]
Patch

Attachment 266399 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/503874

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266404 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266399 [details]
Patch

Attachment 266399 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/503891

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266405 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266399 [details]
Patch

Attachment 266399 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/503933

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 266407 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Dave Hyatt]

Created attachment 266457 [details]
Patch

[Dave Hyatt]

Landed in r192953. This is just basic support. Dynamic viewport changes need to be handled still (and the double downloads issue needs to be addressed).

[Ryan Haddad]

Do we need to rebaseline imported/w3c/web-platform-tests/html/dom/interfaces.html as a result of this change?

<https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r192953%20(1577)/results.html>

[Ryan Haddad]

Rebaseline committed in r192962.

[Csaba Osztrogonác]

(In reply to comment #46)
> Landed in r192953. This is just basic support. Dynamic viewport changes need
> to be handled still (and the double downloads issue needs to be addressed).

It broke the !ENABLE(VIDEO) build, please fix it.

[Csaba Osztrogonác]

build log:

../../Source/WebCore/html/HTMLSourceElement.cpp: In member function ‘virtual WebCore::Node::InsertionNotificationRequest WebCore::HTMLSourceElement::insertedInto(WebCore::ContainerNode&)’:
../../Source/WebCore/html/HTMLSourceElement.cpp:63:48: error: invalid use of incomplete type ‘WTF::match_constness<WebCore::Element, WebCore::HTMLMediaElement>::type {aka class WebCore::HTMLMediaElement}’
             downcast<HTMLMediaElement>(*parent).sourceWasAdded(this);
                                                ^
In file included from ../../Source/WebCore/dom/Element.h:29:0,
                 from ../../Source/WebCore/dom/StyledElement.h:31,
                 from ../../Source/WebCore/html/HTMLElement.h:26,
                 from ../../Source/WebCore/html/HTMLSourceElement.h:29,
                 from ../../Source/WebCore/html/HTMLSourceElement.cpp:27:
../../Source/WebCore/dom/Document.h:117:7: error: forward declaration of ‘WTF::match_constness<WebCore::Element, WebCore::HTMLMediaElement>::type {aka class WebCore::HTMLMediaElement}’
 class HTMLMediaElement;
       ^
../../Source/WebCore/html/HTMLSourceElement.cpp: In member function ‘virtual void WebCore::HTMLSourceElement::removedFrom(WebCore::ContainerNode&)’:
../../Source/WebCore/html/HTMLSourceElement.cpp:77:48: error: invalid use of incomplete type ‘WTF::match_constness<WebCore::Element, WebCore::HTMLMediaElement>::type {aka class WebCore::HTMLMediaElement}’
             downcast<HTMLMediaElement>(*parent).sourceWasRemoved(this);
                                                ^
In file included from ../../Source/WebCore/dom/Element.h:29:0,
                 from ../../Source/WebCore/dom/StyledElement.h:31,
                 from ../../Source/WebCore/html/HTMLElement.h:26,
                 from ../../Source/WebCore/html/HTMLSourceElement.h:29,
                 from ../../Source/WebCore/html/HTMLSourceElement.cpp:27:
../../Source/WebCore/dom/Document.h:117:7: error: forward declaration of ‘WTF::match_constness<WebCore::Element, WebCore::HTMLMediaElement>::type {aka class WebCore::HTMLMediaElement}’
 class HTMLMediaElement;
       ^
In file included from ../../Source/WebCore/css/CSSValue.h:29:0,
                 from ../../Source/WebCore/css/CSSPrimitiveValue.h:26,
                 from ../../Source/WebCore/dom/StyledElement.h:28,
                 from ../../Source/WebCore/html/HTMLElement.h:26,
                 from ../../Source/WebCore/html/HTMLSourceElement.h:29,
                 from ../../Source/WebCore/html/HTMLSourceElement.cpp:27:
../../Source/WTF/wtf/TypeCasts.h: In instantiation of ‘typename WTF::match_constness<Source, Target>::type& WTF::downcast(Source&) [with Target = WebCore::HTMLMediaElement; Source = WebCore::Element; typename WTF::match_constness<Source, Target>::type = WebCore::HTMLMediaElement]’:
../../Source/WebCore/html/HTMLSourceElement.cpp:63:47:   required from here
../../Source/WTF/wtf/TypeCasts.h:81:79: error: invalid static_cast from type ‘WebCore::Element’ to type ‘WTF::match_constness<WebCore::Element, WebCore::HTMLMediaElement>::type& {aka WebCore::HTMLMediaElement&}’
     return static_cast<typename match_constness<Source, Target>::type&>(source);
                                                                               ^
In file included from ../../Source/WebCore/css/CSSValue.h:29:0,
                 from ../../Source/WebCore/css/CSSPrimitiveValue.h:26,
                 from ../../Source/WebCore/dom/StyledElement.h:28,
                 from ../../Source/WebCore/html/HTMLElement.h:26,
                 from ../../Source/WebCore/html/HTMLSourceElement.h:29,
                 from ../../Source/WebCore/html/HTMLSourceElement.cpp:27:
../../Source/WTF/wtf/TypeCasts.h: In instantiation of ‘static bool WTF::TypeCastTraits<ExpectedType, ArgType, isBaseType>::isOfType(ArgType&) [with ExpectedType = const WebCore::HTMLMediaElement; ArgType = const WebCore::Element; bool isBaseType = false]’:
../../Source/WTF/wtf/TypeCasts.h:59:78:   required from ‘bool WTF::is(ArgType&) [with ExpectedType = WebCore::HTMLMediaElement; ArgType = WebCore::Element]’
../../Source/WebCore/html/HTMLSourceElement.cpp:62:41:   required from here
../../Source/WTF/wtf/TypeCasts.h:42:9: error: static assertion failed: Missing TypeCastTraits specialization
         static_assert(std::is_void<ExpectedType>::value, "Missing TypeCastTraits specialization");
         ^

[Csaba Osztrogonác]

Comment on attachment 266457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266457&action=review

> Source/WebCore/html/HTMLSourceElement.h:-29
> -#if ENABLE(VIDEO)

The problem can be removing this guard.

[Dave Hyatt]

(In reply to comment #51)
> Comment on attachment 266457 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266457&action=review
> 
> > Source/WebCore/html/HTMLSourceElement.h:-29
> > -#if ENABLE(VIDEO)
> 
> The problem can be removing this guard.

Yeah, I need to make the guards more precise now within HTMLSourceElement.cpp. Let me fix that up.

[Olivier Blin]

I have tracked the !ENABLE(VIDEO) build issue in bug 152431

[Csaba Osztrogonác]

(In reply to comment #46)
> Landed in r192953. This is just basic support. Dynamic viewport changes need
> to be handled still (and the double downloads issue needs to be addressed).

The commit log was repeated 47 times.  :(  Dave, could you check your script?

[Alexey Proskuryakov]

Comment on attachment 266457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266457&action=review

> Source/WebCore/html/HTMLImageElement.idl:-38
> -#if ENABLE_CURRENTSRC
>      readonly attribute DOMString currentSrc;
> -#endif

This is surprising. What does ENABLE_CURRENTSRC guard now? Should it be removed?