Bug 71054

Summary:

Loading track files and firing onload and onerror events on HTMLTrackElement

Product:

WebKit

Reporter:

Anna Cavender <annacc>

Component:

Media

Assignee:

Anna Cavender <annacc>

Status:

RESOLVED FIXED

Severity:

Normal

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

71125

Bug Blocks:

63042

Attachments:

Description	Flags
Patch	none
Patch for landing	none
updating to ToT	none
updating to ToT	none
Patch for landing	annacc: commit-queue+

Anna Cavender

Reported 2011-10-27 12:46:07 PDT

Implement load notifications.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Anna Cavender

Comment 1 2011-10-27 13:06:29 PDT

Created attachment 112736 [details] Patch

Anna Cavender

Comment 2 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

Eric Carlson

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

Anna Cavender

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

Anna Cavender

Comment 5 2011-10-28 10:58:19 PDT

http://trac.webkit.org/changeset/98736

Anna Cavender

Comment 6 2011-10-29 17:51:07 PDT

Created attachment 112979 [details] Patch for landing

Anna Cavender

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

Anna Cavender

Comment 8 2011-10-29 19:45:58 PDT

Created attachment 112982 [details] updating to ToT One more try, let's see if this will apply.

Anna Cavender

Comment 9 2011-10-31 07:21:52 PDT

Created attachment 113047 [details] Patch for landing

Anna Cavender

Comment 10 2011-10-31 09:36:19 PDT

Committed r98860: <http://trac.webkit.org/changeset/98860>

Note You need to log in before you can comment on or make changes to this bug.