[webkit-reviews] review denied: [Bug 70451] Implement TextTrackCueList : [Attachment 111700] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Anna Cavender <annacc at chromium.org>'s
request for review:
Bug 70451: Implement TextTrackCueList
https://bugs.webkit.org/show_bug.cgi?id=70451

Attachment 111700: Patch
https://bugs.webkit.org/attachment.cgi?id=111700&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111700&action=review


> Source/WebCore/ChangeLog:9
> +	   Covered by existing tests:
> +	   Any test in media/track, especially track-text-track-cue-list.html

How do the tests change? Is it a progression?  It makes reviewing much easier
if the tests are included in the patch.

> Source/WebCore/html/TextTrackCueList.cpp:49
> +    for (Vector<RefPtr<TextTrackCue> >::const_iterator it = m_list.begin();
it != m_list.end(); it++) {
> +	   TextTrackCue* cue = it->get();
> +	   if (cue->id() == id)
> +	       return cue;
> +    }

We generally use index style iteration for Vectors instead of iterator style.

> Source/WebCore/html/TextTrackCueList.cpp:63
> +    for (Vector<RefPtr<TextTrackCue> >::const_iterator it = newCues.begin();
it != newCues.end(); it++)
> +	   add(*it);

Again, index style indexing preferred.

> Source/WebCore/html/TextTrackCueList.cpp:69
> +    RefPtr<TextTrackCue> newCue = cue;
> +    add(newCue, 0, m_list.size());

There doesn't seem a reason here to put cue in a RefPtr, just pass it along to
the other add() function as is.

> Source/WebCore/html/TextTrackCueList.cpp:74
> +void TextTrackCueList::add(PassRefPtr<TextTrackCue> cue, size_t start,
size_t end)
> +{
> +    // Maintain text track cue order:

It probably makes sense to sanity check start and end here in an ASSERT.

> Source/WebCore/html/TextTrackCueList.cpp:96
> +    if (m_list.contains(cue)) {
> +	   cue->setIsActive(false);
> +	   m_list.remove(m_list.find(cue));
> +    }

This looks like it does two searches of m_list.  Instead, you should use
find(), test for not found, and if found, use the index to remove it.

> Source/WebCore/html/TextTrackCueList.h:60
> +    Iterator begin();
> +    Iterator end();

These don't seem to be used.