[webkit-reviews] review denied: [Bug 129749] Revise Out-of-band VTT support for better integration with AVFoundation engine : [Attachment 225896] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 129749: Revise Out-of-band VTT support for better integration with
AVFoundation engine
https://bugs.webkit.org/show_bug.cgi?id=129749

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

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


> Source/WebCore/html/HTMLMediaElement.cpp:1603
> +    if (track->trackType() == TextTrack::TrackElement) {
> +	   if (m_player)

Nit: this would be slightly simpler with only one "if": if (track->trackType()
== TextTrack::TrackElement && m_player)

> Source/WebCore/html/HTMLMediaElement.cpp:5574
> +	   int uniqueId = track ? track->uniqueId() : 0;

HTMLTrackElement::track will never return NULL (yes, it should return a
reference instead of a pointer), so both the local comparison and variable are
unnecessary.

> Source/WebCore/html/HTMLMediaElement.cpp:5577
> +	   if (trackElement.track()) {

Not needed.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1265
> +void MediaPlayer::outOfBandTrackModeChanged(TextTrack* track)

Nit: I don't think this method should have "out of band" in its name.  

A much bigger problem is that this is a layering violation - TextTrack is in
/html. Luckily I don't think you need to pass the track, the media engine can
just call outOfBandTrackSources to get current track configurations.

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:66
>      virtual bool isLegacyClosedCaptionsTrack() const = 0;
> -
> +    virtual bool isOutOfBandTextTrack() const = 0;

Nit: these are mutually exclusive, so you could use a type enum instead.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:566
> +    RetainPtr<CFStringRef> uniqueID =
String::number(track->uniqueId()).createCFString();
> +    const AtomicString& mode = track->mode();
> +    
> +    for (auto& textTrack : m_textTracks) {
> +	   if (!textTrack->isOutOfBandTextTrack())
> +	       continue;
> +	   
> +	   RefPtr<OutOfBandTextTrackPrivateAVF> track =
static_cast<OutOfBandTextTrackPrivateAVF*>(textTrack.get());
> +	   RetainPtr<AVMediaSelectionOptionType> currentOption =
track->mediaSelectionOption();
> +	   
> +	   if ([[currentOption.get() outOfBandIdentifier] isEqual:
reinterpret_cast<const NSString*>(uniqueID.get())]) {
> +	       if (mode == TextTrack::disabledKeyword())
> +		   textTrack->setMode(InbandTextTrackPrivate::Disabled);
> +	       else if (mode == TextTrack::hiddenKeyword())
> +		   textTrack->setMode(InbandTextTrackPrivate::Hidden);
> +	       else if (mode == TextTrack::showingKeyword())
> +		   textTrack->setMode(InbandTextTrackPrivate::Showing);
> +	       else
> +		   ASSERT_NOT_REACHED();
> +	       
> +	       break;
> +	   }
> +    }

This logic needs to be reworked because you don't have access to the TextTrack
at this level.

>
Source/WebCore/platform/graphics/avfoundation/objc/OutOfBandTextTrackPrivateAVF
.h:45
> +    void processCue(CFArrayRef, double) { }
> +    void resetCueValues() { }

Shouldn't you make these virtual in the base class and "override" them here?

>
Source/WebCore/platform/graphics/avfoundation/objc/OutOfBandTextTrackPrivateAVF
.h:59
> +    void processCueAttributes(CFAttributedStringRef, GenericCueData*) { }

Why is this necessary?