[webkit-reviews] review granted: [Bug 162971] Implement the Remote Playback API. : [Attachment 378947] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 162971: Implement the Remote Playback API.
https://bugs.webkit.org/show_bug.cgi?id=162971

Attachment 378947: Patch

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




--- Comment #48 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 378947
  --> https://bugs.webkit.org/attachment.cgi?id=378947
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.