[webkit-reviews] review granted: [Bug 205379] Add remote media resource loader for the GPU process : [Attachment 386484] Patch for review (fixed gtk and wpe build failures)

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 205379: Add remote media resource loader for the GPU process
https://bugs.webkit.org/show_bug.cgi?id=205379

Attachment 386484: Patch for review (fixed gtk and wpe build failures)

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




--- Comment #15 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 386484
  --> https://bugs.webkit.org/attachment.cgi?id=386484
Patch for review (fixed gtk and wpe build failures)

Please fix RemoteMediaResourceManager.cpp.
We also might not need to implement some WebCore interfaces, which would
simplify the implementation, in this patch or as a follow-up.
I am thinking of RemoteMediaResourceProxy for instance.

Would it be possible add a test as well?
One possibility is to enable media loading from GPU Process for just this test.
Then make a page that is controlled by a service worker, the page then triggers
a media load that is intercepted by the service worker.
The test would succeed as soon as the load is intercepted by the service
worker.
You can look at LayoutTests/http/wpt/service-workers or
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker for
service worker tests.

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

> Source/WebCore/loader/MediaResourceLoader.cpp:175
> +	   m_client->responseReceived(*this, response, [this, protectedThis =
makeRef(*this), completionHandler = completionHandlerCaller.release()]
(PolicyChecker::ShouldContinue shouldContinue) mutable {

Could use auto

> Source/WebCore/loader/PolicyChecker.h:75
> +    using NewWindowPolicyDecisionFunction = CompletionHandler<void(const
ResourceRequest&, WeakPtr<FormState>&&, const String& frameName, const
NavigationAction&, PolicyChecker::ShouldContinue)>;

s/PolicyChecker::ShouldContinue/ShouldContinue/

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:110
> +} // namespace WebKit

This patch is already big so we could also have separated these refactoring in
preliminary patches.
This makes reviews/rollback/diagnosis easier.

> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:73
> +    if (m_client) {

Usually we do if(!m_client) return if the follow-up code is multi line.

> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:95
> +    return true;

One-liner maybe?

> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:48
> +    return m_remoteMediaPlayerProxy->requestResource(WTFMove(request),
options);

How do we know that m_remoteMediaPlayerProxy is not null?

> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:44
> +    RefPtr<WebCore::PlatformMediaResource>
requestResource(WebCore::ResourceRequest&&, LoadOptions) final;

private?

> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:74
> +    if (!m_remoteMediaResources.get(id)->ready()) {

We probably cannot guarantee that m_remoteMediaResources.get(id) returns non
null given it is coming from IPC.
The easiest would be to do something like:
auto* resource = m_remoteMediaResources.get(id);
if (!resource || !resource->ready())
    return completionHandler();
...
Ditto for other methods called from IPC.

> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:79
> +    RELEASE_ASSERT(m_remoteMediaResources.contains(id) &&
m_remoteMediaResources.get(id)->ready());

This RELEASE_ASSERT is covered by the above if check.

> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:39
> +#include <wtf/WeakPtr.h>

We do not need WeakPtr.
We could probably forward declare Connection, Decoder, DataReference,
ResourceRequest and remove the corresponding includes.
We probably need in theory HashMap.h

> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:52
> +    void addMediaResource(RemoteMediaResourceIdentifier,
RemoteMediaResource*);

Would probably be better as a RemoteMediaResource&

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:66
> +    , m_mediaResourceLoader(player->createResourceLoader())

Since we are in MediaPlayerPrivateRemote, we could try to create our own
resource loader that would not need to implement PlatformMediaResourceLoader.
Maybe not for this patch though.
It seems that we could even remove m_mediaResourceLoader and just do IPC to
NetworkProcess from MediaPlayerPrivateRemote::requestResource

I also wonder whether we need RemoteMediaResourceProxy to implement
WebCore::PlatformMediaResourceClient.
It seems this object does not surface at WebCore level.
That could further simplify the code.

> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:38
> +RemoteMediaResourceProxy::RemoteMediaResourceProxy(Ref<IPC::Connection>&&
connection, WebCore::PlatformMediaResource* platformMediaResource,
RemoteMediaResourceIdentifier id)

We can probably take a PlatformMediaResource&