Bug 84460

Summary: [CSS Filters] Move m_filter and related fields from RenderLayer to a different structure and only allocate it when needed
Product: WebKit Reporter: Alexandru Chiculita <achicu>
Component: Layout and RenderingAssignee: Alexandru Chiculita <achicu>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, jchaffraix
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82803    
Attachments:
Description Flags
Patch V1
none
Patch V2 dino: review+, achicu: commit-queue-

Description Alexandru Chiculita 2012-04-20 09:13:29 PDT
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.
Comment 1 Alexandru Chiculita 2012-04-23 16:55:54 PDT
Created attachment 138461 [details]
Patch V1
Comment 2 Alexandru Chiculita 2012-04-23 17:17:41 PDT
Created attachment 138467 [details]
Patch V2

Rebased.
Comment 3 Dean Jackson 2012-04-23 17:50:48 PDT
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?
Comment 4 Alexandru Chiculita 2012-04-23 20:12:00 PDT
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.
Comment 5 Alexandru Chiculita 2012-04-24 10:18:50 PDT
Landed in http://trac.webkit.org/changeset/115079.