Bug 129726

Created attachment 225882 [details] Patch

I did not add any tests for zero radii here because I ran into Bug 129739. I will add tests for that when I fix that.

Comment on attachment 225882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225882&action=review > Source/WebCore/rendering/shapes/Shape.cpp:131 > + float topRadiiExtent = radii.topLeft().width() + radii.topRight().width(); > + float rightRadiiExtent = radii.topRight().height() + radii.bottomRight().height(); > + float bottomRadiiExtent = radii.bottomLeft().width() + radii.bottomRight().width(); > + float leftRadiiExtent = radii.topLeft().height() + radii.bottomLeft().height(); > + > + float topRatio = topRadiiExtent ? (rect.width() / topRadiiExtent) : 1; > + float rightRatio = rightRadiiExtent ? (rect.height() / rightRadiiExtent) : 1; > + float bottomRatio = bottomRadiiExtent ? (rect.width() / bottomRadiiExtent) : 1; > + float leftRatio = leftRadiiExtent ? (rect.height() / leftRadiiExtent) : 1; > + > + float reductionRatio = std::min<float>(topRatio, std::min<float>(rightRatio, std::min<float>(bottomRatio, leftRatio))); > + if (reductionRatio < 1) > + roundedRect.scaleRadii(reductionRatio); Is there anything that can be shared with border-radius?

Created attachment 225917 [details] Patch

(In reply to comment #3) > (From update of attachment 225882 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225882&action=review > > > Source/WebCore/rendering/shapes/Shape.cpp:131 > > + float topRadiiExtent = radii.topLeft().width() + radii.topRight().width(); > > + float rightRadiiExtent = radii.topRight().height() + radii.bottomRight().height(); > > + float bottomRadiiExtent = radii.bottomLeft().width() + radii.bottomRight().width(); > > + float leftRadiiExtent = radii.topLeft().height() + radii.bottomLeft().height(); > > + > > + float topRatio = topRadiiExtent ? (rect.width() / topRadiiExtent) : 1; > > + float rightRatio = rightRadiiExtent ? (rect.height() / rightRadiiExtent) : 1; > > + float bottomRatio = bottomRadiiExtent ? (rect.width() / bottomRadiiExtent) : 1; > > + float leftRatio = leftRadiiExtent ? (rect.height() / leftRadiiExtent) : 1; > > + > > + float reductionRatio = std::min<float>(topRatio, std::min<float>(rightRatio, std::min<float>(bottomRatio, leftRatio))); > > + if (reductionRatio < 1) > > + roundedRect.scaleRadii(reductionRatio); > > Is there anything that can be shared with border-radius? Yeah, I should have done that in the first place. The new patch reuses the same function as border-radius.

Comment on attachment 225917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225917&action=review > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29 > +template <class Rect, class Radii> > +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii) Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header?

(In reply to comment #6) > (From update of attachment 225917 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225917&action=review > > > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29 > > +template <class Rect, class Radii> > > +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii) > > Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header? I would unfortunately have to add it to both of them. The shapes geometry code uses floats, and the style and layout code uses LayoutUnits. Converting in the middle of the geometry code to LayoutUnits and then back to floats has caused problems in the past due to the imprecise nature of that conversion. And I don't think we want to be converting to FloatRoundedRect and back every time we need to do this calculation in RenderStyle (or in my fix for Bug 127982, for that matter). I did notice that there is an adjustRadii() call in RoundedRect, but it doesn't do exactly the same calculation as the border radii case, and it looks like it is only used for box shadow (RenderBoxModelObject::paintBoxShadow).

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 225917 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=225917&action=review > > > > > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29 > > > +template <class Rect, class Radii> > > > +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii) > > > > Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header? > > I would unfortunately have to add it to both of them. The shapes geometry code uses floats, and the style and layout code uses LayoutUnits. Converting in the middle of the geometry code to LayoutUnits and then back to floats has caused problems in the past due to the imprecise nature of that conversion. And I don't think we want to be converting to FloatRoundedRect and back every time we need to do this calculation in RenderStyle (or in my fix for Bug 127982, for that matter). > > I did notice that there is an adjustRadii() call in RoundedRect, but it doesn't do exactly the same calculation as the border radii case, and it looks like it is only used for box shadow (RenderBoxModelObject::paintBoxShadow). Is it possible to create two inline functions where one calls the other but does the conversion first?

Created attachment 226004 [details] Patch

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (From update of attachment 225917 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=225917&action=review > > > > > > > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29 > > > > +template <class Rect, class Radii> > > > > +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii) > > > > > > Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header? > > > > I would unfortunately have to add it to both of them. The shapes geometry code uses floats, and the style and layout code uses LayoutUnits. Converting in the middle of the geometry code to LayoutUnits and then back to floats has caused problems in the past due to the imprecise nature of that conversion. And I don't think we want to be converting to FloatRoundedRect and back every time we need to do this calculation in RenderStyle (or in my fix for Bug 127982, for that matter). > > > > I did notice that there is an adjustRadii() call in RoundedRect, but it doesn't do exactly the same calculation as the border radii case, and it looks like it is only used for box shadow (RenderBoxModelObject::paintBoxShadow). > > Is it possible to create two inline functions where one calls the other but does the conversion first? I looked into this and discovered that there are some implicit conversions from LayoutRect/RoundedRect -> FloatRect/FloatRoundedRect. So I just changed the signature to take the float varieties, and not have a template. WDYT?

Created attachment 226011 [details] Patch

Comment on attachment 226011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226011&action=review > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:31 > +inline float calcBorderRadiiConstraintScaleFor(const FloatRect& rect, const FloatRoundedRect::Radii& radii) looks nicer already. Do you think we can put that on FloatRoundedRect.h instead of creating a new header file?

Created attachment 226019 [details] Patch

(In reply to comment #12) > (From update of attachment 226011 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226011&action=review > > > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:31 > > +inline float calcBorderRadiiConstraintScaleFor(const FloatRect& rect, const FloatRoundedRect::Radii& radii) > > looks nicer already. Do you think we can put that on FloatRoundedRect.h instead of creating a new header file? Ok, WDYT? (I wish I could make it just take a FloatRoundedRect, but that would require some serious surgery to how RenderStyle does it's thing.)

Comment on attachment 226019 [details] Patch r=me

Comment on attachment 226019 [details] Patch Clearing flags on attachment: 226019 Committed r165261: <http://trac.webkit.org/changeset/165261>

All reviewed patches have been landed. Closing bug.

Committed r165262: <http://trac.webkit.org/changeset/165262>

Re-opened since this is blocked by bug 129887

The reopen was spurious; a different commit got tangled up with this one, and that's the one that got reverted.