Bug 116963 - (picture) Implement `picture` element
(picture)
: Implement `picture` element
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Dave Hyatt
http://www.w3.org/TR/html-picture-ele...
: InRadar
Depends on:
Blocks: 152431
  Show dependency treegraph
 
Reported: 2013-05-29 07:46 PDT by Anselm Hannemann
Modified: 2016-01-26 15:02 PST (History)
27 users (show)

See Also:


Attachments
Patch (first cut) (97.96 KB, patch)
2015-11-30 13:10 PST, Dave Hyatt
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.76 MB, application/zip)
2015-11-30 14:11 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.72 MB, application/zip)
2015-11-30 14:20 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.07 MB, application/zip)
2015-11-30 14:20 PST, Build Bot
no flags Details
Patch (377.61 KB, patch)
2015-12-01 09:58 PST, Dave Hyatt
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (854.54 KB, application/zip)
2015-12-01 10:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (889.28 KB, application/zip)
2015-12-01 10:58 PST, Build Bot
no flags Details
Patch (385.03 KB, patch)
2015-12-01 12:38 PST, Dave Hyatt
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Patch (384.31 KB, patch)
2015-12-01 13:01 PST, Dave Hyatt
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.27 MB, application/zip)
2015-12-01 13:29 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (898.54 KB, application/zip)
2015-12-01 13:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (898.26 KB, application/zip)
2015-12-01 13:56 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.08 MB, application/zip)
2015-12-01 14:00 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (970.67 KB, application/zip)
2015-12-01 14:09 PST, Build Bot
no flags Details
Patch (385.69 KB, patch)
2015-12-01 15:04 PST, Dave Hyatt
dino: review+
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.01 MB, application/zip)
2015-12-01 15:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (901.02 KB, application/zip)
2015-12-01 16:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (812.54 KB, application/zip)
2015-12-01 16:24 PST, Build Bot
no flags Details
Patch (536.37 KB, patch)
2015-12-02 11:32 PST, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anselm Hannemann 2013-05-29 07:46:41 PDT
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
Comment 1 Yoav Weiss 2014-01-11 05:49:23 PST
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
Comment 2 Radar WebKit Bug Importer 2015-02-04 09:07:55 PST
<rdar://problem/19715560>
Comment 3 erick 2015-06-20 23:17:19 PDT
what about  Safari 9 and picture element ?
it works perfectly with Chrome and Firefox  and it is necessary for responsive design
Comment 4 Dave Hyatt 2015-11-30 13:10:26 PST
Created attachment 266265 [details]
Patch (first cut)
Comment 5 WebKit Commit Bot 2015-11-30 13:11:32 PST
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.
Comment 6 Build Bot 2015-11-30 14:11:49 PST
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
Comment 7 Build Bot 2015-11-30 14:11:55 PST
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
Comment 8 Build Bot 2015-11-30 14:20:00 PST
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
Comment 9 Build Bot 2015-11-30 14:20:06 PST
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
Comment 10 Build Bot 2015-11-30 14:20:38 PST
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
Comment 11 Build Bot 2015-11-30 14:20:43 PST
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
Comment 12 Yoav Weiss 2015-11-30 23:58:54 PST
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.
Comment 13 Simon Pieters 2015-12-01 07:26:39 PST
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`).
Comment 14 Dave Hyatt 2015-12-01 09:58:53 PST
Created attachment 266357 [details]
Patch
Comment 15 Dave Hyatt 2015-12-01 09:59:44 PST
(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.
Comment 16 Darin Adler 2015-12-01 10:44:50 PST
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.
Comment 17 Build Bot 2015-12-01 10:55:26 PST
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
Comment 18 Build Bot 2015-12-01 10:55:32 PST
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
Comment 19 Build Bot 2015-12-01 10:58:47 PST
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
Comment 20 Build Bot 2015-12-01 10:58:53 PST
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
Comment 21 Dave Hyatt 2015-12-01 12:38:16 PST
Created attachment 266378 [details]
Patch

Incorporate Darin's suggested changes and do what Simon suggested by adding HTMLPictureElement.idl.
Comment 22 WebKit Commit Bot 2015-12-01 12:40:13 PST
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.
Comment 23 Dave Hyatt 2015-12-01 13:01:32 PST
Created attachment 266380 [details]
Patch
Comment 24 Simon Fraser (smfr) 2015-12-01 13:07:22 PST
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.
Comment 25 Dean Jackson 2015-12-01 13:08:05 PST
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?
Comment 26 Dean Jackson 2015-12-01 13:08:46 PST
r=me whenever you get it to build.
Comment 27 Build Bot 2015-12-01 13:29:21 PST
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
Comment 28 Build Bot 2015-12-01 13:29:34 PST
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
Comment 29 Build Bot 2015-12-01 13:33:42 PST
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
Comment 30 Build Bot 2015-12-01 13:33:48 PST
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
Comment 31 Build Bot 2015-12-01 13:56:08 PST
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
Comment 32 Build Bot 2015-12-01 13:56:13 PST
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
Comment 33 Build Bot 2015-12-01 14:00:26 PST
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
Comment 34 Build Bot 2015-12-01 14:00:31 PST
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
Comment 35 Build Bot 2015-12-01 14:08:58 PST
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
Comment 36 Build Bot 2015-12-01 14:09:02 PST
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
Comment 37 Dave Hyatt 2015-12-01 15:04:19 PST
Created attachment 266399 [details]
Patch
Comment 38 Dean Jackson 2015-12-01 15:08:16 PST
Comment on attachment 266399 [details]
Patch

r=me based on previous reviews, even though you didn't fix my minor nits in the ChangeLog :)
Comment 39 Build Bot 2015-12-01 15:59:52 PST
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
Comment 40 Build Bot 2015-12-01 15:59:59 PST
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
Comment 41 Build Bot 2015-12-01 16:01:56 PST
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
Comment 42 Build Bot 2015-12-01 16:02:02 PST
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
Comment 43 Build Bot 2015-12-01 16:24:37 PST
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
Comment 44 Build Bot 2015-12-01 16:24:43 PST
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
Comment 45 Dave Hyatt 2015-12-02 11:32:51 PST
Created attachment 266457 [details]
Patch
Comment 46 Dave Hyatt 2015-12-02 12:15:27 PST
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).
Comment 47 Ryan Haddad 2015-12-02 13:04:53 PST
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>
Comment 48 Ryan Haddad 2015-12-02 13:42:19 PST
Rebaseline committed in r192962.
Comment 49 Csaba Osztrogonác 2015-12-04 06:59:02 PST
(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.
Comment 50 Csaba Osztrogonác 2015-12-04 06:59:46 PST
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");
         ^
Comment 51 Csaba Osztrogonác 2015-12-04 07:03:21 PST
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.
Comment 52 Dave Hyatt 2015-12-09 13:44:48 PST
(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.
Comment 53 Olivier Blin 2015-12-18 11:22:16 PST
I have tracked the !ENABLE(VIDEO) build issue in bug 152431
Comment 54 Csaba Osztrogonác 2016-01-05 00:55:29 PST
(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?
Comment 55 Alexey Proskuryakov 2016-01-26 15:02:20 PST
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?