Bug 162971 – Implement the Remote Playback API.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Implement the Remote Playback API.

[Jer Noble]

Created attachment 290712 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/28632488>

[Build Bot]

Comment on attachment 290712 [details]
Patch

Attachment 290712 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2225113

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-prompt.html
media/remoteplayback-watch-disableremoteplayback.html

[Build Bot]

Created attachment 290724 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290712 [details]
Patch

Attachment 290712 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2225127

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 290726 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 290712 [details]
Patch

Attachment 290712 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2225199

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 290729 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290712 [details]
Patch

Attachment 290712 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2225275

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 290731 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Eric Carlson]

Comment on attachment 290712 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        One addition provided by the Remote Playback API is an event in the case the user
> +        dismisses the picker UI without making a choice, so this concept needs to be plumbed
> +        through from the ChromeClient to the PlatformMediaSession.

The AppleTV interface doesn't inform a client when a user dismisses the picker so we won't be able to implement this.

[Jer Noble]

(In reply to comment #11)
> Comment on attachment 290712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290712&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        One addition provided by the Remote Playback API is an event in the case the user
> > +        dismisses the picker UI without making a choice, so this concept needs to be plumbed
> > +        through from the ChromeClient to the PlatformMediaSession.
> 
> The AppleTV interface doesn't inform a client when a user dismisses the
> picker so we won't be able to implement this.

The AVOutputDeviceMenuController API tells you when the menu closed, and from that you can determine whether the user dismissed the menu or made an affirmative choice. We may not be able to tell on iOS though.

[Jer Noble]

(In reply to comment #9)
> Comment on attachment 290712 [details]
> Patch
> 
> Attachment 290712 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/2225275
> 
> New failing tests:
> media/remoteplayback-target-availability.html
> media/remoteplayback-cancel-invalid.html
> media/remoteplayback-watch-disableremoteplayback.html
> media/remoteplayback-prompt.html

It looks like these tests are failing on the EWS due to the inherent raciness of our remote playback implementation (e.g., if you set a route in one "tab", it must affect remote playback in a second "tab".)  I'll take care of this in a separate patch that, when using a mock remote device, will give each Page its own picker.

[Jer Noble]

(In reply to comment #13)
> (In reply to comment #9)
> > Comment on attachment 290712 [details]
> > Patch
> > 
> > Attachment 290712 [details] did not pass mac-ews (mac):
> > Output: http://webkit-queues.webkit.org/results/2225275
> > 
> > New failing tests:
> > media/remoteplayback-target-availability.html
> > media/remoteplayback-cancel-invalid.html
> > media/remoteplayback-watch-disableremoteplayback.html
> > media/remoteplayback-prompt.html
> 
> It looks like these tests are failing on the EWS due to the inherent
> raciness of our remote playback implementation (e.g., if you set a route in
> one "tab", it must affect remote playback in a second "tab".)  I'll take
> care of this in a separate patch that, when using a mock remote device, will
> give each Page its own picker.

Filed <https://bugs.webkit.org/show_bug.cgi?id=163044> to track the AirPlay test flakiness.

[Jer Noble]

Created attachment 291121 [details]
Patch

[WebKit Commit Bot]

Attachment 291121 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebMediaPlaybackTargetPicker.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebView/WebViewInternal.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebView/WebPreferencesPrivate.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
Total errors found: 5 in 74 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 291121 [details]
Patch

Attachment 291121 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2256858

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 291135 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291121 [details]
Patch

Attachment 291121 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2256904

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 291137 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291121 [details]
Patch

Attachment 291121 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2256907

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 291138 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291121 [details]
Patch

Attachment 291121 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2256901

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 291139 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Jer Noble]

(In reply to comment #23)
> Comment on attachment 291121 [details]
> Patch
> 
> Attachment 291121 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/2256901
> 
> New failing tests:
> media/remoteplayback-target-availability.html
> media/remoteplayback-prompt.html

(In reply to comment #24)
> Created attachment 291139 [details]
> Archive of layout-test-results from ews123 for ios-simulator-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> ios-sim-ews.
> Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

The Yosemite EWS bots are failing because WIRELESS_PLAYBACK_TARGET is disabled on Yosemite.

The iOS EWS bots are failing because iOS does not have a Mock wireless playback device.

I'll update the TestExpectations and re-upload.

[Jer Noble]

Created attachment 291276 [details]
Patch

[WebKit Commit Bot]

Attachment 291276 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebMediaPlaybackTargetPicker.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebView/WebViewInternal.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebView/WebPreferencesPrivate.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
Total errors found: 5 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 291276 [details]
Patch

Attachment 291276 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2264139

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 291283 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291276 [details]
Patch

Attachment 291276 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2264160

New failing tests:
media/remoteplayback-target-availability.html
media/remoteplayback-cancel-invalid.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 291286 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291276 [details]
Patch

Attachment 291276 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2264166

New failing tests:
media/remoteplayback-cancel-invalid.html
media/remoteplayback-target-availability.html
media/remoteplayback-watch-disableremoteplayback.html
media/remoteplayback-prompt.html

[Build Bot]

Created attachment 291287 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jer Noble]

Created attachment 292263 [details]
Patch

[Brady Eidson]

Comment on attachment 292263 [details]
Patch

Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form.

If this patch is still important please rebase it and post it for review again.

[Jer Noble]

The Second Screen WG is interested in seeing a second implementation of RemotePlayback API. I'll be rebasing this patch to apply against ToT.

[Jer Noble]

Created attachment 375978 [details]
Patch

[EWS Watchlist]

Attachment 375978 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:475:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 74 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 376085 [details]
Patch

[EWS Watchlist]

Attachment 376085 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:475:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 376088 [details]
Patch

[EWS Watchlist]

Attachment 376088 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:475:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 376088 [details]
Patch

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

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:60
> +void RemotePlayback::watchAvailability(Ref<RemotePlaybackAvailabilityCallback>&& callback, Ref<DeferredPromise>&& promise)

It seems this method is always settling the promise synchronously.
Is it temporary, our implementation choice, for future-proofing, or a case of over-promisification?
Same comment for cancelWatchAvailability.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:90
> +    m_callbackMap.set(callbackId, WTFMove(callback));

add is a bit more efficient than set.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:101
> +            Ref<RemotePlaybackAvailabilityCallback> callback = *m_callbackMap.get(callbackId);

I guess the ref is to protect the callback?
Maybe worth using protectedCallback?

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:154
> +    // https://w3c.github.io/remote-playback/#stop-observing-remote-playback-devices-availability

https://w3c.github.io/remote-playback/#prompt-user-for-changing-remote-playback-state

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:155
> +    // W3C Editor's Draft 15 July 2016

Is this comment needed?

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:172
> +    // NOTE: consider implementing

That might be nice to implement and would allow not having a vector of promises.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:236
> +        if (shouldPlayToRemoteTarget) {

This if is not really needed right now.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:388
> +        // Can't use copyValuesToVector() here because Ref<> has a deleted assignment operator.

WTF::map with copyRef should do the trick.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:390
> +            callbacks.append(callback.copyRef());

If not WTF::map, use uncheckedAppend.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.h:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2019

> Source/WebCore/Modules/remoteplayback/RemotePlayback.h:33
> +#include "GenericTaskQueue.h"

Probably not needed.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.h:45
> +class RemotePlayback : public RefCounted<RemotePlayback>, public ActiveDOMObject, public EventTargetWithInlineData {

final maybe?

> Source/WebCore/Modules/remoteplayback/RemotePlayback.h:74
> +    RemotePlayback(HTMLMediaElement&);

explicit

> Source/WebCore/Modules/remoteplayback/RemotePlayback.h:92
> +    void stop() override;

final?

> Source/WebCore/Modules/remoteplayback/RemotePlayback.h:96
> +    ScriptExecutionContext* scriptExecutionContext() const override { return ActiveDOMObject::scriptExecutionContext(); }

final?

> Source/WebCore/Modules/remoteplayback/RemotePlayback.h:98
> +    HTMLMediaElement* m_mediaElement;

WeakPtr for extra safety?

> LayoutTests/ChangeLog:3
> +        Implement the Remote Playback API.

There are a few WPT tests related to remote playback API.
Should we import them?

[Jer Noble]

(In reply to youenn fablet from comment #43)
> Comment on attachment 376088 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376088&action=review
> 
> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:60
> > +void RemotePlayback::watchAvailability(Ref<RemotePlaybackAvailabilityCallback>&& callback, Ref<DeferredPromise>&& promise)
> 
> It seems this method is always settling the promise synchronously.
> Is it temporary, our implementation choice, for future-proofing, or a case
> of over-promisification?
> Same comment for cancelWatchAvailability.

I think it's just an oversight. I'll move the "run the remaining steps asynchronously" section into a task.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:90
> > +    m_callbackMap.set(callbackId, WTFMove(callback));
> 
> add is a bit more efficient than set.

Changed.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:101
> > +            Ref<RemotePlaybackAvailabilityCallback> callback = *m_callbackMap.get(callbackId);
> 
> I guess the ref is to protect the callback?
> Maybe worth using protectedCallback?

Changed.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:154
> > +    // https://w3c.github.io/remote-playback/#stop-observing-remote-playback-devices-availability
> 
> https://w3c.github.io/remote-playback/#prompt-user-for-changing-remote-
> playback-state
> 
> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:155
> > +    // W3C Editor's Draft 15 July 2016
> 
> Is this comment needed?

I have found in the past that referencing which version of the spec was used to generate the implementation really helps when the spec changes.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:172
> > +    // NOTE: consider implementing
> 
> That might be nice to implement and would allow not having a vector of
> promises.

It might be, but it would also require cancelling all the tasks associated with all those previous promises.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:236
> > +        if (shouldPlayToRemoteTarget) {
> 
> This if is not really needed right now.

Cleaned this up.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:388
> > +        // Can't use copyValuesToVector() here because Ref<> has a deleted assignment operator.
> 
> WTF::map with copyRef should do the trick.

I'm not sure what you're suggesting here?

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:390
> > +            callbacks.append(callback.copyRef());
> 
> If not WTF::map, use uncheckedAppend.

Ok.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.h:2
> > + * Copyright (C) 2016 Apple Inc. All rights reserved.
> 
> 2019

Done.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.h:33
> > +#include "GenericTaskQueue.h"
> 
> Probably not needed.

Ok.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.h:45
> > +class RemotePlayback : public RefCounted<RemotePlayback>, public ActiveDOMObject, public EventTargetWithInlineData {
> 
> final maybe?

Ok.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.h:74
> > +    RemotePlayback(HTMLMediaElement&);
> 
> explicit

Sure.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.h:92
> > +    void stop() override;
> 
> final?

Ok.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.h:96
> > +    ScriptExecutionContext* scriptExecutionContext() const override { return ActiveDOMObject::scriptExecutionContext(); }
> 
> final?

Ok.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.h:98
> > +    HTMLMediaElement* m_mediaElement;
> 
> WeakPtr for extra safety?

Good idea!

> > LayoutTests/ChangeLog:3
> > +        Implement the Remote Playback API.
> 
> There are a few WPT tests related to remote playback API.
> Should we import them?

I'll look into this.

[Jer Noble]

Created attachment 378947 [details]
Patch

[Jer Noble]

Comment on attachment 378947 [details]
Patch

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

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:95
> +        // 8. Fulfill promise with the callbackId and run the following steps in parallel:
> +        promise->whenSettled([this, strongThis = makeRefPtr(this), callbackId] {

I needed to make this change in order to pass one of the WPT tests that requires the promise to fire before the callback is called.

[youenn fablet]

(In reply to Jer Noble from comment #46)
> Comment on attachment 378947 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378947&action=review
> 
> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:95
> > +        // 8. Fulfill promise with the callbackId and run the following steps in parallel:
> > +        promise->whenSettled([this, strongThis = makeRefPtr(this), callbackId] {
> 
> I needed to make this change in order to pass one of the WPT tests that
> requires the promise to fire before the callback is called.

This method is returning a promise and calling a completion callback.
I guess this is the migrate-from-callback-to-promise pattern.
Do we care of contents using the callback? Can we just ship the promise version?

[youenn fablet]

Comment on attachment 378947 [details]
Patch

LGTM once bots are green.
Bots are red because of the missing -expected.txt I believe but all tests seem to be PASS.

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

> Source/WebCore/Modules/remoteplayback/HTMLMediaElementRemotePlayback.h:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2019

> Source/WebCore/Modules/remoteplayback/HTMLMediaElementRemotePlayback.idl:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2019

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:50
> +    , m_mediaElement(element.weakPtrFactory().createWeakPtr(element))

makeWeakPtr(element)

>>> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:95
>>> +        promise->whenSettled([this, strongThis = makeRefPtr(this), callbackId] {
>> 
>> I needed to make this change in order to pass one of the WPT tests that requires the promise to fire before the callback is called.
> 
> This method is returning a promise and calling a completion callback.
> I guess this is the migrate-from-callback-to-promise pattern.
> Do we care of contents using the callback? Can we just ship the promise version?

Jer explained me the API, which returns a promise and then calls the callback repeatedly.
Seems a bit strange but this is the current spec.

s/strongThis/protectedThis/

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:103
> +                    Ref<RemotePlaybackAvailabilityCallback> protectedCallback = foundCallback->value.copyRef();

auto.

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:104
> +                    protectedCallback->handleEvent(m_available);

Do we need protectedCallback?

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:242
> +    for (auto& promise : promptPromises) {

Could do a one-liner like for (auto& promise : std::exchange(promptPromises, { }))
Or use auto promptPromises = ...

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:273
> +        m_eventQueue.enqueueEvent(Event::create(eventNames().connectEvent, Event::CanBubble::No, Event::IsCancelable::No));

So we have two queues, one for tasks and one for events.
Can it create some flaky behaviours, like sometime a task is run before the event is fired and sometime the reverse?
Should we have just one task queue and use it for events?

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:344
> +    PromiseVector promptPromises = WTFMove(m_promptPromises);

auto/std::exchange

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:392
> +            callbacks.uncheckedAppend(callback.copyRef());

Would the following work?
auto callbacks = WTF::map(m_callbackMap.values(), [](auto& callback) { return callback.copyRef(); });

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:418
> +    case State::Connected:

Do we want to return true for connected state?

> Source/WebCore/html/HTMLMediaElement.cpp:470
> +    , m_remote(RemotePlayback::create(*this))

Does it make sense to lazily initialise it?

> Source/WebCore/page/Page.cpp:2575
> +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {

auto*

> Source/WebCore/testing/Internals.cpp:4144
> +    Page* page = contextDocument()->frame()->page();

auto

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:342
> +WK_EXPORT void WKPreferencesSetRemotePlaybackEnabled(WKPreferencesRef preferencesRef, bool enabled);

We usually want to have a corresponding ObjC API.

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:510
> +    PlaybackTargetPickerWasDismissed(uint64_t contextId);

Would be nice to use ObjectIdentifier if possible.

> LayoutTests/imported/w3c/resources/import-expectations.json:197
> +    "web-platform-tests/html/infrastructure": "skip", 

unnecessary change.

[Jer Noble]

(In reply to youenn fablet from comment #48)
> Comment on attachment 378947 [details]
> Patch
> 
> LGTM once bots are green.
> Bots are red because of the missing -expected.txt I believe but all tests
> seem to be PASS.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378947&action=review
> 
> > Source/WebCore/Modules/remoteplayback/HTMLMediaElementRemotePlayback.h:2
> > + * Copyright (C) 2016 Apple Inc. All rights reserved.
> 
> 2019

Fixed.

> > Source/WebCore/Modules/remoteplayback/HTMLMediaElementRemotePlayback.idl:2
> > + * Copyright (C) 2016 Apple Inc. All rights reserved.
> 
> 2019

Fixed.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:50
> > +    , m_mediaElement(element.weakPtrFactory().createWeakPtr(element))
> 
> makeWeakPtr(element)

Fixed.

> >>> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:95
> >>> +        promise->whenSettled([this, strongThis = makeRefPtr(this), callbackId] {
> >> 
> >> I needed to make this change in order to pass one of the WPT tests that requires the promise to fire before the callback is called.
> > 
> > This method is returning a promise and calling a completion callback.
> > I guess this is the migrate-from-callback-to-promise pattern.
> > Do we care of contents using the callback? Can we just ship the promise version?
> 
> Jer explained me the API, which returns a promise and then calls the
> callback repeatedly.
> Seems a bit strange but this is the current spec.
> 
> s/strongThis/protectedThis/

Fixed.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:104
> > +                    protectedCallback->handleEvent(m_available);
> 
> Do we need protectedCallback?

Nope!

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:242
> > +    for (auto& promise : promptPromises) {
> 
> Could do a one-liner like for (auto& promise : std::exchange(promptPromises,
> { }))
> Or use auto promptPromises = ...

Did the std::exchange change.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:273
> > +        m_eventQueue.enqueueEvent(Event::create(eventNames().connectEvent, Event::CanBubble::No, Event::IsCancelable::No));
> 
> So we have two queues, one for tasks and one for events.
> Can it create some flaky behaviours, like sometime a task is run before the
> event is fired and sometime the reverse?
> Should we have just one task queue and use it for events?

Nope, the event queues use task queues, and all task queues fire in the order items are added to the queues globally.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:344
> > +    PromiseVector promptPromises = WTFMove(m_promptPromises);
> 
> auto/std::exchange

Changed.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:392
> > +            callbacks.uncheckedAppend(callback.copyRef());
> 
> Would the following work?
> auto callbacks = WTF::map(m_callbackMap.values(), [](auto& callback) {
> return callback.copyRef(); });

Nope! Gives a crazy template error about `type` not being defined on std::result_of<....lambda>.

> > Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:418
> > +    case State::Connected:
> 
> Do we want to return true for connected state?

Yes!

> > Source/WebCore/html/HTMLMediaElement.cpp:470
> > +    , m_remote(RemotePlayback::create(*this))
> 
> Does it make sense to lazily initialise it?

Not really. It's a very lightweight class.

> > Source/WebCore/page/Page.cpp:2575
> > +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {
> 
> auto*

Changed.

> > Source/WebCore/testing/Internals.cpp:4144
> > +    Page* page = contextDocument()->frame()->page();
> 
> auto

Changed.

> > Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:342
> > +WK_EXPORT void WKPreferencesSetRemotePlaybackEnabled(WKPreferencesRef preferencesRef, bool enabled);
> 
> We usually want to have a corresponding ObjC API.

Added.

> > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:510
> > +    PlaybackTargetPickerWasDismissed(uint64_t contextId);
> 
> Would be nice to use ObjectIdentifier if possible.

We'd have to change all the methods in this area at once.

> > LayoutTests/imported/w3c/resources/import-expectations.json:197
> > +    "web-platform-tests/html/infrastructure": "skip", 
> 
> unnecessary change.

Removed.

[Jer Noble]

Created attachment 380579 [details]
Patch for landing

[Jer Noble]

Created attachment 382083 [details]
Patch for landing

[EWS Watchlist]

Comment on attachment 382083 [details]
Patch for landing

Attachment 382083 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13185945

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases

[Jer Noble]

Created attachment 382194 [details]
Patch for landing

[Jer Noble]

Created attachment 382200 [details]
Patch for landing

[Jer Noble]

Committed r251737: <https://trac.webkit.org/changeset/251737>

[Truitt Savell]

The new test media/remoteplayback-prompt.html

is a flakey timeout on Mojave Release so far.

History:
https://results.webkit.org/?suite=layout-tests&test=media%2Fremoteplayback-prompt.html