[webkit-reviews] review granted: [Bug 51567] Enhance ShadowBlur to tile inset box shadows : [Attachment 81673] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 51567: Enhance ShadowBlur to tile inset box shadows
https://bugs.webkit.org/show_bug.cgi?id=51567

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81673&action=review

r=me but with comments.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:410
> +    leftSlice= twiceRadius + max(radii.topLeft().width(),
radii.bottomLeft().width()); 

missing space after leftSlice

> Source/WebCore/platform/graphics/ShadowBlur.cpp:-505
> -	   1 | 2 | 3
> -	  -----------
> -	   4 |	 | 5
> -	  -----------
> -	   6 | 7 | 8
> -
> -	The corners are directly copied from the template rectangle to the
> -	real one and the side tiles are 1 pixel width, we use them as
> -
> -	tiles to cover the destination side. The corner tiles are bigger
> -	than just the side of the rounded corner, we need to increase it
> -	because the modifications caused by the corner over the blur
> -	effect. We fill the central part with solid color to complete the
> -	shadow.

I think this comment is useful. I wouldn't remove it.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:539
> +    boundingRect.move(m_offset.width(), m_offset.height());

boundingRect.move(m_offset);

> Source/WebCore/platform/graphics/ShadowBlur.cpp:542
> +    destHoleRect.move(m_offset.width(), m_offset.height());

ditto

> Source/WebCore/platform/graphics/ShadowBlur.cpp:607
> +    const int templateSideLength = 1;

You are using this at least twice. Can you add a static const at the beginning
of the document?

> Source/WebCore/platform/graphics/ShadowBlur.cpp:686
> +    {
> +	   IntRect blurRect(IntPoint(), templateSize);
> +	   RefPtr<ByteArray> layerData =
m_layerImage->getUnmultipliedImageData(blurRect);
> +	   blurLayerImage(layerData->data(), blurRect.size(), blurRect.width()
* 4);
> +	   m_layerImage->putUnmultipliedImageData(layerData.get(),
blurRect.size(), blurRect, IntPoint());
> +    }

The braces seem useless to me.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:692
> +    shadowContext->fillRect(FloatRect(0, 0, templateSize.width(),
templateSize.height()));

argh. Why don't we have a ctor FloatRect(IntPoint, IntSize), you're using this
multiple times :-)