From 3badaca765667066e82e77a2551c4c5d1c044a05 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Mon, 19 Jan 2009 22:12:29 +1300 Subject: [PATCH] Bug 474257. Implement SVG 1.1 erratum to make beginElementAt/endElementAt return void. r+sr=roc --- content/smil/nsSMILTimedElement.cpp | 51 +++------- content/smil/nsSMILTimedElement.h | 8 +- content/smil/test/test_smilRestart.xhtml | 96 ++++++++++--------- .../svg/content/src/nsSVGAnimationElement.cpp | 12 +-- 4 files changed, 70 insertions(+), 97 deletions(-) diff --git a/content/smil/nsSMILTimedElement.cpp b/content/smil/nsSMILTimedElement.cpp index 2b9e4e4da98..302a9d60e53 100644 --- a/content/smil/nsSMILTimedElement.cpp +++ b/content/smil/nsSMILTimedElement.cpp @@ -93,63 +93,39 @@ nsSMILTimedElement::nsSMILTimedElement() // Animation and SVG 1.1. In SMIL Animation all methods have a void return // type and the new instance time is simply added to the list and restart // semantics are applied as with any other instance time. In the SVG definition -// the methods return a bool depending on the restart mode. There are some -// cases where this is problematic. +// the methods return a bool depending on the restart mode. // -// For example, if a call is made to beginElementAt and the resolved time -// after including the offset falls outside the current interval then using -// the SMIL Animation definition an element with restart == whenNotActive -// would restart with this new instance time. The SVG definition however seems -// to imply that in this case the implementation should ignore the new -// instance time if the restart mode == whenNotActive and the element is -// currently active and return false. +// This inconsistency has now been addressed by an erratum in SVG 1.1: // -// It is tempting to try and determine when a new instance time will actually -// cause a restart but this is not possible as in the meantime a new event may -// trump the new instance time. We take a compromise of returning true and -// false according to the SVG definition but adding the instance time to the -// list regardless. This may produce different results to an implementation that -// follows strictly the behaviour implied by the SVG spec. +// http://www.w3.org/2003/01/REC-SVG11-20030114-errata#elementtimecontrol-interface // +// which favours the definition in SMIL, i.e. instance times are just added +// without first checking the restart mode. -/* boolean beginElementAt (in float offset); */ -PRBool +nsresult nsSMILTimedElement::BeginElementAt(double aOffsetSeconds, const nsSMILTimeContainer* aContainer) { - // If restart == never or restart == whenNotActive, check whether we're - // in a state that allows us to restart. - if ((mRestartMode == RESTART_NEVER && - (mElementState == STATE_ACTIVE || mElementState == STATE_POSTACTIVE)) || - (mRestartMode == RESTART_WHENNOTACTIVE && - mElementState == STATE_ACTIVE)) { - return PR_FALSE; - } - if (!AddInstanceTimeFromCurrentTime(aOffsetSeconds, PR_TRUE, aContainer)) { // Probably we don't have a time container NS_ERROR("Failed to begin element"); - return PR_FALSE; + return NS_ERROR_FAILURE; } - return PR_TRUE; + return NS_OK; } -/* boolean endElementAt (in float offset); */ -PRBool +nsresult nsSMILTimedElement::EndElementAt(double aOffsetSeconds, const nsSMILTimeContainer* aContainer) { - if (mElementState != STATE_ACTIVE) - return PR_FALSE; - if (!AddInstanceTimeFromCurrentTime(aOffsetSeconds, PR_FALSE, aContainer)) { // Probably we don't have a time container NS_ERROR("Failed to end element"); - return PR_FALSE; + return NS_ERROR_FAILURE; } - return PR_TRUE; + return NS_OK; } //---------------------------------------------------------------------- @@ -190,10 +166,11 @@ void nsSMILTimedElement::AddInstanceTime(const nsSMILInstanceTime& aInstanceTime, PRBool aIsBegin) { - if (aIsBegin) + if (aIsBegin) { mBeginInstances.AppendElement(aInstanceTime); - else + } else { mEndInstances.AppendElement(aInstanceTime); + } UpdateCurrentInterval(); } diff --git a/content/smil/nsSMILTimedElement.h b/content/smil/nsSMILTimedElement.h index 0eb8d1e9cdd..9ca88a8dc38 100644 --- a/content/smil/nsSMILTimedElement.h +++ b/content/smil/nsSMILTimedElement.h @@ -75,8 +75,8 @@ public: * current time. * @return NS_OK if the operation succeeeded, or an error code otherwise. */ - PRBool BeginElementAt(double aOffsetSeconds, - const nsSMILTimeContainer* aContainer); + nsresult BeginElementAt(double aOffsetSeconds, + const nsSMILTimeContainer* aContainer); /* * Adds a new end instance time at the current document time (as defined by @@ -89,8 +89,8 @@ public: * current time. * @return NS_OK if the operation succeeeded, or an error code otherwise. */ - PRBool EndElementAt(double aOffsetSeconds, - const nsSMILTimeContainer* aContainer); + nsresult EndElementAt(double aOffsetSeconds, + const nsSMILTimeContainer* aContainer); /** * Methods for supporting the nsSVGAnimationElement interface. diff --git a/content/smil/test/test_smilRestart.xhtml b/content/smil/test/test_smilRestart.xhtml index d110de80fc2..bd3c797560d 100644 --- a/content/smil/test/test_smilRestart.xhtml +++ b/content/smil/test/test_smilRestart.xhtml @@ -11,15 +11,15 @@ - - - @@ -30,8 +30,10 @@ /** Test for SMIL Restart Behavior **/ /* Global Variables */ -var dur = 4.0; // this must match the "dur" attribute on animations above. var svg = document.getElementById("svg"); +var always = document.getElementById("always"); +var whenNotActive = document.getElementById("whenNotActive"); +var never = document.getElementById("never"); SimpleTest.waitForExplicitFinish(); @@ -40,60 +42,62 @@ function main() { checkInitialState(); } -// Attempt a "beginElement" call on the given element, and -// complain if we don't get the expected return value. -// 'time' is only provided for better diagnostic output. -function tryRestartElem(id, time, expectedRetVal) { - var elem = document.getElementById(id); - var retVal = elem.beginElement(); - is(retVal, expectedRetVal, - "Error restarting animation '" + id + - "' at time = " + time + ": " + - "expected return value of " + expectedRetVal + - ", but got " + retVal + "."); +function restartAll() { + always.beginElement(); + whenNotActive.beginElement(); + never.beginElement(); } function checkInitialState() { - svg.setCurrentTime(0.0); - setTimeout('doCheckInitialState(0.0)', 0); + // At first everything should be starting at 1s + is(always.getStartTime(), 1); + is(whenNotActive.getStartTime(), 1); + is(never.getStartTime(), 1); + + // Now try to restart everything early, should be allowed by all + var restartTime = svg.getCurrentTime(); + restartAll(); + setTimeout('doCheckInitialState(' + restartTime + ')',0); } -function doCheckInitialState(time) { - tryRestartElem('always', time, true); - tryRestartElem('whenNotActive', time, false); - tryRestartElem('never', time, false); - checkHalfwayState(); +function doCheckInitialState(restartTime) { + is(always.getStartTime(), restartTime); + is(whenNotActive.getStartTime(), restartTime); + is(never.getStartTime(), restartTime); + + // Now skip to half-way + var newTime = restartTime + 0.5 * always.getSimpleDuration(); + svg.setCurrentTime(newTime); + setTimeout('checkHalfwayState()', 0); } function checkHalfwayState() { - svg.setCurrentTime(0.5 * dur); - setTimeout('doCheckHalfwayState(0.5 * dur)', 0); + // Try to restart half-way through--only 'always' should succeed + var restartTime = svg.getCurrentTime(); + restartAll(); + setTimeout('doCheckHalfwayState(' + restartTime + ')',0); } -function doCheckHalfwayState(time) { - tryRestartElem('always', time, true); - tryRestartElem('whenNotActive', time, false); - tryRestartElem('never', time, false); - checkEndingState(); +function doCheckHalfwayState(restartTime) { + is(always.getStartTime(), restartTime); + isnot(whenNotActive.getStartTime(), restartTime); + isnot(never.getStartTime(), restartTime); + + // Now skip to the end + var newTime = always.getStartTime() + always.getSimpleDuration() + 1; + svg.setCurrentTime(newTime); + setTimeout('checkEndingState()', 0); } function checkEndingState() { - svg.setCurrentTime(dur); - setTimeout('doCheckEndingState(dur)', 0); + // Try to restart after all animations have finished--'always' and + // 'whenNotActive' should succeed + var restartTime = svg.getCurrentTime(); + restartAll(); + setTimeout('doCheckEndingState(' + restartTime + ')',0); } -function doCheckEndingState(time) { - tryRestartElem('always', time, true); - tryRestartElem('whenNotActive', time, true); - tryRestartElem('never', time, false); - checkAfterEndingState(); -} - -function checkAfterEndingState() { - svg.setCurrentTime(dur * 3); - setTimeout('doCheckAfterEndingState(dur * 3)', 0); -} -function doCheckAfterEndingState(time) { - tryRestartElem('always', time, true); - tryRestartElem('whenNotActive', time, true); - tryRestartElem('never', time, false); +function doCheckEndingState(restartTime) { + is(always.getStartTime(), restartTime); + is(whenNotActive.getStartTime(), restartTime); + isnot(never.getStartTime(), restartTime); SimpleTest.finish(); } diff --git a/content/svg/content/src/nsSVGAnimationElement.cpp b/content/svg/content/src/nsSVGAnimationElement.cpp index a5dd4330f9d..f092a58d403 100644 --- a/content/svg/content/src/nsSVGAnimationElement.cpp +++ b/content/svg/content/src/nsSVGAnimationElement.cpp @@ -386,12 +386,7 @@ nsSVGAnimationElement::BeginElementAt(float offset) ownerSVG->RequestSample(); - // We ignore the return code. The SMIL version of this interface has a void - // return type and no exception specification so there's no way to indicate - // that begin failed (e.g. because the element has restart="none"). - mTimedElement.BeginElementAt(offset, mTimedDocumentRoot); - - return NS_OK; + return mTimedElement.BeginElementAt(offset, mTimedDocumentRoot); } /* void endElement (); */ @@ -411,8 +406,5 @@ nsSVGAnimationElement::EndElementAt(float offset) ownerSVG->RequestSample(); - // As with BeginElementAt, ignore the return code. - mTimedElement.EndElementAt(offset, mTimedDocumentRoot); - - return NS_OK; + return mTimedElement.EndElementAt(offset, mTimedDocumentRoot); }