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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 111693: Patch
https://bugs.webkit.org/attachment.cgi?id=111693&action=review

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


I think there are two big chunks missing here, and I'm curious about how you
plan to implement them:
1. incremental repaints: you need to map a rect through the filter chain.
Filters need the ability to say how they affect a given rect. This rect mapping
should be used in the repaint code path.
2. How filters expand the visual overflow of an element (also repaint-related).


> Source/WebCore/rendering/RenderFilter.h:44
> +class RenderFilter : public Filter {

I'm not sure that RenderFilter is the best name for this, since it doesn't
inherit from RenderObject. Maybe RendererFilter? EffectsFilter?

> Source/WebCore/rendering/RenderFilter.h:52
> +    virtual void setSourceImageRect(const FloatRect& sourceImageRect) {
m_sourceDrawingRegion = sourceImageRect; }
> +    virtual FloatRect sourceImageRect() const { return
m_sourceDrawingRegion; }

Should these all be LayoutRects?

> Source/WebCore/rendering/RenderFilter.h:57
> +    virtual bool effectBoundingBoxMode() const { return false; }

This method has a terrible name. What kind of mode is this?

> Source/WebCore/rendering/RenderLayer.cpp:2696
> +	   m_filter->setSavedContext(p);

Why does the filter have to know what the original destination context is?

> Source/WebCore/rendering/RenderLayer.cpp:2709
> +	   p->save();
> +	   p->concatCTM(transform.toAffineTransform());

I don't really like the save here, and the restore in some other code block a
long way below.

Can you do what we do for transforms, and call back into paintLayer?

Does this filter before or after CSS shadows are applied? Which should it be?

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

Doesn't the color space need to be passed through the filter chain somehow?

> Source/WebCore/rendering/RenderLayer.cpp:4385
> +void RenderLayer::createFilter()
> +{
> +    ASSERT(!m_filter);
> +    m_filter = RenderFilter::create();
> +}
> +
> +void RenderLayer::removeFilter()
> +{
> +    m_filter = 0;
> +}
> +

It's not clear to me that RenderLayer is the right thing to own the filter.
Maybe the RenderObject should own it?

> Source/WebCore/rendering/RenderLayer.cpp:4398
> +    // For the moment all filter effects are hardcoded to
hue-rotate(180deg).
> +    // See https://bugs.webkit.org/show_bug.cgi?id=68474 and
> +    // https://bugs.webkit.org/show_bug.cgi?id=68475
> +
> +    RefPtr<SourceGraphic> sourceGraphic =
SourceGraphic::create(m_filter.get());
> +    m_filter->setSourceGraphic(sourceGraphic);
> +    Vector<float> inputParameters;
> +    inputParameters.append(180);
> +    RefPtr<FEColorMatrix> matrix = FEColorMatrix::create(m_filter.get(),
FECOLORMATRIX_TYPE_HUEROTATE, inputParameters);
> +    matrix->inputEffects().append(sourceGraphic);
> +    m_filter->setEffect(matrix);

I don't think it's appropriate to check in a hard-coded filter effect.

> Source/WebCore/rendering/RenderObject.h:926
> +    bool m_hasFilter 		: 1;

See comment higher up:
    // 32 bits have been used here. THERE ARE NO FREE BITS AVAILABLE.