Bug 126205

Summary: Create clipping path from <box> value
Product: WebKit Reporter: Dirk Schulze <krit>
Component: CSSAssignee: Bem Jones-Bey <bjonesbe>
Status: RESOLVED FIXED    
Severity: Normal CC: bjonesbe, commit-queue, dino, esprehn+autocc, glenn, kling, kondapallykalyan, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://dev.w3.org/csswg/css-shapes/#shapes-from-box-values
Bug Depends on:    
Bug Blocks: 126207    
Attachments:
Description Flags
Patch
none
Patch none

Description Dirk Schulze 2013-12-23 23:21:21 PST
If just a <box> value is specified a clipping path needs to be created from this box value. See CSS Shapes link.
Comment 1 Bem Jones-Bey 2014-01-30 22:28:18 PST
Created attachment 222795 [details]
Patch
Comment 2 Dirk Schulze 2014-01-31 05:41:42 PST
Comment on attachment 222795 [details]
Patch

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

Some comments and questions to the patch.

> Source/WebCore/rendering/RenderLayer.cpp:3784
> +template <class ReferenceBoxClipPathOperation>
> +static inline LayoutRect computeReferenceBox(const RenderObject& renderer, const ReferenceBoxClipPathOperation& clippingPath, const LayoutRect& rootRelativeBounds)

Two questions here: Why does it need to be a template function and why can't the code be added to ReferenceBoxClipPathOperation instead?

> Source/WebCore/rendering/RenderLayer.cpp:3827
> +    if (!rootRelativeBoundsComputed) {
> +        rootRelativeBounds = calculateLayerBounds(paintingInfo.rootLayer, &offsetFromRoot, 0);
> +        rootRelativeBoundsComputed = true;
> +    }

Since we need this calculation for all cases now, it is ok to move it up here (was an initial reviewer request just to do it when absolutely necessary). But please remove it from the SVG clipPath code underneath as well.

> Source/WebCore/rendering/RenderLayer.cpp:3844
> +        // FIXME this does not properly compute the rounded corners as specified in all conditions

Real sentence please.

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:95
> +        // FIXME this does not properly compute the rounded corners as specified in all conditions

Ditto.
Comment 3 Bem Jones-Bey 2014-01-31 07:30:06 PST
Comment on attachment 222795 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3784
>> +static inline LayoutRect computeReferenceBox(const RenderObject& renderer, const ReferenceBoxClipPathOperation& clippingPath, const LayoutRect& rootRelativeBounds)
> 
> Two questions here: Why does it need to be a template function and why can't the code be added to ReferenceBoxClipPathOperation instead?

1) There is no ReferenceBoxClipPathOperation class, that's just what I named the template parameter. I figured that creating the class and making the class hierarchy more complex wasn't worth it for this singe instance.

2) ClipPathOperation.h does not include RenderBox.h, and since it is only the header file, including it causes circular deps. I could create a ClipPathOperation.cpp and also a ReferenceBoxClipPathOperation class, if you think that is better than having this templated helper here.

>> Source/WebCore/rendering/RenderLayer.cpp:3827
>> +    }
> 
> Since we need this calculation for all cases now, it is ok to move it up here (was an initial reviewer request just to do it when absolutely necessary). But please remove it from the SVG clipPath code underneath as well.

Oops! I'll fix that.

>> Source/WebCore/rendering/RenderLayer.cpp:3844
>> +        // FIXME this does not properly compute the rounded corners as specified in all conditions
> 
> Real sentence please.

Ok.

>> Source/WebCore/rendering/shapes/ShapeInfo.cpp:95
>> +        // FIXME this does not properly compute the rounded corners as specified in all conditions
> 
> Ditto.

Ok.
Comment 4 Bem Jones-Bey 2014-01-31 07:30:08 PST
Comment on attachment 222795 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3784
>> +static inline LayoutRect computeReferenceBox(const RenderObject& renderer, const ReferenceBoxClipPathOperation& clippingPath, const LayoutRect& rootRelativeBounds)
> 
> Two questions here: Why does it need to be a template function and why can't the code be added to ReferenceBoxClipPathOperation instead?

1) There is no ReferenceBoxClipPathOperation class, that's just what I named the template parameter. I figured that creating the class and making the class hierarchy more complex wasn't worth it for this singe instance.

2) ClipPathOperation.h does not include RenderBox.h, and since it is only the header file, including it causes circular deps. I could create a ClipPathOperation.cpp and also a ReferenceBoxClipPathOperation class, if you think that is better than having this templated helper here.

>> Source/WebCore/rendering/RenderLayer.cpp:3827
>> +    }
> 
> Since we need this calculation for all cases now, it is ok to move it up here (was an initial reviewer request just to do it when absolutely necessary). But please remove it from the SVG clipPath code underneath as well.

Oops! I'll fix that.

>> Source/WebCore/rendering/RenderLayer.cpp:3844
>> +        // FIXME this does not properly compute the rounded corners as specified in all conditions
> 
> Real sentence please.

Ok.

>> Source/WebCore/rendering/shapes/ShapeInfo.cpp:95
>> +        // FIXME this does not properly compute the rounded corners as specified in all conditions
> 
> Ditto.

Ok.
Comment 5 Bem Jones-Bey 2014-01-31 08:23:35 PST
Created attachment 222815 [details]
Patch

Fix comments and remove extra if statement
Comment 6 Dirk Schulze 2014-01-31 14:07:25 PST
Comment on attachment 222815 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2014-01-31 14:39:44 PST
Comment on attachment 222815 [details]
Patch

Clearing flags on attachment: 222815

Committed r163205: <http://trac.webkit.org/changeset/163205>
Comment 8 WebKit Commit Bot 2014-01-31 14:39:47 PST
All reviewed patches have been landed.  Closing bug.