Disable animated clipping on SVG content

Disable animated clipping on SVG content

Animated clipping has to be disabled until it is not supported properly.
With removing the clip property from animatedPropertyTypeForCSSAttribute
animted clipping is considered as an unknown animation and won't cause
assertion failure later.

R=kouhei@chromium.org,fs@opera.com
BUG=422955
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183671

[fs, 2014-10-13 16:26:11]

On 2014/10/13 15:57:11, reni wrote:

The 'clip' property is a strange beast IIRC - the value is a <shape> (and only
'rect(...)' is supported - at least in the 1.1 spec.). So SVGRect doesn't feel
like the right type here. I get the feeling that animating 'clip' may not work
at all ATM (and might require additional work). If it's supported at all (on SVG
content) that is =).

Maybe we should just stop pretending that it works instead? How do others feel
about this?

[fs, 2014-10-13 16:53:47]

On 2014/10/13 16:26:11, fs wrote:
> On 2014/10/13 15:57:11, reni wrote:
> 
> The 'clip' property is a strange beast IIRC - the value is a <shape> (and only
> 'rect(...)' is supported - at least in the 1.1 spec.). So SVGRect doesn't feel
> like the right type here. I get the feeling that animating 'clip' may not work
> at all ATM (and might require additional work). If it's supported at all (on
SVG
> content) that is =).
> 
> Maybe we should just stop pretending that it works instead? How do others feel
> about this?

Example: http://jsfiddle.net/jLL6L6g9/2/ - works [1] in Presto and Gecko.

[1] "Works" as in "Works on SVG content which isn't the outermost root".

[kouhei (in TOK), 2014-10-14 04:53:35]

On 2014/10/13 16:53:47, fs wrote:
> On 2014/10/13 16:26:11, fs wrote:
> > On 2014/10/13 15:57:11, reni wrote:
> > 
> > The 'clip' property is a strange beast IIRC - the value is a <shape> (and
only
> > 'rect(...)' is supported - at least in the 1.1 spec.). So SVGRect doesn't
feel
> > like the right type here. I get the feeling that animating 'clip' may not
work
> > at all ATM (and might require additional work). If it's supported at all (on
> SVG
> > content) that is =).
> > 
> > Maybe we should just stop pretending that it works instead? How do others
feel
> > about this?
> 
> Example: http://jsfiddle.net/jLL6L6g9/2/ - works [1] in Presto and Gecko.
> 
> [1] "Works" as in "Works on SVG content which isn't the outermost root".

I support fs@'s comments.
If this works, would you add a test to actually animate the clip and not just
test crash?
If this doesn't work, I think we should disable it and remove the property from
animatedPropertyTypeForCSSAttribute.

https://codereview.chromium.org/656443002/diff/1/LayoutTests/svg/animations/a...
File LayoutTests/svg/animations/animate-clip-crash.html (right):

https://codereview.chromium.org/656443002/diff/1/LayoutTests/svg/animations/a...
LayoutTests/svg/animations/animate-clip-crash.html:8:
testRunner.waitUntilDone();
Do we really need waitUntilDone() on this test?

[reni, 2014-10-14 12:27:52]

On 2014/10/14 04:53:35, kouhei wrote:
> On 2014/10/13 16:53:47, fs wrote:
> > On 2014/10/13 16:26:11, fs wrote:
> > > On 2014/10/13 15:57:11, reni wrote:
> > > 
> > > The 'clip' property is a strange beast IIRC - the value is a <shape> (and
> only
> > > 'rect(...)' is supported - at least in the 1.1 spec.). So SVGRect doesn't
> feel
> > > like the right type here. I get the feeling that animating 'clip' may not
> work
> > > at all ATM (and might require additional work). If it's supported at all
(on
> > SVG
> > > content) that is =).
> > > 
> > > Maybe we should just stop pretending that it works instead? How do others
> feel
> > > about this?
> > 
> > Example: http://jsfiddle.net/jLL6L6g9/2/ - works [1] in Presto and Gecko.
> > 
> > [1] "Works" as in "Works on SVG content which isn't the outermost root".
> 
> I support fs@'s comments.
> If this works, would you add a test to actually animate the clip and not just
> test crash?
> If this doesn't work, I think we should disable it and remove the property
from
> animatedPropertyTypeForCSSAttribute.

Indeed, this feature doesn't work right now hence I've disabled it.

>
https://codereview.chromium.org/656443002/diff/1/LayoutTests/svg/animations/a...
> File LayoutTests/svg/animations/animate-clip-crash.html (right):
> 
>
https://codereview.chromium.org/656443002/diff/1/LayoutTests/svg/animations/a...
> LayoutTests/svg/animations/animate-clip-crash.html:8:
> testRunner.waitUntilDone();
> Do we really need waitUntilDone() on this test?

No, we don't.

[kouhei (in TOK), 2014-10-14 12:32:10]

lgtm. Thanks for the patch!

[reni, 2014-10-14 12:39:44]

On 2014/10/14 12:32:10, kouhei wrote:
> lgtm. Thanks for the patch!

Thanks for the review! :)

[reni, 2014-10-14 12:39:48]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-10-14 12:40:56]

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

[commit-bot: I haz the power, 2014-10-14 13:19:22]

Committed patchset #2 (id:90001) as 183671