[webkit-reviews] review granted: [Bug 232844] [GPU Process] [Filters 21/23] Add the encoding/decoding for Filter and FilterEffect : [Attachment 445983] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 232844: [GPU Process] [Filters 21/23] Add the encoding/decoding for Filter
and FilterEffect
https://bugs.webkit.org/show_bug.cgi?id=232844

Attachment 445983: Patch

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




--- Comment #7 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 445983
  --> https://bugs.webkit.org/attachment.cgi?id=445983
Patch

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

> Source/WebCore/platform/graphics/filters/FEColorMatrix.h:99
> +template<> struct EnumTraits<WebCore::ColorMatrixType> {

Nit - we should consider making ColorMatrixType an `enum class : uint8_t`,
perhaps in a separate patch.

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:179
> +	   WebCore::ComponentTransferType,

We should consider making ComponentTransferType an enum class as well (in a
followup).

> Source/WebCore/platform/graphics/filters/FEComposite.h:137
> +	   WebCore::CompositeOperationType,

(Ditto for CompositeOperationType)

> Source/WebCore/platform/graphics/filters/FEMerge.h:43
> +    unsigned m_numberOfEffectInputs;

Nit - let's make this initialize to `{ 0 };`

> Source/WebCore/platform/graphics/filters/Filter.h:79
> +    using FilterFunction::FilterFunction;

Is this necessary? I don't see any nested `FilterFunction` symbol defined in
the `FilterFunction` class (or a separate namespace called `FilterFunction`)

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:229
> +    auto sendResult =
sendSync(Messages::RemoteRenderingBackend::GetFilteredImageForImageBuffer(image
Buffer, IPC::FilterReference { filter }),
Messages::RemoteRenderingBackend::GetFilteredImageForImageBuffer::Reply(handle)
, renderingBackendIdentifier(), 1_s);

I'm a little surprised that this actually works, given that RRB only receives
stream messages. Did you mean to use sendSyncToStream() instead?