[CSS Shapes] shape-margin in percentage units always computes to 0px

[CSS Shapes] shape-margin in percentage units always computes to 0px

Make shape-margin parsing accept percentages and properly compute the
length once the percentage is accepted.

This is a merge of http://trac.webkit.org/changeset/166787 by Bem Jones-Bey <bjonesbe@adobe.com>;.

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

[Bem Jones-Bey (adobe), 2014-04-04 23:03:53]

https://codereview.chromium.org/226373002/diff/20001/Source/core/rendering/sh...
File Source/core/rendering/shapes/ShapeInfo.cpp (right):

https://codereview.chromium.org/226373002/diff/20001/Source/core/rendering/sh...
Source/core/rendering/shapes/ShapeInfo.cpp:117: LayoutUnit maximumValue =
m_renderer.containingBlock() ? m_renderer.containingBlock()->contentWidth() :
LayoutUnit();
Isn't there an implicit conversion from LayoutUnit to float? If so, wouldn't
using 0.0f instead of LayoutUnit() here mean that you could store this in a
float and not need the explicit toFloat() below?

[leviw_travelin_and_unemployed, 2014-04-07 17:54:38]

https://codereview.chromium.org/226373002/diff/20001/Source/core/rendering/sh...
File Source/core/rendering/shapes/ShapeInfo.cpp (right):

https://codereview.chromium.org/226373002/diff/20001/Source/core/rendering/sh...
Source/core/rendering/shapes/ShapeInfo.cpp:117: LayoutUnit maximumValue =
m_renderer.containingBlock() ? m_renderer.containingBlock()->contentWidth() :
LayoutUnit();
On 2014/04/04 23:03:53, Bem Jones-Bey (adobe) wrote:
> Isn't there an implicit conversion from LayoutUnit to float? If so, wouldn't
> using 0.0f instead of LayoutUnit() here mean that you could store this in a
> float and not need the explicit toFloat() below?

We're actually working on killing the implicit conversions to/from LayoutUnits,
as they're quite prone to misuse.

[rwlbuis, 2014-04-10 18:44:02]

So I think this patch is fine IMHO, but it only passes on Mac. After some
investigation, the layer calculation seems different for Mac and Win/Linux.
Basically the paragraph above the shape container seems to have the effect of
pushing it down on Win/Linux. The height of the textbox is one pixel bigger for
one
thing, after looking at the layout text dump. So I am guessing it is either
compositor/font related (no default font is forced for the overall doc). To be
honest
I think that all that is not what the test is about, so my suggestion would be
to add top to the shape container, which uses absolute positioning anyway.

[Bem Jones-Bey (adobe), 2014-04-11 20:52:32]

lgtm

[rwlbuis, 2014-04-11 20:55:02]

This patch finally seems to be in good shape :)

[leviw_travelin_and_unemployed, 2014-04-11 21:04:16]

lgtm

https://codereview.chromium.org/226373002/diff/60001/LayoutTests/fast/shapes/...
File
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-shape-margin-percent.html
(right):

https://codereview.chromium.org/226373002/diff/60001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-shape-margin-percent.html:22:
-webkit-shape-margin: 10%;
We don't need these prefixes, right?

https://codereview.chromium.org/226373002/diff/60001/Source/core/rendering/sh...
File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/226373002/diff/60001/Source/core/rendering/sh...
Source/core/rendering/shapes/ShapeOutsideInfo.cpp:37: 
supernit: why this empty space? I'm only used to seeing a space between the
config/header for this cpp includes, not for core vs platform.

[rwlbuis, 2014-04-11 21:11:03]

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

[commit-bot: I haz the power, 2014-04-11 21:11:07]

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

[rwlbuis, 2014-04-11 21:12:02]

On 2014/04/11 21:04:16, Levi wrote:
> lgtm
> 
>
https://codereview.chromium.org/226373002/diff/60001/LayoutTests/fast/shapes/...
> File
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-shape-margin-percent.html
> (right):
> 
>
https://codereview.chromium.org/226373002/diff/60001/LayoutTests/fast/shapes/...
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-shape-margin-percent.html:22:
> -webkit-shape-margin: 10%;
> We don't need these prefixes, right?

Oops, correct, removed them.

>
https://codereview.chromium.org/226373002/diff/60001/Source/core/rendering/sh...
> File Source/core/rendering/shapes/ShapeOutsideInfo.cpp (right):
> 
>
https://codereview.chromium.org/226373002/diff/60001/Source/core/rendering/sh...
> Source/core/rendering/shapes/ShapeOutsideInfo.cpp:37: 
> supernit: why this empty space? I'm only used to seeing a space between the
> config/header for this cpp includes, not for core vs platform.

Yep, no particular reason, fixed.
Thanks for the review!

[commit-bot: I haz the power, 2014-04-11 22:58:51]

Change committed as 171378