[webkit-reviews] review granted: [Bug 109822] Extending WebVTT Rendering with Regions : [Attachment 226754] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 109822: Extending WebVTT Rendering with Regions
https://bugs.webkit.org/show_bug.cgi?id=109822

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

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


> Source/WebCore/html/shadow/MediaControlElements.cpp:1301
> +	   TextTrackRegion* region =
cue->track()->regions()->getRegionById(regionId);

Both TextTrackCue::track and TextTrack::regions() can return NULL, either of
which will make this crash.

> Source/WebCore/html/track/TextTrackRegion.cpp:334
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString,
trackRegionCueContainerScrollingClass, ("scrolling",
AtomicString::ConstructFromLiteral));

Nit: you might as well go straight to NeverDestroyed<T>.

> Source/WebCore/html/track/TextTrackRegion.cpp:341
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString,
trackRegionCueContainerPseudoId,

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:349
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString,
trackRegionShadowPseudoId,

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:366
> +    LOG(Media, "TextTrackRegion::displayLastTextTrackCueBox");

Nit: won't this will be call really frequently? if so, you might not want to
log here.

> Source/WebCore/html/track/VTTCue.cpp:223
> +    displayTreeInternal()->remove(ASSERT_NO_EXCEPTION);

displayTreeInternal allocates the display tree if it is NULL, so you should
return "if (!hasDisplayTree())"

> Source/WebCore/html/track/VTTCue.cpp:747
> +    TextTrackRegion* region = track()->regions()->getRegionById(m_regionId);


TextTrackCue::track() and TextTrack::regions() can both return NULL