[webkit-reviews] review granted: [Bug 233849] [GPU Process] (REGRESSION r285597): Set the filterRegion of the CSSFilter after it is created : [Attachment 445961] Patch

[bugzilla-daemon at webkit.org]

Cameron McCormack (:heycam) <heycam at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 233849: [GPU Process] (REGRESSION r285597): Set the filterRegion of the
CSSFilter after it is created
https://bugs.webkit.org/show_bug.cgi?id=233849

Attachment 445961: Patch

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




--- Comment #4 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 445961
  --> https://bugs.webkit.org/attachment.cgi?id=445961
Patch

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

> Source/WebCore/ChangeLog:27
> +	      b) Recreate the sourceImage if needed.
> +	      c) setup the context for drawing the target renderer.

"e)" and "f)"

> Source/WebCore/rendering/RenderLayerFilters.cpp:174
> +	   filterRegion += filter.outsets();
> +	   filterRegion = intersection(filterBoxRect, filterRegion);

What about:

filterRegion += filter.outsets();
filterRegion.intersect(filterBoxRect);

> Source/WebCore/rendering/RenderLayerFilters.cpp:185
> +    if (m_sourceImageRect != filterRegion) {
> +	   m_sourceImageRect = filterRegion;
> +	   hasUpdatedBackingStore = true;
>      }

After this point, m_sourceImageRect and filterRegion are the same, so I think
we should consistently use one or the other in the remainder of the function.

> Source/WebCore/rendering/RenderLayerFilters.cpp:202
> +    filter.setFilterRegion(m_sourceImageRect);

So here I think we should use filterRegion.

> Source/WebCore/rendering/RenderLayerFilters.cpp:-189
> -    if (!sourceGraphicsContext || filter.filterRegion().isEmpty() ||
ImageBuffer::sizeNeedsClamping(filter.filterRegion().size()))

In this old code, how would it be that ImageBuffer::sizeNeedsClamping would
return true, since earlier we call clampFilterRegionIfNeeded?

> Source/WebCore/svg/graphics/filters/SVGFilter.h:50
> +    SVGFilter(RenderingMode, const FloatSize& filterScale, ClipOperation,
const FloatRect& filterRegion, const FloatRect& targetBoundingBox, 
SVGUnitTypes::SVGUnitType primitiveUnits);

Nit: two spaces after "targetBoundingBox,".