Properties labelled !important must be ignored in keyframes

Properties labelled !important must be ignored in keyframes

According to css3 specifications keyframes should ignore
properties marked as !important
http://dev.w3.org/csswg/css-animations/#keyframes

Added Layout Test

Moving font-face !important check from CSSPropertyParser to CSSParserImpl

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

[patro, 2015-05-22 12:00:57]

behara.ms@samsung.com changed reviewers:
+ timloh@chromium.org

[patro, 2015-05-22 12:00:58]

Kindly Review

Thanks
Shyam Patro

[rwlbuis, 2015-05-22 20:45:59]

On 2015/05/22 12:00:58, patro wrote:
> Kindly Review
> 
> Thanks
> Shyam Patro

Hi Shyam,

First of all, I see the bug is already assigned. Usually this means you should
not touch it, since it was assigned 2 months ago maybe the assignee really is
too busy to work on it, but still it is better to ping first.
Second, the font-face part may already be fixed, see bug 408484.
Finally, a layout test is definitely required. But better first make sure with
@timloh who will work on this.

[rwlbuis, 2015-05-22 20:46:18]

rob.buis@samsung.com changed reviewers:
+ rob.buis@samsung.com

[patro, 2015-05-23 17:29:31]

On 2015/05/22 20:45:59, rwlbuis wrote:
> On 2015/05/22 12:00:58, patro wrote:
> > Kindly Review
> > 
> > Thanks
> > Shyam Patro
> 
> Hi Shyam,
> 
> First of all, I see the bug is already assigned. Usually this means you should
> not touch it, since it was assigned 2 months ago maybe the assignee really is
> too busy to work on it, but still it is better to ping first.
> Second, the font-face part may already be fixed, see bug 408484.
> Finally, a layout test is definitely required. But better first make sure with
> @timloh who will work on this.

Hi rwlbuis,
Thank you for your inputs. I will keep this in mind from next time.
I will check with timloh if i can further work on this bug.

Thanks
Shyam Patro

[Timothy Loh, 2015-05-28 04:47:03]

Yeah this is probably already working for @font-face. I think maybe this is a
better location for the check than the property parser, so we should remove the
check for font-face !important there.

https://codereview.chromium.org/1151893003/diff/1/Source/core/css/parser/CSSP...
File Source/core/css/parser/CSSParserImpl.cpp (right):

https://codereview.chromium.org/1151893003/diff/1/Source/core/css/parser/CSSP...
Source/core/css/parser/CSSParserImpl.cpp:679: // Properties labelled !important
in @keyframes or @font-face should be ignored
I don't think this comment adds any value

[patro, 2015-05-28 08:20:52]

Thanks for Reviewing
Added Layout Tests

-Shyam Patro

https://codereview.chromium.org/1151893003/diff/1/Source/core/css/parser/CSSP...
File Source/core/css/parser/CSSParserImpl.cpp (right):

https://codereview.chromium.org/1151893003/diff/1/Source/core/css/parser/CSSP...
Source/core/css/parser/CSSParserImpl.cpp:679: // Properties labelled !important
in @keyframes or @font-face should be ignored
On 2015/05/28 04:47:03, Timothy Loh wrote:
> I don't think this comment adds any value

Removed it 
Done.

[Timothy Loh, 2015-05-29 03:35:11]

The patch description should have a BUG= line to link to the relevant bug.

https://codereview.chromium.org/1151893003/diff/20001/LayoutTests/animations/...
File LayoutTests/animations/cascade-important-keyframe-properties-expected.txt
(right):

https://codereview.chromium.org/1151893003/diff/20001/LayoutTests/animations/...
LayoutTests/animations/cascade-important-keyframe-properties-expected.txt:2:
FAIL - "background-color" property for "test" element at 0.25s expected:
rgb(128, 0, 0) but saw: rgb(0, 128, 0)
We shouldn't have failing expectations in new tests

[patro, 2015-05-29 03:58:37]

Added BUG number

Kindly Review

Thanks
Shyam Patro

https://codereview.chromium.org/1151893003/diff/20001/LayoutTests/animations/...
File LayoutTests/animations/cascade-important-keyframe-properties-expected.txt
(right):

https://codereview.chromium.org/1151893003/diff/20001/LayoutTests/animations/...
LayoutTests/animations/cascade-important-keyframe-properties-expected.txt:2:
FAIL - "background-color" property for "test" element at 0.25s expected:
rgb(128, 0, 0) but saw: rgb(0, 128, 0)
On 2015/05/29 03:35:11, Timothy Loh wrote:
> We shouldn't have failing expectations in new tests

Done
Changed to PASS expectation

[Timothy Loh, 2015-06-02 00:31:55]

lgtm

[patro, 2015-06-02 04:49:22]

The CQ bit was checked by behara.ms@samsung.com

[commit-bot: I haz the power, 2015-06-02 04:50:59]

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

[commit-bot: I haz the power, 2015-06-02 06:22:46]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196288