[Webkit-unassigned] [Bug 72171] <track>-related events cuechange, enter, and exit should be sorted and filtered before dispatching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72171





--- Comment #3 from Eric Carlson <eric.carlson at apple.com>  2011-12-18 16:35:57 PST ---
(From update of attachment 119785)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.