New parser mode for CSSOM @viewport descriptors.

New parser mode for CSSOM @viewport descriptors.

CSSOM value and descriptor parsing for @viewport used parsing rules for normal
CSS property declarations instead, meaning width and height were not parsed as
shorthands.

This is an issue for other CSSOM access to other descriptors too, like for
@font-face.

This fix is minimal for the @viewport case and intended for feedback on the
general approach. I considered not adding new parser modes, but use a bit in
StylePropertySet to indicate a CSSParserMode or a descriptor mode and let two
bits store the mode. With both the current changes and the split, a descriptor
mode would implicitly be strict.

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

[eseidel, 2013-09-10 16:33:57]

https://codereview.chromium.org/22917005/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/22917005/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:1200: markViewportRuleBodyStart();
Bleh.  Do we need a helper function around cssyyparse?  Should this be a RAII?

These sort of paired state changes are always ugly and error-prone. This will
not scale well with a second such change.

https://codereview.chromium.org/22917005/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/22917005/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.h:141: unsigned m_arraySize : 28;
Do we have a constant somewhere to help make sure we never overflow this?

[rune, 2013-09-12 13:05:01]

https://codereview.chromium.org/22917005/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/22917005/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:1200: markViewportRuleBodyStart();
On 2013/09/10 16:33:58, eseidel wrote:
> Bleh.  Do we need a helper function around cssyyparse?  Should this be a RAII?
> 
> These sort of paired state changes are always ugly and error-prone. This will
> not scale well with a second such change.

Did an RAII.

https://codereview.chromium.org/22917005/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/22917005/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.h:141: unsigned m_arraySize : 28;
On 2013/09/10 16:33:58, eseidel wrote:
> Do we have a constant somewhere to help make sure we never overflow this?

Done.

[rune, 2013-09-17 07:29:46]

ping

[eseidel, 2013-09-17 16:28:54]

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (left):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:1221: cssyyparse(this);
I might have wrapped this in a block, depending on how long you want the scope
to live for after the parse.

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:546: if (cssParserMode == ViewportMode ||
!isSimpleLengthPropertyID(propertyId, acceptsNegativeNumbers))
So this whole change is to disallow negative numbers?  Which is more strict than
normal strict mode?

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.h:139: ,
m_arraySize(std::min(immutableArraySize, unsigned(MaxArraySize)))
Do we have any asserts on array modification which need to be changed?

[rune, 2013-09-17 20:17:02]

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (left):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:1221: cssyyparse(this);
On 2013/09/17 16:28:55, eseidel wrote:
> I might have wrapped this in a block, depending on how long you want the scope
> to live for after the parse.

Will do.

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:546: if (cssParserMode == ViewportMode ||
!isSimpleLengthPropertyID(propertyId, acceptsNegativeNumbers))
On 2013/09/17 16:28:55, eseidel wrote:
> So this whole change is to disallow negative numbers?  Which is more strict
than
> normal strict mode?

No. This is to avoid the simple-length-fast-path for "width", which is a
shorthand descriptor in @viewport and cannot be parsed as a simple length.

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.h:139: ,
m_arraySize(std::min(immutableArraySize, unsigned(MaxArraySize)))
On 2013/09/17 16:28:55, eseidel wrote:
> Do we have any asserts on array modification which need to be changed?

So, m_arraySize won't change after the initialization. For mutable property
sets, the size of m_propertyVector is used as the property count instead.

But, there's a use of the non-clamped length in the ImmutablePropertySet
constructor that needs to be fixed.

[rune, 2013-09-17 20:40:41]

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (left):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:1221: cssyyparse(this);
On 2013/09/17 20:17:03, rune wrote:
> On 2013/09/17 16:28:55, eseidel wrote:
> > I might have wrapped this in a block, depending on how long you want the
scope
> > to live for after the parse.
> 
> Will do.

Done.

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/22917005/diff/12001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.h:139: ,
m_arraySize(std::min(immutableArraySize, unsigned(MaxArraySize)))
On 2013/09/17 20:17:03, rune wrote:
> On 2013/09/17 16:28:55, eseidel wrote:
> > Do we have any asserts on array modification which need to be changed?
> 
> So, m_arraySize won't change after the initialization. For mutable property
> sets, the size of m_propertyVector is used as the property count instead.
> 
> But, there's a use of the non-clamped length in the ImmutablePropertySet
> constructor that needs to be fixed.

Done.

[rune, 2013-09-17 20:47:46]

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:83: for (unsigned i = 0; i < m_arraySize;
++i) {
Here, I've fixed unmatched ref/unref since m_arraySize was used in the
destructor.

What's left is that ImmutableStylePropertySet::create creates a larger array
than what's used if the length is >= 2^28 since the allocation is done with
"length" before the constructor is called. Should I fix that as well?

[eseidel, 2013-09-17 22:26:00]

lgtm

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/CSSParser...
File Source/core/css/CSSParserMode.h (right):

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/CSSParser...
Source/core/css/CSSParserMode.h:49: ViewportMode
If you're willing to go one more round here, it would be nice to have a comment
explaining what is different aboug this mode than others.  If it's just the
"width" thing, that seems easy to put in a comment.  If its complicated, we
could link to the spec.

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:83: for (unsigned i = 0; i < m_arraySize;
++i) {
On 2013/09/17 20:47:46, rune wrote:
> Here, I've fixed unmatched ref/unref since m_arraySize was used in the
> destructor.
> 
> What's left is that ImmutableStylePropertySet::create creates a larger array
> than what's used if the length is >= 2^28 since the allocation is done with
> "length" before the constructor is called. Should I fix that as well?

Probably should fix all the places while we're at it.  Not necessarily in a
single CL, just before we forget. :)

[rune, 2013-09-18 08:13:09]

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/CSSParser...
File Source/core/css/CSSParserMode.h (right):

https://codereview.chromium.org/22917005/diff/31001/Source/core/css/CSSParser...
Source/core/css/CSSParserMode.h:49: ViewportMode
On 2013/09/17 22:26:00, eseidel wrote:
> If you're willing to go one more round here, it would be nice to have a
comment
> explaining what is different aboug this mode than others.  If it's just the
> "width" thing, that seems easy to put in a comment.  If its complicated, we
> could link to the spec.

The main thing about this mode is to use viewport descriptor parsing rules
instead of style declaration parsing rules. While parsing a stylesheet, this is
triggered by setting inViewport() when the rule header (@viewport) is
encountered. This context is (or was) lost in the StylePropertySet, so modifying
the StylePropertySet through CSSOM would use the parsing rules for style
declarations instead of viewport descriptors.

I've extended/added comments.

[commit-bot: I haz the power, 2013-09-18 08:13:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/22917005/37001

[commit-bot: I haz the power, 2013-09-18 11:25:23]

Change committed as 157955