[webkit-reviews] review granted: [Bug 137474] [Mac] Represent AVMediaSelectionOptions as AudioTracks : [Attachment 239413] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 137474: [Mac] Represent AVMediaSelectionOptions as AudioTracks
https://bugs.webkit.org/show_bug.cgi?id=137474

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=239413&action=review


I think this needs some cleanup before landing, but otherwise looks great!

> Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.h:78
> +    RefPtr<MediaSelectionOptionAVFObjC> m_mediaSelectionOption;

It almost seems like these should be two distinct classes; AVTrackPrivates that
are backed by AVPlayerItemTracks, and AVTrackPrivates that are backed by media
selection options. But that would probably add needless complexity...

The upside would be that it would do away with the weird "dual" nature of all
the methods below.

>
Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:122

> +

Extra space...

>
Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.h:5
8
> +    static RefPtr<AudioTrackPrivateAVFObjC>
create(MediaSelectionOptionAVFObjC& option)

Should this be a const reference?

>
Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.mm:
42
> +    : m_impl(std::make_unique<AVTrackPrivateAVFObjCImpl>(option))

Const reference for option?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1860
> +    for (auto &oldItem : oldItems) {

What!?! Ugh! Is this some horrible ObjC coding standard we use? Hmm. Elsewhere
in the file we use "for (auto& oldItem..." like human beings, so I think this
is a typo.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1876
> +    for (auto & addedItem : addedItems)

Zounds! Another variation on weird reference spacing. No!

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1916
> +    for (auto &oldItem : oldItems) {

Nit: Bad reference spacing.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1932
> +    for (auto & addedItem : addedItems)

Nit: Extra space here. On the LEFT side of the '&'.

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp
:47
>
+VideoTrackPrivateAVFObjC::VideoTrackPrivateAVFObjC(MediaSelectionOptionAVFObjC
& option)

Should this be a const reference, since we're using it as an argument to a
constructor?

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp
:88
> +void
VideoTrackPrivateAVFObjC::setMediaSelectonOption(MediaSelectionOptionAVFObjC&
option)

Ditto: Const reference perhaps?

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.h:5
7
> +    static RefPtr<VideoTrackPrivateAVFObjC>
create(MediaSelectionOptionAVFObjC& option)

Nit: Maybe this should be a const reference since we're only using it to
construct an object?