[webkit-reviews] review granted: [Bug 30960] [CAIRO] shadow support for Canvas and SVG : [Attachment 43089] Update patch for Simon Fraser and Eric Seidel's comments.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Brent Fulgham
<bfulgham 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 43089: Update patch for Simon Fraser and Eric Seidel's comments.
https://bugs.webkit.org/attachment.cgi?id=43089&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/GNUmakefile.am
> ===================================================================
> --- WebCore/GNUmakefile.am	(revision 50863)
> +++ WebCore/GNUmakefile.am	(working copy)
> @@ -2625,6 +2625,8 @@ webcore_sources += \
>	WebCore/platform/graphics/filters/Filter.h \
>	WebCore/platform/graphics/filters/FilterEffect.cpp \
>	WebCore/platform/graphics/filters/FilterEffect.h \
> +	WebCore/platform/graphics/filters/GraphicsContextFilter.cpp \
> +	WebCore/platform/graphics/filters/GraphicsContextFilter.h \

Stale file names.

Should you add ImageBufferFilter to the xcode project as well? It seems
generally useful.

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


> +	   Color fillColor = colorWithOverrideAlpha(context->fillColor().rgb(),
context->fillColor().alpha() / 255.f * gcp->state.globalAlpha);

If alpha() returns a float no need to make the 255 literal a float; the
compiler will promote it.

> +static inline void drawPathShadow(GraphicsContext* context,
GraphicsContextPrivate* gcp, bool fillShadow, bool strokeShadow)
> +{
> +#if ENABLE(FILTERS)
> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (context->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +	   //calculate filter values
> +	   cairo_t* cr = context->platformContext();
> +	   cairo_path_t* path = cairo_copy_path(cr);
> +	   double x0, x1, y0, y1;
> +	   cairo_stroke_extents(cr, &x0, &y0, &x1, &y1);
> +	   FloatRect rect(x0, y0, x1 - x0, y1 - y0);
> +
> +	   IntSize shadowBufferSize;
> +	   FloatRect shadowRect;
> +	   float kernelSize (0.0);
> +	   GraphicsContext::calculateShadowBufferDimensions(shadowBufferSize,
shadowRect, kernelSize, rect, shadowSize, shadowBlur);
> +
> +	   // 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() + kernelSize, -rect.y() +
kernelSize);
> +	   cairo_new_path(shadowContext);
> +	   cairo_append_path(shadowContext, path);
> +
> +	   if (fillShadow)
> +	       setPlatformFill(context, shadowContext, gcp);
> +	   if (strokeShadow)
> +	       setPlatformStroke(context, shadowContext, gcp);
> +
> +	   context->createPlatformShadow(shadowBuffer.release(), shadowColor,
shadowRect, kernelSize);
> +    }

This would be cleaner with an early return.

> -void GraphicsContext::setPlatformShadow(IntSize const&, int, Color const&,
ColorSpace)
> +void GraphicsContext::setPlatformShadow(IntSize const& size, int, Color
const&, ColorSpace)
>  {
> -    notImplemented();
> +    // Cairo doesn't support shadows natively, they are drawn manually in
the draw*
> +    // functions
> +
> +    if (m_common->state.shadowsIgnoreTransforms) {
> +	   // Meaning that this graphics context is associated with a
CanvasRenderingContext

For now, but what if this changes? We may decide that shadows ignore CSS
transforms at some point.

> Index: WebCore/platform/graphics/cairo/ImageCairo.cpp
> ===================================================================
> --- WebCore/platform/graphics/cairo/ImageCairo.cpp	(revision 50863)
> +++ WebCore/platform/graphics/cairo/ImageCairo.cpp	(working copy)
> @@ -132,6 +132,30 @@ void BitmapImage::draw(GraphicsContext* 
>      cairo_matrix_t matrix = { scaleX, 0, 0, scaleY, srcRect.x(), srcRect.y()
};
>      cairo_pattern_set_matrix(pattern, &matrix);
>  
> +    // Draw the shadow
> +#if ENABLE(FILTERS)
> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (context->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +	   IntSize shadowBufferSize;
> +	   FloatRect shadowRect;
> +	   float kernelSize (0.0);
> +	   GraphicsContext::calculateShadowBufferDimensions(shadowBufferSize,
shadowRect, kernelSize, dstRect, shadowSize, shadowBlur);
> +	   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);
> +    }
> +#endif
> +
>      // Draw the image.
>      cairo_translate(cr, dstRect.x(), dstRect.y());
>      cairo_set_source(cr, pattern);
> Index: WebCore/platform/graphics/filters/Filter.h
> ===================================================================

>	   // SVG specific
> +	   virtual void calculateEffectSubRegion(FilterEffect*) const = 0;

It's not clear why this can be const. Does it change state in |this|, or the
filter effect?

> +	   virtual bool effectBoundingBoxMode() const = 0;

I can't figure out what this means from the signature. Bool return? Not your
fault, but it seems like this needs an enum for the return value.

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

> +namespace WebCore {
> +
> +    class ImageBufferFilter : public Filter {

We don't generally indent for namespaces.