[webkit-reviews] review granted: [Bug 72179] JSC wrappers for TextTrack and TextTrackCue should not be collected during event dispatch or when owner is reachable : [Attachment 117985] Updated patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 72179: JSC wrappers for TextTrack and TextTrackCue should not be collected
during event dispatch or when owner is reachable
https://bugs.webkit.org/show_bug.cgi?id=72179

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=117985&action=review


r=me

> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:57
> +    return
visitor.containsOpaqueRoot(root(textTrackCue->track()->mediaElement()));

Slightly nicer to call "root(textTrackCue->track())", and put root(TextTrack*)
inline in a header. That way, if a text track ever changes its mind about what
its root is, we only have to update one function.

> Source/WebCore/bindings/js/JSTextTrackCustom.cpp:52
> +    return visitor.containsOpaqueRoot(root(textTrack->mediaElement()));

This would be "root(textTrack)", if you followed my advice above.

> Source/WebCore/html/track/TextTrackList.cpp:93
> +    scheduleAddTrackEvent(track);

Can this be "track.release()"?

> Source/WebCore/html/track/TextTrackList.cpp:96
> +void TextTrackList::remove(PassRefPtr<TextTrack> prpTrack)

Seems like prpTrack should just be a raw pointer, since we don't take
ownership.