[CSS Shapes] Normalize distances to closest/farthest side

[CSS Shapes] Normalize distances to closest/farthest side

When calculating the radius given the center coordinate
and reference box we could end up with negative radius
for closest-side and center x/y larger than reference
box width/height. Also just giving negative center x/y
for closest-side can return negative radius. Catch all
these cases by normalizing the distances to the sides
for both ellipse and circle.

BUG=457807, 455837
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190524

[rwlbuis, 2015-02-12 23:31:48]

rob.buis@samsung.com changed reviewers:
+ bjonesbe@adobe.com, zoltan@adobe.com

[rwlbuis, 2015-02-12 23:31:49]

PTAL. Likely a problem in WebKit too.

[rwlbuis, 2015-02-17 18:32:43]

On 2015/02/12 23:31:49, rwlbuis wrote:
> PTAL. Likely a problem in WebKit too.

ping :)

[Zoltan, 2015-02-17 18:43:53]

On 2015/02/17 18:32:43, rwlbuis wrote:
> On 2015/02/12 23:31:49, rwlbuis wrote:
> > PTAL. Likely a problem in WebKit too.
> 
> ping :)

The logic looks good. I wouldn't use the if-else shorthands for these cases to
improve readability though.

[rwlbuis, 2015-02-17 21:46:05]

On 2015/02/17 18:43:53, Zoltan wrote:
> On 2015/02/17 18:32:43, rwlbuis wrote:
> > On 2015/02/12 23:31:49, rwlbuis wrote:
> > > PTAL. Likely a problem in WebKit too.
> > 
> > ping :)
> 
> The logic looks good. I wouldn't use the if-else shorthands for these cases to
> improve readability though.

Done!

[Bem Jones-Bey (adobe), 2015-02-19 18:41:06]

So it took me a bit to wrap my mind around this one because of the naming
confusion that I outline inline. Assuming that you make the names clearer, lgtm.

https://codereview.chromium.org/923533004/diff/80001/Source/core/layout/style...
File Source/core/layout/style/BasicShapes.cpp (right):

https://codereview.chromium.org/923533004/diff/80001/Source/core/layout/style...
Source/core/layout/style/BasicShapes.cpp:83: float radiusX, radiusY;
This naming confused me. What we have here is not the radius, but it's the
distance between the side of the reference box and the center of the circle.

Maybe call it widthDelta and heightDelta? (Unless you have a better name)

[rwlbuis, 2015-02-19 18:54:23]

On 2015/02/19 18:41:06, Bem Jones-Bey (adobe) wrote:
> So it took me a bit to wrap my mind around this one because of the naming
> confusion that I outline inline. Assuming that you make the names clearer,
lgtm.

Fair enough. Yeah it is used to calculate radius but not the radius itself. I'll
try to
come up with a better name :)

[rwlbuis, 2015-02-19 19:00:12]

New patchsets have been uploaded after l-g-t-m from bjonesbe@adobe.com

[rwlbuis, 2015-02-19 21:50:42]

rob.buis@samsung.com changed reviewers:
+ leviw@chromium.org

[rwlbuis, 2015-02-19 21:50:42]

@leviw PTAL

[leviw_travelin_and_unemployed, 2015-02-19 21:59:44]

https://codereview.chromium.org/923533004/diff/100001/Source/core/layout/styl...
File Source/core/layout/style/BasicShapes.cpp (right):

https://codereview.chromium.org/923533004/diff/100001/Source/core/layout/styl...
Source/core/layout/style/BasicShapes.cpp:91: heightDelta = center.y() -
boxSize.height();
Will heightDelta ever not be the same as std::abs(boxSize.height() - center.y())

[rwlbuis, 2015-02-19 22:55:31]

On 2015/02/19 21:59:44, leviw wrote:
>
https://codereview.chromium.org/923533004/diff/100001/Source/core/layout/styl...
> File Source/core/layout/style/BasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/923533004/diff/100001/Source/core/layout/styl...
> Source/core/layout/style/BasicShapes.cpp:91: heightDelta = center.y() -
> boxSize.height();
> Will heightDelta ever not be the same as std::abs(boxSize.height() -
center.y())

Very well spotted :) Yes that should work, triggering a new buildbot run for the
new patch.

[leviw_travelin_and_unemployed, 2015-02-19 22:56:53]

https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
File Source/core/layout/style/BasicShapes.cpp (right):

https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
Source/core/layout/style/BasicShapes.cpp:131: return std::min(std::abs(center),
boxWidthOrHeight > center ? boxWidthOrHeight - center : center -
boxWidthOrHeight);
Same applies here?

https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
Source/core/layout/style/BasicShapes.cpp:134: return std::max(center,
boxWidthOrHeight > center ? boxWidthOrHeight - center : center -
boxWidthOrHeight);
Ditto?

[Bem Jones-Bey (adobe), 2015-02-19 23:06:30]

https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
File Source/core/layout/style/BasicShapes.cpp (right):

https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
Source/core/layout/style/BasicShapes.cpp:84: float heightDelta =
std::abs(boxSize.height() - center.y());
I don't seem to me that these variables make anything clearer now.

[rwlbuis, 2015-02-19 23:12:17]

On 2015/02/19 23:06:30, Bem Jones-Bey (adobe) wrote:
>
https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
> File Source/core/layout/style/BasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
> Source/core/layout/style/BasicShapes.cpp:84: float heightDelta =
> std::abs(boxSize.height() - center.y());
> I don't seem to me that these variables make anything clearer now.

Well, it is shorter for sure. Personally I prefer that. Or do you mean the
variable name is still not ideal?

[Bem Jones-Bey (adobe), 2015-02-19 23:16:33]

On 2015/02/19 23:12:17, rwlbuis wrote:
> On 2015/02/19 23:06:30, Bem Jones-Bey (adobe) wrote:
> >
>
https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
> > File Source/core/layout/style/BasicShapes.cpp (right):
> > 
> >
>
https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
> > Source/core/layout/style/BasicShapes.cpp:84: float heightDelta =
> > std::abs(boxSize.height() - center.y());
> > I don't seem to me that these variables make anything clearer now.
> 
> Well, it is shorter for sure. Personally I prefer that. Or do you mean the
> variable name is still not ideal?

Wasn't commenting on the variable name, just the use of the variables.

I don't feel particularly strongly, so if you like them, feel free to keep them.
:-)

[rwlbuis, 2015-02-19 23:18:13]

On 2015/02/19 23:16:33, Bem Jones-Bey (adobe) wrote:
> On 2015/02/19 23:12:17, rwlbuis wrote:
> > On 2015/02/19 23:06:30, Bem Jones-Bey (adobe) wrote:
> > >
> >
>
https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
> > > File Source/core/layout/style/BasicShapes.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/923533004/diff/120001/Source/core/layout/styl...
> > > Source/core/layout/style/BasicShapes.cpp:84: float heightDelta =
> > > std::abs(boxSize.height() - center.y());
> > > I don't seem to me that these variables make anything clearer now.
> > 
> > Well, it is shorter for sure. Personally I prefer that. Or do you mean the
> > variable name is still not ideal?
> 
> Wasn't commenting on the variable name, just the use of the variables.
> 
> I don't feel particularly strongly, so if you like them, feel free to keep
them.
> :-)

Thanks for clearing it up :) Hopefully we can go with patch set 8 assuming it
turns out green.

[rwlbuis, 2015-02-20 01:51:13]

On 2015/02/19 23:18:13, rwlbuis wrote:
> Thanks for clearing it up :) Hopefully we can go with patch set 8 assuming it
> turns out green.

All green! :)

[leviw_travelin_and_unemployed, 2015-02-20 01:54:31]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-02-20 01:54:31]

LGTM.

[commit-bot: I haz the power, 2015-02-20 01:56:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923533004/140001

[commit-bot: I haz the power, 2015-02-20 02:15:02]

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

[commit-bot: I haz the power, 2015-02-20 02:15:03]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)

[rwlbuis, 2015-02-20 03:59:34]

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

[commit-bot: I haz the power, 2015-02-20 04:00:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923533004/140001

[commit-bot: I haz the power, 2015-02-20 04:11:48]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190524