Bug 45902 - [Cairo] Port drawTiledShadow to the new ContextShadow
Summary: [Cairo] Port drawTiledShadow to the new ContextShadow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 47076
Blocks: 46475
  Show dependency treegraph
 
Reported: 2010-09-16 09:55 PDT by Martin Robinson
Modified: 2010-10-05 01:09 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (14.38 KB, patch)
2010-10-04 04:15 PDT, Alejandro G. Castro
mrobinson: review-
Details | Formatted Diff | Diff
Proposed patch (14.91 KB, patch)
2010-10-04 11:29 PDT, Alejandro G. Castro
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-09-16 09:55:40 PDT
drawTiledShadow should be a method on ContextShadow.
Comment 1 Alejandro G. Castro 2010-10-04 04:15:01 PDT
Created attachment 69614 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-10-04 04:20:07 PDT
Attachment 69614 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4185065
Comment 3 Alejandro G. Castro 2010-10-04 04:23:32 PDT
(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 4 Martin Robinson 2010-10-04 08:21:58 PDT
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.
Comment 5 Alejandro G. Castro 2010-10-04 08:28:34 PDT
(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 :).
Comment 6 Alejandro G. Castro 2010-10-04 11:29:54 PDT
Created attachment 69650 [details]
Proposed patch
Comment 7 Martin Robinson 2010-10-04 11:45:43 PDT
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());
Comment 8 Alejandro G. Castro 2010-10-05 01:09:22 PDT
Thanks, reviewed and landed http://trac.webkit.org/changeset/69092