A thread safe CSS calc parser for sizes

A thread safe CSS calc parser for sizes

The sizes attribute parsing requires calc() parsing and evaluation.
This CL adds a thread safe calc parser (for unsigned length values).

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

[eseidel, 2014-04-28 19:05:53]

So currently our calc parsing is not thread-safe?  As in via the bison?  And
this adds a new non-bison way?

[Yoav Weiss, 2014-04-28 19:59:52]

On 2014/04/28 19:05:53, eseidel wrote:
> So currently our calc parsing is not thread-safe?  As in via the bison?  And
> this adds a new non-bison way?

Current calc parsing relies on CSSValue (which AFAIU is not safe off main
thread) and CSSParserValues produced by the bison parser.
This add a parser based on MediaQueryToken.

[eseidel, 2014-04-28 22:39:52]

Is the plan to switch to your new calc parser on the main thread too?

[eseidel, 2014-04-29 04:50:07]

This looks fine to me.  I would like Alan or Mike to take a look as well if they
can.

My big question is just if we get to delete a bunch more code after landing
this?

https://codereview.chromium.org/252743004/diff/20001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryToken.h (left):

https://codereview.chromium.org/252743004/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryToken.h:30: StringToken = 15,
Was this causing other bugs?  Do we need to mark these invididually with their
values or can we just let the compiler generate them?

[Mike Lawther (Google), 2014-04-29 06:29:16]

I don't understand why this needs to be threadsafe in media query parsing, but I
don't have the context.

This isn't right in terms of handing percents - 50% means the same as '50%' in
CSS, not 0.5. So "50% * 100px" is illegal in calc. 

So right now, this could not be used to replace the existing implementation.

[Yoav Weiss, 2014-04-29 07:09:39]

On 2014/04/29 04:50:07, eseidel wrote:
> This looks fine to me.  I would like Alan or Mike to take a look as well if
they
> can.
> 
> My big question is just if we get to delete a bunch more code after landing
> this?

Not immediately. This can be used for all MQs that use the MQ parser (rather
than Bison), but not for CSS parsed by the Bison parser.
It'd probably make sense to converge the code in the future though.
> 
>
https://codereview.chromium.org/252743004/diff/20001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryToken.h (left):
> 
>
https://codereview.chromium.org/252743004/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryToken.h:30: StringToken = 15,
> Was this causing other bugs?  Do we need to mark these invididually with their
> values or can we just let the compiler generate them?
I'm not aware of bugs this caused. The main reason I explicitly enumerated them
is to make debugging easier.
I can replace the explicit enumeration with line comments.

[Yoav Weiss, 2014-04-29 07:16:13]

On 2014/04/29 06:29:16, Mike Lawther (Google) wrote:
> I don't understand why this needs to be threadsafe in media query parsing, but
I
> don't have the context.

In order to perform media attribute evaluation and img@sizes parsing inside the
HTMLPreloadScanner, we need thread safe MQ parsing and CSS length calculations.
A thread safe calc evaluation implementation will enable to support calc()
inside img@sizes and media attributes.

> 
> This isn't right in terms of handing percents - 50% means the same as '50%' in
> CSS, not 0.5. So "50% * 100px" is illegal in calc.
 
Oh, good catch! Will fix.
Do we have a set of test cases I can use to sanity test my impl?

> 
> So right now, this could not be used to replace the existing implementation.

Yeah. My current intention was not to replace the existing implementation, but
to offer a threadsafe alternative.
I think that later on, we can look into converging the two implementations.

[Yoav Weiss, 2014-04-29 08:43:18]

On 2014/04/29 07:16:13, Yoav Weiss wrote:
> On 2014/04/29 06:29:16, Mike Lawther (Google) wrote:
> > I don't understand why this needs to be threadsafe in media query parsing,
but
> I
> > don't have the context.
> 
> In order to perform media attribute evaluation and img@sizes parsing inside
the
> HTMLPreloadScanner, we need thread safe MQ parsing and CSS length
calculations.
> A thread safe calc evaluation implementation will enable to support calc()
> inside img@sizes and media attributes.

A small correction - I was under the impression that calc() is supported in MQ
evaluation, and I need to maintain that support.
That is not the case. calc() is not currently supported in MQ evaluation neither
in Blink, nor on other engines.

So, this implementation is for the purpose of calc() evaluation in img@sizes
only, at least for the moment.
If I'd become convinced that the use cases for calc() eval inside MQs are worth
while, I'll send an "intent to implement" regarding it.

[Yoav Weiss, 2014-04-29 10:02:29]

I've aligned the implementation according to the spec, and minimized its
coverage.
It now only calculates unsigned length values, since this is what's required for
img@sizes. Percentages, as well as non-length values, are invalid. negative
lengths get clamped to 0.

[eseidel, 2014-04-29 17:30:36]

OK, so this is sizes-only.  Our options are either to teach the existing calc
implementation to be sizes-only or to create a new one?  And we can't use the
existing one on the parser thread because it deasl with CSSValue which is not
necessarily thread safe?

The impl looks fine to me.  Please get a calc expert like mike lawther or alan
cutter to sign off on this before landing.

lgtm.

https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryToken.h (right):

https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryToken.h:15: FunctionToken, // 1
I would just remove these comments.  They're going to get stale.  A smart
debugger can map these for you.

https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/S...
File Source/core/css/parser/SizesCalcParser.h (right):

https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/S...
Source/core/css/parser/SizesCalcParser.h:37: static bool
parse(MediaQueryTokenIterator start,
We don't wrap to 80c in blink, I wouldn't bother doing these one per line.

[Yoav Weiss, 2014-04-29 19:32:01]

On 2014/04/29 17:30:36, eseidel wrote:
> OK, so this is sizes-only.  Our options are either to teach the existing calc
> implementation to be sizes-only or to create a new one?  And we can't use the
> existing one on the parser thread because it deasl with CSSValue which is not
> necessarily thread safe?

Yeah.
> 
> The impl looks fine to me.  Please get a calc expert like mike lawther or alan
> cutter to sign off on this before landing.
Awesome! I'll get one of them to take a look.
> 
> lgtm.
> 
>
https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryToken.h (right):
> 
>
https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryToken.h:15: FunctionToken, // 1
> I would just remove these comments.  They're going to get stale.  A smart
> debugger can map these for you.
Removed them.

> 
>
https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/S...
> File Source/core/css/parser/SizesCalcParser.h (right):
> 
>
https://codereview.chromium.org/252743004/diff/80001/Source/core/css/parser/S...
> Source/core/css/parser/SizesCalcParser.h:37: static bool
> parse(MediaQueryTokenIterator start,
> We don't wrap to 80c in blink, I wouldn't bother doing these one per line.
unwrapped.

[alancutter (OOO until 2018), 2014-04-30 04:02:17]

Would you be able to add a test to check that we get the same results from this
parser and the CSS calc parser? You can flatten the CSS Calc tree with
accumulateLengthArray() to get a comparable value.

https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryToken.h (right):

https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryToken.h:69: BlockType blockType() const {
return m_blockType; }
It concerns me that none of these getters have ASSERTS on m_type.

https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
File Source/core/css/parser/SizesCalcParserTest.cpp (right):

https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParserTest.cpp:19: } TestCase;
struct TestCase {}; notation would be preferred.

https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParserTest.cpp:25: // Otherwise, the output
string is identical to the first string.
This comment doesn't seem to match the struct. The second value appears to be
the resolved value of the calc instead of a string. The "Otherwise" case doesn't
really make sense.

https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParserTest.cpp:54: {0, 0, true} // Do not remove
the terminator line.
It would be good to see more invalid inputs tested.
See the "invalid formulas" section in css3/calc/calc-errors.html

And perhaps these cases as well:
calc(((4) * ((10px))))
calc(50px / 0)
calc(50px / (10 + 10))
calc(50px / (10 - 10))
calc(50px / (10 * 10))
calc(50px / (10 / 10))

[Yoav Weiss, 2014-04-30 13:55:46]

On 2014/04/30 04:02:17, alancutter wrote:
> Would you be able to add a test to check that we get the same results from
this
> parser and the CSS calc parser? You can flatten the CSS Calc tree with
> accumulateLengthArray() to get a comparable value.

I've added such tests.
As a side note, some of my tests are failing on the CSSCalculationValue (a test
that involved an internal "calc(" function) and some are ASSERTing it (tests
that involve addition of "px" and "s" units).
I've added comments and turned off these tests for CSSCalculationValue.
> 
>
https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryToken.h (right):
> 
>
https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryToken.h:69: BlockType blockType() const {
> return m_blockType; }
> It concerns me that none of these getters have ASSERTS on m_type.
Added asserts.

> 
>
https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
> File Source/core/css/parser/SizesCalcParserTest.cpp (right):
> 
>
https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParserTest.cpp:19: } TestCase;
> struct TestCase {}; notation would be preferred.
Changed;
> 
>
https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParserTest.cpp:25: // Otherwise, the output
> string is identical to the first string.
> This comment doesn't seem to match the struct. The second value appears to be
> the resolved value of the calc instead of a string. The "Otherwise" case
doesn't
> really make sense.

Leftover comment. Removed.
> 
>
https://codereview.chromium.org/252743004/diff/100001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParserTest.cpp:54: {0, 0, true} // Do not
remove
> the terminator line.
> It would be good to see more invalid inputs tested.
> See the "invalid formulas" section in css3/calc/calc-errors.html
> 
> And perhaps these cases as well:
> calc(((4) * ((10px))))
> calc(50px / 0)
> calc(50px / (10 + 10))
> calc(50px / (10 - 10))
> calc(50px / (10 * 10))
> calc(50px / (10 / 10))

Added, found a bug and fixed it! :)

[Yoav Weiss, 2014-04-30 14:48:51]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-30 14:49:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/252743004/160001

[commit-bot: I haz the power, 2014-04-30 15:27:59]

Change committed as 172992