Make CSSCalcValue work with CSSParserTokenRange

Make CSSCalcValue work with CSSParserTokenRange

Make CSSCalcValue work with CSSParserTokenRange by using consumeBlock
on the calc() tokens and storing the resulting CSSParserTokenRange in
the args parameter, and then passing these args to CSSCalcValue::create. 

The CSSCalcExpressionNodeParser is adjusted to operate on CSSParserTokens.

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

[rwlbuis, 2015-07-17 20:23:11]

Patchset #3 (id:40001) has been deleted

[rwlbuis, 2015-07-20 02:29:13]

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

[rwlbuis, 2015-07-20 02:29:13]

@timloh, PTAL. I don't think this is ready for a real review yet but I hope this
is a sane direction. I think it shows:

- CSSValueList can work fine with iterator instead of array like indexing.
- auto closing can proof to be tricky. In fact I am not sure if I got it right.
In order to avoid creating the extra Vector for the calc args, I think the only
way would be to build the auto closing into the tokenizer. Not sure if we want
that? OTOH the current patch should not give worse performance than creating
CSSParserValues all the time, it would just be nice to be as efficient as
possible IMHO.

[Timothy Loh, 2015-07-21 07:33:17]

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

[Timothy Loh, 2015-07-21 07:33:18]

Probably it's easier if we just use consumeBlock and whatever and have calc
parsing operate on ranges recursively (instead of being streaming). There's
already a calc parser (SizesCalcParser?), so we should think about avoiding
duplication.

alancutter might have some comments on calc stuff.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:624: char
operatorValue(CSSParserTokenRange& tokens)
I don't think this function helps, or at least it isn't obvious that it'll
consume whitespace.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:713: return false; // calc(1px+ 2px) is
invalid
Probably need to check the other side too, calc(1px +/**/2px) is also invalid...

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserValues.cpp:65: unsigned calcDepth = 0;
don't need this anymore

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserValues.cpp:265: static void
destroy(Vector<CSSParserValue, 4>& values)
need to update this

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserValues.h (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserValues.h:122: Vector<CSSParserToken> args;
Just use a CSSParserTokenRange

[alancutter (OOO until 2018), 2015-07-21 07:44:26]

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:642: CSSPrimitiveValue::UnitType type =
token.unitType();
This line should stay after the token.type() check.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:752:
PassRefPtrWillBeRawPtr<CSSCalcValue> CSSCalcValue::create(const
CSSParserTokenRange& parserValueList, ValueRange range)
s/parserValueList/tokens/

[rwlbuis, 2015-07-21 21:29:52]

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:642: CSSPrimitiveValue::UnitType type =
token.unitType();
On 2015/07/21 07:44:26, alancutter wrote:
> This line should stay after the token.type() check.

Done.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:752:
PassRefPtrWillBeRawPtr<CSSCalcValue> CSSCalcValue::create(const
CSSParserTokenRange& parserValueList, ValueRange range)
On 2015/07/21 07:44:26, alancutter wrote:
> s/parserValueList/tokens/

Done.

[rwlbuis, 2015-07-22 02:19:06]

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:624: char
operatorValue(CSSParserTokenRange& tokens)
On 2015/07/21 07:33:17, Timothy Loh wrote:
> I don't think this function helps, or at least it isn't obvious that it'll
> consume whitespace.

Perhaps. I did not want to change the original code too much though to play it
safe.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:713: return false; // calc(1px+ 2px) is
invalid
On 2015/07/21 07:33:17, Timothy Loh wrote:
> Probably need to check the other side too, calc(1px +/**/2px) is also
invalid...

I think it works, I found a similar test in calc-errors.html(?).

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserValues.cpp:65: unsigned calcDepth = 0;
On 2015/07/21 07:33:17, Timothy Loh wrote:
> don't need this anymore

Done.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserValues.cpp:265: static void
destroy(Vector<CSSParserValue, 4>& values)
On 2015/07/21 07:33:17, Timothy Loh wrote:
> need to update this

Ouch I thought it was not needed after I introduced CSSParserTokenRange for args
somehow, but it is still needed. Will fix tomorrow.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserValues.h (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserValues.h:122: Vector<CSSParserToken> args;
On 2015/07/21 07:33:17, Timothy Loh wrote:
> Just use a CSSParserTokenRange

Done.
Note that I did it this way for the auto closing.

[rwlbuis, 2015-07-22 02:25:23]

On 2015/07/21 07:33:18, Timothy Loh wrote:
> Probably it's easier if we just use consumeBlock and whatever and have calc

I did base it of consumeBlock in my latest patch rather than custom and it does
look nicer. Downside is REMS again...

> parsing operate on ranges recursively (instead of being streaming). There's
> already a calc parser (SizesCalcParser?), so we should think about avoiding
> duplication.

AFAICS SizesCalcParser and CSSCalculationValue are a bit different, ie. the
former calculates the result in the constructor, whereas the latter makes a
syntax tree and calculates the result upon each doubleValue call. Not sure if
this
makes a difference in practice?

> alancutter might have some comments on calc stuff.

Feedback always welcome :)

[rwlbuis, 2015-07-22 20:53:02]

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserValues.cpp:265: static void
destroy(Vector<CSSParserValue, 4>& values)
On 2015/07/22 02:19:06, rwlbuis wrote:
> On 2015/07/21 07:33:17, Timothy Loh wrote:
> > need to update this
> 
> Ouch I thought it was not needed after I introduced CSSParserTokenRange for
args
> somehow, but it is still needed. Will fix tomorrow.

Ok, fixed now :)
PTAL.

[Timothy Loh, 2015-07-28 08:17:41]

I wrote the below comments last week, I think I didn't finish reviewing but I'll
just send these out first.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:624: char
operatorValue(CSSParserTokenRange& tokens)
On 2015/07/22 02:19:06, rwlbuis wrote:
> On 2015/07/21 07:33:17, Timothy Loh wrote:
> > I don't think this function helps, or at least it isn't obvious that it'll
> > consume whitespace.
> 
> Perhaps. I did not want to change the original code too much though to play it
> safe.

I think this function makes it hard to verify that whitespace is handled
correctly, I'd still prefer we removed this.

https://codereview.chromium.org/1239983004/diff/60001/Source/core/css/CSSCalc...
Source/core/css/CSSCalculationValue.cpp:713: return false; // calc(1px+ 2px) is
invalid
On 2015/07/22 02:19:06, rwlbuis wrote:
> On 2015/07/21 07:33:17, Timothy Loh wrote:
> > Probably need to check the other side too, calc(1px +/**/2px) is also
> invalid...
> 
> I think it works, I found a similar test in calc-errors.html(?).

I didn't see any such tests. You need to check the previous and next tokens for
whitespace for both add and subtract, and add tests for the various permutations
of whitespace/comments, e.g. calc(1px/**/- 2px), calc(1px -/**/2px) and so on.

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:610: Value result;
tokens.consumeWhitespace();

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:660: if (operatorValue(tokens) == '(') {
tokens.peek().type() == LeftParenthesisToken

Using consumeBlock, this would look like:

if (tokens.peek().type() == LeftParenthesisToken) {
    CSSParserTokenRange innerRange = tokens.consumeBlock();
    tokens.consumeWhitespace();
    return parseValueExpression(innerRange, ..);
}

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:661: tokens.consume();
consumeIncludingWhitespace

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:668: if (operatorValue(tokens) != ')')
tokens.peek().type() == RightParenthesisToken

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:670: tokens.consume();
consumeIncludingWhitespace

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:689: tokens.consume();
consumeIncludingWhitespace

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:717: tokens.consume();
consumeIncludingWhitespace

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/parser...
File Source/core/css/parser/CSSParserTokenRange.h (right):

https://codereview.chromium.org/1239983004/diff/120001/Source/core/css/parser...
Source/core/css/parser/CSSParserTokenRange.h:54: const CSSParserToken&
consumeSkippingWhitespace()
The CSS parsing code uses a strategy of consuming trailing whitespace, we should
probably be consistent and do the same for calc instead of adding this opposite
strategy.

[rwlbuis, 2015-07-28 22:48:26]

On 2015/07/28 08:17:41, Timothy Loh wrote:
> I wrote the below comments last week, I think I didn't finish reviewing but
I'll
> just send these out first.

Thanks, I think patch set #7 should address all issues, except that I forgot to
look for more tests for the whitespace/comments permutations. I'll try to add
more tomorrow but apart from that the patch should be in good shape to review.

[Timothy Loh, 2015-07-29 03:52:04]

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/CSSCal...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:702:
tokens.consumeIncludingWhitespace();
tokens.peek() will return a static eof token if we've consumed everything, so
you probably want to instead do:

tokens.consume()
if (tokens.peek()->type() != WhitespaceToken)
    return false;
tokens.consumeWhitespace();

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/parser...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/parser...
Source/core/css/parser/CSSParserValues.cpp:73: // FIXME: calc expression could
contain REM units
We.. probably shouldn't intentionally add regressions like this. Are we going to
fix this straight away? :)

[rwlbuis, 2015-07-29 15:04:13]

Patchset #8 (id:160001) has been deleted

[rwlbuis, 2015-07-29 15:45:33]

Patchset #8 (id:180001) has been deleted

[rwlbuis, 2015-07-29 20:06:25]

Patchset #9 (id:220001) has been deleted

[rwlbuis, 2015-07-29 21:33:26]

PTAL, I also added more subtests for the comment+addition+substraction
permutations.

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/CSSCal...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/CSSCal...
Source/core/css/CSSCalculationValue.cpp:702:
tokens.consumeIncludingWhitespace();
On 2015/07/29 03:52:03, Timothy Loh wrote:
> tokens.peek() will return a static eof token if we've consumed everything, so
> you probably want to instead do:
> 
> tokens.consume()
> if (tokens.peek()->type() != WhitespaceToken)
>     return false;
> tokens.consumeWhitespace();

Done.

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/parser...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/1239983004/diff/140001/Source/core/css/parser...
Source/core/css/parser/CSSParserValues.cpp:73: // FIXME: calc expression could
contain REM units
On 2015/07/29 03:52:04, Timothy Loh wrote:
> We.. probably shouldn't intentionally add regressions like this. Are we going
to
> fix this straight away? :)

Good point :) I am picking up the rem patch again.

[Timothy Loh, 2015-07-30 01:10:32]

lgtm, but land the rem patch first :)

[rwlbuis, 2015-07-30 14:38:47]

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

[commit-bot: I haz the power, 2015-07-30 14:38:58]

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

[commit-bot: I haz the power, 2015-07-30 15:41:03]

Committed patchset #9 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199741

[dmurph, 2015-07-30 16:52:57]

On 2015/07/30 at 15:41:03, commit-bot wrote:
> Committed patchset #9 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199741

I will probably have to revert this as well
https://code.google.com/p/chromium/issues/detail?id=515535

[rwlbuis, 2015-07-30 17:35:29]

On 2015/07/30 16:52:57, dmurph wrote:
> On 2015/07/30 at 15:41:03, commit-bot wrote:
> > Committed patchset #9 (id:240001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=199741
> 
> I will probably have to revert this as well
> https://code.google.com/p/chromium/issues/detail?id=515535

Yes please, these two go together.

[rwlbuis, 2015-07-30 20:13:46]

On 2015/07/30 17:35:29, rwlbuis wrote:
> On 2015/07/30 16:52:57, dmurph wrote:
> > On 2015/07/30 at 15:41:03, commit-bot wrote:
> > > Committed patchset #9 (id:240001) as
> > https://src.chromium.org/viewvc/blink?view=rev&revision=199741
> > 
> > I will probably have to revert this as well
> > https://code.google.com/p/chromium/issues/detail?id=515535
> 
> Yes please, these two go together.

OTOH, I think I am close to a fix :) Let's watch
https://codereview.chromium.org/1181023004/ ....