[webkit-reviews] review denied: [Bug 70452] Implement storage and retrieval of TextTrackCues during video playback. : [Attachment 114304] uses PODIntervalTree as data structure

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 70452: Implement storage and retrieval of TextTrackCues during video
playback.
https://bugs.webkit.org/show_bug.cgi?id=70452

Attachment 114304: uses PODIntervalTree as data structure
https://bugs.webkit.org/attachment.cgi?id=114304&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=114304&action=review


Every non-DOM object that is an EventTarget needs code (for JSC at lease, I
don't know about V8) to make sure that the wrapper isn't collected during event
dispatch or while the parent/owner object is reachable. For example, see
JSTextTrackListOwner::isReachableFromOpaqueRoots and the
tracklist-is-reachable.html test added in
https://bugs.webkit.org/show_bug.cgi?id=71123. 

I think this is fine to do in a follow up patch, but please file a bug for the
fix (and I am happy to make the changes for JSC).

> Source/WebCore/html/HTMLMediaElement.cpp:852
> +void HTMLMediaElement::updateActiveTextTrackCues()
> +{
> +    float movieTime = currentTime();
> +

Nit: it would be slightly more efficient if this function took the current time
because one of the places it is called will have just fetched it.

> Source/WebCore/html/HTMLMediaElement.h:522
> +    typedef PODInterval<float, TextTrackCue*> CueInterval;
> +    PODIntervalTree<float, TextTrackCue*> m_cueTree;

TextTrackCue startTime and endTime times both doubles, why use float ?

> Source/WebCore/html/TextTrackCue.cpp:154
> +    if (active)
> +	   dispatchEvent(Event::create(eventNames().enterEvent, false, false),
ec);
> +    else
> +	   dispatchEvent(Event::create(eventNames().exitEvent, false, false),
ec);
> +

This is missing a number of the steps listed in the spec. All of the cue events
for a time should be put into a queue, sorted and filtered, and then fired
asynchronously.

Because the events have to fire in order across all of the tracks/cues, maybe
it make sense for the media element to own and maintain a queue of cue events?

> Source/WebCore/html/TextTrackCue.cpp:156
> +    if (m_track)
> +	   m_track->fireCueChangeEvent();

The 'cuechange' event must fire on both the TextTrack and <track> : 

    ... queue a task to fire a simple event named cuechange at the TextTrack
object, and, if the text track has a corresponding track element, to then fire
a simple event named cuechange at the track element as well ...

> LayoutTests/media/track/track-cues-cuechange.html:25
> +	   function cueChanged()
> +	   {
> +	       var cues = testTrack.track.cues;
> +	       
> +	       var currentCue = cues[Math.floor(cueChangeCount/2)];
> +

Nit: I think it would be helpful to log "EVENT(cuechange)" here so the results
make it obvious these checks happen in response to the event.

> LayoutTests/media/track/track-cues-cuechange.html:49
> +		   testTrack = document.getElementById("testTrack");
> +		   testExpected("testTrack.track.cues.length", 3);
> +		   testTrack.track.addEventListener('cuechange', cueChanged);

This should also verify that 'cuechange' is fired on the <track>.

> LayoutTests/media/track/track-cues-enter-exit.html:10
> +	   <p>Tests that TextTrack's cues are indexed and updated within 500ms
during video playback. Test uses the enter and exits events on
TextTrackCue.</p>

How did you decide on 500ms?

> LayoutTests/media/track/track-cues-enter-exit.html:22
> +	   function cueEntered()
> +	   {
> +	       var currentCue = testTrack.track.cues[cueCount];

Nit: I think it would be helpful to log "EVENT(enter)" here so the results make
it obvious these checks happen in response to the event.

> LayoutTests/media/track/track-cues-enter-exit.html:33
> +	   function cueExited()
> +	   {
> +	       var currentCue = testTrack.track.cues[cueCount];

Logging the event name would be good here as well.