Bug 84460 - [CSS Filters] Move m_filter and related fields from RenderLayer to a different structure and only allocate it when needed
Summary: [CSS Filters] Move m_filter and related fields from RenderLayer to a differen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on:
Blocks: 82803
  Show dependency treegraph
 
Reported: 2012-04-20 09:13 PDT by Alexandru Chiculita
Modified: 2013-05-13 09:29 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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.