Bug 231484 - Introduce RemoteDisplayListRecorderProxy and RemoteDisplayListRecorder
Summary: Introduce RemoteDisplayListRecorderProxy and RemoteDisplayListRecorder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 230860
  Show dependency treegraph
 
Reported: 2021-10-09 17:06 PDT by Wenson Hsieh
Modified: 2021-10-11 15:38 PDT (History)
10 users (show)

See Also:


Attachments
For EWS (96.24 KB, patch)
2021-10-10 16:06 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
For EWS (96.77 KB, patch)
2021-10-10 16:34 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
For EWS (97.24 KB, patch)
2021-10-10 17:18 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix typo (97.21 KB, patch)
2021-10-10 17:32 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (97.22 KB, patch)
2021-10-11 12:45 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-10-09 17:06:47 PDT
.
Comment 1 Wenson Hsieh 2021-10-10 16:06:53 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-10-10 16:34:55 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-10-10 17:18:44 PDT Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-10-10 17:32:11 PDT
Created attachment 440746 [details]
Fix typo
Comment 5 Simon Fraser (smfr) 2021-10-11 11:07:36 PDT
Comment on attachment 440746 [details]
Fix typo

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

> Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:46
> +    static Ref<RemoteDisplayListRecorder> create(WebCore::ImageBuffer& imageBuffer, QualifiedRenderingResourceIdentifier imageBufferIdentifier, RemoteRenderingBackend& renderingBackend)

Can this ImageBuffer type be template-qualified so that we know at compile time that it's the right kind of image buffer?

> Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:52
> +    void startListeningForIPC();
> +    void stopListeningForIPC();

These feel a bit weird on this API.

> Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h:42
> +    RemoteDisplayListRecorderProxy(WebCore::ImageBuffer&, RemoteRenderingBackendProxy&, const WebCore::FloatRect& initialClip, const WebCore::AffineTransform&, WebCore::DrawGlyphsRecorder::DeconstructDrawGlyphs = WebCore::DrawGlyphsRecorder::DeconstructDrawGlyphs::Yes);

Same question: can ImageBuffer be specialized? It's a concrete image buffer here, right?

> Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h:45
> +    ~RemoteDisplayListRecorderProxy() { }

= default?
Comment 6 Wenson Hsieh 2021-10-11 12:35:54 PDT
Comment on attachment 440746 [details]
Fix typo

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

Thanks for the review!

>> Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:46
>> +    static Ref<RemoteDisplayListRecorder> create(WebCore::ImageBuffer& imageBuffer, QualifiedRenderingResourceIdentifier imageBufferIdentifier, RemoteRenderingBackend& renderingBackend)
> 
> Can this ImageBuffer type be template-qualified so that we know at compile time that it's the right kind of image buffer?

I think it's possible, but then we'll need to templatize RemoteDisplayListRecorder with an ImageBufferBackendType as well, since the remote image buffer could be a AcceleratedRemoteImageBuffer or UnacceleratedRemoteImageBuffer.
This means we'll need to teach the IPC message receiver generator to take template arguments for a given message receiver, such that the generated message receiver implementation can take this form:

```
template <typename ImageBufferBackendType>
void RemoteDisplayListRecorder<ImageBufferBackendType>::didReceiveStreamMessage(IPC::StreamServerConnectionBase& connection, IPC::Decoder& decoder)
```

Let's tackle this separately in a followup bug.

>> Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:52
>> +    void stopListeningForIPC();
> 
> These feel a bit weird on this API.

I believe `(start|stop)ListeningForIPC` is the established pattern for setting up and tearing down IPC message receivers in the GPU Process; but now that I think more about it, we only need to expose `stopListeningForIPC()` here to make it work with ScopedActiveMessageReceiveQueue, since we should always `startListeningForIPC()` right after creating this object anyways.

I'll adjust this so that `startListeningForIPC()` is just a private helper method that's invoked in `RemoteDisplayListRecorder::create`, which is consistent with `RemoteRenderingBackend::create`.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h:45
>> +    ~RemoteDisplayListRecorderProxy() { }
> 
> = default?

Fixed!
Comment 7 Wenson Hsieh 2021-10-11 12:45:07 PDT
Created attachment 440824 [details]
For EWS
Comment 8 EWS 2021-10-11 15:37:12 PDT
Committed r283941 (242800@main): <https://commits.webkit.org/242800@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440824 [details].
Comment 9 Radar WebKit Bug Importer 2021-10-11 15:38:23 PDT
<rdar://problem/84119482>