Bug 143251 - [Mac] Do not include route button if element does not support target playback
Summary: [Mac] Do not include route button if element does not support target playback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-30 20:38 PDT by Eric Carlson
Modified: 2015-04-01 12:51 PDT (History)
3 users (show)

See Also:

Attachments
Proposed patch (49.11 KB, patch)
2015-03-31 13:44 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Updated Patch (49.23 KB, patch)
2015-03-31 14:57 PDT, Eric Carlson 		jer.noble: review+
Details | Formatted Diff | Diff
Patch for landing (49.07 KB, patch)
2015-04-01 10:10 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2015-03-30 20:38:18 PDT 
Not all media engines support playback to an external device.
Comment 1 Radar WebKit Bug Importer 2015-03-31 13:38:20 PDT 
<rdar://problem/20369636>
Comment 2 Eric Carlson 2015-03-31 13:44:05 PDT 
Created attachment 249847 [details]
Proposed patch
Comment 3 Eric Carlson 2015-03-31 14:57:51 PDT 
Created attachment 249856 [details]
Updated Patch
Comment 4 Jon Lee 2015-03-31 19:25:28 PDT 
rdar://problem/19130597
Comment 5 Jer Noble 2015-04-01 10:00:47 PDT 
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.
Comment 6 Eric Carlson 2015-04-01 10:10:20 PDT 
Created attachment 249928 [details]
Patch for landing
Comment 7 Eric Carlson 2015-04-01 10:11:24 PDT 
(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!
Comment 8 Eric Carlson 2015-04-01 11:09:19 PDT 
committed r182240: https://trac.webkit.org/r182240
Comment 9 Eric Carlson 2015-04-01 12:51:40 PDT 
Committed https://trac.webkit.org/r182250 to fix the iOS build.