[Webkit-unassigned] [Bug 72171] <track>-related events cuechange, enter, and exit should be sorted and filtered before dispatching
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 9 15:08:07 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=72171
Eric Carlson <eric.carlson at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #125964|review? |review-
Flag| |
--- Comment #8 from Eric Carlson <eric.carlson at apple.com> 2012-02-09 15:08:07 PST ---
(From update of attachment 125964)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list