[webkit-reviews] review granted: [Bug 71930] Filters need to affect visual overflow : [Attachment 119471] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 71930: Filters need to affect visual overflow
https://bugs.webkit.org/show_bug.cgi?id=71930

Attachment 119471: Patch
https://bugs.webkit.org/attachment.cgi?id=119471&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119471&action=review


> Source/WebCore/rendering/FilterEffectRenderer.h:74
> +    IntRect outputRect() { return lastEffect()->hasResult() ?
lastEffect()->requestedRegionOfInputImageData(IntRect(m_filterRegion)) :
IntRect(); }

Should be const.

> Source/WebCore/rendering/RenderBox.cpp:3516
>  void RenderBox::addBoxShadowAndBorderOverflow()

The method name is wrong after this change. Maybe it should be renamed to
addBoxDecorationOverflow, addVisualEffectOverflow or something?

> Source/WebCore/rendering/RenderLayerBacking.cpp:741
>  static bool hasBorderOutlineOrShadow(const RenderStyle* style)

Method name is wrong now.

> Source/WebCore/rendering/style/FilterOperations.cpp:35
> +static inline void outsetSizeForBlur(unsigned& outsetX, unsigned& outsetY,
float stdX, float stdY)

Can't this return an IntSize?

> Source/WebCore/rendering/style/FilterOperations.cpp:68
> +	   if (operationType == FilterOperation::BLUR || operationType ==
FilterOperation::DROP_SHADOW)

Is it worth checking for non-zero blur radius/shadow offset?

> Source/WebCore/rendering/style/RenderStyle.h:406
> +	   if (hasFilter())
> +	       filter().getOutsets(top, right, bottom, left, borderBoxSize);

I think you should set the parameters to zero if there is no filter.

> LayoutTests/ChangeLog:4
> +	   Filters need to affect visual overflow
> +	   https://bugs.webkit.org/show_bug.cgi?id=71930

You should probably add some margins between the elements in these filter tests
to avoid adjacent blurs overlapping.