Add CSS parser recovery from errors while parsing @-webkit-keyframes key values.

Add CSS parser recovery from errors while parsing @-webkit-keyframes key values.

If not a percentage, "from", or "to" value is used in a key list,
the rule is erroneous, and due to the absense of recovery, the parser skips
the following, valid CSS rule.

On a related note, keyframes, whose selectors contain invalid keys, should be discarded altogether, according to http://www.w3.org/TR/css3-animations/#keyframes

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

[apavlov, 2013-04-16 15:29:10]

A devtools user was recently unable to edit his styles due to the CSS parser not
recovering when parsing invalid keyframe selectors (as such, the subsequent
correct style rule was dropped altogether).

This patch introduces the respective error recovery and also brings the
keyframes construction in accordance with the spec saying that "If a keyframe
selector specifies negative percentage values or values higher than 100%, then
the keyframe will be ignored." It is assumed that the same should happen if a
keyframe selector specifies values of other types (plain numbers, unknown
identifiers, etc.)

[abarth-chromium, 2013-04-16 17:01:55]

I'm probably not the right person to review this CL.

[eseidel, 2013-04-17 00:35:50]

Tab is probably the person with the most familiarity with CSS parsers. :)  We
don't really have an expert in Blink yet.

[hayato, 2013-04-17 04:01:18]

Unofficial lgtm since I am not an OWNER.

https://codereview.chromium.org/13871007/diff/2001/Source/WebCore/css/CSSGram...
File Source/WebCore/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/13871007/diff/2001/Source/WebCore/css/CSSGram...
Source/WebCore/css/CSSGrammar.y.in:859: $$.isInt = true;
'isInt' looks confusing name since it is not a number.
'isInvalid' or something might be better.

[hayato, 2013-04-17 08:22:56]

https://codereview.chromium.org/13871007/diff/8001/Source/core/css/CSSParserV...
File Source/core/css/CSSParserValues.h (right):

https://codereview.chromium.org/13871007/diff/8001/Source/core/css/CSSParserV...
Source/core/css/CSSParserValues.h:119: Invalid  = 0x100000,
I am wondering why we need to set an explicit value to each enum value here. Is
there any code which checks CSSParserValue::unit using bit-wise operation?

Looks like we can remove explicit enum values here.

e.g.
enum {
  Invalid, Operator, Function, Q_EMS
};

[dglazkov, 2013-04-17 18:11:41]

LGTM after Mike gives his blessing.

[TabAtkins, 2013-04-17 19:47:20]

This is a genuine (and terrible) bug.  I'm not 100% sure that the patch does the
right thing, but that's not a bad thing - I'm just not familiar enough with the
code to say yay/nay.  The tests look good to me.

(On a side note, I think keyframes-invalid-keys is testing a pattern that is
declared invalid by the spec, but shouldn't be.  I believe the spec specifies
that "from" or "to" must appear by themselves, but that's silly.  They're just
aliases for 0%/100%, so they should be usable in a comma-separated list with
other keyframe selectors.)

[apavlov, 2013-04-17 20:38:21]

On 2013/04/17 19:47:20, tabatkins wrote:
> This is a genuine (and terrible) bug.  I'm not 100% sure that the patch does
the
> right thing, but that's not a bad thing - I'm just not familiar enough with
the
> code to say yay/nay.  The tests look good to me.
> 
> (On a side note, I think keyframes-invalid-keys is testing a pattern that is
> declared invalid by the spec, but shouldn't be.  I believe the spec specifies
> that "from" or "to" must appear by themselves, but that's silly.  They're just

Hmm... I couldn't find a related statement in the spec. It says, "The keyframe
selector for a keyframe style rule consists of a comma-separated list of
percentage values or the keywords ‘from’ or ‘to’". That seems to mean that the
keywords are on par with percentage values - no more, no less. And I have seen a
bunch of real-world examples of such usage (where the animated property values
for 0% and 100% are the same).

> aliases for 0%/100%, so they should be usable in a comma-separated list with
> other keyframe selectors.)

I would expect the spec to imply the same...

[Mike Lawther (Google), 2013-04-17 22:26:15]

Thanks for fixing this!

[Mike Lawther (Google), 2013-04-17 22:26:37]

https://codereview.chromium.org/13871007/diff/15001/Source/core/css/CSSParser...
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/13871007/diff/15001/Source/core/css/CSSParser...
Source/core/css/CSSParser.cpp:11298: }
Nice catch! I can't see where these cases are tested in your testcases though?

[apavlov, 2013-04-18 04:33:49]

https://codereview.chromium.org/13871007/diff/15001/Source/core/css/CSSParser...
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/13871007/diff/15001/Source/core/css/CSSParser...
Source/core/css/CSSParser.cpp:11298: }
On 2013/04/17 22:26:37, mikelawther wrote:
> Nice catch! I can't see where these cases are tested in your testcases though?

Argh, indeed...I added this check after I had written the test. Will add
out-of-range percentage values.

[darktears, 2013-04-18 14:29:19]

On 2013/04/18 04:33:49, apavlov wrote:
>
https://codereview.chromium.org/13871007/diff/15001/Source/core/css/CSSParser...
> File Source/core/css/CSSParser.cpp (right):
> 
>
https://codereview.chromium.org/13871007/diff/15001/Source/core/css/CSSParser...
> Source/core/css/CSSParser.cpp:11298: }
> On 2013/04/17 22:26:37, mikelawther wrote:
> > Nice catch! I can't see where these cases are tested in your testcases
though?
> 
> Argh, indeed...I added this check after I had written the test. Will add
> out-of-range percentage values.

lgtm

[Mike Lawther (Google), 2013-04-18 23:44:15]

lgtm

Thanks for the updated tests.

[commit-bot: I haz the power, 2013-04-19 08:11:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/13871007/24001

[commit-bot: I haz the power, 2013-04-19 08:17:21]

Presubmit check for 13871007-24001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 6 file(s).

[commit-bot: I haz the power, 2013-04-19 08:32:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/13871007/24001

[commit-bot: I haz the power, 2013-04-19 08:38:30]

Presubmit check for 13871007-24001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 6 file(s).

[commit-bot: I haz the power, 2013-04-19 08:44:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/13871007/24001

[commit-bot: I haz the power, 2013-04-19 08:50:14]

Presubmit check for 13871007-24001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 6 file(s).

[apavlov, 2013-04-19 09:52:20]

Committed patchset #6 manually as r148714 (presubmit successful).