Bug 72171 – <track>-related events cuechange, enter, and exit should be sorted and filtered before dispatching

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

See WHATWG spec under 4.8.10.8 Playing the media resource: http://www.whatwg.org/specs/web-apps/current-work/#playing-the-media-resource
 
Specifically, "When the current playback position of a media element changes..."

[Radar WebKit Bug Importer]

<rdar://problem/10464462>

[Victor Carbune]

Created attachment 119785 [details]
Implemented the specifications from the standard

Initial work, related tests shold be added

[Eric Carlson]

Comment on attachment 119785 [details]
Implemented the specifications from the standard

View in context: https://bugs.webkit.org/attachment.cgi?id=119785&action=review

Marking r- for the lack of tests.

> Source/WebCore/html/HTMLMediaElement.cpp:949
> +            return a.second->track()->cues()->getCueIndex(a.second) -
> +                    b.second->track()->cues()->getCueIndex(b.second) < 0;

Nit: I think the line break make this slightly harder to read.

> Source/WebCore/html/HTMLMediaElement.cpp:963
> +    return a->mediaElement()->textTracks()->getTrackIndex(a) -
> +            b->mediaElement()->textTracks()->getTrackIndex(b);

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:975
> +    m_currentlyVisibleCues =
> +        m_cueTree.allOverlaps(m_cueTree.createInterval(movieTime, movieTime));

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:978
> +
> +    if (m_lastTextTrackUpdate && !seeking) {

I think it can be extremely helpful to include spec text as comments. This makes it much easier to spot errors in the code logic (and to review).

> Source/WebCore/html/HTMLMediaElement.cpp:990
> +    // TODO(vcarbune): Step 5.

Please add the spec text for Step 5 as a comment. We typically use "FIXME" instead of "TODO" and include a bug number to track the fix.

> Source/WebCore/html/HTMLMediaElement.cpp:992
> +    bool shouldAbord = true;

Typo: I think you want "shouldAbort".

> Source/WebCore/html/HTMLMediaElement.cpp:995
> +        if (!m_currentlyVisibleCues.contains(previouslyVisibleCues[i])
> +            && previouslyVisibleCues[i].data()->isActive()) {

I think the line break makes this slightly more difficult to read.

> Source/WebCore/html/HTMLMediaElement.cpp:1013
> +    // TODO(vcarbune): Step 7.

"TODO" -> "FIXME" plus bug number.

[Victor Carbune]

Created attachment 120898 [details]
Updated code, tests and spec in comments

[Eric Carlson]

Comment on attachment 120898 [details]
Updated code, tests and spec in comments

View in context: https://bugs.webkit.org/attachment.cgi?id=120898&action=review

> Source/WebCore/ChangeLog:30
> +        * html/HTMLMediaElement.h: Added variables to keep
> +        state information required by the text track update algorithm (last time
> +        the algorithm was run, and whether a seeking event has occured)
> +        * html/TextTrackCue.cpp:

Nit: Many people but a blank line between non-associated files to make the changelog easier to read.

> Source/WebCore/ChangeLog:8212
> +>>>>>>> master

Left over from a bad merge?

> Source/WebCore/html/HTMLMediaElement.cpp:966
> +    // 12 - Sort the tasks in events in ascending time order (tasks with earlier
> +    // times first).
> +    if (a.first == b.first) {
> +        int aCueIndex = a.second->track()->cues()->getCueIndex(a.second);
> +        int bCueIndex = b.second->track()->cues()->getCueIndex(b.second);
> +
> +        if (a.second->track() == b.second->track()) {
> +            // 12 - Further sort tasks in events that have the same time by the
> +            // relative text track cue order of the text track cues associated
> +            // with these tasks.
> +            return aCueIndex - bCueIndex < 0;
> +        }
> +    }
> +
> +    return a.first - b.first < 0;

Nit: The comments don't quite follow the code logic. It might make more sense to return early if the times are not the same:

// 12 - Sort the tasks in events in ascending ...
if ((a.first != b.first)
    return a.first - b.first < 0;

// Further sort tasks in events ...
int aCueIndex = a.second->track()->cues()->getCueIndex(a.second);

> Source/WebCore/html/HTMLMediaElement.cpp:984
> +    Vector<CueIntervalTree::IntervalType> affectedCues;
> +    Vector<CueIntervalTree::IntervalType> previouslyVisibleCues;
> +    Vector<CueIntervalTree::IntervalType> missedCues;

Nit: an early return when m_cueTree is empty would prevent a lot of unnecessary setup churn.

> Source/WebCore/html/HTMLMediaElement.cpp:986
> +    // 2 - other cues

Why is the first comment #2?

> Source/WebCore/html/HTMLMediaElement.cpp:990
> +    // 1 - current cues
>      m_currentlyActiveCues = m_cueTree.allOverlaps(m_cueTree.createInterval(movieTime, movieTime));

When I suggested including comments from the spec, I meant to literally include spec text as we have in other parts of this file. Eg:

// 4.8.10.8 Playing the media resource
// 1. Let current cues be a list of cues, initialized to contain all the cues of all the hidden, showing,
// or showing by default text tracks of the media element (not the disabled ones) whose start times are
// less than or equal to the current playback position and whose end times are greater than the current
// playback position.
m_currentlyActiveCues = m_cueTree.allOverlaps(m_cueTree.createInterval(movieTime, movieTime));

etc.

> Source/WebCore/html/HTMLMediaElement.cpp:995
> +    if (m_lastTextTrackUpdate && !m_seekingBeforeTextTrackUpdate) {
> +        Vector<CueIntervalTree::IntervalType> potentiallySkippedCues =
> +            m_cueTree.allOverlaps(m_cueTree.createInterval(m_lastTextTrackUpdate, movieTime));

Nit: I had to read this several times before I figured out what it did, so I think "m_lastTextTrackUpdate" would be better as "m_lastTextTrackUpdateTime".

> Source/WebCore/html/HTMLMediaElement.cpp:999
> +            if (potentiallySkippedCues[i].low() > m_lastTextTrackUpdate
> +                && potentiallySkippedCues[i].high() < movieTime)

Nit: I think the line break makes this much harder to read.

> Source/WebCore/html/HTMLMediaElement.cpp:1017
> +    bool activeSetChanged = false;

If any cues were skipped you know that the active set must be updated, so initialize activeSetChanged to missedCues.size() so you won't check previous and currently visible cues.

> Source/WebCore/html/HTMLMediaElement.cpp:1018
> +    for (size_t i = 0; i < previouslyVisibleCues.size() && !activeSetChanged; ++i) {

Nit: 1) If !activeSetChanged is checked first, you won't have to check the queue size once it is true. 2) The queue size can't change in this loop so you should put the size into a local variable instead of fetching it from the queue every time.

> Source/WebCore/html/HTMLMediaElement.cpp:1020
> +        if (!m_currentlyActiveCues.contains(previouslyVisibleCues[i])
> +            && previouslyVisibleCues[i].data()->isActive())

Nit: I think the line break makes this much harder to read.

> Source/WebCore/html/HTMLMediaElement.cpp:1024
> +    for (size_t i = 0; i < m_currentlyActiveCues.size() && !activeSetChanged; ++i) {

Nit: As above.

> Source/WebCore/html/HTMLMediaElement.cpp:1077
> +    // 12 - Sort the events before dispatching.

12. Sort the tasks in events in ascending time order (tasks with earlier times first).

> Source/WebCore/html/HTMLMediaElement.cpp:1088
> +        // 13 - Queue each task in events, in list order (we can already dispatch).
> +        if (eventTasks[i].first == eventTasks[i].second->startTime())
> +            eventTasks[i].second->setIsActive(true);
> +        else
> +            eventTasks[i].second->setIsActive(false);

"Queue each task in events ..." means the events should be fired asynchronously, but TextTrackCue::setIsActive fires the event immediately.

> Source/WebCore/html/HTMLMediaElement.cpp:1100
> +    for (size_t i = 0; i < affectedTracks.size(); ++i)
> +        affectedTracks[i]->fireCueChangeEvent();

"queue a task to fire a simple event" means the event is fired asynchronously, but TextTrack:: fireCueChangeEvent fires the event immediately.

> Source/WebCore/html/HTMLMediaElement.cpp:1717
> +    m_seekingBeforeTextTrackUpdate = true;

Nit: This might be clearer if named something like "m_seekedSinceLastTextTrackUpdate".

> Source/WebCore/html/HTMLMediaElement.h:564
> +    typedef std::pair <double, TextTrackCue*> EventTimeCuePair;

This typedef isn't used in the header so you should define in the .cpp file where it is used.

> Source/WebCore/html/HTMLMediaElement.h:566
> +    typedef PODIntervalTree <double, TextTrackCue*> CueIntervalTree;

Either this diff is missing something, or this is a re-definition of the CueIntervalTree typedef.

> Source/WebCore/html/HTMLMediaElement.h:568
> -    CueList m_currentlyActiveCues;
> +    Vector<CueIntervalTree::IntervalType> m_currentlyActiveCues;

Is there a reason to not use the CueList type?

> Source/WebCore/html/TextTrackCueList.cpp:51
> +unsigned long TextTrackCueList::getCueIndex(TextTrackCue* cue) const
> +{
> +    for (unsigned long index = 0; index < m_list.size(); ++index) {
> +        if (m_list[index].get() == cue)
> +            return index;
> +    }
> +
> +    return 0;
> +}

Could this use Vector::find? 

Actually, a cue's index will change *much* less infrequently than this function will be called so I wonder if it would be better to have a cue store its index. You could invalidate a cue's stored index whenever a new cue is inserted into the list, and have a cue look up the index only when stored index is invalid.

> Source/WebCore/html/track/TextTrackList.cpp:66
> +unsigned TextTrackList::getTrackIndex(TextTrack *textTrack)
> +{
> +    for (unsigned index = 0; index < m_elementTracks.size(); ++index) {
> +        if (m_elementTracks[index].get() == textTrack)
> +            return index;
> +    }
> +
> +    return 0;
> +}

Could this use Vector::find?

> LayoutTests/media/track/track-cues-missed-expected.txt:13
> +Tests that events are triggered for missed (skipped) cues during normal playback.
> +
> +EVENT(canplaythrough)
> +EXPECTED (testTrack.track.cues.length == '3') OK
> +RUN(video.play())
> +EVENT(enter)
> +EXPECTED ([object TextTrackCue] == '[object TextTrackCue]') OK
> +EXPECTED (currentCue.id == '3') OK
> +EVENT(exit)
> +EXPECTED ([object TextTrackCue] == '[object TextTrackCue]') OK
> +EXPECTED (currentCue.id == '3') OK
> +EVENT(ended)
> +END OF TEST

I would rather that this check more than one cue because it is possible, if unlikely, that a single cue will not be skipped.

> LayoutTests/media/track/track-cues-missed.html:16
> +            function runTests() {
> +                if (!trackLoaded || !videoCanPlayThrough)

WebKit style is for a function's opening brace to be on a new line.

> LayoutTests/media/track/track-cues-missed.html:47
> +            waitForEvent('ended', function() {
> +                endTest();
> +            });

Nit: Why define a new function to call a single function?  "waitForEvent('ended', endTest);"

> LayoutTests/media/track/track-cues-sorted-before-dispatch-expected.txt:9
> +Cue event: enter id: 1 time: 5.1
> +Cue event: enter id: 3 time: 5.1
> +Cue event: enter id: 2 time: 5.1

Shouldn't these file in ID order because of "Further sort tasks in events that have the same time by the relative text track cue order of the text track cues associated with these tasks"?

> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:17
> +            function runTests() {

As above, the opening brace goes on a new line.

> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:27
> +                for (var i = 0; i < testTrack.track.cues.length; ++i) {
> +                    testTrack.track.cues[i].addEventListener('enter', cueEntered);
> +                    testTrack.track.cues[i].addEventListener('exit', cueExited);
> +                }

Is there an advantage to registering a listeners on each cue instead of on the <track>?

> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:52
> +            function cueEntered() {
> +                currentCue = event.target;
> +
> +                var eventObj = {};
> +                eventObj.time = currentCue.startTime;
> +                eventObj.type = "enter";
> +                eventObj.cue = currentCue;
> +
> +                dispatchedEvents.push(eventObj);
> +            }
> +
> +            function cueExited() {
> +                currentCue = event.target;
> +
> +                var eventObj = {};
> +                eventObj.time = currentCue.endTime;
> +                eventObj.type = "exit";
> +                eventObj.cue = currentCue;
> +
> +                dispatchedEvents.push(eventObj);
> +            }

Nit: These functions are identical except for "eventObj.type = 'XXX'", but you could use "eventObj.type =  event.type" and have a single function.

[Victor Carbune]

Created attachment 125964 [details]
Updated patch for current trunk

Solved issues from the previous review and changed significant parts within the code

[Victor Carbune]

(In reply to comment #5)
> (From update of attachment 120898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120898&action=review
> 
> > Source/WebCore/ChangeLog:30
> > +        * html/HTMLMediaElement.h: Added variables to keep
> > +        state information required by the text track update algorithm (last time
> > +        the algorithm was run, and whether a seeking event has occured)
> > +        * html/TextTrackCue.cpp:
> 
> Nit: Many people but a blank line between non-associated files to make the changelog easier to read.
Done.
> > Source/WebCore/ChangeLog:8212
> > +>>>>>>> master
> 
> Left over from a bad merge?
Sorry for the mistakes of this kind - fixed.

> > Source/WebCore/html/HTMLMediaElement.cpp:966
> > +    // 12 - Sort the tasks in events in ascending time order (tasks with earlier
> > +    // times first).
> > +    if (a.first == b.first) {
> > +        int aCueIndex = a.second->track()->cues()->getCueIndex(a.second);
> > +        int bCueIndex = b.second->track()->cues()->getCueIndex(b.second);
> > +
> > +        if (a.second->track() == b.second->track()) {
> > +            // 12 - Further sort tasks in events that have the same time by the
> > +            // relative text track cue order of the text track cues associated
> > +            // with these tasks.
> > +            return aCueIndex - bCueIndex < 0;
> > +        }
> > +    }
> > +
> > +    return a.first - b.first < 0;
> 
> Nit: The comments don't quite follow the code logic. It might make more sense to return early if the times are not the same:
> 
> // 12 - Sort the tasks in events in ascending ...
> if ((a.first != b.first)
>     return a.first - b.first < 0;
> 
> // Further sort tasks in events ...
> int aCueIndex = a.second->track()->cues()->getCueIndex(a.second);
Done.

> > Source/WebCore/html/HTMLMediaElement.cpp:984
> > +    Vector<CueIntervalTree::IntervalType> affectedCues;
> > +    Vector<CueIntervalTree::IntervalType> previouslyVisibleCues;
> > +    Vector<CueIntervalTree::IntervalType> missedCues;
> 
> Nit: an early return when m_cueTree is empty would prevent a lot of unnecessary setup churn.
I forgot to change this, but will fix it in the iteration.

> > Source/WebCore/html/HTMLMediaElement.cpp:986
> > +    // 2 - other cues
> 
> Why is the first comment #2?
Refactored to have them correctly ordered.
 
> > Source/WebCore/html/HTMLMediaElement.cpp:990
> > +    // 1 - current cues
> >      m_currentlyActiveCues = m_cueTree.allOverlaps(m_cueTree.createInterval(movieTime, movieTime));
> 
> When I suggested including comments from the spec, I meant to literally include spec text as we have in other parts of this file. Eg:
> 
> // 4.8.10.8 Playing the media resource
> // 1. Let current cues be a list of cues, initialized to contain all the cues of all the hidden, showing,
> // or showing by default text tracks of the media element (not the disabled ones) whose start times are
> // less than or equal to the current playback position and whose end times are greater than the current
> // playback position.
> m_currentlyActiveCues = m_cueTree.allOverlaps(m_cueTree.createInterval(movieTime, movieTime));
> 
> etc.
Done 
> > Source/WebCore/html/HTMLMediaElement.cpp:995
> > +    if (m_lastTextTrackUpdate && !m_seekingBeforeTextTrackUpdate) {
> > +        Vector<CueIntervalTree::IntervalType> potentiallySkippedCues =
> > +            m_cueTree.allOverlaps(m_cueTree.createInterval(m_lastTextTrackUpdate, movieTime));
> 
> Nit: I had to read this several times before I figured out what it did, so I think "m_lastTextTrackUpdate" would be better as "m_lastTextTrackUpdateTime".
Done.
> > Source/WebCore/html/HTMLMediaElement.cpp:999
> > +            if (potentiallySkippedCues[i].low() > m_lastTextTrackUpdate
> > +                && potentiallySkippedCues[i].high() < movieTime)
> 
> Nit: I think the line break makes this much harder to read.
Done. 

> > Source/WebCore/html/HTMLMediaElement.cpp:1017
> > +    bool activeSetChanged = false;
> 
> If any cues were skipped you know that the active set must be updated, so initialize activeSetChanged to missedCues.size() so you won't check previous and currently visible cues.
Done, thanks.

> > Source/WebCore/html/HTMLMediaElement.cpp:1018
> > +    for (size_t i = 0; i < previouslyVisibleCues.size() && !activeSetChanged; ++i) {
> 
> Nit: 1) If !activeSetChanged is checked first, you won't have to check the queue size once it is true. 2) The queue size can't change in this loop so you should put the size into a local variable instead of fetching it from the queue every time.
Done for these lines.

> > Source/WebCore/html/HTMLMediaElement.cpp:1020
> > +        if (!m_currentlyActiveCues.contains(previouslyVisibleCues[i])
> > +            && previouslyVisibleCues[i].data()->isActive())
> 
> Nit: I think the line break makes this much harder to read.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1024
> > +    for (size_t i = 0; i < m_currentlyActiveCues.size() && !activeSetChanged; ++i) {
> 
> Nit: As above.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1077
> > +    // 12 - Sort the events before dispatching.
> 
> 12. Sort the tasks in events in ascending time order (tasks with earlier times first).
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1088
> > +        // 13 - Queue each task in events, in list order (we can already dispatch).
> > +        if (eventTasks[i].first == eventTasks[i].second->startTime())
> > +            eventTasks[i].second->setIsActive(true);
> > +        else
> > +            eventTasks[i].second->setIsActive(false);
> 
> "Queue each task in events ..." means the events should be fired asynchronously, but TextTrackCue::setIsActive fires the event immediately.
Fixed using the GenericEventQueue.
 
> > Source/WebCore/html/HTMLMediaElement.cpp:1100
> > +    for (size_t i = 0; i < affectedTracks.size(); ++i)
> > +        affectedTracks[i]->fireCueChangeEvent();
> 
> "queue a task to fire a simple event" means the event is fired asynchronously, but TextTrack:: fireCueChangeEvent fires the event immediately.
Fixed.

> > Source/WebCore/html/HTMLMediaElement.cpp:1717
> > +    m_seekingBeforeTextTrackUpdate = true;
> 
> Nit: This might be clearer if named something like "m_seekedSinceLastTextTrackUpdate".
Done.
 
> > Source/WebCore/html/HTMLMediaElement.h:564
> > +    typedef std::pair <double, TextTrackCue*> EventTimeCuePair;
> 
> This typedef isn't used in the header so you should define in the .cpp file where it is used.
Removed it as it was not bringing any significant value.
 
> > Source/WebCore/html/HTMLMediaElement.h:566
> > +    typedef PODIntervalTree <double, TextTrackCue*> CueIntervalTree;
> 
> Either this diff is missing something, or this is a re-definition of the CueIntervalTree typedef.
Mistake, removed.

> > Source/WebCore/html/HTMLMediaElement.h:568
> > -    CueList m_currentlyActiveCues;
> > +    Vector<CueIntervalTree::IntervalType> m_currentlyActiveCues;
> 
> Is there a reason to not use the CueList type?
No, fixed back.
 
> > Source/WebCore/html/TextTrackCueList.cpp:51
> > +unsigned long TextTrackCueList::getCueIndex(TextTrackCue* cue) const
> > +{
> > +    for (unsigned long index = 0; index < m_list.size(); ++index) {
> > +        if (m_list[index].get() == cue)
> > +            return index;
> > +    }
> > +
> > +    return 0;
> > +}
> 
> Could this use Vector::find? 
> 
> Actually, a cue's index will change *much* less infrequently than this function will be called so I wonder if it would be better to have a cue store its index. You could invalidate a cue's stored index whenever a new cue is inserted into the list, and have a cue look up the index only when stored index is invalid.
Done.
 
> > Source/WebCore/html/track/TextTrackList.cpp:66
> > +unsigned TextTrackList::getTrackIndex(TextTrack *textTrack)
> > +{
> > +    for (unsigned index = 0; index < m_elementTracks.size(); ++index) {
> > +        if (m_elementTracks[index].get() == textTrack)
> > +            return index;
> > +    }
> > +
> > +    return 0;
> > +}
> 
> Could this use Vector::find?
Done.
 
> > LayoutTests/media/track/track-cues-missed-expected.txt:13
> > +Tests that events are triggered for missed (skipped) cues during normal playback.
> > +
> > +EVENT(canplaythrough)
> > +EXPECTED (testTrack.track.cues.length == '3') OK
> > +RUN(video.play())
> > +EVENT(enter)
> > +EXPECTED ([object TextTrackCue] == '[object TextTrackCue]') OK
> > +EXPECTED (currentCue.id == '3') OK
> > +EVENT(exit)
> > +EXPECTED ([object TextTrackCue] == '[object TextTrackCue]') OK
> > +EXPECTED (currentCue.id == '3') OK
> > +EVENT(ended)
> > +END OF TEST
> 
> I would rather that this check more than one cue because it is possible, if unlikely, that a single cue will not be skipped.
Added several more possible missed cues.
 
> > LayoutTests/media/track/track-cues-missed.html:16
> > +            function runTests() {
> > +                if (!trackLoaded || !videoCanPlayThrough)
> 
> WebKit style is for a function's opening brace to be on a new line.
Sorry, fixed.

> > LayoutTests/media/track/track-cues-missed.html:47
> > +            waitForEvent('ended', function() {
> > +                endTest();
> > +            });
> 
> Nit: Why define a new function to call a single function?  "waitForEvent('ended', endTest);"
Fixed.

> > LayoutTests/media/track/track-cues-sorted-before-dispatch-expected.txt:9
> > +Cue event: enter id: 1 time: 5.1
> > +Cue event: enter id: 3 time: 5.1
> > +Cue event: enter id: 2 time: 5.1
> 
> Shouldn't these file in ID order because of "Further sort tasks in events that have the same time by the relative text track cue order of the text track cues associated with these tasks"?
Not quite. This is about "relative text track cue order", which is directly related to the startTimes and endTimes. In our case, 3 and 2 have the same startTime, but endTime(3) > endTime(2), hence the ordering.

> > LayoutTests/media/track/track-cues-sorted-before-dispatch.html:17
> > +            function runTests() {
> 
> As above, the opening brace goes on a new line.
Fixed.

> > LayoutTests/media/track/track-cues-sorted-before-dispatch.html:27
> > +                for (var i = 0; i < testTrack.track.cues.length; ++i) {
> > +                    testTrack.track.cues[i].addEventListener('enter', cueEntered);
> > +                    testTrack.track.cues[i].addEventListener('exit', cueExited);
> > +                }
> 
> Is there an advantage to registering a listeners on each cue instead of on the <track>?
I *think* enter / exit events are dispatched only on individual cues? Maybe I mis-read the spec. 

> > LayoutTests/media/track/track-cues-sorted-before-dispatch.html:52
> > +            function cueEntered() {
> > +                currentCue = event.target;
> > +
> > +                var eventObj = {};
> > +                eventObj.time = currentCue.startTime;
> > +                eventObj.type = "enter";
> > +                eventObj.cue = currentCue;
> > +
> > +                dispatchedEvents.push(eventObj);
> > +            }
> > +
> > +            function cueExited() {
> > +                currentCue = event.target;
> > +
> > +                var eventObj = {};
> > +                eventObj.time = currentCue.endTime;
> > +                eventObj.type = "exit";
> > +                eventObj.cue = currentCue;
> > +
> > +                dispatchedEvents.push(eventObj);
> > +            }
> 
> Nit: These functions are identical except for "eventObj.type = 'XXX'", but you could use "eventObj.type =  event.type" and have a single function.
Fixed, thanks.

[Eric Carlson]

Comment on attachment 125964 [details]
Updated patch for current trunk

View in context: https://bugs.webkit.org/attachment.cgi?id=125964&action=review

This is close, but I am marking it r- for now because there are enough small things that need to be changed before it lands.

> Source/WebCore/html/HTMLMediaElement.cpp:949
> +    return aTrackIndex - bTrackIndex;

Won't this return true unless they have the same index? Do you mean "return aTrackIndex - bTrackIndex < 0"?

> Source/WebCore/html/HTMLMediaElement.cpp:960
> +    // Treat the special case when cues belong to different tracks.

More explanation in this comment about why it makes sense to special case would be nice: eg. something like "If the cues are from different tracks, sort by track order instead of cue index."

> Source/WebCore/html/HTMLMediaElement.cpp:1045
> +        if (!currentCues.contains(previousCues[i]) && previousCues[i].data()->isActive()) {
>              activeSetChanged = true;
>          }

WebKit style is not not use braces around a single line condition.

> Source/WebCore/html/HTMLMediaElement.cpp:1050
> +    for (size_t i = 0; !activeSetChanged && i < currentCuesSize; ++i)
> +        if (!currentCues[i].data()->isActive())
> +            activeSetChanged = true;

On the other hand, this loop should have braces because it contains two lines (even though it is just a single conditional).

> Source/WebCore/html/HTMLMediaElement.cpp:1054
> +    if (!activeSetChanged) {
> +        return;
>      }

Braces are not necessary here either.

> Source/WebCore/html/HTMLMediaElement.cpp:1115
> +        if (eventTasks[i].first == eventTasks[i].second->startTime())
> +            event = Event::create(eventNames().enterEvent, false, false);
> +        else
> +            event = Event::create(eventNames().exitEvent, false, false);

As you know I had to scratch my head for a bit to figure out how this works, so a brief comment here would be helpful.

> Source/WebCore/html/HTMLMediaElement.cpp:1131
> +        RefPtr<Event> event =
> +                Event::create(eventNames().cuechangeEvent, false, false);

Nit: I don't think the line break here helps.

> Source/WebCore/html/HTMLMediaElement.cpp:1136
> +        m_asyncEventQueue.enqueueEvent(event.release());
> +    }
> +

What about the 'cuechange' event that should be fired synchronously at the <track> element?

> Source/WebCore/html/LoadableTextTrack.cpp:132
> +        ExceptionCode ec = 0;
> +        bool result2 = m_trackElement->dispatchEvent(Event::create(eventNames().cuechangeEvent, false, false), ec);

I think this should be in <track> event in HTMLMediaElement::updateActiveTextTrackCues

> Source/WebCore/html/TextTrackCue.cpp:96
> +    , m_validCueIndex(false)

Instead of using a separate bool to track the validity of the index, you could define a constant invalid index (eg. -1) instead.

> Source/WebCore/html/TextTrackCue.cpp:332
> +    if (!m_validCueIndex)
> +        m_cueIndex = track()->cues()->getCueIndex(this);

"if (m_cueIndex != invalidCueIndex) ..."

> Source/WebCore/html/TextTrackCue.cpp:340
> +    m_validCueIndex = false;

"m_cueIndex = invalidCueIndex;"

> Source/WebCore/html/TextTrackCue.cpp:358
> +    LOG(Media, "TextTrackCue::Dispatching event...");

I don't think it makes sense to log from a function that will be called as often as this as it adds so much noise to the log stream. If you think it is useful to have some of the time, why don't you do something like we do for events in HTMLMediaElement (look for "#ifndef LOG_MEDIA_EVENTS").

> Source/WebCore/html/TextTrackCue.cpp:364
> +    LOG(Media, "TextTrackCue::Calling superclass dispatch...");

Ditto.

> Source/WebCore/html/TextTrackCue.cpp:370
> +    LOG(Media, "TextTrackCue::Calling own event dispatch with exceptions...");

Ditto.

> Source/WebCore/html/track/TextTrackList.cpp:61
> +unsigned TextTrackList::getTrackIndex(TextTrack *textTrack)
> +{
> +    return m_elementTracks.find(textTrack);
> +}

This function will be called frequently, so I think it would be good to use the same approach you use with cues and have the TextTrack cache its index the first time is it accessed.

> LayoutTests/media/track/captions-webvtt/sorted-dispatch.vtt:6
> +00:00:05.100 --> 00:00:05.800 A:start T:20%
> +Bear is Coming!!!!!

Nit: it wouldn't break my heart if you used something other than "Bear is Coming!!!" text ;-)

> LayoutTests/media/track/track-cues-missed.html:26
> +                    testTrack.track.cues[cueCount].addEventListener('enter', cueEnteredOrExited);
> +                    testTrack.track.cues[cueCount].addEventListener('exit', cueEnteredOrExited);

Nit: putting "testTrack.track.cues[cueCount]" into a local variable would speed this up slightly.

> LayoutTests/media/track/track-cues-missed.html:38
> +                testExpected(currentCue, testTrack.track.cues.getCueById(cueCount));

Nit: the result logged would be clearer if you quote the first argument, e.g.

    testExpected("testTrack.track.cues.getCueById(cueCount)", currentCue)

will produce 

    EXPECTED (testTrack.track.cues.getCueById(cueCount) == '[object TextTrackCue]') OK

instead of 

    EXPECTED ([object TextTrackCue] == '[object TextTrackCue]') OK

> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:57
> +                for (var i = 0; i < dispatchedEvents.length; ++i) {
> +                    consoleWrite("Cue event: " + dispatchedEvents[i].type +
> +                                 " id: " + dispatchedEvents[i].cue.id +
> +                                 " time: " + dispatchedEvents[i].time);
> +                }
> +
> +                endTest();
> +            });

Nit: this is complex and long enough that I think it would be better as a separate function.

[Victor Carbune]

Created attachment 126850 [details]
Updated patch and tests

Fixed the comments mentioned previously and better documented parts in the code

[WebKit Review Bot]

Comment on attachment 126850 [details]
Updated patch and tests

Attachment 126850 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11521027

New failing tests:
media/track/track-cues-sorted-before-dispatch.html

[Victor Carbune]

Comment on attachment 126850 [details]
Updated patch and tests

I'm still looking why exactly this test fails. Seems to be a corner-case situation I'm missing.

[Eric Carlson]

Comment on attachment 126850 [details]
Updated patch and tests

View in context: https://bugs.webkit.org/attachment.cgi?id=126850&action=review

Looks good with the minor suggestions noted, but I won't mark it cq+ because of the test failure.

> Source/WebCore/ChangeLog:848
> +>>>>>>> master

Is this left over from a bad merge?

> Source/WebCore/ChangeLog:9061
> +>>>>>>> master

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:1139
> +            static_cast<LoadableTextTrack*>(affectedTracks[i])->fireTrackElementCueChangeEvent();

I think it might be better if you left fireCueChangeEvent as a virtual method on the base class and ASSERT in the back class implementation instead. I don't have a really strong opinion about this, but please consider it.

> Source/WebCore/html/LoadableTextTrack.cpp:126
> +bool LoadableTextTrack::fireTrackElementCueChangeEvent()

Nit: I think the old name made more sense

> Source/WebCore/html/TextTrack.cpp:275
> +    if (m_trackIndex == -1)

I would rather see a named const used here instead of "-1".

> Source/WebCore/html/TextTrack.cpp:283
> +    m_trackIndex = -1;

Ditto.

> Source/WebCore/html/TextTrackCue.cpp:331
> +    if (m_cueIndex == -1)

Ditto.

> Source/WebCore/html/TextTrackCue.cpp:339
> +    m_cueIndex = -1;

Ditto.

> LayoutTests/ChangeLog:302
> +>>>>>>> master

??

> LayoutTests/ChangeLog:3899
> +>>>>>>> master

??

> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:62
> +            function logEndTest() {

This brace should go on the next line.

[Victor Carbune]

Created attachment 127610 [details]
Merged with trunk and corrected issues and tests

The previous flaky test should be working now

[Eric Carlson]

Comment on attachment 127610 [details]
Merged with trunk and corrected issues and tests

Thanks Victor!

[WebKit Review Bot]

Comment on attachment 127610 [details]
Merged with trunk and corrected issues and tests

Clearing flags on attachment: 127610

Committed r108241: <http://trac.webkit.org/changeset/108241>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.