Bug 136550 – Add TextTrack cueadd event

• Status: ASSIGNED
• Component: Media

[Brendan Long]

Add TextTrack cueadd event

[Brendan Long]

Created attachment 237650 [details]
Patch

[Brendan Long]

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.

[Brendan Long]

Created attachment 237651 [details]
Patch

Add changelog

[Brendan Long]

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.

[Brendan Long]

Created attachment 238544 [details]
Patch

[Brendan Long]

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

[Chris Dumez]

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?

[Brendan Long]

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.

[Eric Carlson]

This event obviously isn't in the spec yet, is there a bug you can reference?

[Brendan Long]

Created attachment 238605 [details]
Patch

Might as well try to fix the Windows version of this patch too..

[Brendan Long]

(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

[Eric Carlson]

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.

[Brendan Long]

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

[Brendan Long]

Created attachment 239135 [details]
Patch

[Brendan Long]

I'm having a lot of trouble getting Xcode builds working. This patch fixes everything else..

[Brendan Long]

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.

[Michael Catanzaro]

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.

[Ahmad Saleem]

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?