Bug 95619 - Add ClipPathOperation for -webkit-clip-path organization
Summary: Add ClipPathOperation for -webkit-clip-path organization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks: 95389 96876
  Show dependency treegraph
 
Reported: 2012-08-31 15:53 PDT by Dirk Schulze
Modified: 2012-09-15 21:29 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.84 KB, patch)
2012-09-11 00:56 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (21.84 KB, patch)
2012-09-11 03:28 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (21.82 KB, patch)
2012-09-11 04:44 PDT, Dirk Schulze
dino: review+
Details | Formatted Diff | Diff

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