Move line-height property handling into CSSPropertyParser

Move line-height property handling into CSSPropertyParser

Move line-height property handling from LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780
Committed: https://crrev.com/d16ce74a235c849da49ce86a7a8c97dee6e5a37b
Cr-Commit-Position: refs/heads/master@{#352023}

[rwlbuis, 2015-10-01 20:40:40]

rob.buis@samsung.com changed reviewers:
+ alancutter@chromium.org, timloh@chromium.org

[rwlbuis, 2015-10-01 20:40:41]

PTAL.

[rwlbuis, 2015-10-01 20:55:19]

On 2015/10/01 20:40:41, rwlbuis wrote:
> PTAL.

Forgot to say, after this patch we can convert font shorthand and use the code
in this patch, so then finally we can drop more code (things like
FontFamilyBuilder) then we add :)

[Timothy Loh, 2015-10-02 01:45:28]

Shouldn't consumeLineHeight check consumeNumber and then consumeLengthOrPercent?
We reject the number+percent calcs anyway, so there's no point to using
consumeNumberOrPercent here.

[alancutter (OOO until 2018), 2015-10-02 01:54:10]

https://codereview.chromium.org/1377203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1377203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:196: if
(calculation->category() != CalcNumber || (valueRange == ValueRangeNonNegative
&& calculation->isNegative()))
I don't think calcs should be subject to parse time range checks at all
according to the spec: https://drafts.csswg.org/css-values-3/#calc-range
Altering this would be changing behaviour in a refactor patch though so perhaps
just add a TODO with the spec link to remove this check.

[rwlbuis, 2015-10-02 03:24:10]

On 2015/10/02 01:54:10, alancutter wrote:
>
https://codereview.chromium.org/1377203002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1377203002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:196: if
> (calculation->category() != CalcNumber || (valueRange == ValueRangeNonNegative
> && calculation->isNegative()))
> I don't think calcs should be subject to parse time range checks at all
> according to the spec: https://drafts.csswg.org/css-values-3/#calc-range
> Altering this would be changing behaviour in a refactor patch though so
perhaps
> just add a TODO with the spec link to remove this check.

Both this and timloh's issue above should be fixed in patch set 4, PTAL :)

[alancutter (OOO until 2018), 2015-10-02 03:31:11]

On 2015/10/02 at 01:45:28, timloh wrote:
> Shouldn't consumeLineHeight check consumeNumber and then
consumeLengthOrPercent? We reject the number+percent calcs anyway, so there's no
point to using consumeNumberOrPercent here.

I don't think anything will use consumeNumberOrPercent(), it's not supported in
rgb() or rgba().

Spec: http://www.w3.org/TR/css3-color/#rgb-color
"The format of an RGB value in the functional notation is ‘rgb(’ followed by a
comma-separated list of three numerical values (either three integer values or
three percentage values) followed by ‘)’."
Impl: CSSPropertyParser::parseColorParameters()

[alancutter (OOO until 2018), 2015-10-02 03:40:54]

lgtm

[alancutter (OOO until 2018), 2015-10-02 03:50:10]

https://codereview.chromium.org/1377203002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1377203002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:200: return
calcParser.consumeValue();
Last minute catch, this should be consumeNumber().

[rwlbuis, 2015-10-02 12:39:34]

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

[rwlbuis, 2015-10-02 12:39:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1377203002/#ps80001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-10-02 12:39:44]

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

[commit-bot: I haz the power, 2015-10-02 14:01:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-02 14:01:57]

Patchset 5 (id:??) landed as
https://crrev.com/d16ce74a235c849da49ce86a7a8c97dee6e5a37b
Cr-Commit-Position: refs/heads/master@{#352023}