Bug 162971 - Implement the Remote Playback API.
Summary: Implement the Remote Playback API.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on: 163044
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-05 08:42 PDT by Jer Noble
Modified: 2019-10-30 08:05 PDT (History)
32 users (show)

See Also:


Attachments
Patch (106.62 KB, patch)
2016-10-05 09:15 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (1.75 MB, application/zip)
2016-10-05 10:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2 (deleted)
2016-10-05 10:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.30 MB, application/zip)
2016-10-05 11:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (1.01 MB, application/zip)
2016-10-05 11:13 PDT, Build Bot
no flags Details
Patch (119.35 KB, patch)
2016-10-10 10:51 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (916.12 KB, application/zip)
2016-10-10 12:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.70 MB, application/zip)
2016-10-10 12:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.29 MB, application/zip)
2016-10-10 12:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (deleted)
2016-10-10 12:27 PDT, Build Bot
no flags Details
Patch (120.60 KB, patch)
2016-10-11 10:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.01 MB, application/zip)
2016-10-11 12:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.34 MB, application/zip)
2016-10-11 12:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.84 MB, application/zip)
2016-10-11 12:18 PDT, Build Bot
no flags Details
Patch (121.31 KB, patch)
2016-10-20 14:30 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (118.96 KB, patch)
2019-08-09 16:23 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (119.85 KB, patch)
2019-08-12 12:49 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (119.86 KB, patch)
2019-08-12 13:08 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (138.91 KB, patch)
2019-09-17 00:59 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (141.16 KB, patch)
2019-10-09 15:42 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (141.00 KB, patch)
2019-10-28 11:05 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (141.58 KB, patch)
2019-10-29 11:05 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (141.53 KB, patch)
2019-10-29 11:47 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2016-10-05 08:42:10 PDT
Implement the Remote Playback API.
Comment 1 Jer Noble 2016-10-05 09:15:36 PDT
Created attachment 290712 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2016-10-05 09:15:51 PDT
<rdar://problem/28632488>
Comment 3 Build Bot 2016-10-05 10:39:53 PDT
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
Comment 4 Build Bot 2016-10-05 10:39:56 PDT
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
Comment 5 Build Bot 2016-10-05 10:50:23 PDT
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
Comment 6 Build Bot 2016-10-05 10:50:27 PDT
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
Comment 7 Build Bot 2016-10-05 11:05:45 PDT
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
Comment 8 Build Bot 2016-10-05 11:05:49 PDT
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
Comment 9 Build Bot 2016-10-05 11:13:14 PDT
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
Comment 10 Build Bot 2016-10-05 11:13:18 PDT
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
Comment 11 Eric Carlson 2016-10-05 20:12:47 PDT
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.
Comment 12 Jer Noble 2016-10-06 08:30:00 PDT
(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.
Comment 13 Jer Noble 2016-10-06 09:14:04 PDT
(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.
Comment 14 Jer Noble 2016-10-06 11:46:48 PDT
(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.
Comment 15 Jer Noble 2016-10-10 10:51:27 PDT
Created attachment 291121 [details]
Patch
Comment 16 WebKit Commit Bot 2016-10-10 10:54:09 PDT
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.
Comment 17 Build Bot 2016-10-10 12:11:29 PDT
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
Comment 18 Build Bot 2016-10-10 12:11:32 PDT
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
Comment 19 Build Bot 2016-10-10 12:21:34 PDT
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
Comment 20 Build Bot 2016-10-10 12:21:38 PDT
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
Comment 21 Build Bot 2016-10-10 12:25:29 PDT
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
Comment 22 Build Bot 2016-10-10 12:25:33 PDT
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
Comment 23 Build Bot 2016-10-10 12:27:17 PDT
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
Comment 24 Build Bot 2016-10-10 12:27:21 PDT
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
Comment 25 Jer Noble 2016-10-11 10:15:35 PDT
(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.
Comment 26 Jer Noble 2016-10-11 10:44:04 PDT
Created attachment 291276 [details]
Patch
Comment 27 WebKit Commit Bot 2016-10-11 10:46:37 PDT
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.
Comment 28 Build Bot 2016-10-11 12:03:59 PDT
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
Comment 29 Build Bot 2016-10-11 12:04:03 PDT
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
Comment 30 Build Bot 2016-10-11 12:18:08 PDT
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
Comment 31 Build Bot 2016-10-11 12:18:12 PDT
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
Comment 32 Build Bot 2016-10-11 12:18:19 PDT
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
Comment 33 Build Bot 2016-10-11 12:18:23 PDT
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
Comment 34 Jer Noble 2016-10-20 14:30:33 PDT
Created attachment 292263 [details]
Patch
Comment 35 Brady Eidson 2018-02-14 10:34:37 PST
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.
Comment 36 Jer Noble 2018-10-25 02:23:04 PDT
The Second Screen WG is interested in seeing a second implementation of RemotePlayback API. I'll be rebasing this patch to apply against ToT.
Comment 37 Jer Noble 2019-08-09 16:23:59 PDT
Created attachment 375978 [details]
Patch
Comment 38 Build Bot 2019-08-09 16:27:21 PDT
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.
Comment 39 Jer Noble 2019-08-12 12:49:39 PDT
Created attachment 376085 [details]
Patch
Comment 40 Build Bot 2019-08-12 12:52:00 PDT
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.
Comment 41 Jer Noble 2019-08-12 13:08:23 PDT
Created attachment 376088 [details]
Patch
Comment 42 Build Bot 2019-08-12 13:11:43 PDT
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.
Comment 43 youenn fablet 2019-08-23 06:50:23 PDT
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?
Comment 44 Jer Noble 2019-09-16 21:20:46 PDT
(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.
Comment 45 Jer Noble 2019-09-17 00:59:43 PDT
Created attachment 378947 [details]
Patch
Comment 46 Jer Noble 2019-09-17 01:02:21 PDT
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.
Comment 47 youenn fablet 2019-09-17 01:12:04 PDT
(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?
Comment 48 youenn fablet 2019-09-17 17:43:48 PDT
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.
Comment 49 Jer Noble 2019-10-07 14:31:53 PDT
(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.
Comment 50 Jer Noble 2019-10-09 15:42:16 PDT
Created attachment 380579 [details]
Patch for landing
Comment 51 Jer Noble 2019-10-28 11:05:12 PDT
Created attachment 382083 [details]
Patch for landing
Comment 52 Build Bot 2019-10-28 15:26:39 PDT
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
Comment 53 Jer Noble 2019-10-29 11:05:46 PDT
Created attachment 382194 [details]
Patch for landing
Comment 54 Jer Noble 2019-10-29 11:47:48 PDT
Created attachment 382200 [details]
Patch for landing
Comment 55 Jer Noble 2019-10-29 14:54:23 PDT
Committed r251737: <https://trac.webkit.org/changeset/251737>
Comment 56 Truitt Savell 2019-10-30 08:05:51 PDT
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