Bug 95619

Summary: Add ClipPathOperation for -webkit-clip-path organization
Product: WebKit Reporter: Dirk Schulze <krit>
Component: CSSAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dino, donggwan.kim, eric, macpherson, menard, pdr, schenney, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 95389, 96876    
Attachments:
Description Flags
Patch
none
Patch
none
Patch dino: review+

Description Dirk Schulze 2012-08-31 15:53:47 PDT 
Investigate if storing a Path member in RenderStyle can improve -webkit-clip-path.
Comment 1 Dirk Schulze 2012-09-11 00:56:07 PDT 
Created attachment 163296 [details]
Patch
Comment 2 Dirk Schulze 2012-09-11 03:28:52 PDT 
Created attachment 163322 [details]
Patch
Comment 3 Dirk Schulze 2012-09-11 04:44:48 PDT 
Created attachment 163337 [details]
Patch
Comment 4 Dean Jackson 2012-09-11 09:27:08 PDT 
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.
Comment 5 Dean Jackson 2012-09-11 09:27:40 PDT 
Comment on attachment 163337 [details]
Patch

You should update the bug title. Did you actually measure that this is an improvement?
Comment 6 Simon Fraser (smfr) 2012-09-11 14:06:03 PDT 
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.
Comment 7 Dirk Schulze 2012-09-11 20:17:59 PDT 
(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.
Comment 8 Dirk Schulze 2012-09-15 20:40:17 PDT 
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.
Comment 9 Eric Seidel (no email) 2012-09-15 20:46:41 PDT 
(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?
Comment 10 Dirk Schulze 2012-09-15 21:00:39 PDT 
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.
Comment 11 Dirk Schulze 2012-09-15 21:24:13 PDT 
Committed r128700: <http://trac.webkit.org/changeset/128700>