Bug 71054 - Loading track files and firing onload and onerror events on HTMLTrackElement
Summary: Loading track files and firing onload and onerror events on HTMLTrackElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anna Cavender
URL:
Keywords:
Depends on: 71125
Blocks: 63042
  Show dependency treegraph
 
Reported: 2011-10-27 12:46 PDT by Anna Cavender
Modified: 2011-10-31 09:36 PDT (History)
0 users

See Also:


Attachments
Patch (13.69 KB, patch)
2011-10-27 13:06 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch for landing (14.85 KB, patch)
2011-10-29 17:51 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
updating to ToT (14.88 KB, patch)
2011-10-29 19:39 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
updating to ToT (14.79 KB, patch)
2011-10-29 19:45 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch for landing (14.71 KB, patch)
2011-10-31 07:21 PDT, Anna Cavender
annacc: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2011-10-27 12:46:07 PDT
Implement load notifications.
Comment 1 Anna Cavender 2011-10-27 13:06:29 PDT
Created attachment 112736 [details]
Patch
Comment 2 Anna Cavender 2011-10-27 13:08:23 PDT
Comment on attachment 112736 [details]
Patch

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

> LayoutTests/media/track/track-load-error-readyState.html:10
> +        endTest();

These constant will change to CAPS (e.g. TextTrack.ERROR) after this patch lands: https://bugs.webkit.org/show_bug.cgi?id=70951
Comment 3 Eric Carlson 2011-10-28 06:45:43 PDT
Comment on attachment 112736 [details]
Patch

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.
Comment 4 Anna Cavender 2011-10-28 10:42:31 PDT
Comment on attachment 112736 [details]
Patch

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

Thanks Eric!

>> Source/WebCore/html/HTMLMediaElement.cpp:850
>> +}
> 
> Nit: It is probably worth including a FIXME here noting the further setup that will be required to make this work correctly.

Done.

>> Source/WebCore/html/HTMLTrackElement.cpp:64
>> +        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.

Done.  I added a FIXME in HTMLMediaElement::loadNextTextTrack() because text tracks should always be loaded asynchronously.

>> LayoutTests/media/track/track-load-error-readyState.html:6
>> +    {
> 
> 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.

Done.

>> 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='...'>".

Done.

>> 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.

Done.
Comment 5 Anna Cavender 2011-10-28 10:58:19 PDT
http://trac.webkit.org/changeset/98736
Comment 6 Anna Cavender 2011-10-29 17:51:07 PDT
Created attachment 112979 [details]
Patch for landing
Comment 7 Anna Cavender 2011-10-29 19:39:27 PDT
Created attachment 112981 [details]
updating to ToT

Looks like this patch just needed an update to apply, but I'll need another r+.  Thanks.
Comment 8 Anna Cavender 2011-10-29 19:45:58 PDT
Created attachment 112982 [details]
updating to ToT

One more try, let's see if this will apply.
Comment 9 Anna Cavender 2011-10-31 07:21:52 PDT
Created attachment 113047 [details]
Patch for landing
Comment 10 Anna Cavender 2011-10-31 09:36:19 PDT
Committed r98860: <http://trac.webkit.org/changeset/98860>