Bug 231484 – Introduce RemoteDisplayListRecorderProxy and RemoteDisplayListRecorder

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 440739 [details]
For EWS

[Wenson Hsieh]

Created attachment 440740 [details]
For EWS

[Wenson Hsieh]

Created attachment 440745 [details]
For EWS

[Wenson Hsieh]

Created attachment 440746 [details]
Fix typo

[Simon Fraser (smfr)]

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?

[Wenson Hsieh]

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!

[Wenson Hsieh]

Created attachment 440824 [details]
For EWS

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/84119482>