Add TextTrack cueadd event
Created attachment 237650 [details] Patch
This is an implementation based on my suggestion to the W3C: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25693 Right now, it's easy to look through cues for static .vtt files by waiting for the track to load, then listing them. Unfortunately, there's no reasonable way to do that for live cues, since they can be added at any point. You can see *some* of them when they fire with the cuechange event, but the cueadd event would let you see every cue, as soon as the media player is aware of them.
Created attachment 237651 [details] Patch Add changelog
Based on some feedback from Ian, I'm going to rewrite this to do one event per cue, which will incidentally make this patch much simpler.
Created attachment 238544 [details] Patch
The new patch sends a CueEvent for each cue. I'm not sure why the additions in JSDictionary are needed, and I can't figure out why toTextTrackCue() isn't in JSTextTrackCue.h. Also the test times out if I use run-webkit-test, but it works fine if I run it manually: TEST_RUNNER_TEST_PLUGIN_PATH= TEST_RUNNER_INJECTED_BUNDLE_FILENAME=WebKitBuild/Release/lib/libTestRunnerInjectedBundle.so WebKitBuild/Release/bin/WebKitTestRunner -v LayoutTests/media/track/text-track-oncueadd.html
Comment on attachment 238544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238544&action=review > Source/WebCore/bindings/js/JSDictionary.cpp:189 > + JSObject* object = asObject(value); Wouldn't "result = JSTextTrackCue::toWrapped(value);" work?
Created attachment 238604 [details] Patch Using JSTextTrackCue::toWrapped now. I feel like I tried this before and it didn't work, but maybe it was something different.
This event obviously isn't in the spec yet, is there a bug you can reference?
Created attachment 238605 [details] Patch Might as well try to fix the Windows version of this patch too..
(In reply to comment #9) > This event obviously isn't in the spec yet, is there a bug you can reference? Here it is: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25693
Comment on attachment 238605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238605&action=review > Source/WebCore/ChangeLog:13 > + * CMakeLists.txt: > + * DerivedSources.cpp: > + * bindings/js/JSDictionary.cpp: > + (WebCore::JSDictionary::convertValue): I know it is a pain but I really prefer to have per-method (or per-file) comments in the ChangeLog. That makes it much easier for someone to understand what changed when reading the ChangeLog later. > Source/WebCore/html/track/CueEvent.cpp:39 > + Nit: don't need this extra blank line. > LayoutTests/media/track/text-track-oncueadd.html:11 > + var i = 0; Nit: something like "cueNumber" would be clearer.
(In reply to comment #12) > (From update of attachment 238605 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238605&action=review > > > Source/WebCore/ChangeLog:13 > > + * CMakeLists.txt: > > + * DerivedSources.cpp: > > + * bindings/js/JSDictionary.cpp: > > + (WebCore::JSDictionary::convertValue): > > I know it is a pain but I really prefer to have per-method (or per-file) comments in the ChangeLog. That makes it much easier for someone to understand what changed when reading the ChangeLog later. > > > Source/WebCore/html/track/CueEvent.cpp:39 > > + > > Nit: don't need this extra blank line. > > > LayoutTests/media/track/text-track-oncueadd.html:11 > > + var i = 0; > > Nit: something like "cueNumber" would be clearer. At the moment I'm trying to fix the Mac build for this. I'll incorporate these suggestions too.
Created attachment 239135 [details] Patch
I'm having a lot of trouble getting Xcode builds working. This patch fixes everything else..
Eric - Do you think this event is useful, and if so, could you leave a comment on the W3C bug? I'm trying to prove that other people would like this.
Comment on attachment 239135 [details] Patch Hi, Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting. To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.
I can't find this here 'oncueadd': https://html.spec.whatwg.org/multipage/media.html#cue-events I checked all browsers: Blink: TextTrack.idl (from source.chromium.org) Gecko: TextTrack.idl (from searchfox.org) and none have 'oncueadd'. Is it something still needed?