[webkit-reviews] review granted: [Bug 114629] [Mac] in-band cues sometimes displayed late : [Attachment 198178] Updated patch.

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 114629: [Mac] in-band cues sometimes displayed late
https://bugs.webkit.org/show_bug.cgi?id=114629

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

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=198178&action=review


> Source/WebCore/html/track/InbandTextTrack.cpp:240
> +void InbandTextTrack::updateGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData> prpCueData)

This should take a GenericCueData*, as you're not passing ownership of the
cueData parameter.

> Source/WebCore/html/track/InbandTextTrack.cpp:253
> +void InbandTextTrack::removeGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData> prpCueData)

Ditto.

> Source/WebCore/html/track/InbandTextTrack.cpp:263
> +void InbandTextTrack::removeCue(PassRefPtr<TextTrackCue> prpCue,
ExceptionCode& ec)

Ditto for TextTrackCue* and cue.

> Source/WebCore/html/track/InbandTextTrack.h:82
> +    virtual void addGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData>) OVERRIDE;
> +    virtual void updateGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData>) OVERRIDE;
> +    virtual void removeGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData>) OVERRIDE;
> +    virtual void removeCue(PassRefPtr<TextTrackCue>, ExceptionCode&)
OVERRIDE;

It makes sense to pass a PassRefPtr to addGenericCue, as you're passing
ownership of the parameter.  But updateGenericCue and removeGenericCue should
just take a raw GenericCueData* pointer. Same for removeCue() and
TextTrackCue*.

> Source/WebCore/html/track/TextTrackCue.cpp:-313
> -    cueWillChange();
>      m_pauseOnExit = value;
> -    cueDidChange();

Presumably you want to replace these with willChange() and didChange(), not
remove them entirely.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:134
> +    virtual void addGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData>) = 0;
> +    virtual void updateGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData>) = 0;
> +    virtual void removeGenericCue(InbandTextTrackPrivate*,
PassRefPtr<GenericCueData>) = 0;

Same here as for TextTrack.

>
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:388

> +	   client()->addGenericCue(this, cueData);

You should pass cueData.release() to addGenericCue() since it takes a
PassRefPtr.