Bug 84460 - [CSS Filters] Move m_filter and related fields from RenderLayer to a different structure and only allocate it when needed
: [CSS Filters] Move m_filter and related fields from RenderLayer to a differen...
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 82803
  Show dependency treegraph
 
Reported: 2012-04-20 09:13 PST by
Modified: 2013-05-13 09:29 PST (History)


Attachments
Patch V1 (26.28 KB, patch)
2012-04-23 16:55 PST, Alexandru Chiculita
no flags Review Patch | Details | Formatted Diff | Diff
Patch V2 (26.71 KB, patch)
2012-04-23 17:17 PST, Alexandru Chiculita
dino: review+
achicu: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-20 09:13:29 PST
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 From 2012-04-23 16:55:54 PST -------
Created an attachment (id=138461) [details]
Patch V1
------- Comment #2 From 2012-04-23 17:17:41 PST -------
Created an attachment (id=138467) [details]
Patch V2

Rebased.
------- Comment #3 From 2012-04-23 17:50:48 PST -------
(From update of attachment 138467 [details])
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 From 2012-04-23 20:12:00 PST -------
(From update of attachment 138467 [details])
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 From 2012-04-24 10:18:50 PST -------
Landed in http://trac.webkit.org/changeset/115079.