[webkit-reviews] review granted: [Bug 205379] Add remote media resource loader for the GPU process : [Attachment 386516] Update the patch based on comments from Youenn and Eric

[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 386516: Update the patch based on comments from Youenn and Eric

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




--- Comment #21 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 386516
  --> https://bugs.webkit.org/attachment.cgi?id=386516
Update the patch based on comments from Youenn and Eric

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

> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:88
> +	   m_client->redirectReceived(*this, WTFMove(request), response,
[](auto&&) { });

See comment below, we should probably pass a real completion handler here.

> Source/WebKit/GPUProcess/media/RemoteMediaResource.h:53
> +    bool shouldCacheResponse(const WebCore::ResourceResponse&);

This does not seem to be used right now, should we remove the code from now?

> Source/WebKit/GPUProcess/media/RemoteMediaResource.h:67
> +    bool m_ready;

/sm_ready;/m_ready { false };
And remove init in constructor.

> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:66
> +    if (!resource || !resource->ready()) {

I am not exactly sure what the ready check is supposed to cover.
Instead of doing the ready check, could we simply add resource to
m_remoteMediaResources in the asyncReply of
m_webProcessConnection->sendWithAsyncReply(Messages::RemoteMediaPlayerManager::
RequestResource?
Then the if (!resource) would cover both cases.
In practice, I do not see how ready() could be false in any of the  methods
called from IPC.

> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:82
> +    m_remoteMediaResources.get(id)->redirectReceived(WTFMove(request),
response);

In theory, the call to redirectReceived could mutate the request.
We should then have completionHandler be a
CompletionHandler<void(ResourceRequest&&)>
And we would write something like
m_remoteMediaResources.get(id)->redirectReceived(WTFMove(request), response,
WTFMove(completionHandler));

In the if (!resource) case, we would call completionHandler({ }) which would
probably cancel the load in WebProcess side.

> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:59
> +	   completionHandler(WTFMove(request));

Then here we would not need to capture request since it would be given by the
async reply.

> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:65
> +    return true;

We should probably return false, or return what the GPU Process would tell us
through sync IPC.