[webkit-reviews] review denied: [Bug 39582] [Gtk] very slow page scrolling on big -webkit-box-shadow areas : [Attachment 64004] Proposed patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied  review:
Bug 39582: [Gtk] very slow page scrolling on big -webkit-box-shadow areas
https://bugs.webkit.org/show_bug.cgi?id=39582

Attachment 64004: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=64004&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
A great patch, but maybe a bit more complicated than it should be.

> +#if PLATFORM(CAIRO)
> +    // Decide if the shadow buffer should be blitted or released (required
for tiling)
> +    enum ResultBufferOp {
> +	   Blit,
> +	   Release
> +    };
> +#endif
Can you add a better naming schema? I have no idea what ResultBufferOp means
and Blit or Relase are not very helpful (I even don't know what Blit means :-P)


> +/*
> +  This function uses tiling to improve the performance of the shadow
> +  drawing of rounded rectangles. The code basically does the following
> +  steps:
> +
> +	1. Calculate the minimum rectangle size required to create the
> +	tiles
> +
> +	2. If that size is smaller than the real rectangle render the new
> +	small rectangle and its shadow, in other case render the shadow
> +	of the real rectangle.
> +
> +	3. Calculate the sizes and positions of the tiles and their
> +	destinations and use drawPattern to render the final shadow. The
> +	code divides the rendering in 8 tiles:
> +
> +	   1 | 2 | 3
> +	  -----------
> +	   4 |	 | 5
> +	  -----------
> +	   6 | 7 | 8
> +
> +	The corners are directly copied from the small 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.
> + */
> +void GraphicsContext::drawFastShadow(GraphicsContext* context,
GraphicsContextPrivate* gcp, const IntRect& r, const FloatSize& topLeftRadius,
const FloatSize& topRightRadius, const FloatSize& bottomLeftRadius, const
FloatSize& bottomRightRadius, ColorSpace colorSpace)
> +{
> +#if ENABLE(FILTERS)
> +    FloatSize shadowSize;
> +    FloatSize shadowOffset;
...
> +	   destRect.setX(shadowRect.x() + rect.width() -
topRightRadius.width());
> +	   destRect.setY(shadowRect.y() + rect.height() -
bottomRightRadius.height());
> +	   destRect.setWidth(tileRect.width());
> +	   destRect.setHeight(tileRect.height());
> +	   phase.setX(destRect.x() - tileRect.x());
> +	   phase.setY(destRect.y() - tileRect.y());
> +
> +	   shadowBuffer->image()->drawPattern(context, tileRect,
patternTransform,
> +					      phase, colorSpace,
CompositeSourceOver, destRect);
> +
> +	   // bottom left corner
> +	   tileRect.setX(0);
> +	   tileRect.setY(smallBufferSize.height() - 2 * radius -
bottomRightRadius.height());
> +	   tileRect.setWidth(2 * radius + bottomLeftRadius.width());
> +	   tileRect.setHeight(2 * radius + bottomLeftRadius.height());
> +	   destRect.setX(shadowRect.x());
> +	   destRect.setY(shadowRect.y() + rect.height() -
bottomRightRadius.height());
> +	   destRect.setWidth(tileRect.width());
> +	   destRect.setHeight(tileRect.height());
> +	   phase.setX(destRect.x() - tileRect.x());
> +	   phase.setY(destRect.y() - tileRect.y());
> +
> +	   shadowBuffer->image()->drawPattern(context, tileRect,
patternTransform,
> +					      phase, colorSpace,
CompositeSourceOver, destRect);
> +
> +    } else {
> +	   // Create suitably-sized ImageBuffer to hold the shadow.
> +	   OwnPtr<ImageBuffer> shadowBuffer =
ImageBuffer::create(shadowBufferSize);
> +
> +	   // Draw shadow into a new ImageBuffer.
> +	   cairo_t* shadowContext = shadowBuffer->context()->platformContext();

> +	   copyContextProperties(cr, shadowContext);
> +	   cairo_translate(shadowContext, -rect.x() + radius, -rect.y() +
radius);
> +	   cairo_new_path(shadowContext);
> +	   cairo_append_path(shadowContext, path);
> +
> +	   setPlatformFill(context, shadowContext, gcp);
> +
> +	   context->createPlatformShadow(shadowBuffer.release(), shadowColor,
shadowRect, radius, Blit);
> +    }
> +#endif
> +}
This is pretty hard to understand. first,there shouldn't be that much code in
an if clause. I suggest to test for the else, move the content of the else in
this if and return earlier. the rest don't need to be checked again:

> +    if (...) {
> +	   // Create suitably-sized ImageBuffer to hold the shadow.
> +	   OwnPtr<ImageBuffer> shadowBuffer =
ImageBuffer::create(shadowBufferSize);
> +
> +	   // Draw shadow into a new ImageBuffer.
> +	   cairo_t* shadowContext = shadowBuffer->context()->platformContext();

> +	   copyContextProperties(cr, shadowContext);
> +	   cairo_translate(shadowContext, -rect.x() + radius, -rect.y() +
radius);
> +	   cairo_new_path(shadowContext);
> +	   cairo_append_path(shadowContext, path);
> +
> +	   setPlatformFill(context, shadowContext, gcp);
> +
> +	   context->createPlatformShadow(shadowBuffer.release(), shadowColor,
shadowRect, radius, Blit);
>	   return;
> +    }
> ....



> +	   tileRect.setX(0);
> +	   tileRect.setY(smallBufferSize.height() - 2 * radius -
bottomRightRadius.height());
> +	   tileRect.setWidth(2 * radius + bottomLeftRadius.width());
> +	   tileRect.setHeight(2 * radius + bottomLeftRadius.height());
This should be set at once:
   tileRect = FloatRect(0, smallBufferSize.height() - 2 * radius -
bottomRightRadius.height(), ...)
Of course you can use multiple line for that.

You should use the logic of FloatRect/Size/Point much more in your code. and
also it's funtcions. E.g. inflateX and inflateY extends the rect. Please use
them to enlarge your rect to respect the blurring.
Don't mix up the enlargement of your rects with the offset. You can apply the
blur radius to the with and position of the rects and move the rect by the
shadowOffset.
Some multiplications are done multiple times (radius * 2) can you store them in
a new variable like radiusTwice and so on?

It's great that you try to reuse the drawPattern code.

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1303
 +	    if (offsetHeight > min (bottomLeftRadius.height(),
bottomRightRadius.height()))
no space between min (

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1296
 +	if (offsetWidth > 0)
use if (offsetWidth)
The same in other lines.