<animate> not working on SVG Polygons.

<animate> not working on SVG Polygons.

The animation is taking place, but the rendering side didn't use the
computed values.

R=
BUG=196435
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161329

[Erik Dahlström (inactive), 2013-11-01 13:43:39]

I'm not that familiar with the code here, but in any case the values used for
rendering are not the animated ones. The rendering code should always use the
animVal value(s). 

The values are being animated correctly (verified by an existing LayoutTest),
but there is no test that checks that the animated values are actually used for
rendering.

As an aside, I found this part of the code to be rather confusing.

[pdr., 2013-11-01 18:53:29]

On 2013/11/01 13:43:39, Erik Dahlström wrote:
> I'm not that familiar with the code here, but in any case the values used for
> rendering are not the animated ones. The rendering code should always use the
> animVal value(s). 

LGTM for this code change.

> 
> The values are being animated correctly (verified by an existing LayoutTest),
> but there is no test that checks that the animated values are actually used
for
> rendering.

I would expect to see a test change but I don't see the updated expectations in
this patch.


> 
> As an aside, I found this part of the code to be rather confusing.

You are not alone! The tearoff code is just terrible. Kouhei@ is in the process
of rewriting the tearoff code as we speak.

[Erik Dahlström (inactive), 2013-11-04 08:14:04]

On 2013/11/01 18:53:29, pdr wrote:
> On 2013/11/01 13:43:39, Erik Dahlström wrote:
> > I'm not that familiar with the code here, but in any case the values used
for
> > rendering are not the animated ones. The rendering code should always use
the
> > animVal value(s). 
> 
> LGTM for this code change.
> 
> > 
> > The values are being animated correctly (verified by an existing
LayoutTest),
> > but there is no test that checks that the animated values are actually used
> for
> > rendering.
> 
> I would expect to see a test change but I don't see the updated expectations
in
> this patch.

Oh, seems I didn't make myself clear enough, I meant that there is already at
least one test (which passes before this patch) -
LayoutTests/svg/animations/svgpointlist-animation*. However, those tests don't
verify that the values are actually used for rendering, just that the values are
computed correctly and are available through the DOM.

> > As an aside, I found this part of the code to be rather confusing.
> 
> You are not alone! The tearoff code is just terrible. Kouhei@ is in the
process
> of rewriting the tearoff code as we speak.

Cool, looking forward to that.

[pdr., 2013-11-04 18:29:48]

On 2013/11/04 08:14:04, Erik Dahlström wrote:
> > I would expect to see a test change but I don't see the updated expectations
> in
> > this patch.
> 
> Oh, seems I didn't make myself clear enough, I meant that there is already at
> least one test (which passes before this patch) -
> LayoutTests/svg/animations/svgpointlist-animation*. However, those tests don't
> verify that the values are actually used for rendering, just that the values
are
> computed correctly and are available through the DOM.
> 

LGTM to land with one question: In this patch you changed
SVGRenderTreeAsText.cpp to now print the animating values. Does this cause us to
lose coverage for the non-animating values?

[Erik Dahlström (inactive), 2013-11-05 09:14:25]

On 2013/11/04 18:29:48, pdr wrote:
> On 2013/11/04 08:14:04, Erik Dahlström wrote:
> > > I would expect to see a test change but I don't see the updated
expectations
> > in
> > > this patch.
> > 
> > Oh, seems I didn't make myself clear enough, I meant that there is already
at
> > least one test (which passes before this patch) -
> > LayoutTests/svg/animations/svgpointlist-animation*. However, those tests
don't
> > verify that the values are actually used for rendering, just that the values
> are
> > computed correctly and are available through the DOM.
> > 
> 
> LGTM to land with one question: In this patch you changed
> SVGRenderTreeAsText.cpp to now print the animating values. Does this cause us
to
> lose coverage for the non-animating values?

A quick grep shows that there are tests both for accessing .points and
.animatedPoints via the DOM, so no lost coverage from this change.

I just followed the convention used for all the other value printing in
SVGRenderTreeAsText.cpp, which is to print the current animated value(s).

[commit-bot: I haz the power, 2013-11-05 09:15:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/55893002/1

[commit-bot: I haz the power, 2013-11-05 12:30:15]

Change committed as 161329