Bug 84460 – [CSS Filters] Move m_filter and related fields from RenderLayer to a different structure and only allocate it when needed

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexandru Chiculita]

There are a growing number of fields in RenderLayer. Move that to a different rare structure and only allocate it for RenderLayers that need it.

[Alexandru Chiculita]

Created attachment 138461 [details]
Patch V1

[Alexandru Chiculita]

Created attachment 138467 [details]
Patch V2

Rebased.

[Dean Jackson]

Comment on attachment 138467 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=138467&action=review

Some small questions.

> Source/WebCore/rendering/RenderLayer.cpp:313
>  bool RenderLayer::requiresFullLayerImageForFilters() const 
>  {
> -    // FIXME: This can be optimized to enlarge the repaint rect exactly with the amount that is going to be used.
> -    // https://bugs.webkit.org/show_bug.cgi?id=81263
> -    return paintsWithFilters() && filter() && filter()->hasFilterThatMovesPixels();
> +    if (!paintsWithFilters())
> +        return false;
> +    FilterEffectRenderer* filter = filterRenderer();
> +    return filter ? filter->hasFilterThatMovesPixels() : false;
>  }

Why change this?  I guess because filterRenderer() is a little more complex now?

> Source/WebCore/rendering/RenderLayer.h:56
> -#if ENABLE(CSS_FILTERS)
> -#include "FilterEffectObserver.h"
> -#endif
>  #include "PaintInfo.h"
>  #include "RenderBox.h"
>  #include "ScrollableArea.h"
>  #include <wtf/OwnPtr.h>
>  
> +#if ENABLE(CSS_FILTERS)
> +#include "FilterEffectObserver.h"
> +#include "RenderLayerFilterInfo.h"
> +#endif
> +

I'm not sure about the style rules here. It might not be strict. Do other files guard include blocks with ENABLE?

> Source/WebCore/rendering/RenderLayerFilterInfo.h:38
> +#if ENABLE(CSS_FILTERS)
> +#include "LayoutTypes.h"
> +
> +#include <wtf/HashMap.h>
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefPtr.h>

Typically it is a space after the ENABLE and then the includes are grouped together.

> Source/WebCore/rendering/RenderLayerFilterInfo.h:55
> +    void addDirtySourceRect(const LayoutRect& rect) { m_dirtySourceRect.unite(rect); }

Maybe this should be expandDirtySourceRect? What do you think?

[Alexandru Chiculita]

Comment on attachment 138467 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=138467&action=review

Thanks for the r+. I will fix it and commit tomorrow.

>> Source/WebCore/rendering/RenderLayer.cpp:313
>>  }
> 
> Why change this?  I guess because filterRenderer() is a little more complex now?

Yes, filterRenderer() is doing a HashMap search, so I wanted to avoid doing it twice.

>> Source/WebCore/rendering/RenderLayer.h:56
>> +
> 
> I'm not sure about the style rules here. It might not be strict. Do other files guard include blocks with ENABLE?

There are other files that guard headers with ifdefs, but most that I found are in cpp files. There are some examples in headers, in general related to SVG. ie. Source/WebCore/rendering/PaintInfo.h.
There should be no difference, as the header is also guarding for the same ifdef inside it, but I suppose it makes code pre-processing faster.
Most of the other files with #include guarding have them grouped at the bottom.

>> Source/WebCore/rendering/RenderLayerFilterInfo.h:38
>> +#include <wtf/RefPtr.h>
> 
> Typically it is a space after the ENABLE and then the includes are grouped together.

Ok. Thanks.

>> Source/WebCore/rendering/RenderLayerFilterInfo.h:55
>> +    void addDirtySourceRect(const LayoutRect& rect) { m_dirtySourceRect.unite(rect); }
> 
> Maybe this should be expandDirtySourceRect? What do you think?

Yes, thanks for the tip, "expand" makes more sense in this context.

[Alexandru Chiculita]

Landed in http://trac.webkit.org/changeset/115079.