[webkit-reviews] review granted: [Bug 72171] <track>-related events cuechange, enter, and exit should be sorted and filtered before dispatching : [Attachment 126850] Updated patch and tests

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted  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 126850: Updated patch and tests
https://bugs.webkit.org/attachment.cgi?id=126850&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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])->fireTrackElementCueChangeEv
ent();

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.