RESOLVED FIXED 71054
Loading track files and firing onload and onerror events on HTMLTrackElement 
https://bugs.webkit.org/show_bug.cgi?id=71054
Summary Loading track files and firing onload and onerror events on HTMLTrackElement
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
DetailsFormatted DiffDiff
Patch for landing (14.85 KB, patch)
2011-10-29 17:51 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
updating to ToT (14.88 KB, patch)
2011-10-29 19:39 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
updating to ToT (14.79 KB, patch)
2011-10-29 19:45 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
Patch for landing (14.71 KB, patch)
2011-10-31 07:21 PDT, Anna Cavender 		annacc: commit-queue+
DetailsFormatted DiffDiff
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.