Parse animation and transition shorthands in CSSPropertyParser with CSSParserTokens

Parse animation and transition shorthands in CSSPropertyParser with CSSParserTokens

This patch finishes moving animation and transition properties from the
LegacyCSSPropertyParser to the CSSPropertyParser. Instead of having
separate almost-identical functions for the two shorthands, I've unified
these into a single function.

Similar to parseSingleValue, I've updated parseShorthand to take an
unresolved property instead of a resolved property. This is necessary as
-webkit-animation will accept <string>s for the animation-name component.

This patch also fixes an edge case in parsing the shorthands where we
currently allow shorthand values with leading or trailing commas, for
example "transition: left," and "animation: , 0s".

BUG=499780
Committed: https://crrev.com/eb8b6acea091169e7063d11076ac1ad0c482209f
Cr-Commit-Position: refs/heads/master@{#356816}

[Timothy Loh, 2015-10-19 05:57:38]

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

[rwlbuis, 2015-10-20 14:32:24]

Nice work, almost there I think, still some questions.

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/animations-parsing.html (right):

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/animations-parsing.html:1110:
shouldBe("computedStyle.webkitAnimation", "'none 0s ease 0s 1 normal none
running'");
Do these extra test passes mean the behavior has improved? If so you may want to
mention it in the commit message.

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1217: bool
foundProperty = false;
I really doubt we need two bools. Can't foundAnyProperty be deduced from
longhands?

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1236: //
TODO(timloh): This will make invalid longhands, see crbug.com/386459
Why not do this in the end like in the legacy code?

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1245: if
(!isValidAnimationPropertyList(shorthand.properties()[i], *longhands[i]))
Is this done in the legacy code too? If not, is it an improvement and reflected
in the tests?

[Timothy Loh, 2015-10-21 06:07:39]

Rob: The sydney blink team is away on an offsite for the rest of the week, so we
probably won't get to any more reviews until next week.

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/animations-parsing.html (right):

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/animations-parsing.html:1110:
shouldBe("computedStyle.webkitAnimation", "'none 0s ease 0s 1 normal none
running'");
On 2015/10/20 14:32:23, rwlbuis wrote:
> Do these extra test passes mean the behavior has improved? If so you may want
to
> mention it in the commit message.

It's already mentioned there. The last added test here is the only one that
currently works.

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1217: bool
foundProperty = false;
On 2015/10/20 14:32:23, rwlbuis wrote:
> I really doubt we need two bools. Can't foundAnyProperty be deduced from
> longhands? 

Best I can come up with is as follows:

do { // previously while (true) {
    bool foundProperty = false;
    for (..) { .. }
    if (!foundProperty)
        return false;
} while (!range.atEnd() && range.peek().type() != CommaToken);

Do you think this is better? I guess it makes things slightly more efficient
because we won't try parsing each comma as each different animation property.

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1236: //
TODO(timloh): This will make invalid longhands, see crbug.com/386459
On 2015/10/20 14:32:23, rwlbuis wrote:
> Why not do this in the end like in the legacy code?

It's actually done at the start of the while loop and after the loop in the
legacy code. Moving this outside the loop will make the longhand lists get
misaligned (e.g. "transition: left, 1s" might end up like "transition: left 1s")

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1245: if
(!isValidAnimationPropertyList(shorthand.properties()[i], *longhands[i]))
On 2015/10/20 14:32:23, rwlbuis wrote:
> Is this done in the legacy code too? If not, is it an improvement and
reflected
> in the tests?

This is the transition-property check, which is also in the existing code, but I
wrote it this way because I figured it be better to not refer to
transition-property in here.

[rwlbuis, 2015-10-21 20:58:39]

Thanks for letting me know about OOO. I guess one more iteration is needed.

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1217: bool
foundProperty = false;
On 2015/10/21 06:07:39, Timothy Loh wrote:
> On 2015/10/20 14:32:23, rwlbuis wrote:
> > I really doubt we need two bools. Can't foundAnyProperty be deduced from
> > longhands? 
> 
> Best I can come up with is as follows:
> 
> do { // previously while (true) {
>     bool foundProperty = false;
>     for (..) { .. }
>     if (!foundProperty)
>         return false;
> } while (!range.atEnd() && range.peek().type() != CommaToken);
> 
> Do you think this is better? I guess it makes things slightly more efficient
> because we won't try parsing each comma as each different animation property.

Yes, seems better.

https://codereview.chromium.org/1408063004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1245: if
(!isValidAnimationPropertyList(shorthand.properties()[i], *longhands[i]))
On 2015/10/21 06:07:39, Timothy Loh wrote:
> On 2015/10/20 14:32:23, rwlbuis wrote:
> > Is this done in the legacy code too? If not, is it an improvement and
> reflected
> > in the tests?
> 
> This is the transition-property check, which is also in the existing code, but
I
> wrote it this way because I figured it be better to not refer to
> transition-property in here.

I see. it is just a pity that it is less efficient than before.

[Timothy Loh, 2015-10-29 03:53:59]

updated & rebased, ptal :)

[rwlbuis, 2015-10-29 04:35:51]

lgtm

[Timothy Loh, 2015-10-29 11:52:42]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2015-10-29 11:53:02]

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

[commit-bot: I haz the power, 2015-10-29 11:56:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-29 11:57:29]

Patchset 3 (id:??) landed as
https://crrev.com/eb8b6acea091169e7063d11076ac1ad0c482209f
Cr-Commit-Position: refs/heads/master@{#356816}