Bug 95619 – Add ClipPathOperation for -webkit-clip-path organization

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dirk Schulze]

Investigate if storing a Path member in RenderStyle can improve -webkit-clip-path.

[Dirk Schulze]

Created attachment 163296 [details]
Patch

[Dirk Schulze]

Created attachment 163322 [details]
Patch

[Dirk Schulze]

Created attachment 163337 [details]
Patch

[Dean Jackson]

Comment on attachment 163337 [details]
Patch

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

> Source/WebCore/rendering/ClipPathOperation.h:104
> +class ReferenceClipPathOperation : public ClipPathOperation {
> +public:

Why not wait until you use this class before adding it?

> Source/WebCore/rendering/ClipPathOperation.h:135
> +    void* m_data;

It's a bit frustrating that we can't reference a real type from here.

[Dean Jackson]

Comment on attachment 163337 [details]
Patch

You should update the bug title. Did you actually measure that this is an improvement?

[Simon Fraser (smfr)]

Comment on attachment 163337 [details]
Patch

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

>> Source/WebCore/rendering/ClipPathOperation.h:135
>> +    void* m_data;
> 
> It's a bit frustrating that we can't reference a real type from here.

I'd like to see a comment that tells me what this data really is, and its ownership model.

[Dirk Schulze]

(In reply to comment #6)
> (From update of attachment 163337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163337&action=review
> 
> >> Source/WebCore/rendering/ClipPathOperation.h:135
> >> +    void* m_data;
> > 
> > It's a bit frustrating that we can't reference a real type from here.
> 
> I'd like to see a comment that tells me what this data really is, and its ownership model.

I'll land the patch without this class as suggested by Dean. But I'll add the comment with the next patch.

[Dirk Schulze]

The improvement is about 5% on the same build (w/ and w/o patch). More improvement is possible. Looking into that on a next patch further. It still seems cleaner to move out the path creation from RenderLayer and SVGRenderContext.

[Eric Seidel (no email)]

(In reply to comment #8)
> The improvement is about 5% on the same build (w/ and w/o patch). More improvement is possible. Looking into that on a next patch further. It still seems cleaner to move out the path creation from RenderLayer and SVGRenderContext.

Curious which benchmark you're using here? Is it one we have checked-in to PerformanceTests/  If not, could we?

[Dirk Schulze]

I was checking with WebInspector :P I have a test that creates a number of divs (default 1000) with clip-path and changes the clip-path after that. I'll definitely create a PerformanceTest, once I learned how to write it, because I have more possible improvements in mind.

[Dirk Schulze]

Committed r128700: <http://trac.webkit.org/changeset/128700>