Fix number handling in accumulatePixelsAndPercent.

Fix number handling in accumulatePixelsAndPercent.

blink::CSSCalcPrimitiveValue::accumulatePixelsAndPercent supposes
that it only can receive values with length or percentage unit types
and misses the case when a number is received. The patch makes this
function aware of such situations.

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

[Timothy Loh, 2014-12-15 22:28:14]

timloh@chromium.org changed reviewers:
+ alancutter@chromium.org

[Timothy Loh, 2014-12-15 22:28:14]

+alancutter

I'm not sure if we should actually be allowed to calc(percent + number) here (or
anywhere). In line-height, a percentage is relative to the element's font-size,
and CSS Values says:

"If percentages are accepted in the context in which the expression is placed, a
<percentage-token> has the type of the value that percentages are relative to;
otherwise, a math expression containing percentages is invalid."

[reni, 2014-12-16 19:08:18]

On 2014/12/15 22:28:14, Timothy Loh wrote:
> +alancutter
> 
> I'm not sure if we should actually be allowed to calc(percent + number) here
(or
> anywhere). In line-height, a percentage is relative to the element's
font-size,
> and CSS Values says:
> 
> "If percentages are accepted in the context in which the expression is placed,
a
> <percentage-token> has the type of the value that percentages are relative to;
> otherwise, a math expression containing percentages is invalid."

Indeed, you are right. calc(percent + num) is not correct. I was confused
because 'line-height' can accept both numbers and percentages.
However, it seems that a similar confusion causes the real problem too.
calc(percent + num) is identified correctly as a CalcPercentNumber type of
calculation and it will also be accepted as a valid calculation unit for the
line-height property since it enables both types.
The problem is that StyleBuilderConverter::convertLineHeight supposes that it
receives only valid CSS values at this point and it's not aware of the case when
a CalcPercentNumber is received.
In the second patch we accept the only valid calc type (CalcPercentLength) and
fall back to the default 'normal' value in every other cases (that's in case of
the explicit 'normal' value and in case of CalcPercentNumbers).

[Timothy Loh, 2014-12-17 03:22:32]

On 2014/12/16 19:08:18, reni wrote:
> On 2014/12/15 22:28:14, Timothy Loh wrote:
> > +alancutter
> > 
> > I'm not sure if we should actually be allowed to calc(percent + number) here
> (or
> > anywhere). In line-height, a percentage is relative to the element's
> font-size,
> > and CSS Values says:
> > 
> > "If percentages are accepted in the context in which the expression is
placed,
> a
> > <percentage-token> has the type of the value that percentages are relative
to;
> > otherwise, a math expression containing percentages is invalid."
> 
> Indeed, you are right. calc(percent + num) is not correct. I was confused
> because 'line-height' can accept both numbers and percentages.
> However, it seems that a similar confusion causes the real problem too.
> calc(percent + num) is identified correctly as a CalcPercentNumber type of
> calculation and it will also be accepted as a valid calculation unit for the
> line-height property since it enables both types.
> The problem is that StyleBuilderConverter::convertLineHeight supposes that it
> receives only valid CSS values at this point and it's not aware of the case
when
> a CalcPercentNumber is received.
> In the second patch we accept the only valid calc type (CalcPercentLength) and
> fall back to the default 'normal' value in every other cases (that's in case
of
> the explicit 'normal' value and in case of CalcPercentNumbers).

The StyleBuilder should be able to assume it has valid CSSValues, so we should
update the parser to disallow these for line-height.

[alancutter (OOO until 2018), 2014-12-31 03:38:07]

alancutter@chromium.org changed reviewers:
+ timloh@chromium.org

[alancutter (OOO until 2018), 2014-12-31 03:38:08]

Non-OWNER lgtm with nits.
Description needs updating to match patch.

https://codereview.chromium.org/787073006/diff/60001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/787073006/diff/60001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:4690: validPrimitive = false;
You should release() m_parsedCalulation here if it's no longer going to be
consumed by parseValidPrimitive().

[Timothy Loh, 2015-01-07 06:29:33]

On 2014/12/31 03:38:08, alancutter wrote:
> Non-OWNER lgtm with nits.
> Description needs updating to match patch.
> 
>
https://codereview.chromium.org/787073006/diff/60001/Source/core/css/parser/C...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/787073006/diff/60001/Source/core/css/parser/C...
> Source/core/css/parser/CSSPropertyParser.cpp:4690: validPrimitive = false;
> You should release() m_parsedCalulation here if it's no longer going to be
> consumed by parseValidPrimitive().

rslgtm

[reni, 2015-01-07 16:34:27]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2015-01-07 16:35:18]

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

[commit-bot: I haz the power, 2015-01-07 16:41:47]

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

[commit-bot: I haz the power, 2015-01-07 16:41:48]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[reni, 2015-01-07 19:25:14]

Thanks for the reviews! But it seems that we still lack of an OWNER lgtm
(according to the presubmit bot).

[Timothy Loh, 2015-01-07 22:50:30]

On 2015/01/07 19:25:14, reni wrote:
> Thanks for the reviews! But it seems that we still lack of an OWNER lgtm
> (according to the presubmit bot).

Err, I forget that rietveld is now slightly stricter on searching for lgtm in
comments, hopefully this'll do it...

[reni, 2015-01-08 07:43:39]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2015-01-08 07:44:10]

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

[commit-bot: I haz the power, 2015-01-08 07:48:55]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188029