[webkit-reviews] review granted: [Bug 71054] Loading track files and firing onload and onerror events on HTMLTrackElement : [Attachment 112736] Patch

Fri Oct 28 06:45:43 PDT 2011

Eric Carlson <eric.carlson at apple.com> has granted Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 71054: Loading track files and firing onload and onerror events on
HTMLTrackElement
https://bugs.webkit.org/show_bug.cgi?id=71054

Attachment 112736: Patch
https://bugs.webkit.org/attachment.cgi?id=112736&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112736&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:850
> +void HTMLMediaElement::loadNextTextTrack(HTMLTrackElement* track)
> +{
> +    track->load(ActiveDOMObject::scriptExecutionContext(), this);
> +}

Nit: It is probably worth including a FIXME here noting the further setup that
will be required to make this work correctly.

> Source/WebCore/html/HTMLTrackElement.cpp:64
> +    if (parent && parent->isMediaElement())
> +	   static_cast<HTMLMediaElement*>(parentNode())->trackWasAdded(this);

This callback will result in an immediate call to HTMLTrackElement::load. We
probably shouldn't trigger loading synchronously from here, but a FIXME to
investigate is probably fine for now.

> LayoutTests/media/track/track-load-error-readyState.html:6
> +<script src=../media-file.js></script>
> +<script src=../video-test.js></script>
> +<script>
> +
> +    function trackError()
> +    {

Nit: would rather that we use fully formed HTML files for media tests, eg.
include an HTML5 DOCTYPE, <html>, <head>, <body>, etc.	Obviously fragments
work just fine and there isn't an official WebKit convention, but I think the
structure makes files easier to understand.

> LayoutTests/media/track/track-load-from-element-readyState-expected.txt:1
> +Tests the load event on HTMLTrackElement for src="..." and LOADED readyState
on TextTrack.

Nit: this comment is confusing, I expected to it was literally "<track
src='...'>".

> LayoutTests/media/track/track-load-from-src-readyState-expected.txt:1
> +Tests the load event on HTMLTrackElement for src defined in JavaScript and
LOADED readyState on TextTrack.

Nit: the src attribute is *set* from JavaScript, not defined.