[webkit-reviews] review granted: [Bug 68472] Move/copy/merge SVG filter rendering code to generic rendering : [Attachment 114331] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 68472: Move/copy/merge SVG filter rendering code to generic rendering
https://bugs.webkit.org/show_bug.cgi?id=68472

Attachment 114331: Patch
https://bugs.webkit.org/attachment.cgi?id=114331&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=114331&action=review


> Source/WebCore/WebCore.vcproj/WebCore.vcproj:33261
> +					ExcludedFromBuild="true"

I don't think you want this here.

> Source/WebCore/rendering/FilterEffectRenderer.h:69
> +    virtual void setSourceGraphic(PassRefPtr<SourceGraphic> sourceGraphic) {
m_sourceGraphic = sourceGraphic; }
> +    virtual RefPtr<SourceGraphic> sourceGraphic() const { return
m_sourceGraphic; }

I don't get why the input isn't an ImageBuffer, but that's just me.

> Source/WebCore/rendering/RenderLayer.cpp:2691
> +	   GraphicsContext* sourceGraphicsContext =
m_filter->sourceImage()->context();

This all seems rather weird. I filed bug 71962.

> Source/WebCore/rendering/RenderLayer.cpp:2693
> +	   FloatRect paintRect = FloatRect(0, 0, size().width(),
size().height());

FloatRect paintRect(FloatPoint(), size()).
Should this be in LayoutRects?

> Source/WebCore/rendering/RenderLayer.cpp:2706
> +	   LayoutRect filterBounds = LayoutRect(layerOrigin.x(),
layerOrigin.y(), size().width(), size().height());

filterBounds(layerOrigin(), size())?

> Source/WebCore/rendering/RenderLayer.cpp:2709
> +	   p->drawImageBuffer(m_filter->effect()->asImageBuffer(),
renderer()->style()->colorSpace(), filterBounds, CompositeSourceOver);

Should we clear the filter here? Does that throw away buffers?

> Source/WebCore/rendering/RenderLayer.cpp:4244
> +    if (hasFilter())
> +	   updateFilterEffect();

You could have updateFilterEffect() just do the hasFilter() test.

> Source/WebCore/rendering/RenderLayer.cpp:4393
> +    // For the moment we only support a single hue-rotate() filter.
> +    // See https://bugs.webkit.org/show_bug.cgi?id=68474 and
> +    // https://bugs.webkit.org/show_bug.cgi?id=68475
> +
> +    RefPtr<FilterEffect> effect;
> +    FilterOperations filterOperations = renderer()->style()->filter();

It would be nice to be able to optimize this to do nothing if the filter style
hasn't changed.

> Source/WebCore/rendering/RenderLayer.cpp:4416
> +void RenderLayer::updateFilterBackingStore()
> +{

Seems pretty bogus to have to do this every time. Maybe covered by bug 71962.

> LayoutTests/css3/filters/add-filter-rendering-expected.txt:9
> +	 text run at (0,0) width 572: "This paragraph will have a filter
applied. Once applied, it should have a green background."

Try to avoid text in pixel results. You could put the text in an HTML comment.