[webkit-reviews] review granted: [Bug 213673] CoreImage Implementation of SourceGraphic and saturate(), hue-rotate(), grayscale() and sepia() : [Attachment 407790] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted guowei_yang at apple.com's request for
review:
Bug 213673: CoreImage Implementation of SourceGraphic and saturate(),
hue-rotate(), grayscale() and sepia()
https://bugs.webkit.org/show_bug.cgi?id=213673

Attachment 407790: Patch

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




--- Comment #25 from Darin Adler <darin at apple.com> ---
Comment on attachment 407790
  --> https://bugs.webkit.org/attachment.cgi?id=407790
Patch

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

Looks good. A few optional ideas for further refinement.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:53
> +    static NeverDestroyed<RetainPtr<CIContext>> ciContext = [CIContext
contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge
id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}];

Another way to write this is:

    static auto context = makeNeverDestroyed([CIContext ...]);

I think it’s nicer.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:156
> +    ImageBuffer* sourceImage = effect.filter().sourceImage();

auto

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:170
> +    Vector<float> values = FEColorMatrix::normalizedFloats(effect.values());

auto

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:190
> +    CIFilter *colorMatrixFilter = [CIFilter
filterWithName:@"CIColorMatrix"];

auto

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:252
> +    auto ciContext = getCIContext();

Not sure we need a local variable.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:179
> +	   Vector<float> normalizedValues(values.size());
> +	   for (size_t i = 0; i < values.size(); ++i)
> +	       normalizedValues[i] = normalizedFloat(values[i]);
> +	   return normalizedValues;

This could be nicer if it used Vector::map instead of writing a loop.

> Source/WebCore/rendering/CSSFilter.cpp:343
> -	   setSourceImage(ImageBuffer::create(logicalSize, renderingMode(),
filterScale()));
> +	   if (m_filterRenderer)
> +	       setSourceImage(ImageBuffer::create(logicalSize,
RenderingMode::Accelerated, filterScale()));
> +	   else
> +	       setSourceImage(ImageBuffer::create(logicalSize, renderingMode(),
filterScale()));

Could do it with a local instead of an if statement:

    auto mode = m_filterRenderer ? RenderingMode::Accelerated :
renderingMode();
    setSourceImage(ImageBuffer::create(logicalSize, mode, filterScale()));