Move columns related properties into CSSPropertyParser

Move columns related properties into CSSPropertyParser

Move columns related properties handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

[rwlbuis, 2015-10-12 20:01:00]

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

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

PTAL, this groups various related columns properties in one CL.

[Timothy Loh, 2015-10-13 00:30:03]

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

https://codereview.chromium.org/1399003004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:697: if
(range.peek().type() != NumberToken)
I probably wouldn't bother with this check

https://codereview.chromium.org/1399003004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1102: bool
CSSPropertyParser::consumeColumns(bool important)
Do we check somewhere that the input isn't blank (whitespace-only)? Otherwise
this might allow "-webkit-columns: ;"

https://codereview.chromium.org/1399003004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1173:
m_currentShorthand = oldShorthand;
?

[rwlbuis, 2015-10-13 02:27:02]

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

https://codereview.chromium.org/1399003004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:697: if
(range.peek().type() != NumberToken)
On 2015/10/13 00:30:03, Timothy Loh wrote:
> I probably wouldn't bother with this check

The check was done to filter out any calc() usage.

https://codereview.chromium.org/1399003004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1102: bool
CSSPropertyParser::consumeColumns(bool important)
On 2015/10/13 00:30:03, Timothy Loh wrote:
> Do we check somewhere that the input isn't blank (whitespace-only)? Otherwise
> this might allow "-webkit-columns: ;"

Good point, I'll check tomorrow, likely with this patch we do accept that, I
wonder if we accepted it before too.

https://codereview.chromium.org/1399003004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1173:
m_currentShorthand = oldShorthand;
On 2015/10/13 00:30:03, Timothy Loh wrote:
> ?

Columns did not use ShorthandScope, causing the output in
inspector/elements/styles-2/media-emulation.html to only state the longhands. I
am ok either way, i.e. removing above line and using the new expected result.

[Timothy Loh, 2015-10-15 06:59:38]

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

https://codereview.chromium.org/1399003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:172: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeInteger(CSSParserTokenRange&
range, CSSParserMode cssParserMode, double minimumValue =
-std::numeric_limits<double>::max())
Unrelated, but I just noticed this doesn't need a parser mode

[rwlbuis, 2015-10-15 22:53:13]

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

https://codereview.chromium.org/1399003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:172: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeInteger(CSSParserTokenRange&
range, CSSParserMode cssParserMode, double minimumValue =
-std::numeric_limits<double>::max())
On 2015/10/15 06:59:38, Timothy Loh wrote:
> Unrelated, but I just noticed this doesn't need a parser mode

Well spotted, I removed it and adjusted the callsites.

[Timothy Loh, 2015-10-19 04:56:00]

lgtm

https://codereview.chromium.org/1399003004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1399003004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1414:
m_currentShorthand = oldShorthand;
ok, but TODO to remove this

[rwlbuis, 2015-10-19 13:48:49]

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

[rwlbuis, 2015-10-19 13:48:50]

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

[commit-bot: I haz the power, 2015-10-19 13:49:23]

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

[commit-bot: I haz the power, 2015-10-19 16:26:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-19 16:28:04]

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