Re-use CSSParser logic to parse keyframe keys

Re-use CSSParser logic to parse keyframe keys

This allows the CSSParser logic to be shared between ...
- Creating the keyframe from CSS
- Setting the keyText as a string from JavaScript

Also update StyleKeyframe to cache the string and vector representations of the keys.

This is purely an optimisation. No change in behavior.

R=apavlov

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

[apavlov, 2013-07-11 07:05:11]

https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
File Source/core/css/CSSKeyframeRule.h (right):

https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
Source/core/css/CSSKeyframeRule.h:49: void setKeyText(const String& s) {
m_keyText = s; }
Shouldn't you reset m_cachedKeys at this point?

[Steve Block, 2013-07-11 07:33:25]

https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
File Source/core/css/CSSKeyframeRule.h (right):

https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
Source/core/css/CSSKeyframeRule.h:49: void setKeyText(const String& s) {
m_keyText = s; }
There's no need to, as this method is only ever called once, immediately after
construction of the object.

It's probably a good idea to make it more robust though.

[apavlov, 2013-07-11 07:53:03]

On 2013/07/11 07:33:25, Steve Block wrote:
>
https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
> File Source/core/css/CSSKeyframeRule.h (right):
> 
>
https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
> Source/core/css/CSSKeyframeRule.h:49: void setKeyText(const String& s) {
> m_keyText = s; }
> There's no need to, as this method is only ever called once, immediately after
> construction of the object.

Hmm, according to http://www.w3.org/TR/css3-animations/#csskeyframerule, the
|keyText| attribute is not marked "readonly".

> It's probably a good idea to make it more robust though.

The reason I took a look at this patch is, I've been reviewing
https://codereview.chromium.org/13943004/ having a goal similar to yours, but
the iterations have been very slow, and the change is not finished yet. Also,
the patch author told me over IRC:

shalamov: I'm fixing patch related to keyframes. I fixed bug (rebase mistake).
However I found one case that is not handled, im writing layout test for that
shalamov: basically, if someone is trying to set keyframe value using cssom and
value is invalid, should we keep old keyframe values?
shalamov: ...i was reading cssom spec back and forth and I think i know how it
should behave
shalamov: its not specified, but imo :) it should behave like
CSSRuleInterface::cssText attribute
http://dev.w3.org/csswg/cssom/#the-cssrule-interface
shalamov: "If new rule is a syntax error, terminate this algorithm."
shalamov: but, i tested in mozilla and they replace values, even with invalid
keyframes

So, our consensus was that it should also be tested on IE[10], and if it appears
to be the same as Mozilla, Blink should just follow this...

[apavlov, 2013-07-11 08:31:43]

On 2013/07/11 07:33:25, Steve Block wrote:
>
https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
> File Source/core/css/CSSKeyframeRule.h (right):
> 
>
https://codereview.chromium.org/19037003/diff/1/Source/core/css/CSSKeyframeRu...
> Source/core/css/CSSKeyframeRule.h:49: void setKeyText(const String& s) {
> m_keyText = s; }
> There's no need to, as this method is only ever called once, immediately after
> construction of the object.

Found it. CSSKeyframeRule::setKeyText() is called from V8WebKitCSSKeyframeRule,
static void keyTextAttrSetter().

[Steve Block, 2013-07-11 12:24:23]

> Found it. CSSKeyframeRule::setKeyText() is called from
V8WebKitCSSKeyframeRule,
> static void keyTextAttrSetter().
You're right, I spotted this too just after I sent the last update. Sorry for
the trouble.

I'll take another look to see if there's a better way to share between
CSSKeyframeRule and CSSParser the logic that parses floats from the key string.

As for changing the behavior to match other browsers, I'll look into that, but
even if we do make such a change, I think it's best done in a separate patch.

[apavlov, 2013-07-11 12:32:21]

On 2013/07/11 12:24:23, Steve Block wrote:
> > Found it. CSSKeyframeRule::setKeyText() is called from
> V8WebKitCSSKeyframeRule,
> > static void keyTextAttrSetter().
> You're right, I spotted this too just after I sent the last update. Sorry for
> the trouble.
> 
> I'll take another look to see if there's a better way to share between
> CSSKeyframeRule and CSSParser the logic that parses floats from the key
string.

Perhaps, use the approach of CSSParser::parseMediaQueryList(const String&
string)?
Create some symbol like "@-internal-keyframe-key" and use it in
CSSParser/CSSGrammar.y.in for parsing the floats?

> As for changing the behavior to match other browsers, I'll look into that, but
> even if we do make such a change, I think it's best done in a separate patch.

Indeed.

[Steve Block, 2013-07-12 03:59:49]

Updated to share parsing logic from CSSParser

Note that this depends on https://codereview.chromium.org/18864008

[Steve Block, 2013-07-12 07:56:49]

I'm on vacation for the next few weeks, so feel free to land/modify/take-over
this patch or https://codereview.chromium.org/18864008 while I'm away.
Otherwise, I'll pick these up when I get back.

[apavlov, 2013-07-15 11:54:37]

https://codereview.chromium.org/19037003/diff/22001/Source/core/css/CSSKeyfra...
File Source/core/css/CSSKeyframeRule.cpp (right):

https://codereview.chromium.org/19037003/diff/22001/Source/core/css/CSSKeyfra...
Source/core/css/CSSKeyframeRule.cpp:72: if (!m_keys) {
The braces are optional here

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

https://codereview.chromium.org/19037003/diff/22001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:105: PassOwnPtr<Vector<float> >
createKeyframeKeyList(WebCore::CSSParserValueList* keys)
Looks like this should actually belong to StyleKeyframe...

[Steve Block, 2013-08-14 00:44:04]

https://codereview.chromium.org/19037003/diff/22001/Source/core/css/CSSKeyfra...
File Source/core/css/CSSKeyframeRule.cpp (right):

https://codereview.chromium.org/19037003/diff/22001/Source/core/css/CSSKeyfra...
Source/core/css/CSSKeyframeRule.cpp:72: if (!m_keys) {
No longer relevant

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

https://codereview.chromium.org/19037003/diff/22001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:105: PassOwnPtr<Vector<float> >
createKeyframeKeyList(WebCore::CSSParserValueList* keys)
Done

[apavlov, 2013-09-07 09:01:11]

Back to work!

LGTM

[commit-bot: I haz the power, 2013-09-09 00:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/19037003/68001

[commit-bot: I haz the power, 2013-09-09 01:42:28]

Change committed as 157426