[webkit-reviews] review granted: [Bug 72554] Implement addCue and removeCue in TextTrack : [Attachment 115846] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 72554: Implement addCue and removeCue in TextTrack
https://bugs.webkit.org/show_bug.cgi?id=72554

Attachment 115846: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=115846&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=115846&action=review


> Source/WebCore/html/HTMLTrackElement.cpp:195
>  bool HTMLTrackElement::canLoadUrl(LoadableTextTrack*, const KURL& url)

The spelling Url here is quite strange.

> Source/WebCore/html/TextTrack.cpp:181
> +    TextTrack* cueTrack = cue.get()->track();

Should not need .get() here. If you do, the reason is probably that you are
using PassRefPtr instead of RefPtr for cue.

> Source/WebCore/html/TextTrack.cpp:189
> +    RefPtr<TextTrackCue> newCue = cue;

The normal style is to name the argument prpCue and the local variable just
"cue". And do this closer to the start of the function.

> Source/WebCore/html/TextTrack.cpp:194
> +    if (m_cues->contains(newCue.get())) {

Seems inefficient to separately search for the cue before adding it. Instead
the add function could also check if the cue is in the array. I’m a little
unclear about why it doesn’t.

> Source/WebCore/html/TextTrack.cpp:205
> +void TextTrack::removeCue(PassRefPtr<TextTrackCue> cue, ExceptionCode& ec)

A remove function should take a raw pointer, not a PassRefPtr.

> Source/WebCore/html/TextTrack.cpp:216
> +    if (cue.get()->track() != this) {

Should not need get() here.

> Source/WebCore/html/TextTrack.cpp:223
> +    if (!m_cues->contains(cue.get())) {

It seems inefficient to search for the cue twice, once to check if it’s
contained and a second time to actually remove it. It would be better if remove
just indicated failure.

> Source/WebCore/html/TextTrack.cpp:233
> +

Extra blank line here.