[webkit-reviews] review denied: [Bug 30960] [CAIRO] shadow support for Canvas and SVG : [Attachment 42227] shadow support for Cairo

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 30960: [CAIRO] shadow support for Canvas and SVG
https://bugs.webkit.org/show_bug.cgi?id=30960

Attachment 42227: shadow support for Cairo
https://bugs.webkit.org/attachment.cgi?id=42227&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/platform/graphics/GraphicsContext.h
> ===================================================================
> +	   void createPlatformShadow(PassOwnPtr<ImageBuffer> buffer, Color
shadowColor, FloatRect shadowRect, float kernelSize);

You should pass Color and FloatRect as const references.

> Index: WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp
> ===================================================================

> +static inline void drawPathShadow(GraphicsContext* context,
GraphicsContextPrivate* gcp, bool fillShadow, bool strokeShadow)
> +{
> +
> +	   // calculate the kernel size according to the HTML5 specification

Huh? HTML5 has something to say about filters? Oh, canvas. You should say
canvas (but this isn't just called for canvas).


> Index: WebCore/platform/graphics/cairo/ImageCairo.cpp
> ===================================================================

> +    // Draw the shadow
> +#if ENABLE(FILTERS)
> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (context->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +	   // calculate the kernel size according to the HTML5 specification
> +	   float kernelSize = (shadowBlur < 8 ? shadowBlur / 2.f :
sqrt(shadowBlur * 2.f));
> +	   int blurRadius = ceil(kernelSize);
> +	   IntSize shadowBufferSize(dstRect.width() + blurRadius * 2,
dstRect.height() + blurRadius * 2);
> +	   FloatRect shadowRect = FloatRect(dstRect.location(),
shadowBufferSize);
> +	   shadowRect.move(shadowSize.width() - kernelSize, shadowSize.height()
- kernelSize);
> +	   shadowColor = colorWithOverrideAlpha(shadowColor.rgb(),
(shadowColor.alpha() *	context->getAlpha()) / 255.f);
> +
> +	   //draw shadow into a new ImageBuffer
> +	   OwnPtr<ImageBuffer> shadowBuffer =
ImageBuffer::create(shadowBufferSize);
> +	   cairo_t* shadowContext = shadowBuffer->context()->platformContext();

> +	   cairo_set_source(shadowContext, pattern);
> +	   cairo_translate(shadowContext, -dstRect.x(), -dstRect.y());
> +	   cairo_rectangle(shadowContext, 0, 0, dstRect.width(),
dstRect.height());
> +	   cairo_fill(shadowContext);
> +
> +	   context->createPlatformShadow(shadowBuffer.release(), shadowColor,
shadowRect, kernelSize);

Can you share this code?

> Index: WebCore/platform/graphics/filters/GraphicsContextFilter.h
> ===================================================================

> +    class GraphicsContextFilter : public Filter {

The name of this class is confusing. There's an SVGFilter, but that ultimately
renders into a GraphicsContext. What is special about this one? Should this
instead be some kind of helper class that makes it easy to apply some filter to
drawing into a GraphicsContext?

> +    public:
> +	   static PassRefPtr<GraphicsContextFilter> create();
> +
> +	   bool effectBoundingBoxMode() { return false; }

This method should be const, and I prefer to see 'virtual' in all subclasses.

> +	   void calculateEffectSubRegion(FilterEffect*) { }
> +	   FloatRect filterRegion() { return FloatRect(); }

This should be const (in the base class and all subclasses).

> +	   FloatRect sourceImageRect() { return FloatRect(); }

This should be const (in the base class and all subclasses).

It looks OK, but I'd like to hear some thoughts about GraphicsContextFilter.