[webkit-reviews] review granted: [Bug 129156] Extend media support for WebVTT sources : [Attachment 224882] WIP1

[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 129156: Extend media support for WebVTT sources
https://bugs.webkit.org/show_bug.cgi?id=129156

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

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


r=me with the suggested changes.

> Source/WTF/wtf/Platform.h:1010
> +#define WTF_USE_AVF_CAPTIONS 1

Support for this depends on the system version, so it should be defined
conditionally in the various FeatureDefines.xcconfig files.

> Source/WebCore/html/HTMLMediaElement.cpp:5555
> +	   if (!trackElement.canLoadURL(url))
> +	       continue;

You should modify canLoadURL because it always fires a 'beforeload' event if
the url is valid, and we don't want to do that in this case.

> Source/WebCore/html/HTMLMediaElement.cpp:5563
> +	   // FIXME: Do we need to do any other filtering to avoid InBand or
Script-based content?

I don't think so, "childrenOfType<HTMLTrackElement>" will only return <track>
elements.

> Source/WebCore/html/HTMLMediaElement.cpp:5564
> +	   // FIXME: Where would we get the extended language tag e.g.,
"English" -> "en_US"?

Nothing more should be necessary, the srclan attribute is supposed to be a BCP
47 language.

> Source/WebCore/platform/graphics/PlatformOutOfBandTextTrackInfo.h:36
> +class PlatformOutOfBandTextTrackInfo : public
RefCounted<PlatformOutOfBandTextTrackInfo> {

Can you update and use PlatformTextTrack instead of creating this new, almost
identical, class?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:201
> +

Nit: extra blank line

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:568
> +		       reinterpret_cast<const NSString*>(language.get()),
AVOutOfBandAlternateTrackDisplayNameKey,

language -> track display name? Don't you want label instead?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:569
> +		       // FIXME: Where would we get the extended language?,
AVOutOfBandAlternateTrackExtendedLanguageTagKey,

I believe <track srclang> is the same as "extended language". AVF "language" is
an ISO 639-2/T code, srclang is supposed to be a BCP 47 language.