Bug 65013 – instanceof HTMLSourceElement Fails

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

Created attachment 101699 [details]
HTML source which demonstrates problem

For audio/video tag fallback, W3C recommends an onerror which, among other things, tests child nodes of the element to see if they are instanceof HTMLElementSource. This test works (on windows 7) in Firefox, Opera, and IE9; it fails in Safari and Chrome (Uncaught ReferenceError: HTMLSourceElement is not defined). HTML to demonstrate this problem is attached.

[Alexey Proskuryakov]

There is no HTMLSourceElement in DOMWindow.idl; there should be.

[Tom Zakrajsek]

Just to throw it out there, I think the fix to get this working is simple but there's a wrinkle.  The interface is statically conditional on video.  So if the build isn't built with video enabled, you'll still get this exception.

[Tom Zakrajsek]

(In reply to comment #2)
> Just to throw it out there, I think the fix to get this working is simple but there's a wrinkle.  The interface is statically conditional on video.  So if the build isn't built with video enabled, you'll still get this exception.

I was wrong.  The "source" tag (as an HTMLUnknownElement) handles it correctly by not executing the onerror method.  The continued exception was because the "run separate instance" link is calling instanceof HTMLSourceElement unconditionally. That's not ok.  I'll upload the code patch and test-results update shortly.

[Tom Zakrajsek]

Created attachment 104555 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 104555 [details]
Patch

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

> Source/WebCore/page/DOMWindow.idl:472
> +        attribute [Conditional=VIDEO] HTMLSourceElementConstructor HTMLSourceElement;

Should this be EnabledAtRuntime, like HTMLVideoElement?

[Ryosuke Niwa]

Comment on attachment 104555 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

There's no description was to why we're adding this property nor what it does.
We should definitely add such a description and refer to the relevant part of whatever spec that mandantes this behavior.
r- for the lack of documentation.

[Alexey Proskuryakov]

I don't think that this warrants a lot of discussion - constructors for all elements should be available on DOIMWindow. The missing EnabledAtRuntime should be almost certainly added though.

[Tom Zakrajsek]

I agree about the EnabledAtRuntime, and have re-built and verified the effect.  I was just sitting here toiling with how to describe the necessity for putting this is, other than to say "for consistency with all the other elements".  I guess that will be the note then.  Will upload the patch shortly.

[Tom Zakrajsek]

Created attachment 104589 [details]
Patch - per reviewer feedback

[Alexey Proskuryakov]

Comment on attachment 104589 [details]
Patch - per reviewer feedback

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

This patch is not marked for review.

> Source/WebCore/ChangeLog:4
> +        Add HTMLSourceElement to DOMWindow.idl for consistency.  Constructors for
> +        all elements should be available on DOMWindow.

This is not how we usually do this. Bug title goes above the bug URL, and explanations go to the lines generated by the script (like "* page/DOMWindow.idl:" line below), or a general overview could go above those lines.

[Alexey Proskuryakov]

Comment on attachment 104589 [details]
Patch - per reviewer feedback

Per IRC discussion, this was meant for review, and I think that this is fine to land.

[WebKit Review Bot]

Comment on attachment 104589 [details]
Patch - per reviewer feedback

Attachment 104589 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9438511

[Tom Zakrajsek]

Created attachment 104617 [details]
Patch

Fixed V8 bindings.

[WebKit Review Bot]

Comment on attachment 104617 [details]
Patch

Clearing flags on attachment: 104617

Committed r93482: <http://trac.webkit.org/changeset/93482>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 104617 [details]
Patch

This patch missed many affected tests!

[Alexey Proskuryakov]

Adam, Eric, do you know why the commit queue landed the patch if it didn't pass some tests?

/me recalls his unsuccessful argument that mega-tests dumping every property of the global object are evil, as maintaining them is a significant ongoing cost, and benefit is trivial.

[Adam Barth]

The commily tests chromium-linux.  Did any of the tests fail there?  I'd recommend setting up a Mac EWS bot if you'd like folks not to breaks tests on Mac.

[Adam Barth]

(I agree with Alexey that the tests that dump the global object are more work to maintain than they're worth.)

[Eric Seidel (no email)]

They should all just work from some snapshot of objects accessible from the global object. :)