Bug 163729 – [Modern Media Controls] Media Controller: Airplay support

• Component: Media
• Status: RESOLVED FIXED

[Antoine Quint]

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.

[Antoine Quint]

<rdar://problem/27989484>

[Antoine Quint]

Created attachment 292188 [details]
Patch

[Antoine Quint]

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

[Dean Jackson]

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]

(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]

Created attachment 293450 [details]
Patch for landing

[WebKit Commit Bot]

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]

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]

Created attachment 293466 [details]
Patch for landing

[Antoine Quint]

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

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[Joseph Pecoraro]

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]

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]

(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]

(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]

Created attachment 293560 [details]
Patch

[Build Bot]

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]

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]

Created attachment 293588 [details]
Patch

[Antoine Quint]

Created attachment 293594 [details]
Patch

[Antoine Quint]

Created attachment 293604 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 293604 [details]
Patch for landing

Clearing flags on attachment: 293604

Committed r208254: <http://trac.webkit.org/changeset/208254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.