Bug 143251 – [Mac] Do not include route button if element does not support target playback

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Not all media engines support playback to an external device.

[Radar WebKit Bug Importer]

<rdar://problem/20369636>

[Eric Carlson]

Created attachment 249847 [details]
Proposed patch

[Eric Carlson]

Created attachment 249856 [details]
Updated Patch

[Jon Lee]

rdar://problem/19130597

[Jer Noble]

Comment on attachment 249856 [details]
Updated Patch

r=me, with nits.

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1702
> +        if (!'webkitCurrentPlaybackTargetIsWireless' in this.video || !'webkitWirelessVideoPlaybackDisabled' in this.video)
> +            return false;

It seems like these are unnecessary.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1753
> +        if ('webkitWirelessVideoPlaybackDisabled' in this.video && this.video.webkitWirelessVideoPlaybackDisabled)

ditto the 'in this.video'.

> Source/WebCore/platform/graphics/MediaPlaybackTarget.h:57
> +    void setDevicePickerContext(AVOutputContext *) { }
> +    AVOutputContext *devicePickerContext() const { return nullptr; }
> +    bool hasActiveRoute() const { return false; }

It seems unfortunate to expose AVOutputContext to non-Cocoa clients.  Maybe in a follow-up we can come up with a "PlatformOutputContext" which wraps AVOutputContext in Cocoa, but not for all other ports.

[Eric Carlson]

Created attachment 249928 [details]
Patch for landing

[Eric Carlson]

(In reply to comment #5)
> Comment on attachment 249856 [details]
> Updated Patch
> 
> r=me, with nits.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249856&action=review
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1702
> > +        if (!'webkitCurrentPlaybackTargetIsWireless' in this.video || !'webkitWirelessVideoPlaybackDisabled' in this.video)
> > +            return false;
> 
> It seems like these are unnecessary.
> 
Agreed, fixed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1753
> > +        if ('webkitWirelessVideoPlaybackDisabled' in this.video && this.video.webkitWirelessVideoPlaybackDisabled)
> 
> ditto the 'in this.video'.
> 
Ditto.

> > Source/WebCore/platform/graphics/MediaPlaybackTarget.h:57
> > +    void setDevicePickerContext(AVOutputContext *) { }
> > +    AVOutputContext *devicePickerContext() const { return nullptr; }
> > +    bool hasActiveRoute() const { return false; }
> 
> It seems unfortunate to expose AVOutputContext to non-Cocoa clients.  Maybe
> in a follow-up we can come up with a "PlatformOutputContext" which wraps
> AVOutputContext in Cocoa, but not for all other ports.
>
Good point, I will do that in a followup.

Thanks!

[Eric Carlson]

committed r182240: https://trac.webkit.org/r182240

[Eric Carlson]

Committed https://trac.webkit.org/r182250 to fix the iOS build.