[webkit-reviews] review granted: [Bug 122122] [Mac] Add support for VideoTrack to MediaPlayerPrivateAVFObjC : [Attachment 213033] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 122122: [Mac] Add support for VideoTrack to MediaPlayerPrivateAVFObjC
https://bugs.webkit.org/show_bug.cgi?id=122122

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

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


> Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:83

> +    else

Nit: the final "else" isn't needed.

> Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:98

> +    else

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:121
> +    else

Ditto.

> Source/WebCore/platform/graphics/avfoundation/VideoTrackPrivateAVF.h:35
> +class VideoTrackPrivateAVF : public VideoTrackPrivate {

FINAL

> Source/WebCore/platform/graphics/avfoundation/VideoTrackPrivateAVF.h:42
> +    virtual Kind kind() const { return m_kind; }
> +    virtual AtomicString id() const { return m_id; }
> +    virtual AtomicString label() const { return m_label; }
> +    virtual AtomicString language() const { return m_language; }

OVERRIDE

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1134
> +void MediaPlayerPrivateAVFoundationObjC::updateVideoTracks()

This looks *awfully* similar to updateAudioTracks. Is there any way to share
code like MediaPlayerPrivateAVFoundation::processNewAndRemovedTextTracks does
for processing in-band text tracks?

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.h:4
0
> +class VideoTrackPrivateAVFObjC : public VideoTrackPrivateAVF {

FINAL

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.h:4
8
> +    virtual void setSelected(bool);

OVERRIDE

> LayoutTests/media/track/video-track.html:37
> +	       function canplaythrough()
> +	       {
> +		   consoleWrite("<br><i>** Check initial in-band track states<"
+ "/i>");
> +		   testExpected("video.videoTracks.length", 1);
> +
> +		   consoleWrite("<br><i>** Unload video file, check track
count<" + "/i>");
> +		   run("video.src = ''");
> +		   testExpected("video.videoTracks.length", 0);
> +
> +		   consoleWrite("");
> +		   endTest();
> +	       }

This should also test the "only one video track enabled" spec requirement.