[webkit-reviews] review granted: [Bug 232469] [GPU Process] [Filters 3/] Make CSSFilter holds a list of FilterFunctions : [Attachment 442785] Patch for review

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 232469: [GPU Process]  [Filters 3/] Make CSSFilter holds a list of
FilterFunctions
https://bugs.webkit.org/show_bug.cgi?id=232469

Attachment 442785: Patch for review

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




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 442785
  --> https://bugs.webkit.org/attachment.cgi?id=442785
Patch for review

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

> Source/WebCore/ChangeLog:8
> +	   In this patch, the CSS referenced filter is built as an SVGFilter
and it

"reference filter"

> Source/WebCore/css/CSSFilterImageValue.cpp:120
> +    auto texture = ImageBuffer::create(size, renderingMode, 1,
DestinationColorSpace::SRGB(), PixelFormat::BGRA8);

texture is a weird name for this.

> Source/WebCore/css/CSSFilterImageValue.cpp:130
> +    auto cssFilter = CSSFilter::create(m_filterOperations, renderingMode);
> +    
>      cssFilter->setSourceImageRect(imageRect);
>      if (!cssFilter->build(renderer, m_filterOperations,
FilterConsumer::FilterFunction))

Why do we have to pass m_filterOperations both to the constructor, and to the
build() function?

> Source/WebCore/rendering/CSSFilter.cpp:223
> +    auto builder = makeUnique<SVGFilterBuilder>();

Can SVGFilterBuilder just be on the stack?

> Source/WebCore/rendering/RenderLayerFilters.h:66
> +    GraphicsContext* beginFilterEffect(RenderElement&, const LayoutRect&
filterBoxRect, const LayoutRect& dirtyRect, const LayoutRect& layerRepaintRect,
GraphicsContext& destinationContext);

I tend to prefer GraphicsContext& destinationContext as the first argument.

> Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:436
> +	   auto builder = makeUnique<SVGFilterBuilder>();

Can this live on the stack?