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+

Dirk Schulze
Reported 2012-08-31 15:53:47 PDT
Investigate if storing a Path member in RenderStyle can improve -webkit-clip-path.
Attachments
Patch (21.84 KB, patch)
2012-09-11 00:56 PDT, Dirk Schulze 		no flags
DetailsFormatted DiffDiff
Patch (21.84 KB, patch)
2012-09-11 03:28 PDT, Dirk Schulze 		no flags
DetailsFormatted DiffDiff
Patch (21.82 KB, patch)
2012-09-11 04:44 PDT, Dirk Schulze 		dino: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dirk Schulze
Comment 1 2012-09-11 00:56:07 PDT
Created attachment 163296 [details] Patch
Dirk Schulze
Comment 2 2012-09-11 03:28:52 PDT
Created attachment 163322 [details] Patch
Dirk Schulze
Comment 3 2012-09-11 04:44:48 PDT
Created attachment 163337 [details] Patch
Dean Jackson
Comment 4 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.
Dean Jackson
Comment 5 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?
Simon Fraser (smfr)
Comment 6 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.
Dirk Schulze
Comment 7 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.
Dirk Schulze
Comment 8 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.
Eric Seidel (no email)
Comment 9 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?
Dirk Schulze
Comment 10 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.
Dirk Schulze
Comment 11 2012-09-15 21:24:13 PDT
Committed r128700: <http://trac.webkit.org/changeset/128700>
Note You need to log in before you can comment on or make changes to this bug.