Add consumeInteger/consumeLength

Add consumeInteger/consumeLength

Add consumeInteger/consumeLength and make use of them to
convert word-spacing, letter-spacing and tab-size.

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

[rwlbuis, 2015-09-18 22:00:40]

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

[rwlbuis, 2015-09-18 22:00:40]

PTAL. I did a first stab at implementing consumeNumericValue as we discussed in
a previous CL. Note that calc() support is not implemented yet (also not
required for these properties I think). Before that I wanted to hear people's
opinion on ValueWithCalculation, m_parsedCalculation or some other approach.
Given that this seems the most important/complex part of bug 499780, feel free
to discuss at length :)

[Timothy Loh, 2015-09-22 13:54:25]

IMO calc() should be pretty easy to wire up since we just return a different
CSSValue type? The m_parsedCalculation stuff was because the legacy property
parser separated validation from creating CSSValues.

I think that integer values should be parsed separately from the other numeric
values. This could just be consumeInteger(CSSParserTokenRange& range, int
minimum = std::numeric_limits<int>::min()) (..not sure if that's a valid default
argument). Integer values are never allowed in calcs with other types.

This also needs to do some sort of range validation. We're (as of a couple of
days ago) only allowing values which fit in floats, see
CSSPropertyParser.cpp::isValidNumericValue, and should probably keep this
behaviour. (I just noticed that calcs aren't getting these checks...)

https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:96: CSSParserToken numericToken =
range.peek();
const CSSParserToken& :-)

I really ought to make these not be copy constructible...

https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:97: if (unitflags &
CSSPropertyParser::FNonNeg && numericToken.numericValue() < 0)
numericValue() isn't set up to work for non-numeric types (i.e. not a
PercentageToken/NumberToken/DimensionToken). There's nothing really unsafe about
this, but ASAN will probably complain. Let's just check the type is numeric up
front first.

https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:101: case
CSSPrimitiveValue::UnitType::Number:
IMO we should do this as:

if (token.type() == NumberToken) {
} else if (token.type() == PercentageToken() {
} else { // DimensionToken
}

In particular, I'm not convinced that having unitType() do something for
non-<dimension-token>s is a good idea.

https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:149: validNumber = unitflags &
CSSPropertyParser::FResolution;
No point adding this if we don't have any properties that use it right now.

[rwlbuis, 2015-09-22 22:06:19]

On 2015/09/22 13:54:25, Timothy Loh wrote:
> IMO calc() should be pretty easy to wire up since we just return a different
> CSSValue type? The m_parsedCalculation stuff was because the legacy property
> parser separated validation from creating CSSValues.

Yes I added it now, not too hard indeed.

> I think that integer values should be parsed separately from the other numeric
> values. This could just be consumeInteger(CSSParserTokenRange& range, int
> minimum = std::numeric_limits<int>::min()) (..not sure if that's a valid
default
> argument). Integer values are never allowed in calcs with other types.

Sounds good, but maybe we can add once needed.

> This also needs to do some sort of range validation. We're (as of a couple of
> days ago) only allowing values which fit in floats, see
> CSSPropertyParser.cpp::isValidNumericValue, and should probably keep this
> behaviour. (I just noticed that calcs aren't getting these checks...)

I see. Should it be added now? No tests cover it for the props I added so far.

>
https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:96: CSSParserToken numericToken =
> range.peek();
> const CSSParserToken& :-)
> 
> I really ought to make these not be copy constructible...

So I needed to do this because of setUnitType :) But I think I found a better
way.

>
https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:97: if (unitflags &
> CSSPropertyParser::FNonNeg && numericToken.numericValue() < 0)
> numericValue() isn't set up to work for non-numeric types (i.e. not a
> PercentageToken/NumberToken/DimensionToken). There's nothing really unsafe
about
> this, but ASAN will probably complain. Let's just check the type is numeric up
> front first.

Done.

>
https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:101: case
> CSSPrimitiveValue::UnitType::Number:
> IMO we should do this as:
> 
> if (token.type() == NumberToken) {
> } else if (token.type() == PercentageToken() {
> } else { // DimensionToken
> }
> 
> In particular, I'm not convinced that having unitType() do something for
> non-<dimension-token>s is a good idea.

Good point, done.

>
https://codereview.chromium.org/1348363004/diff/40001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:149: validNumber = unitflags &
> CSSPropertyParser::FResolution;
> No point adding this if we don't have any properties that use it right now.

Removed.

PTAL! :)

[rwlbuis, 2015-09-24 20:25:38]

Patchset #8 (id:140001) has been deleted

[rwlbuis, 2015-09-24 22:47:05]

> 
> PTAL! :)

Note that latest CL is now applicable post-merge.

[alancutter (OOO until 2018), 2015-09-25 04:13:06]

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

https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:182: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
consumeNumericValue(CSSParserTokenRange& range, CSSPropertyParser::Units
unitflags, CSSParserMode cssParserMode)
After discussing with timloh we decided that splitting up this switch from
valueUnit() into separate functions would make the individual call sites easier
to reason about. Currently you have to consider the entire function to determine
what the behaviour is for any given set of flags.

It could be split up into the following functions:
consumeAngle()
consumeTime()
consumeInteger(int minimum)
consumePercent(NonNeg)
consumeLength(NonNeg, UnitlessQuirks, CSSParserMode)
consumeNumber(NonNeg)
consumeLengthOrPercent(NonNeg, UnitlessQuirks, CSSParserMode)
consumeNumberOrPercent(NonNeg)

Places that currently use validUnit(FLength | FNumber) would use consumeLength()
and consumeNumber() consecutively. This makes it more obvious that it won't
accept calc(25 + 34px).

[Timothy Loh, 2015-09-25 04:40:00]

On 2015/09/25 04:13:06, alancutter wrote:
> Places that currently use validUnit(FLength | FNumber) would use
consumeLength()
> and consumeNumber() consecutively. This makes it more obvious that it won't
> accept calc(25 + 34px).

BTW there'd be a minor difference between consumeLength/consumeNumber calling
orders since 0 would normally be accepted by both, so we should be careful to
maintain the existing behaviour here.

[Timothy Loh, 2015-09-25 04:44:33]

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

https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:186:
CSSParserTokenRange args = consumeFunction(range);
BTW this will advance the token stream for invalid calcs, which will probably
end up causing bugs later...

https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:522: return
consumeNumericValue(m_range, FLength | FUnitlessQuirk, m_context.mode());
// TODO(rwlbuis): Don't allow unitless values, and allow <percentage>s in
word-spacing

(or mark me as the TODO owner, I don't mind)

[rwlbuis, 2015-09-27 21:03:20]

On 2015/09/25 04:44:33, Timothy Loh wrote:
>
https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:186:
> CSSParserTokenRange args = consumeFunction(range);
> BTW this will advance the token stream for invalid calcs, which will probably
> end up causing bugs later...

Is it really a problem in this case? AFAIK if it a calc token and the calc turns
out to be invalid, the whole property will be invalid so it does not matter the
stream is advanced?

>
https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:522: return
> consumeNumericValue(m_range, FLength | FUnitlessQuirk, m_context.mode());
> // TODO(rwlbuis): Don't allow unitless values, and allow <percentage>s in
> word-spacing
> 
> (or mark me as the TODO owner, I don't mind)

I personally do not really like assigning in the TODO, though I guess it avoids
confusion, it can also block if the assignee doesn't address it. Anyway I
assigned to you since this whole refactor will keep me busy for a while :)

[rwlbuis, 2015-09-27 21:04:22]

On 2015/09/25 04:40:00, Timothy Loh wrote:
> On 2015/09/25 04:13:06, alancutter wrote:
> > Places that currently use validUnit(FLength | FNumber) would use
> consumeLength()
> > and consumeNumber() consecutively. This makes it more obvious that it won't
> > accept calc(25 + 34px).
> 
> BTW there'd be a minor difference between consumeLength/consumeNumber calling
> orders since 0 would normally be accepted by both, so we should be careful to
> maintain the existing behaviour here.

You mean because one code path may do 0 -> 0px and the other not? Anyway we do
need to keep that in mind and hopefully the tests will catch it.

[rwlbuis, 2015-09-27 21:07:40]

On 2015/09/25 04:13:06, alancutter wrote:
> After discussing with timloh we decided that splitting up this switch from
> valueUnit() into separate functions would make the individual call sites
easier
> to reason about. Currently you have to consider the entire function to
determine
> what the behaviour is for any given set of flags.
> 
> It could be split up into the following functions:
> consumeAngle()
> consumeTime()
> consumeInteger(int minimum)
> consumePercent(NonNeg)
> consumeLength(NonNeg, UnitlessQuirks, CSSParserMode)
> consumeNumber(NonNeg)
> consumeLengthOrPercent(NonNeg, UnitlessQuirks, CSSParserMode)
> consumeNumberOrPercent(NonNeg)
> 
> Places that currently use validUnit(FLength | FNumber) would use
consumeLength()
> and consumeNumber() consecutively. This makes it more obvious that it won't
> accept calc(25 + 34px).

I like it! I am a bit worried that we'll end up creating the same calc multiple
times this way, but we'll have to see.

I implemented a subset of above, PTAL. I think we can introduce the others by
converting properties (ideally just one to keep the patch small) that use
exactly one of the above consume functions.

[alancutter (OOO until 2018), 2015-09-27 23:13:38]

> I implemented a subset of above, PTAL. I think we can introduce the others by
converting properties (ideally just one to keep the patch small) that use
exactly one of the above consume functions.

Good idea.

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

https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:144: // TODO:
Always resolve calc() to a UnitType::Number if there are no non-numbers
specified in the unitflags.
No more unitflags, this can be resolved now. (:

https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:487: if
((parsedValue = consumeInteger(range, cssParserMode, 0)))
May as well do the assignment in the declaration, having it in the condition is
more complicated to read.

https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right):

https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:383: inline bool
shouldAcceptUnitLessValues(double fValue, CSSPropertyParser::Units unitflags,
CSSParserMode cssParserMode)
This should move back into LegacyCSSPropertyParser.cpp.

[rwlbuis, 2015-09-28 14:03:02]

On 2015/09/27 23:13:38, alancutter wrote:
>
https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:144: // TODO:
> Always resolve calc() to a UnitType::Number if there are no non-numbers
> specified in the unitflags.
> No more unitflags, this can be resolved now. (:

Comment removed.

>
https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:487: if
> ((parsedValue = consumeInteger(range, cssParserMode, 0)))
> May as well do the assignment in the declaration, having it in the condition
is
> more complicated to read.

Sure, done.

>
https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right):
> 
>
https://codereview.chromium.org/1348363004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:383: inline bool
> shouldAcceptUnitLessValues(double fValue, CSSPropertyParser::Units unitflags,
> CSSParserMode cssParserMode)
> This should move back into LegacyCSSPropertyParser.cpp.

Good point. moved back to its original state.

Surprised I got no comment on re-using the ValueRange for non-negative testing
:) I guess that means it is a good enough solution.

PTAL.

[rwlbuis, 2015-09-28 21:54:23]

On 2015/09/28 14:03:02, rwlbuis wrote:
> PTAL.

Ouch, I noticed that consumeFunction calls were lacking after the
consumeInteger/consumeLength additions, added them. Note that this must mean we
do not test calc in combination with letter-spacing/word-spacing/tab-size....

I have some follow-up patches ready building on this. But for now, this time for
real, PTAL :)

[alancutter (OOO until 2018), 2015-09-28 23:40:04]

lgtm with nit and comment.

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

https://codereview.chromium.org/1348363004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:133: if
(token.numericValueType() == NumberValueType || token.numericValue() <
static_cast<double>(minimumValue))
No need to cast.

https://codereview.chromium.org/1348363004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:143: return
nullptr;
calc()s are special and clamp to the allowed range:
https://drafts.csswg.org/css-values-3/#calc-range
We should probably add some layout test coverage for uses of calc() with the
properties you're migrating across to make sure this code is tested.

[Timothy Loh, 2015-09-29 00:21:58]

On 2015/09/27 21:03:20, rwlbuis wrote:
> On 2015/09/25 04:44:33, Timothy Loh wrote:
> >
>
https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:186:
> > CSSParserTokenRange args = consumeFunction(range);
> > BTW this will advance the token stream for invalid calcs, which will
probably
> > end up causing bugs later...
> 
> Is it really a problem in this case? AFAIK if it a calc token and the calc
turns
> out to be invalid, the whole property will be invalid so it does not matter
the
> stream is advanced?

In consumeTabSize here, calc(10px) would get consumed by consumeInteger and we'd
end up failing (simlarly, "calc(bla) 10px" would be accepted).

I chatted more with alancutter@ about this and we came up with a way to do this
that's hopefully not too complicated (it'll only be used in the
consumeNumber/etc. functions anyway). The idea is to have an object which holds
on to the CSSCalcValue, with functions releaseValue() and releaseNumber() which
return CSSPrimitiveValues (calc for the first, number/integer for the second)
and will update the local range when doing so.

Something like:

class CSSCalcValue {
public:
    CSSCalcValue(CSSParserTokenRange& range, ValueRange valueRange);
    // store passed in range in m_originalRange
    // check functionId, quit if definitely not a calc
    // copy range to m_range, consumeFunction there
    // parse calc into m_calcValue

    const CSSCalcValue* value() const;
    PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseValue(); // update
m_originalRange, put m_calcValue in a primitive value and return
    PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseNumber();

private:
    CSSParserTokenRange& m_originalRange;
    CSSParserTokenRange m_range;
    RefPtrWillBeRawPtr<CSSCalcValue> m_calcValue;
    ValueRange m_valueRange;
};

[Timothy Loh, 2015-09-29 00:22:28]

On 2015/09/29 00:21:58, Timothy Loh wrote:
> On 2015/09/27 21:03:20, rwlbuis wrote:
> > On 2015/09/25 04:44:33, Timothy Loh wrote:
> > >
> >
>
https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1348363004/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:186:
> > > CSSParserTokenRange args = consumeFunction(range);
> > > BTW this will advance the token stream for invalid calcs, which will
> probably
> > > end up causing bugs later...
> > 
> > Is it really a problem in this case? AFAIK if it a calc token and the calc
> turns
> > out to be invalid, the whole property will be invalid so it does not matter
> the
> > stream is advanced?
> 
> In consumeTabSize here, calc(10px) would get consumed by consumeInteger and
we'd
> end up failing (simlarly, "calc(bla) 10px" would be accepted).
> 
> I chatted more with alancutter@ about this and we came up with a way to do
this
> that's hopefully not too complicated (it'll only be used in the
> consumeNumber/etc. functions anyway). The idea is to have an object which
holds
> on to the CSSCalcValue, with functions releaseValue() and releaseNumber()
which
> return CSSPrimitiveValues (calc for the first, number/integer for the second)
> and will update the local range when doing so.
> 
> Something like:
> 
> class CSSCalcValue {
> public:
>     CSSCalcValue(CSSParserTokenRange& range, ValueRange valueRange);
>     // store passed in range in m_originalRange
>     // check functionId, quit if definitely not a calc
>     // copy range to m_range, consumeFunction there
>     // parse calc into m_calcValue
> 
>     const CSSCalcValue* value() const;
>     PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseValue(); // update
> m_originalRange, put m_calcValue in a primitive value and return
>     PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseNumber();
> 
> private:
>     CSSParserTokenRange& m_originalRange;
>     CSSParserTokenRange m_range;
>     RefPtrWillBeRawPtr<CSSCalcValue> m_calcValue;
>     ValueRange m_valueRange;
> };

err, not named CSSCalcValue, maybe like CalcParseScope or something

[alancutter (OOO until 2018), 2015-09-29 00:54:36]

>     CSSParserTokenRange& m_originalRange;

s/m_originalRange/m_sourceRange/

>     ValueRange m_valueRange;

The ValueRange will be stored in CSSCalcValue, no need to store it here.

[rwlbuis, 2015-09-29 02:47:20]

On 2015/09/29 00:21:58, Timothy Loh wrote:
> > Is it really a problem in this case? AFAIK if it a calc token and the calc
> turns
> > out to be invalid, the whole property will be invalid so it does not matter
> the
> > stream is advanced?
> 
> In consumeTabSize here, calc(10px) would get consumed by consumeInteger and
we'd
> end up failing (simlarly, "calc(bla) 10px" would be accepted).

Aha, I see what you mean. So we stick to the rule for the consume helpers
"consume only when valid, otherwise leave the range as it was".

> I chatted more with alancutter@ about this and we came up with a way to do
this
> that's hopefully not too complicated (it'll only be used in the
> consumeNumber/etc. functions anyway). The idea is to have an object which
holds
> on to the CSSCalcValue, with functions releaseValue() and releaseNumber()
which
> return CSSPrimitiveValues (calc for the first, number/integer for the second)
> and will update the local range when doing so.
> 
> Something like:
> 
> class CSSCalcValue {
> public:
>     CSSCalcValue(CSSParserTokenRange& range, ValueRange valueRange);
>     // store passed in range in m_originalRange
>     // check functionId, quit if definitely not a calc
>     // copy range to m_range, consumeFunction there
>     // parse calc into m_calcValue
> 
>     const CSSCalcValue* value() const;
>     PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseValue(); // update
> m_originalRange, put m_calcValue in a primitive value and return
>     PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseNumber();
> 
> private:
>     CSSParserTokenRange& m_originalRange;
>     CSSParserTokenRange m_range;
>     RefPtrWillBeRawPtr<CSSCalcValue> m_calcValue;
>     ValueRange m_valueRange;
> };

Sounds good. Follow up patch or this one?

[alancutter (OOO until 2018), 2015-09-29 03:07:08]

On 2015/09/29 at 02:47:20, rob.buis wrote:
> Sounds good. Follow up patch or this one?

It should be in this one otherwise we would be failing the cases that timloh
pointed out.
They should also be test cases added to this patch.

[rwlbuis, 2015-09-29 19:07:02]

Patchset #13 (id:260001) has been deleted

[rwlbuis, 2015-09-29 21:43:32]

On 2015/09/29 03:07:08, alancutter wrote:
> On 2015/09/29 at 02:47:20, rob.buis wrote:
> > Sounds good. Follow up patch or this one?
> 
> It should be in this one otherwise we would be failing the cases that timloh
> pointed out.
> They should also be test cases added to this patch.

I added the tests and the CalcParseScope helper. Would like one final look over
before hitting commit, PTAL :)

[Timothy Loh, 2015-09-30 00:36:47]

lgtm with last couple of comments

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

https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:149: return
cssValuePool().createValue(m_calcValue->doubleValue(),
CSSPrimitiveValue::UnitType::Number);
probably should check if the calc is an integer and pick the appropriate type

https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:210: validNumber
= true;
Slightly better to do without validNumber, just break here and return nullptr in
the default case

https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:220: if
(unitless == UnitlessQuirk::Forbid ||
!shouldAcceptUnitLessValues(token.numericValue(), cssParserMode)
I think this logic is wrong. Unitless values are always allowed for html/svg
attributes and 0px/0deg, the UnitlessQuirk enum is about allowing unitless
values in quirks mode. Maybe shouldAcceptUnitlessValues (btw the capitalization
is weird, probably Unitless instead of UnitLess is better) could take the
UnitlessQuirk enum.

[rwlbuis, 2015-09-30 02:04:46]

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

[rwlbuis, 2015-09-30 02:04:46]

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

[commit-bot: I haz the power, 2015-09-30 02:05:04]

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

[alancutter (OOO until 2018), 2015-09-30 02:07:35]

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

https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:129: class
CalcParseScope {
STACK_ALLOCATED();

Minor naming nit. I think we originally planned to have something happen in the
destructor of this class hence where the Scope part of the name suggestion came
from. Since it doesn't actually have a destructor I'd just call it CalcParser
and consider pulling the parsing logic in CSSCalculationValue.cpp into it in a
future patch.

https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:141:
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseValue()
I think consumeValue() and consumeNumber() are more accurate names for what
these functions do.
Also they should both return nullptr if !m_calcValue and not update the
m_sourceRange.

[commit-bot: I haz the power, 2015-09-30 03:37:44]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2015-09-30 03:38:52]

Patchset 14 (id:??) landed as
https://crrev.com/9ef67bebf05c04671cbf1aa782d4adf8d3cfab26
Cr-Commit-Position: refs/heads/master@{#351483}

[rwlbuis, 2015-09-30 13:31:10]

On 2015/09/30 02:07:35, alancutter wrote:
>
https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:129: class
> CalcParseScope {
> STACK_ALLOCATED();
> 
> Minor naming nit. I think we originally planned to have something happen in
the
> destructor of this class hence where the Scope part of the name suggestion
came
> from. Since it doesn't actually have a destructor I'd just call it CalcParser
> and consider pulling the parsing logic in CSSCalculationValue.cpp into it in a
> future patch.
> 
>
https://codereview.chromium.org/1348363004/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:141:
> PassRefPtrWillBeRawPtr<CSSPrimitiveValue> releaseValue()
> I think consumeValue() and consumeNumber() are more accurate names for what
> these functions do.
> Also they should both return nullptr if !m_calcValue and not update the
> m_sourceRange.

Sorry, I only saw this when I woke up this morning :)
Seems like Yutak addressed the STACK_ALLOCATED issue. The other ones I am
addressing in:
https://codereview.chromium.org/1369353002/

I'll make sure to note it in the description.