[CSS Shapes] inset does not properly clamp large corner radii

[CSS Shapes] inset does not properly clamp large corner radii

Make sure that radii that are larger than 50% of the associated shape
dimension (height or width) are properly reduced the same way as
border-radius radii are.

Ported over from http://trac.webkit.org/changeset/165261.

BUG=351775
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169086

[Bem Jones-Bey (adobe), 2014-03-12 17:16:46]

https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
File
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius-expected.html
(right):

https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius-expected.html:63:
<p>Bug <a href="http://webkit.org/b/129726">129726</a>: [CSS Shapes] inset does
not properly clamp large corner radii</p>
You should update this to be a chromium bug link (like the shape-outside master
bug), or remove it.

https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
File
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html
(right):

https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html:16:
-webkit-shape-outside: inset(0px round 200px);
You should remove the -webkit prefix, as Blink supports it unprefixed.

https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html:37:
<p>Bug <a href="http://webkit.org/b/129726">129726</a>: [CSS Shapes] inset does
not properly clamp large corner radii</p>
Chromium bug link or remove.

[rwlbuis, 2014-03-12 17:29:57]

On 2014/03/12 17:16:46, Bem Jones-Bey (adobe) wrote:
>
https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
> File
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius-expected.html
> (right):
> 
>
https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius-expected.html:63:
> <p>Bug <a href="http://webkit.org/b/129726">129726</a>: [CSS Shapes] inset
does
> not properly clamp large corner radii</p>
> You should update this to be a chromium bug link (like the shape-outside
master
> bug), or remove it.
> 
>
https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
> File
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html
> (right):
> 
>
https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html:16:
> -webkit-shape-outside: inset(0px round 200px);
> You should remove the -webkit prefix, as Blink supports it unprefixed.
> 
>
https://codereview.chromium.org/195793009/diff/10001/LayoutTests/fast/shapes/...
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html:37:
> <p>Bug <a href="http://webkit.org/b/129726">129726</a>: [CSS Shapes] inset
does
> not properly clamp large corner radii</p>
> Chromium bug link or remove.

Great points, they should be addressed in patch set 3.

[Bem Jones-Bey (adobe), 2014-03-12 17:48:20]

A nit, but lgtm. (now all you need is a real owner. ;-)

https://codereview.chromium.org/195793009/diff/30001/LayoutTests/fast/shapes/...
File
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius-expected.html
(right):

https://codereview.chromium.org/195793009/diff/30001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius-expected.html:63:
<p>Bug <a
href="https://code.google.com/p/chromium/issues/detail?id=351775">351775</a>:
[CSS Shapes] inset does not properly clamp large corner radii</p>
Nit: It could be confusing that the "bug title" here doesn't match the one on
the bug that it links to.

https://codereview.chromium.org/195793009/diff/30001/LayoutTests/fast/shapes/...
File
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html
(right):

https://codereview.chromium.org/195793009/diff/30001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-inset-rounded-large-radius.html:37:
<p>Bug <a
href="https://code.google.com/p/chromium/issues/detail?id=351775">351775</a>:
[CSS Shapes] inset does not properly clamp large corner radii</p>
Ditto.

[rwlbuis, 2014-03-12 17:55:44]

@Levi, straightforward port again from WebKit, however we are not sure we need
the change in platform/ or keep it somewhere in rendering/. If the former I need
to add a platform OWNER...

[eseidel, 2014-03-12 18:10:03]

https://codereview.chromium.org/195793009/diff/30001/Source/platform/geometry...
File Source/platform/geometry/FloatRoundedRect.h (right):

https://codereview.chromium.org/195793009/diff/30001/Source/platform/geometry...
Source/platform/geometry/FloatRoundedRect.h:135: inline float
calcBorderRadiiConstraintScaleFor(const FloatRect& rect, const
FloatRoundedRect::Radii& radii)
Why does this need to go in platform?  platform does only exist to serve
blink... but at the same time, this is a CSS-spec-related function, and nothing
to do with FloatRects in the general case.

Why not just RenderStyle.h?

[Bem Jones-Bey (adobe), 2014-03-12 18:34:33]

On 2014/03/12 18:10:03, eseidel wrote:
>
https://codereview.chromium.org/195793009/diff/30001/Source/platform/geometry...
> File Source/platform/geometry/FloatRoundedRect.h (right):
> 
>
https://codereview.chromium.org/195793009/diff/30001/Source/platform/geometry...
> Source/platform/geometry/FloatRoundedRect.h:135: inline float
> calcBorderRadiiConstraintScaleFor(const FloatRect& rect, const
> FloatRoundedRect::Radii& radii)
> Why does this need to go in platform?  platform does only exist to serve
> blink... but at the same time, this is a CSS-spec-related function, and
nothing
> to do with FloatRects in the general case.
> 
> Why not just RenderStyle.h?

It doesn't need to go in platform, but that was what my reviewer suggested in
WebKit, and at that point, I didn't have any better ideas. RenderStyle.h does
sound like a much better idea.

[rwlbuis, 2014-03-12 19:36:47]

On 2014/03/12 18:34:33, Bem Jones-Bey (adobe) wrote:
> On 2014/03/12 18:10:03, eseidel wrote:
> >
>
https://codereview.chromium.org/195793009/diff/30001/Source/platform/geometry...
> > File Source/platform/geometry/FloatRoundedRect.h (right):
> > 
> >
>
https://codereview.chromium.org/195793009/diff/30001/Source/platform/geometry...
> > Source/platform/geometry/FloatRoundedRect.h:135: inline float
> > calcBorderRadiiConstraintScaleFor(const FloatRect& rect, const
> > FloatRoundedRect::Radii& radii)
> > Why does this need to go in platform?  platform does only exist to serve
> > blink... but at the same time, this is a CSS-spec-related function, and
> nothing
> > to do with FloatRects in the general case.
> > 
> > Why not just RenderStyle.h?
> 
> It doesn't need to go in platform, but that was what my reviewer suggested in
> WebKit, and at that point, I didn't have any better ideas. RenderStyle.h does
> sound like a much better idea.

Done that in patch set 4.

[eseidel, 2014-03-12 19:38:31]

If this is not perf critical, please consider making this non-inline, but
otherwise lgtm.

[Bem Jones-Bey (adobe), 2014-03-12 21:27:57]

On 2014/03/12 19:38:31, eseidel wrote:
> If this is not perf critical, please consider making this non-inline, but
> otherwise lgtm.

I don't have any data as to if this is perf critical or not, so seems that the
best course of action is to remove the inline and then add it later if there is
a perf problem.

rwlbuis, it's your call, though. :-)

[rwlbuis, 2014-03-12 22:08:38]

On 2014/03/12 21:27:57, Bem Jones-Bey (adobe) wrote:
> On 2014/03/12 19:38:31, eseidel wrote:
> > If this is not perf critical, please consider making this non-inline, but
> > otherwise lgtm.
> 
> I don't have any data as to if this is perf critical or not, so seems that the
> best course of action is to remove the inline and then add it later if there
is
> a perf problem.
> 
> rwlbuis, it's your call, though. :-)

That sounds sane, will try to do that.

[rwlbuis, 2014-03-12 22:19:44]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-03-12 22:19:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/195793009/70001

[commit-bot: I haz the power, 2014-03-13 01:41:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 01:41:52]

Try jobs failed on following builders:
  tryserver.chromium on linux_blink

[rwlbuis, 2014-03-13 01:48:59]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-03-13 01:49:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/195793009/70001

[commit-bot: I haz the power, 2014-03-13 05:04:37]

Change committed as 169086