Bug 126205 – Create clipping path from <box> value

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dirk Schulze]

If just a <box> value is specified a clipping path needs to be created from this box value. See CSS Shapes link.

[Bem Jones-Bey]

Created attachment 222795 [details]
Patch

[Dirk Schulze]

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.

[Bem Jones-Bey]

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.

[Bem Jones-Bey]

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.

[Bem Jones-Bey]

Created attachment 222815 [details]
Patch

Fix comments and remove extra if statement

[Dirk Schulze]

Comment on attachment 222815 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 222815 [details]
Patch

Clearing flags on attachment: 222815

Committed r163205: <http://trac.webkit.org/changeset/163205>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.