SVGSMILElement should respect SVGTests results

SVGSMILElement should respect SVGTests results

SVGAnimationElement derives SVGTests, but it never checked for its results.
This patch modifies |SVGSMILElement::progress| so it will query |SVGElement::isValid()| before applying animation.

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

[kouhei (in TOK), 2014-07-28 13:08:44]

Would you take a look?

[fs, 2014-07-28 13:21:24]

https://codereview.chromium.org/424753002/diff/20001/LayoutTests/svg/animatio...
File LayoutTests/svg/animations/svg-tests-on-smil-elements.html (right):

https://codereview.chromium.org/424753002/diff/20001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/svg-tests-on-smil-elements.html:3: <rect
id="#reference" x="10" y="10" width="10" height="10" style="fill: red"/>
Doesn't look like this needs to be a pixel (or tree-dump) test? Perhaps convert
reftest?

https://codereview.chromium.org/424753002/diff/20001/Source/core/svg/animatio...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/424753002/diff/20001/Source/core/svg/animatio...
Source/core/svg/animation/SVGSMILElement.cpp:1147: if
(!m_interval.begin.isFinite() || !isValid()) {
It feels like this could be made to contribute to the m_animationValid flag
instead?

[kouhei (in TOK), 2014-07-28 13:27:43]

Thanks for a quick review! I'll fix them tomorrow JST.

https://codereview.chromium.org/424753002/diff/20001/LayoutTests/svg/animatio...
File LayoutTests/svg/animations/svg-tests-on-smil-elements.html (right):

https://codereview.chromium.org/424753002/diff/20001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/svg-tests-on-smil-elements.html:3: <rect
id="#reference" x="10" y="10" width="10" height="10" style="fill: red"/>
On 2014/07/28 13:21:24, fs wrote:
> Doesn't look like this needs to be a pixel (or tree-dump) test? Perhaps
convert
> reftest?

Acknowledged.

https://codereview.chromium.org/424753002/diff/20001/Source/core/svg/animatio...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/424753002/diff/20001/Source/core/svg/animatio...
Source/core/svg/animation/SVGSMILElement.cpp:1147: if
(!m_interval.begin.isFinite() || !isValid()) {
On 2014/07/28 13:21:24, fs wrote:
> It feels like this could be made to contribute to the m_animationValid flag
> instead?

Acknowledged.

[kouhei (in TOK), 2014-07-29 04:06:04]

Comments addressed. PTAL.

[fs, 2014-07-29 08:21:11]

LGTM

https://codereview.chromium.org/424753002/diff/40001/Source/core/svg/SVGAnima...
File Source/core/svg/SVGAnimationElement.h (right):

https://codereview.chromium.org/424753002/diff/40001/Source/core/svg/SVGAnima...
Source/core/svg/SVGAnimationElement.h:63: virtual bool isValid() const OVERRIDE
FINAL { return SVGTests::isValid(); }
Nit: Couldn't this be made private access?

[kouhei (in TOK), 2014-07-29 09:01:51]

Thanks for review!

On 2014/07/29 08:21:11, fs wrote:
> LGTM
> 
>
https://codereview.chromium.org/424753002/diff/40001/Source/core/svg/SVGAnima...
> File Source/core/svg/SVGAnimationElement.h (right):
> 
>
https://codereview.chromium.org/424753002/diff/40001/Source/core/svg/SVGAnima...
> Source/core/svg/SVGAnimationElement.h:63: virtual bool isValid() const
OVERRIDE
> FINAL { return SVGTests::isValid(); }
> Nit: Couldn't this be made private access?

Done.

[kouhei (in TOK), 2014-07-29 09:02:23]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-07-29 09:03:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/424753002/60001

[commit-bot: I haz the power, 2014-07-29 10:14:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19416)

[commit-bot: I haz the power, 2014-07-29 10:48:59]

Change committed as 179122