Implement the Remote Playback API.
Created attachment 290712 [details] Patch
<rdar://problem/28632488>
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
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 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
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 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
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 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
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 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.
(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.
(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.
(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.
Created attachment 291121 [details] Patch
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 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
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 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
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 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
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 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
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
(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.
Created attachment 291276 [details] Patch
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 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
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 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
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 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
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
Created attachment 292263 [details] Patch
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.
The Second Screen WG is interested in seeing a second implementation of RemotePlayback API. I'll be rebasing this patch to apply against ToT.
Created attachment 375978 [details] Patch
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.
Created attachment 376085 [details] Patch
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.
Created attachment 376088 [details] Patch
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 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?
(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.
Created attachment 378947 [details] Patch
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.
(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 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.
(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.
Created attachment 380579 [details] Patch for landing
Created attachment 382083 [details] Patch for landing
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
Created attachment 382194 [details] Patch for landing
Created attachment 382200 [details] Patch for landing
Committed r251737: <https://trac.webkit.org/changeset/251737>
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