Bug 171192 – [macOS] AVTouchBarMediaSelectionOptions should be created with the correct type

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[macOS] AVTouchBarMediaSelectionOptions should be created with the correct type

[Andy Estes]

Created attachment 307925 [details]
Patch

[Andy Estes]

rdar://problem/29875010

[Wenson Hsieh]

Comment on attachment 307925 [details]
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

[Andy Estes]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 307925 [details]
> 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.

Thanks for the review! I made the changes you suggested except for these:

> 
> > 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.

There's quite a bit of cleanup that can be done here. Lots of TextTrack*s that can be TextTrack&s, RefPtr<TextTrack>s that can be Ref<TextTrack>s, etc. Ditto for AudioTrack.

I agree this makes sense to do in a separate 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.

Ditto.

[Andy Estes]

Created attachment 307927 [details]
Patch

[Andy Estes]

Created attachment 307928 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 307928 [details]
Patch

Clearing flags on attachment: 307928

Committed r215672: <http://trac.webkit.org/changeset/215672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.