Bug 154823 – Mac: Adopt the new version of AVOutputDeviceMenuController's showMenuForRect method

• Component: Media
• Status: RESOLVED FIXED

[Ada Chan]

Mac: Adopt the new version of AVOutputDeviceMenuController's showMenuForRect method

[Ada Chan]

<rdar://problem/24806247>

[Ada Chan]

Created attachment 272503 [details]
Patch

[Ada Chan]

Comment on attachment 272503 [details]
Patch

Need to update AVKitSPI.h.  Will send an updated patch soon.

[Ada Chan]

Created attachment 272513 [details]
Patch

[Darin Adler]

Comment on attachment 272513 [details]
Patch

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

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:277
> +    for (size_t i = 0; i < m_clientState.size(); ++i) {
> +        auto& state = m_clientState[i];
> +

Should use a modern for loop:

    for (auto& state : m_clientState) {

> Source/WebCore/dom/Document.cpp:6890
> +    auto it = m_idToClientMap.find(clientId);
> +    if (it == m_idToClientMap.end())
> +        return;
> +
> +    it->value->customPlaybackActionSelected();

When a map’s values are pointers, you can do this in a way that’s less complex with get:

    if (auto* client = m_idToClientMap.get(clientId))
        client->customPlaybackActionSelected();

I have no idea why the functions above don’t do it this simpler way.

> Source/WebCore/html/HTMLMediaElement.cpp:5217
> +    return String();

There’s now a more terse way of writing the null string:

    return { };

> Source/WebCore/html/MediaElementSession.cpp:53
> +#if PLATFORM(MAC)
> +#include "HTMLMediaElementEnums.h"
> +#endif

This seems peculiar. The code below is in #if ENABLE(WIRELESS_PLAYBACK_TARGET) && !PLATFORM(IOS) so it doesn't seem right to put the #include in #if PLATFORM(MAC). Maybe make the include unconditional? Or match the conditional around the code below exactly?

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.h:73
> +    WEBCORE_EXPORT void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override;

A little bit strange to have the argument be WTF::String rather than const WTF::String&, but that’s in the base class too, not just here.

> Source/WebKit/mac/WebView/WebView.mm:8743
> +    [self _devicePicker]->showPlaybackTargetPicker(clientId, rectInScreenCoordinates, hasVideo, String());

Can use { } here instead of String().

[Ada Chan]

Thanks for the feedback!

(In reply to comment #5)
> Comment on attachment 272513 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272513&action=review
> 
> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:277
> > +    for (size_t i = 0; i < m_clientState.size(); ++i) {
> > +        auto& state = m_clientState[i];
> > +
> 
> Should use a modern for loop:
> 
>     for (auto& state : m_clientState) {

Fixed.

> 
> > Source/WebCore/dom/Document.cpp:6890
> > +    auto it = m_idToClientMap.find(clientId);
> > +    if (it == m_idToClientMap.end())
> > +        return;
> > +
> > +    it->value->customPlaybackActionSelected();
> 
> When a map’s values are pointers, you can do this in a way that’s less
> complex with get:
> 
>     if (auto* client = m_idToClientMap.get(clientId))
>         client->customPlaybackActionSelected();
> 
> I have no idea why the functions above don’t do it this simpler way.

Fixed.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5217
> > +    return String();
> 
> There’s now a more terse way of writing the null string:
> 
>     return { };

Fixed.

> 
> > Source/WebCore/html/MediaElementSession.cpp:53
> > +#if PLATFORM(MAC)
> > +#include "HTMLMediaElementEnums.h"
> > +#endif
> 
> This seems peculiar. The code below is in #if
> ENABLE(WIRELESS_PLAYBACK_TARGET) && !PLATFORM(IOS) so it doesn't seem right
> to put the #include in #if PLATFORM(MAC). Maybe make the include
> unconditional? Or match the conditional around the code below exactly?

I made the include unconditional.

> 
> > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.h:73
> > +    WEBCORE_EXPORT void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override;
> 
> A little bit strange to have the argument be WTF::String rather than const
> WTF::String&, but that’s in the base class too, not just here.
> 
> > Source/WebKit/mac/WebView/WebView.mm:8743
> > +    [self _devicePicker]->showPlaybackTargetPicker(clientId, rectInScreenCoordinates, hasVideo, String());
> 
> Can use { } here instead of String().

Fixed.

I had to add a forward declaration for the SPI in MediaPlaybackTargetPickerMac.mm to get things to compile for systems without that SPI.  Will post an updated patch soon.

[Ada Chan]

Created attachment 272581 [details]
Patch

[Ada Chan]

Removing the forward declaration since it's no longer a requirement to support building without the SPI in the headers.

[Ada Chan]

Created attachment 272597 [details]
Patch

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/197429