Bug 95620

Summary: Use -webkit-clip-path shapes to clip SVG elements
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, eric, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 95389    
Attachments:
Description Flags
Patch
none
Patch rwlbuis: review+

Description Dirk Schulze 2012-08-31 15:55:42 PDT
Use -webkit-clip-path shapes to clip SVG elements.
Comment 1 Dirk Schulze 2012-08-31 18:05:06 PDT
Created attachment 161802 [details]
Patch
Comment 2 Dirk Schulze 2012-08-31 18:05:49 PDT
This is actually not for review. I just want to see if it builds everywhere.
Comment 3 Dirk Schulze 2012-08-31 20:17:23 PDT
Created attachment 161812 [details]
Patch
Comment 4 Rob Buis 2012-09-01 09:45:40 PDT
Comment on attachment 161812 [details]
Patch

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

Looks good.

> Source/WebCore/rendering/style/BasicShapes.cpp:47
> +                                   m_cornerRadiusY.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusY, boundingBox.height())));

Seems like indenting is not quite right?

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:152
> +    if (clipper && !style->clipPath()) {

Why not do !style->clipPath() test first? Only if true you'd need to get clipper. You could also reuse the clipShape variable from above.

> LayoutTests/svg/clip-path/clip-path-shape-circle-1-expected.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

Is the xlink stuff needed in any of these tests?

> LayoutTests/svg/clip-path/clip-path-shape-ellipse-2.svg:2
> +<rect width="200" height="200" fill="green" style="-webkit-clip-path: ellipse(100px, 75px, 100px, 75px)"/>

Should height be 150 here?
Comment 5 Dirk Schulze 2012-09-01 17:15:14 PDT
Committed r127383: <http://trac.webkit.org/changeset/127383>
Comment 6 Dirk Schulze 2012-09-01 17:40:58 PDT
Fixed all suggested changes before landing, but not the change on the value for the ellipse. It is rx,ry and not with and height. (just for the logs)