Bug 1423098 - Ignore invalid time value specifications rather than failing; r=dholbert
authorBrian Birtles <birtles@gmail.com>
Thu, 14 Dec 2017 09:58:24 -0600
changeset 448235 b8fdaff3f0af552fd37376be81da201054b26feb
parent 448234 6111cfdd4f938c15e8f8e165b40e31bcac5c849c
child 448236 6f8ed92515d60015cd14f25429f1a944a216bbcc
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1423098
milestone59.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 1423098 - Ignore invalid time value specifications rather than failing; r=dholbert This matches the behavior of Chrome at least, and should reduce compatibility risk when we remove accessKey support in the next patch in this series by ensuring that specifications such as begin="3s; accessKey(s)" will continue to run the animation. MozReview-Commit-ID: E6sh48yrYSm
dom/smil/nsSMILTimedElement.cpp
dom/smil/test/test_smilTiming.xhtml
--- a/dom/smil/nsSMILTimedElement.cpp
+++ b/dom/smil/nsSMILTimedElement.cpp
@@ -1319,31 +1319,33 @@ nsSMILTimedElement::SetBeginOrEndSpec(co
 
   AutoIntervalUpdateBatcher updateBatcher(*this);
 
   nsCharSeparatedTokenizer tokenizer(aSpec, ';');
   if (!tokenizer.hasMoreTokens()) { // Empty list
     return NS_ERROR_FAILURE;
   }
 
-  nsresult rv = NS_OK;
-  while (tokenizer.hasMoreTokens() && NS_SUCCEEDED(rv)) {
+  bool hadFailure = false;
+  while (tokenizer.hasMoreTokens()) {
     nsAutoPtr<nsSMILTimeValueSpec>
       spec(new nsSMILTimeValueSpec(*this, aIsBegin));
-    rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
+    nsresult rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
     if (NS_SUCCEEDED(rv)) {
       timeSpecsList.AppendElement(spec.forget());
+    } else {
+      hadFailure = true;
     }
   }
 
-  if (NS_FAILED(rv)) {
-    ClearSpecs(timeSpecsList, instances, aRemove);
-  }
-
-  return rv;
+  // The return value from this function is only used to determine if we should
+  // print a console message or not, so we return failure if we had one or more
+  // failures but we don't need to differentiate between different types of
+  // failures or the number of failures.
+  return hadFailure ? NS_ERROR_FAILURE : NS_OK;
 }
 
 namespace
 {
   // Adaptor functor for RemoveInstanceTimes that allows us to use function
   // pointers instead.
   // Without this we'd have to either templatize ClearSpecs and all its callers
   // or pass bool flags around to specify which removal function to use here.
--- a/dom/smil/test/test_smilTiming.xhtml
+++ b/dom/smil/test/test_smilTiming.xhtml
@@ -127,20 +127,20 @@ function main() {
   testCases.push(StartTimeTest('05:40\u30D5', 'none'));
   testCases.push(StartTimeTest('05:40β', 'none'));
 
   // List syntax
   testCases.push(StartTimeTest('3', 3));
   testCases.push(StartTimeTest('3;', 3));
   testCases.push(StartTimeTest('3; ', 3));
   testCases.push(StartTimeTest('3 ; ', 3));
-  testCases.push(StartTimeTest('3;;', 'none'));
-  testCases.push(StartTimeTest('3;; ', 'none'));
-  testCases.push(StartTimeTest(';3', 'none'));
-  testCases.push(StartTimeTest(' ;3', 'none'));
+  testCases.push(StartTimeTest('3;;', 3));
+  testCases.push(StartTimeTest('3;; ', 3));
+  testCases.push(StartTimeTest(';3', 3));
+  testCases.push(StartTimeTest(' ;3', 3));
   testCases.push(StartTimeTest('3;4', 3));
   testCases.push(StartTimeTest(' 3 ; 4 ', 3));
 
   // List syntax on end times
   testCases.push({
     'attr' : { 'begin': '0s',
                'end': '1s; 2s' },
     'times': [ [ 0, 0 ],