[webkit-reviews] review granted: [Bug 212649] Multiple SVG Filters Unexpectedly lightens image using linearRGB : [Attachment 401479] Patch - resolved a merge conflict

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted guowei_yang at apple.com's request for
review:
Bug 212649: Multiple SVG Filters Unexpectedly lightens image using linearRGB
https://bugs.webkit.org/show_bug.cgi?id=212649

Attachment 401479: Patch - resolved a merge conflict

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




--- Comment #40 from Darin Adler <darin at apple.com> ---
Comment on attachment 401479
  --> https://bugs.webkit.org/attachment.cgi?id=401479
Patch - resolved a merge conflict

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

Looks OK to me; Simon may also want to review again.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:460
> +    auto convertedData = convertedBuffer->getImageData(outputFormat, {
IntPoint(), convertedBuffer->logicalSize() });
> +    return convertedData;

No need for the local variable here. Can just return.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:65
> +    

Please don’t add the whitespace here.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:67
> +    void copyPremultipliedResult(Uint8ClampedArray& destination, const
IntRect&,  Optional<ColorSpace> = WTF::nullopt);

Extra space before the word Optional here.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:68
> +    

Please don’t add the trailing whitespace here.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:184
> +    RefPtr<ImageData> convertImageDataToColorSpace(ColorSpace,
RefPtr<ImageData>, AlphaPremultiplication);

It’s peculiar to take a RefPtr<ImageData> here. I’d expect it to be ImageData&
instead. Implementation can’t handle a null.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:185
> +    RefPtr<ImageData> convertImageBufferToColorSpace(ColorSpace,
ImageBuffer*, AlphaPremultiplication);

Argument type should be ImageBuffer&. Implementation can’t handle a null.