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..."
<rdar://problem/10464462>
Created attachment 119785 [details] Implemented the specifications from the standard Initial work, related tests shold be added
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.
Created attachment 120898 [details] Updated code, tests and spec in comments
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.
Created attachment 125964 [details] Updated patch for current trunk Solved issues from the previous review and changed significant parts within the code
(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.
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.
Created attachment 126850 [details] Updated patch and tests Fixed the comments mentioned previously and better documented parts in the code
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
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.
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.
Created attachment 127610 [details] Merged with trunk and corrected issues and tests The previous flaky test should be working now
Comment on attachment 127610 [details] Merged with trunk and corrected issues and tests Thanks Victor!
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>
All reviewed patches have been landed. Closing bug.