Bug 163729

Summary:

[Modern Media Controls] Media Controller: Airplay support

Product:

WebKit

Reporter:

Antoine Quint <graouts>

Component:

Media

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, graouts, joepeck, ryanhaddad, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari 10

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none
Archive of layout-test-results from webkit-cq-02 for mac-yosemite	none
Patch for landing	none
Patch	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch for landing	none

Antoine Quint

Reported 2016-10-20 07:46:58 PDT

We need to show the Airplay button only when Airplay is available, make it show the list of Airplay sources when clicked, and turn the button on when an Airplay source is active.

Attachments
Patch (26.82 KB, patch) 2016-10-20 07:49 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch for landing (55.28 KB, patch) 2016-10-31 12:19 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-cq-02 for mac-yosemite (975.08 KB, application/zip) 2016-10-31 13:17 PDT, WebKit Commit Bot	no flags	Details
Patch for landing (56.12 KB, patch) 2016-10-31 14:21 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (13.29 KB, patch) 2016-11-01 11:52 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (9.37 MB, application/zip) 2016-11-01 13:05 PDT, Build Bot	no flags	Details
Patch (13.99 KB, patch) 2016-11-01 14:03 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (14.01 KB, patch) 2016-11-01 14:12 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch for landing (13.83 KB, patch) 2016-11-01 15:23 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Antoine Quint

Comment 1 2016-10-20 07:47:12 PDT

<rdar://problem/27989484>

Antoine Quint

Comment 2 2016-10-20 07:49:11 PDT

Created attachment 292188 [details] Patch

Antoine Quint

Comment 3 2016-10-20 07:50:00 PDT

Tests aren't ready yet but the source can be reviewed already.

Dean Jackson

Comment 4 2016-10-27 14:44:09 PDT

Comment on attachment 292188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292188&action=review r+ assuming some tests come. > Source/WebCore/ChangeLog:13 > + * Modules/modern-media-controls/media/airplay-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js. ! > Source/WebCore/Modules/modern-media-controls/media/airplay-support.js:58 > + const control = this.control; > + control.enabled = !!this._routesAvailable; > + control.on = this.mediaController.media.webkitCurrentPlaybackTargetIsWireless; I don't think it is worth having the local control variable. Also, and I know this is correct JS, it freaks me out that const in JS isn't the same as const in C++. > Source/WebCore/rendering/RenderThemeMac.mm:263 > - NSArray *mediaPaths = @[@"media-controller-support", @"mute-support", @"start-support", @"media-controller"]; > + NSArray *mediaPaths = @[@"media-controller-support", @"airplay-support", @"mute-support", @"start-support", @"media-controller"]; Did you get anyone to review your build script to do this?

Antoine Quint

Comment 5 2016-10-28 15:11:00 PDT

(In reply to comment #4) > Comment on attachment 292188 [details] > Patch > > > Source/WebCore/rendering/RenderThemeMac.mm:263 > > - NSArray *mediaPaths = @[@"media-controller-support", @"mute-support", @"start-support", @"media-controller"]; > > + NSArray *mediaPaths = @[@"media-controller-support", @"airplay-support", @"mute-support", @"start-support", @"media-controller"]; > > Did you get anyone to review your build script to do this? Some guy named Darin Adler.

Antoine Quint

Comment 6 2016-10-31 12:19:15 PDT

Created attachment 293450 [details] Patch for landing

WebKit Commit Bot

Comment 7 2016-10-31 13:17:53 PDT

Comment on attachment 293450 [details] Patch for landing Rejecting attachment 293450 [details] from commit-queue. New failing tests: media/modern-media-controls/airplay-support/airplay-support.html Full output: http://webkit-queues.webkit.org/results/2413181

WebKit Commit Bot

Comment 8 2016-10-31 13:17:55 PDT

Created attachment 293457 [details] Archive of layout-test-results from webkit-cq-02 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-02 Port: mac-yosemite Platform: Mac OS X 10.10.5

Antoine Quint

Comment 9 2016-10-31 14:21:34 PDT

Created attachment 293466 [details] Patch for landing

Antoine Quint

Comment 10 2016-10-31 15:08:46 PDT

https://trac.webkit.org/changeset/208178

WebKit Commit Bot

Comment 11 2016-10-31 15:09:05 PDT

The commit-queue encountered the following flaky tests while processing attachment 293466 [details]: media/modern-media-controls/volume-support/volume-support-click.html bug 164243 (author: graouts@apple.com) The commit-queue is continuing to process your patch.

Joseph Pecoraro

Comment 12 2016-10-31 16:32:23 PDT

Comment on attachment 293466 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=293466&action=review > LayoutTests/media/modern-media-controls/elapsed-time-support/elapsed-time-support.html:32 > <script src="../../../../Source/WebCore/Modules/modern-media-controls/controls/macos-media-controls.js" type="text/javascript"></script> > <script src="../../../../Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js" type="text/javascript"></script> > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js" type="text/javascript"></script> > +<script src="../../../../Source/WebCore/Modules/modern-media-controls/media/airplay-support.js" type="text/javascript"></script> > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/elapsed-time-support.js" type="text/javascript"></script> > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/mute-support.js" type="text/javascript"></script> > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/playback-support.js" type="text/javascript"></script> This is kind of unfortunate that we have to touch every test every time. To workaround this, you could have a single resource: LayoutTests/media/modern-media-controls/resources/load-module-scripts.js Which would include all of the necessary resources: document.write(`<link rel="stylesheet" href=".../airplay-button.css">`); document.write(`<script src=".../media-controller-support.js"></script>`); document.write(`<script src=".../airplay-support.js"></script>`); ... Each of the tests could just include this one script and you would only need to change this one place.

Ryan Haddad

Comment 13 2016-10-31 17:00:04 PDT

Reverted r208178 for reason: The test added with this change fails or times out on macOS and iOS. Committed r208196: <http://trac.webkit.org/changeset/208196>

Ryan Haddad

Comment 14 2016-10-31 17:00:19 PDT

(In reply to comment #13) > Reverted r208178 for reason: > > The test added with this change fails or times out on macOS and iOS. > > Committed r208196: <http://trac.webkit.org/changeset/208196> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Fmodern-media-controls%2Fairplay-support%2Fairplay-support.html

Antoine Quint

Comment 15 2016-11-01 05:18:41 PDT

(In reply to comment #12) > Comment on attachment 293466 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293466&action=review > > > LayoutTests/media/modern-media-controls/elapsed-time-support/elapsed-time-support.html:32 > > <script src="../../../../Source/WebCore/Modules/modern-media-controls/controls/macos-media-controls.js" type="text/javascript"></script> > > <script src="../../../../Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js" type="text/javascript"></script> > > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js" type="text/javascript"></script> > > +<script src="../../../../Source/WebCore/Modules/modern-media-controls/media/airplay-support.js" type="text/javascript"></script> > > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/elapsed-time-support.js" type="text/javascript"></script> > > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/mute-support.js" type="text/javascript"></script> > > <script src="../../../../Source/WebCore/Modules/modern-media-controls/media/playback-support.js" type="text/javascript"></script> > > This is kind of unfortunate that we have to touch every test every time. To > workaround this, you could have a single resource: > > LayoutTests/media/modern-media-controls/resources/load-module-scripts.js > > Which would include all of the necessary resources: > > document.write(`<link rel="stylesheet" href=".../airplay-button.css">`); > document.write(`<script > src=".../media-controller-support.js"></script>`); > document.write(`<script src=".../airplay-support.js"></script>`); > ... > > Each of the tests could just include this one script and you would only need > to change this one place. Yes, I'm planning on doing something like this as soon as I have a minute. I really need to land these remaining patches first.

Antoine Quint

Comment 16 2016-11-01 11:52:03 PDT

Created attachment 293560 [details] Patch

Build Bot

Comment 17 2016-11-01 13:05:41 PDT

Comment on attachment 293560 [details] Patch Attachment 293560 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2445322 New failing tests: media/modern-media-controls/airplay-support/airplay-support.html

Build Bot

Comment 18 2016-11-01 13:05:47 PDT

Created attachment 293575 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Antoine Quint

Comment 19 2016-11-01 14:03:56 PDT

Created attachment 293588 [details] Patch

Antoine Quint

Comment 20 2016-11-01 14:12:54 PDT

Created attachment 293594 [details] Patch

Antoine Quint

Comment 21 2016-11-01 15:23:09 PDT

Created attachment 293604 [details] Patch for landing

WebKit Commit Bot

Comment 22 2016-11-01 15:58:29 PDT

Comment on attachment 293604 [details] Patch for landing Clearing flags on attachment: 293604 Committed r208254: <http://trac.webkit.org/changeset/208254>

WebKit Commit Bot

Comment 23 2016-11-01 15:58:34 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.