[webkit-reviews] review granted: [Bug 70451] Implement TextTrackCueList : [Attachment 113504] now with tests

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 70451: Implement TextTrackCueList
https://bugs.webkit.org/show_bug.cgi?id=70451

Attachment 113504: now with tests
https://bugs.webkit.org/attachment.cgi?id=113504&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=113504&action=review


> Source/WebCore/html/TextTrack.cpp:49
> +    m_cues = TextTrackCueList::create();

Can this be done when newCuesAvailable is called for the first time?

> Source/WebCore/html/TextTrack.cpp:103
>  PassRefPtr<TextTrackCueList> TextTrack::cues() const

Returning a PassRefPtr?

> Source/WebCore/html/TextTrack.cpp:107
> -    // FIXME(62885): Implement.
> +    if (m_mode != TextTrack::DISABLED)
> +	   return m_cues;
>      return 0;

I am a big fan of quoting the spec text and noting the spec section number to
make it easier to find the relevant section in the spec, and to make it easier
to figure out when the logic is out of sync with spec changes.

> Source/WebCore/html/TextTrackCueList.cpp:50
> +TextTrackCue* TextTrackCueList::getCueById(const String& id) const

This would be more efficient if the cue id and parameters were both
AtomicString so the == is a pointer comparison. This change could be made as a
follow up optimization.

> Source/WebCore/html/TextTrackCueList.cpp:86
> +	   || (newCue->startTime() == m_list[index]->startTime()
> +	   && newCue->endTime() > m_list[index]->endTime()))

Having these on the same line will make it easier to follow the logic.

> Source/WebCore/html/TextTrackCueList.cpp:89
> +	   add(newCue, start, index);
> +    else
> +	   add(newCue, index + 1, end);

newCue.release() will prevent reference count churn.

> Source/WebCore/html/TextTrackCueList.h:50
>      PassRefPtr<TextTrackCueList> activeCues();

PassRefPtr?

> LayoutTests/media/track/track-text-track-cue-list.html:12
> +		   cues = testTrack.track.cues;

Using "testTrack" as a global without looking it up works, but it is an old
crusty compat feature so please add a getElementById().