Bug 45902

Created attachment 69614 [details] Proposed patch

Attachment 69614 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4185065

(In reply to comment #2) > Attachment 69614 [details] did not build on gtk: > Build output: http://queues.webkit.org/results/4185065 Gtk depends on the bug 47076. I'll reupload when that patch lands.

Comment on attachment 69614 [details] Proposed patch 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.

(In reply to comment #4) > (From update of attachment 69614 [details]) > 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) > > +{ > > [...] > Thanks for the comments, I'll review them :).

Created attachment 69650 [details] Proposed patch

Comment on attachment 69650 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=69650&action=review Great! Please fix the small issues before landing. > WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:139 > + cairo_set_source_rgba(m_layerContext, 0.f, 0.f, 0.f, alpha); I think this can just be cairo_set_source_rgba(m_layerContext, 0, 0, 0, alpha); > WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:144 > + > + return; No need for this. > WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:199 > + // Calculate filter values to create appropriate shadow. Might want to update this comment, as we are no longer using SVG filters. > WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:201 > + shadowBufferSize = IntSize(rect.width() + m_blurRadius * 2, rect.height() + m_blurRadius * 2); I think this can be: IntSize shadowBufferSize = IntSize(rect.width() + m_blurRadius * 2, rect.height() + m_blurRadius * 2); > WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:248 > + cairo_set_source_rgba(m_layerContext, 0.f, 0.f, 0.f, context->getAlpha()); These can also be cairo_set_source_rgba(m_layerContext, 0, 0, 0, context->getAlpha());

Thanks, reviewed and landed http://trac.webkit.org/changeset/69092