[webkit-reviews] review denied: [Bug 72171] <track>-related events cuechange, enter, and exit should be sorted and filtered before dispatching : [Attachment 125964] Updated patch for current trunk

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Victor Carbune
<victor at rosedu.org>'s request for review:
Bug 72171: <track>-related events cuechange, enter, and exit should be sorted
and filtered before dispatching
https://bugs.webkit.org/show_bug.cgi?id=72171

Attachment 125964: Updated patch for current trunk
https://bugs.webkit.org/attachment.cgi?id=125964&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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.