[webkit-reviews] review denied: [Bug 45902] [Cairo] Port drawTiledShadow to the new ContextShadow : [Attachment 69614] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 45902: [Cairo] Port drawTiledShadow to the new ContextShadow
https://bugs.webkit.org/show_bug.cgi?id=45902

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69614&action=review

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:168
> +void ContextShadow::drawShadowedRect(PlatformContext context, const IntRect&
rect, const FloatSize& topLeftRadius, const FloatSize& topRightRadius, const
FloatSize& bottomLeftRadius, const FloatSize& bottomRightRadius, float alpha)
> +{

I think I'd like to see two more things inside this method:
1. The logic which falls back to the normal shadow path if the context has
transformation that we still do not support.
2. The code that adds the path to the Cairo surface.

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:175
> +    FloatRect shadowRect;
> +    shadowRect = FloatRect(rect.location(), shadowBufferSize);

Probably better just to collapse this down to:
FloatRect shadowRect = FloatRect(rect.location(), shadowBufferSize);

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:216
> +	   beginShadowLayer(context, shadowRect);
> +
> +	   if (!m_layerContext)
> +	       return;
> +
> +	   cairo_new_path(m_layerContext);
> +	   OwnPtr<cairo_path_t> path(cairo_copy_path(context));
> +	   cairo_append_path(m_layerContext, path.get());
> +
> +	   cairo_save(m_layerContext);
> +	   cairo_set_source_rgb(m_layerContext, 0.f, 0.f, 0.f);
> +	   cairo_clip_preserve(m_layerContext);
> +	   cairo_paint_with_alpha(m_layerContext, alpha);
> +	   cairo_restore(m_layerContext);
> +
> +	   endShadowLayer(context);
> +

This should probably be a helper function.

> WebCore/platform/graphics/qt/ContextShadowQt.cpp:164
> +void ContextShadow::drawShadowedRect(PlatformContext context, const IntRect&
rect, const FloatSize& topLeftRadius, const FloatSize& topRightRadius, const
FloatSize& bottomLeftRadius, const FloatSize& bottomRightRadius, float alpha)
> +{
> +    // Not implemented.
> +}
> +
>  }

I think you should skip this and just do #ifdefs in the ContextShadow.h. I
don't think the Qt guys want this optimization.