Bug 705236 part 1 - Allow trailing separator in SMIL values list; r=dholbert
authorBrian Birtles <birtles@gmail.com>
Fri, 24 Feb 2012 09:45:40 +0900
changeset 87596 7d5d8b8121d137e85a9460f45d1da4b9b982660b
parent 87595 7f972541b27412a93507b9789c2c5c2e0f9666b5
child 87597 c1e552fbde97edf1d3c80c361ff78601bec9346e
push id22133
push usermak77@bonardo.net
push dateFri, 24 Feb 2012 10:23:30 +0000
treeherdermozilla-central@fbcdc2c87df8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs705236
milestone13.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 705236 part 1 - Allow trailing separator in SMIL values list; r=dholbert
content/smil/nsSMILParserUtils.cpp
content/smil/nsSMILTimedElement.cpp
content/smil/test/Makefile.in
content/smil/test/smilAnimateMotionValueLists.js
content/smil/test/test_smilAnimateMotionInvalidValues.xhtml
content/smil/test/test_smilTiming.xhtml
content/smil/test/test_smilValues.xhtml
content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
--- a/content/smil/nsSMILParserUtils.cpp
+++ b/content/smil/nsSMILParserUtils.cpp
@@ -610,21 +610,16 @@ nsSMILParserUtils::ParseValuesGeneric(co
 
   while (tokenizer.hasMoreTokens()) {
     nsresult rv = aParser.Parse(tokenizer.nextToken());
     if (NS_FAILED(rv)) {
       return NS_ERROR_FAILURE;
     }
   }
 
-  // Disallow ;-terminated values lists.
-  if (tokenizer.lastTokenEndedWithSeparator()) {
-    return NS_ERROR_FAILURE;
-  }
-
   return NS_OK;
 }
 
 nsresult
 nsSMILParserUtils::ParseRepeatCount(const nsAString& aSpec,
                                     nsSMILRepeatCount& aResult)
 {
   nsresult rv = NS_OK;
--- a/content/smil/nsSMILTimedElement.cpp
+++ b/content/smil/nsSMILTimedElement.cpp
@@ -50,16 +50,17 @@
 #include "nsThreadUtils.h"
 #include "nsIPresShell.h"
 #include "prdtoa.h"
 #include "plstr.h"
 #include "prtime.h"
 #include "nsString.h"
 #include "mozilla/AutoRestore.h"
 #include "mozilla/Util.h"
+#include "nsCharSeparatedTokenizer.h"
 
 using namespace mozilla;
 
 //----------------------------------------------------------------------
 // Helper class: InstanceTimeComparator
 
 // Upon inserting an instance time into one of our instance time lists we assign
 // it a serial number. This allows us to sort the instance times in such a way
@@ -1266,38 +1267,37 @@ nsSMILTimedElement::Unlink()
 // Implementation helpers
 
 nsresult
 nsSMILTimedElement::SetBeginOrEndSpec(const nsAString& aSpec,
                                       Element* aContextNode,
                                       bool aIsBegin,
                                       RemovalTestFunction aRemove)
 {
-  PRInt32 start;
-  PRInt32 end = -1;
-  PRInt32 length;
-  nsresult rv = NS_OK;
   TimeValueSpecList& timeSpecsList = aIsBegin ? mBeginSpecs : mEndSpecs;
   InstanceTimeList& instances = aIsBegin ? mBeginInstances : mEndInstances;
 
   ClearSpecs(timeSpecsList, instances, aRemove);
 
   AutoIntervalUpdateBatcher updateBatcher(*this);
 
-  do {
-    start = end + 1;
-    end = aSpec.FindChar(';', start);
-    length = (end == -1) ? -1 : end - start;
+  nsCharSeparatedTokenizer tokenizer(aSpec, ';');
+  if (!tokenizer.hasMoreTokens()) { // Empty list
+    return NS_ERROR_FAILURE;
+  }
+
+  nsresult rv = NS_OK;
+  while (tokenizer.hasMoreTokens() && NS_SUCCEEDED(rv)) {
     nsAutoPtr<nsSMILTimeValueSpec>
       spec(new nsSMILTimeValueSpec(*this, aIsBegin));
-    rv = spec->SetSpec(Substring(aSpec, start, length), aContextNode);
+    rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
     if (NS_SUCCEEDED(rv)) {
       timeSpecsList.AppendElement(spec.forget());
     }
-  } while (end != -1 && NS_SUCCEEDED(rv));
+  }
 
   if (NS_FAILED(rv)) {
     ClearSpecs(timeSpecsList, instances, aRemove);
   }
 
   return rv;
 }
 
--- a/content/smil/test/Makefile.in
+++ b/content/smil/test/Makefile.in
@@ -86,16 +86,17 @@ include $(topsrcdir)/config/rules.mk
 	  test_smilSync.xhtml \
 	  test_smilSyncbaseTarget.xhtml \
 	  test_smilSyncTransform.xhtml \
 	  test_smilTextZoom.xhtml \
 	  test_smilTimeEvents.xhtml \
 	  test_smilTiming.xhtml \
 	  test_smilTimingZeroIntervals.xhtml \
 	  test_smilUpdatedInterval.xhtml \
+	  test_smilValues.xhtml \
 	  test_smilXHR.xhtml \
 	  $(NULL)
 
 # Tests disabled due to intermittent orange
 # test_smilCSSInherit.xhtml disabled until bug 501183 is fixed
 
 libs:: $(_TEST_FILES)
 	$(INSTALL) $^ $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
--- a/content/smil/test/smilAnimateMotionValueLists.js
+++ b/content/smil/test/smilAnimateMotionValueLists.js
@@ -35,26 +35,27 @@
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 /* Lists of valid & invalid values for the various <animateMotion> attributes */
 const gValidValues = [
   "10 10",
+  "10 10;",  // Trailing semicolons are allowed
+  "10 10;  ",
   "   10   10em  ",
   "1 2  ; 3,4",
   "1,2;3,4",
   "0 0",
   "0,0",
 ];
 
 const gInvalidValues = [
   ";10 10",
-  "10 10;",  // We treat semicolon-terminated value-lists as failure cases
   "10 10;;",
   "1 2 3",
   "1 2 3 4",
   "1,2;3,4 ,",
   ",", " , ",
   ";", " ; ",
   "a", " a; ", ";a;",
   "", " ",
@@ -123,8 +124,35 @@ const gInvalidPath = [
 
 // paths that at least start with a valid "M" segment are valid - the spec says
 // to parse everything up to the first invalid token
 const gValidPathWithErrors = [
  "M20 20em",
  "m0 0 L30,,30",
  "M10 10 L50 50 abc",
 ];
+
+const gValidKeyPoints = [
+  "0; 0.5; 1",
+  "0;.5;1",
+  "0; 0; 1",
+  "0; 1; 1",
+  "0; 0; 1;", // Trailing semicolons are allowed
+  "0; 0; 1; ",
+  "0; 0.000; 1",
+  "0; 0.000001; 1",
+];
+
+const gInvalidKeyPoints = [
+  "0; 1",
+  "0; 1;",
+  "0",
+  "1",
+  "a",
+  "",
+  "  ",
+  "0; -0.1; 1",
+  "0; 1.1; 1",
+  "0; 0.1; 1.1",
+  "-0.1; 0.1; 1",
+  "0; a; 1",
+  "0;;1",
+];
--- a/content/smil/test/test_smilAnimateMotionInvalidValues.xhtml
+++ b/content/smil/test/test_smilAnimateMotionInvalidValues.xhtml
@@ -59,16 +59,23 @@ function testAttr(aAttrName, aAttrValueA
       // and just test the rotation matrix components
       anim.setAttribute("values", "0 0; 50 50");
       componentsToCheck = CTMUtil.CTM_COMPONENTS_ROTATE;
     } else {
       // Apply a supplementary rotation to make sure that we don't apply it if
       // our value is rejected.
       anim.setAttribute("rotate", Math.PI/4);
       componentsToCheck = CTMUtil.CTM_COMPONENTS_ALL;
+      if (aAttrName == "keyPoints") {
+        // Add three times so we can test a greater range of values for
+        // keyPoints
+        anim.setAttribute("values", "0 0; 25 25; 50 50");
+        anim.setAttribute("keyTimes", "0; 0.5; 1");
+        anim.setAttribute("calcMode", "discrete");
+      }
     }
 
     var curCTM = gRect.getCTM();
     if (aIsValid) {
       var errMsg = "CTM should have changed when applying animateMotion " +
         "with '" + aAttrName + "' set to valid value '" + curVal + "'";
       CTMUtil.assertCTMNotEqual(curCTM, gUnAnimatedCTM, componentsToCheck,
                                 errMsg, aIsTodo);
@@ -153,16 +160,19 @@ function main()
 
   testAttr("by", gValidToBy, true, false);
   testAttr("by", gInvalidToBy, false, false);
 
   testAttr("path", gValidPath, true, false);
   testAttr("path", gInvalidPath, false, false);
   testAttr("path", gValidPathWithErrors, true, false);
 
+  testAttr("keyPoints", gValidKeyPoints, true, false);
+  testAttr("keyPoints", gInvalidKeyPoints, false, false);
+
   testMpathElem(gValidPath, true, false);
   testMpathElem(gInvalidPath, false, false);
 
   SimpleTest.finish();
 }
 
 window.addEventListener("load", main, false);
 ]]>
--- a/content/smil/test/test_smilTiming.xhtml
+++ b/content/smil/test/test_smilTiming.xhtml
@@ -38,17 +38,18 @@ function removeAnim(anim) {
   anim.parentNode.removeChild(anim);
 }
 
 function main() {
   ok(svg.animationsPaused(), "should be paused by <svg> load handler");
   is(svg.getCurrentTime(), 0, "should be paused at 0 in <svg> load handler");
 
   var tests =
-    [ testOffsetStartup,
+    [ testListSyntax,
+      testOffsetStartup,
       testMultipleBegins,
       testNegativeBegins,
       testSorting
     ];
   for (var i = 0; i < tests.length; i++) {
     var anim = createAnim();
     svg.setCurrentTime(0);
     tests[i](anim);
@@ -57,16 +58,50 @@ function main() {
   SimpleTest.finish();
 }
 
 function checkSample(time, expectedValue) {
   svg.setCurrentTime(time);
   is(circle.cx.animVal.value, expectedValue);
 }
 
+function getStartTime(anim) {
+  var startTime;
+  try {
+    startTime = anim.getStartTime();
+  } catch(e) {
+    if (e.code == DOMException.INVALID_STATE_ERR) {
+      startTime = 'none';
+    } else {
+      ok(false, "Unexpected exception: " + e);
+    }
+  }
+  return startTime;
+}
+
+function testListSyntax() {
+  var specs = [ [ '0', 0 ],
+                [ '3;', 3 ],
+                [ '3; ', 3 ],
+                [ '3 ; ', 3 ],
+                [ '3;;', 'none' ],
+                [ '3;; ', 'none' ],
+                [ ';3', 'none' ],
+                [ ' ;3', 'none' ],
+                [ '3;4', 3 ],
+                [ ' 3 ; 4 ', 3 ] ];
+  for (var i = 0; i < specs.length; i++) {
+    var anim = createAnim()
+    anim.setAttribute('begin', specs[i][0]);
+    is(getStartTime(anim), specs[i][1],
+       "Got unexpected start time for " + specs[i][0]);
+    removeAnim(anim);
+  }
+}
+
 function testOffsetStartup(anim) {
   anim.setAttribute('begin', '3s');
   checkSample(0,-100);
   checkSample(4,10);
 }
 
 function testMultipleBegins(anim) {
   anim.setAttribute('begin', '2s; 6s');
new file mode 100644
--- /dev/null
+++ b/content/smil/test/test_smilValues.xhtml
@@ -0,0 +1,170 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+  <title>Test for SMIL values</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank"
+  href="https://bugzilla.mozilla.org/show_bug.cgi?id=557885">Mozilla Bug
+  474742</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<svg id="svg" xmlns="http://www.w3.org/2000/svg" width="120px" height="120px">
+  <circle cx="-100" cy="20" r="15" fill="blue" id="circle"/>
+</svg>
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+/** Test for SMIL values **/
+
+var gSvg = document.getElementById("svg");
+SimpleTest.waitForExplicitFinish();
+
+function main()
+{
+  gSvg.pauseAnimations();
+
+  var testCases = Array();
+
+  // Single value
+  testCases.push({
+    'attr' : { 'values': 'a' },
+    'times': [ [ 0, 'a' ] ]
+  });
+
+  // The parsing below is based on the following discussion:
+  //
+  //   http://lists.w3.org/Archives/Public/www-svg/2011Nov/0136.html
+  //
+  // In summary:
+  // * Values lists are semi-colon delimited and semi-colon terminated.
+  // * However, if there are extra non-whitespace characters after the final
+  //   semi-colon then there's an implied semi-colon at the end.
+  //
+  // This differs to what is specified in SVG 1.1 but is consistent with the
+  // majority of browsers and with existing content (particularly that generated
+  // by Ikivo Animator).
+
+  // Trailing semi-colon
+  testCases.push({
+    'attr' : { 'values': 'a;' },
+    'times': [ [ 0, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Trailing semi-colon + whitespace
+  testCases.push({
+    'attr' : { 'values': 'a; ' },
+    'times': [ [ 0, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Whitespace + trailing semi-colon
+  testCases.push({
+    'attr' : { 'values': 'a ;' },
+    'times': [ [ 0, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Empty at end
+  testCases.push({
+    'attr' : { 'values': 'a;;' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, '' ] ]
+  });
+
+  // Empty at end + whitespace
+  testCases.push({
+    'attr' : { 'values': 'a;; ' },
+    'times': [ [ 0, 'a' ], [ 4, 'a' ], [ 5, '' ], [ 10, '' ] ]
+  });
+
+  // Empty in middle
+  testCases.push({
+    'attr' : { 'values': 'a;;b' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, 'b' ] ]
+  });
+
+  // Empty in middle + trailing semi-colon
+  testCases.push({
+    'attr' : { 'values': 'a;;b;' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, 'b' ] ]
+  });
+
+  // Whitespace in middle
+  testCases.push({
+    'attr' : { 'values': 'a; ;b' },
+    'times': [ [ 0, 'a' ], [ 5, '' ], [ 10, 'b' ] ]
+  });
+
+  // Empty at start
+  testCases.push({
+    'attr' : { 'values': ';a' },
+    'times': [ [ 0, '' ], [ 5, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Whitespace at start
+  testCases.push({
+    'attr' : { 'values': ' ;a' },
+    'times': [ [ 0, '' ], [ 5, 'a' ], [ 10, 'a' ] ]
+  });
+
+  // Embedded whitespace
+  testCases.push({
+    'attr' : { 'values': ' a b ; c d ' },
+    'times': [ [ 0, 'a b' ], [ 5, 'c d' ], [ 10, 'c d' ] ]
+  });
+
+  // Whitespace only
+  testCases.push({
+    'attr' : { 'values': '  ' },
+    'times': [ [ 0, '' ], [ 10, '' ] ]
+  });
+
+  for (var i = 0; i < testCases.length; i++) {
+    gSvg.setCurrentTime(0);
+    var test = testCases[i];
+
+    // Create animation elements
+    var anim = createAnim(test.attr);
+
+    // Run samples
+    for (var j = 0; j < test.times.length; j++) {
+      var curSample = test.times[j];
+      gSvg.setCurrentTime(curSample[0]);
+      checkSample(anim, curSample[1], curSample[0], i);
+    }
+
+    anim.parentNode.removeChild(anim);
+  }
+
+  SimpleTest.finish();
+}
+
+function createAnim(attr)
+{
+  const svgns = "http://www.w3.org/2000/svg";
+  var anim = document.createElementNS(svgns, 'animate');
+  anim.setAttribute('attributeName','class');
+  anim.setAttribute('dur','10s');
+  anim.setAttribute('begin','0s');
+  anim.setAttribute('fill','freeze');
+  for (name in attr) {
+    anim.setAttribute(name, attr[name]);
+  }
+  return document.getElementById('circle').appendChild(anim);
+}
+
+function checkSample(anim, expectedValue, sampleTime, caseNum)
+{
+  var msg = "Test case " + caseNum +
+    " (values: '" + anim.getAttribute('values') + "')," +
+    "t=" + sampleTime +
+    ": Unexpected sample value:";
+  is(anim.targetElement.className.animVal, expectedValue, msg);
+}
+
+window.addEventListener("load", main, false);
+]]>
+</script>
+</pre>
+</body>
+</html>
--- a/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
+++ b/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
@@ -442,17 +442,17 @@ SVGMotionSMILAnimationFunction::SetKeyPo
   mHasChanged = true;
 
   return NS_OK;
 }
 
 void
 SVGMotionSMILAnimationFunction::UnsetKeyPoints()
 {
-  mKeyTimes.Clear();
+  mKeyPoints.Clear();
   SetKeyPointsErrorFlag(false);
   mHasChanged = true;
 }
 
 nsresult
 SVGMotionSMILAnimationFunction::SetRotate(const nsAString& aRotate,
                                           nsAttrValue& aResult)
 {