Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 619469 - animation should be disabled for failing conditional-processing conditions
: animation should be disabled for failing conditional-processing conditions
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Robert Longson
: Jet Villegas (:jet)
Depends on: 1010681
  Show dependency treegraph
Reported: 2010-12-15 13:44 PST by Robert Longson
Modified: 2014-05-22 13:29 PDT (History)
3 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (1.60 KB, image/svg+xml)
2011-01-11 00:54 PST, Robert Longson
no flags Details
patch (6.05 KB, patch)
2012-02-05 10:27 PST, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Robert Longson 2010-12-15 13:44:39 PST

‘requiredFeatures’, ‘requiredExtensions’ and ‘systemLanguage’ attributes affect ‘animate’, ‘animateColor’, ‘animateMotion’, ‘animateTransform’, and ‘set’ elements. If the conditional statement on these animation elements fails, the animation will never be triggered.
Comment 1 Brian Birtles (:birtles, high review load, away most of 27 Oct-4 Nov) 2011-01-10 18:42:13 PST
I'm getting 403 on the test URL (and in fact all documents from that domain: Any mirror?
Comment 2 Robert Longson 2011-01-11 00:54:10 PST
Created attachment 502752 [details]
Comment 3 Robert Longson 2012-02-05 10:27:05 PST
Created attachment 594564 [details] [diff] [review]
Comment 4 Daniel Holbert [:dholbert] 2012-02-05 11:38:23 PST
Comment on attachment 594564 [details] [diff] [review]

> /*static*/ void
> nsSMILAnimationController::AddAnimationToCompositorTable(
>   nsISMILAnimationElement* aElement, nsSMILCompositorTable* aCompositorTable)
> {
>+  if (!aElement->PassesConditionalProcessingTests())
>+    // If the animation fails conditional processing
>+    // it is ignored.
>+    return;

My initial reaction was "we should catch this earlier, before even registering the animation element with the animation controller".

But I suppose conditions could change between samples & we could dynamically go from not-passing-conditional to passing-conditional.  So, your patch is probably the most straightforward way to solve this.

However, I think we do want to do this up one stack-level -- in nsSMILAnimationController::SampleAnimation.  I think we want to wrap these three lines in a PassesConditionalProcessingTests() check:

> 690   SampleAnimationParams* params = static_cast<SampleAnimationParams*>(aData);
> 691 
> 692   SampleTimedElement(animElem, params->mActiveContainers);
> 693   AddAnimationToCompositorTable(animElem, params->mCompositorTable);

Two notes on the test:
>+++ b/layout/reftests/svg/smil/anim-conditions-01.svg
>@@ -0,0 +1,11 @@
>+  <rect width="100%" height="100%" fill="lime">
>+    <set systemLanguage="foo" attributeName="width" to="0%" dur="5s" begin="0s"/>
>+  </rect>

Nit: jst-review says that <set> line is more than 80 characters -- perhaps add a newline in the middle somewhere?

Also, could you add an additional <set> element (or an additional test) to be sure we actually render animations that have *succeeding* conditionals?

e.g. you could apply s/lime/orange/ above, and then insert a <set [somePassingConditional] attributeName="fill" to="lime" ... >

r=dholbert with those changes
Comment 6 Daniel Holbert [:dholbert] 2012-02-05 19:32:18 PST
Comment on attachment 594564 [details] [diff] [review]

Thanks for the fix!

Sorry, one other thing I just noticed & was wondering about:
>+  nsCOMPtr<DOMSVGTests> tests(do_QueryInterface(
>+    static_cast<nsSVGElement*>(this)));

Do we really need the static_cast there?  (& if we don't, could you push a followup to remove it?)
Comment 7 Robert Longson 2012-02-05 23:41:26 PST
Couldn't make it compile without it.
Comment 8 Marco Bonardo [::mak] 2012-02-06 00:53:25 PST

Note You need to log in before you can comment on or make changes to this bug.