As the subject says. This is split out of https://bugs.webkit.org/show_bug.cgi?id=219368
Created attachment 415102 [details] Patch
Committed r270286: <https://trac.webkit.org/changeset/270286> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415102 [details].
<rdar://problem/71840749>
Comment on attachment 415102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415102&action=review > Source/WebKit/Shared/ConcreteShareableImageBuffer.h:34 > +class ConcreteShareableImageBuffer : public WebCore::ConcreteImageBuffer<BackendType> { If you expose the backend from ImageBuffer (see below), I think you will not need this class. > Source/WebKit/Shared/ConcreteShareableImageBuffer.h:39 > + static auto create(const WebCore::FloatSize& size, WebCore::RenderingMode renderingMode, float resolutionScale, WebCore::ColorSpace colorSpace, WebCore::PixelFormat pixelFormat) Unused argument renderingMode. > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:119 > + if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) { > + if (m_acceleratesDrawing) > + handle = static_cast<AcceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > + else > + handle = static_cast<UnacceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > + } else { > + if (m_acceleratesDrawing) > + handle = static_cast<ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > + else > + handle = static_cast<ConcreteShareableImageBuffer<UnacceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > + } Can't this casting be cleaned by exposing the ImageBufferBackend from ImageBuffer? This code can be simplified like this: if (m_acceleratesDrawing) handle = static_cast<ImageBufferShareableIOSurfaceBackend&>(*m_frontBuffer.imageBuffer->backend()).createImageBufferBackendHandle(); else handle = static_cast<ImageBufferShareableBitmapBackend&>(*m_frontBuffer.imageBuffer->backend()).createImageBufferBackendHandle(); > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:196 > + m_frontBuffer.imageBuffer = ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend>::create(backingStoreSize(), WebCore::RenderingMode::Accelerated, 1, WebCore::ColorSpace::SRGB, pixelFormat()); No need to pass the rendering mode to a concrete ImageBuffer since the backend type already reveals the type of the backend.
Comment on attachment 415102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415102&action=review >> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:196 >> + m_frontBuffer.imageBuffer = ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend>::create(backingStoreSize(), WebCore::RenderingMode::Accelerated, 1, WebCore::ColorSpace::SRGB, pixelFormat()); > > No need to pass the rendering mode to a concrete ImageBuffer since the backend type already reveals the type of the backend. I meant to say "... the backend type reveals the renderingMode".
(In reply to Said Abou-Hallawa from comment #4) > Comment on attachment 415102 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415102&action=review > > > Source/WebKit/Shared/ConcreteShareableImageBuffer.h:34 > > +class ConcreteShareableImageBuffer : public WebCore::ConcreteImageBuffer<BackendType> { > > If you expose the backend from ImageBuffer (see below), I think you will not > need this class. > > > Source/WebKit/Shared/ConcreteShareableImageBuffer.h:39 > > + static auto create(const WebCore::FloatSize& size, WebCore::RenderingMode renderingMode, float resolutionScale, WebCore::ColorSpace colorSpace, WebCore::PixelFormat pixelFormat) > > Unused argument renderingMode. > > > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:119 > > + if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) { > > + if (m_acceleratesDrawing) > > + handle = static_cast<AcceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > > + else > > + handle = static_cast<UnacceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > > + } else { > > + if (m_acceleratesDrawing) > > + handle = static_cast<ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > > + else > > + handle = static_cast<ConcreteShareableImageBuffer<UnacceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle(); > > + } > > Can't this casting be cleaned by exposing the ImageBufferBackend from > ImageBuffer? This code can be simplified like this: > > if (m_acceleratesDrawing) > handle = > static_cast<ImageBufferShareableIOSurfaceBackend&>(*m_frontBuffer. > imageBuffer->backend()).createImageBufferBackendHandle(); > else > handle = > static_cast<ImageBufferShareableBitmapBackend&>(*m_frontBuffer.imageBuffer- > >backend()).createImageBufferBackendHandle(); I am hoping to figure out how to finagle createImageBufferBackendHandle onto ImageBuffer, so we don't have /any/ bifurcation or static casts. You're right that it will obsolete most of this patch, but it's trickier, and the invalid cast seemed ... very bad. > > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:196 > > + m_frontBuffer.imageBuffer = ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend>::create(backingStoreSize(), WebCore::RenderingMode::Accelerated, 1, WebCore::ColorSpace::SRGB, pixelFormat()); > > No need to pass the rendering mode to a concrete ImageBuffer since the > backend type already reveals the type of the backend. Good point.