Bug 154823

Summary: Mac: Adopt the new version of AVOutputDeviceMenuController's showMenuForRect method
Product: WebKit Reporter: Ada Chan <adachan>
Component: MediaAssignee: Ada Chan <adachan>
Status: RESOLVED FIXED    
Severity: Normal Keywords: InRadar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch thorton: review+

Description Ada Chan 2016-02-29 10:35:49 PST
Mac: Adopt the new version of AVOutputDeviceMenuController's showMenuForRect method
Comment 1 Ada Chan 2016-02-29 10:37:06 PST
<rdar://problem/24806247>
Comment 2 Ada Chan 2016-02-29 10:43:08 PST
Created attachment 272503 [details]
Patch
Comment 3 Ada Chan 2016-02-29 13:30:36 PST
Comment on attachment 272503 [details]
Patch

Need to update AVKitSPI.h.  Will send an updated patch soon.
Comment 4 Ada Chan 2016-02-29 13:37:18 PST
Created attachment 272513 [details]
Patch
Comment 5 Darin Adler 2016-02-29 16:17:20 PST
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().
Comment 6 Ada Chan 2016-03-01 11:35:50 PST
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.
Comment 7 Ada Chan 2016-03-01 11:37:11 PST
Created attachment 272581 [details]
Patch
Comment 8 Ada Chan 2016-03-01 15:25:07 PST
Removing the forward declaration since it's no longer a requirement to support building without the SPI in the headers.
Comment 9 Ada Chan 2016-03-01 15:27:14 PST
Created attachment 272597 [details]
Patch
Comment 10 Ada Chan 2016-03-01 17:15:37 PST
Committed:
http://trac.webkit.org/changeset/197429