[webkit-reviews] review granted: [Bug 171192] [macOS] AVTouchBarMediaSelectionOptions should be created with the correct type : [Attachment 307925] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Andy Estes
<aestes at apple.com>'s request for review:
Bug 171192: [macOS] AVTouchBarMediaSelectionOptions should be created with the
correct type
https://bugs.webkit.org/show_bug.cgi?id=171192

Attachment 307925: Patch

https://bugs.webkit.org/attachment.cgi?id=307925&action=review




--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 307925
  --> https://bugs.webkit.org/attachment.cgi?id=307925
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307925&action=review

r=me! I'm not very familiar with this part of the media code, but the plumbing
looks reasonable.

> Source/WebCore/page/CaptionUserPreferences.cpp:219
> +MediaSelectionOption
CaptionUserPreferences::mediaSelectionOptionForTrack(TextTrack* track) const

I see that displayNameForTrack currently takes a raw TextTrack*, but could we
also make the new CaptionUserPreferences::mediaSelectionOptionForTrack take an
std::optional<TextTrack> instead? Maybe we can do that in a later patch.

> Source/WebCore/page/CaptionUserPreferences.cpp:266
> +MediaSelectionOption
CaptionUserPreferences::mediaSelectionOptionForTrack(AudioTrack* track) const

I think this would also be better as an std::optional<AudioTrack>. However,
that would probably require refactoring displayNameForTrack as well, so maybe
that's for later.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:697
> +    return m_playbackModel ? m_playbackModel->audioMediaSelectionOptions() :
Vector<MediaSelectionOption>();

Can we use a struct initializer here?

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:-709
> -    return m_playbackModel ? m_playbackModel->legibleMediaSelectionOptions()
: Vector<String>();

Can we just use a { } here instead of Vector<MediaSelectionOption>()?

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:234
> +    }

Could we also have a default return value at the end here, just in case
anything in the future tries to static_cast a number to an invalid
MediaSelectionOption::Type?

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:243
> +	   if (RetainPtr<AVTouchBarMediaSelectionOption> webOption =
adoptNS([allocAVTouchBarMediaSelectionOptionInstance()
initWithTitle:option.displayName
type:toAVTouchBarMediaSelectionOptionType(option.type)]))

This could be auto webOption.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:245
> +	   if (RetainPtr<AVTouchBarMediaSelectionOption> webOption =
adoptNS([allocAVFunctionBarMediaSelectionOptionInstance()
initWithTitle:option.displayName]))

auto webOption