937614 - Simplify nsSMILParserUtils

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: parser.txt

Stops creating copies of strings to parse them in general.

Note that ParseNumber can return a very slightly different result to strtod but we generally don't care I did need to tweak the discrete slot calculation because of that though as the unscaled value is 28.9999999 when it was 29.0000000

Comment 1

[Daniel Holbert [:dholbert]]

So it looks like part of this patch is moving some SVGContentUtils methods to nsContentUtils.

Wouldn't it make more sense to just include SVGContentUtils.h in nsSMILParserUtils.cpp?

Also: If you wouldn't mind, it'd be much easier to review this if you could split the patch into several atomic pieces rather than one > 100K patch. (e.g. Hypothetically, if we *do* need the SVGContentUtils --> nsContentUtils move, that would make a nice atomic patch on its own -- moving that code and all of its callers. Not sure if there are any other obvious opportunities for subdivision here.)

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Wouldn't it make more sense to just include SVGContentUtils.h in
> nsSMILParserUtils.cpp?

(Alternately: if you're uncomfortable with nsSMILParserUtils depending on SVGContentUtils, maybe we could instead fork these methods out into SVGParserUtils.h/cpp, which nsSMILParserUtils would more reasonably depend on?)

Comment 3

[Daniel Holbert [:dholbert]]

Unpacking my hesitance on the nsContentUtils-tweak a bit:
 - nsContentUtils is huge, and this will make it huger.
 - This code is only used by SVG (and now SMIL).
 - Hence, I think it'd be better to keep it in an SVG-flavored file.

Tagging Brian for info -- Brian, I know you'd originally designed our SMIL code so that nothing in content/smil depended on content/svg.  And if we made nsSMILParserUtils depend on SVGContentUtils, that would technically violate that idea.  Would you be OK loosening that restriction in this case? (or would you prefer Robert's current strategy of moving all of this SVG(/SMIL)-specific number-parsing code out of SVGContentUtils and into the general nsContentUtils bucket? Or do you have any other ideas/thoughts on this?)

Comment 4

[Daniel Holbert [:dholbert]]

(In particular: IIRC, part of that original design was because SMIL was kind of its own spec, and it was conceivable that we might do some non-SVG SMIL stuff.  At this point, I think it's safe to say that that's not going to happen, so I think it's less problematic to have our SMIL code depend on SVG-specific headers.)

Comment 5

[Robert Longson [:longsonr]]

Attachment: leave the number parsing code where it is

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 831098 [details] [diff] [review]
leave the number parsing code where it is

Haven't gotten through all of this yet, but here's what I've got so far:

>+++ b/content/smil/nsSMILAnimationFunction.cpp
>   if (calcMode == CALC_DISCRETE || NS_FAILED(rv)) {
>-    double scaledSimpleProgress =
>-      ScaleSimpleProgress(simpleProgress, CALC_DISCRETE);
[...]
>     static const double kFloatingPointFudgeFactor = 1.0e-16;
>+    simpleProgress += kFloatingPointFudgeFactor;
>+
>+    double scaledSimpleProgress =
>+      ScaleSimpleProgress(simpleProgress, CALC_DISCRETE);
>+
>     if (scaledSimpleProgress + kFloatingPointFudgeFactor <= 1.0) {
>       scaledSimpleProgress += kFloatingPointFudgeFactor;
>     }

What's the purpose of this change?  It doesn't look related to the rest of this patch. If we need this (& I'm not clear why we do), perhaps it should happen separately?

>@@ -1049,27 +1036,20 @@ nsSMILAnimationFunction::UnsetKeySplines
> 
> nsresult
> nsSMILAnimationFunction::SetKeyTimes(const nsAString& aKeyTimes,
>                                      nsAttrValue& aResult)
> {
[...]
>-  nsresult rv =
>-    nsSMILParserUtils::ParseSemicolonDelimitedProgressList(aKeyTimes, true,
>-                                                           mKeyTimes);
>-
>-  if (NS_SUCCEEDED(rv) && mKeyTimes.Length() < 1)
>-    rv = NS_ERROR_FAILURE;
>-
>-  if (NS_FAILED(rv))
>-    mKeyTimes.Clear();
>-
>-  mHasChanged = true;
>+  if (nsSMILParserUtils::ParseSemicolonDelimitedProgressList(aKeyTimes, true,
>+                                                             mKeyTimes)) {
>+    mHasChanged = true;
>+  }
> 
>   return NS_OK;

It looks to me like this doesn't preserve the old code's "Clear on failure" behavior. (which is important in case ParseSemicolonDelimitedProgressList parses some valid values and then fails).

This also probably wants to be reshuffled to look like how you've got nsSMILAnimationFunction::SetKeySplines looking (early-return for failure, with a Clear() call in that clause.)

>+++ b/content/smil/nsSMILParserUtils.cpp
>+const nsDependentSubstring
>+nsSMILParserUtils::TrimWhitespace(const nsAString& aString)

It looks like this could be implemented with nsTString::Trim(). (modifying a shallow copy of |aString|, since aString is const)

>+++ b/content/svg/content/src/SVGContentUtils.h
>@@ -163,19 +180,29 @@ public:
[...]
>+  /**
>+   * Parse an integer of the form:
>+   * integer ::= [+-]? [0-9]+
>+   * Parsing fails if the number cannot be represented by an int32_t.
>+   */
>+  static bool ParseInteger(const nsAString& aString, int32_t& aValue);

This looks like it's supposed to match
 http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGContentUtils.h?mark=157-165#157
but its documentation doesn't quite match (in particular, it doesn't say that there must be nothing after the number).

Please fix the "Parsing fails" sentence to start as:
  Parsing fails if there is anything left over after the integer,
since that's the key differentiator for this version of ParseInteger.

Comment 7

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> What's the purpose of this change?  It doesn't look related to the rest of
> this patch. If we need this (& I'm not clear why we do), perhaps it should
> happen separately?

See comment 0. There are reftest failures without. Basically strtod is very slightly more accurate than parseNumber so when parsing 0.29 it returns 0.29000000001 whereas parseNumber returns 0.2899999997 so the discrete scale call makes it 0.28

> It looks to me like this doesn't preserve the old code's "Clear on failure"
> behavior. (which is important in case ParseSemicolonDelimitedProgressList
> parses some valid values and then fails).

OK

> It looks like this could be implemented with nsTString::Trim(). (modifying a
> shallow copy of |aString|, since aString is const)

Wouldn't be the right whitespace handling but I'll look at the implementation of Trim and see if I can adjust to be shallow.

> 
> Please fix the "Parsing fails" sentence to start as:
>   Parsing fails if there is anything left over after the integer,
> since that's the key differentiator for this version of ParseInteger.

OK

Comment 8

[Robert Longson [:longsonr]]

Trim is not a member of nsAString (looks like you need an nsAutoString or something for Trim)

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Unpacking my hesitance on the nsContentUtils-tweak a bit:
>  - nsContentUtils is huge, and this will make it huger.
>  - This code is only used by SVG (and now SMIL).
>  - Hence, I think it'd be better to keep it in an SVG-flavored file.
> 
> Tagging Brian for info -- Brian, I know you'd originally designed our SMIL
> code so that nothing in content/smil depended on content/svg.  And if we
> made nsSMILParserUtils depend on SVGContentUtils, that would technically
> violate that idea.  Would you be OK loosening that restriction in this case?
> (or would you prefer Robert's current strategy of moving all of this
> SVG(/SMIL)-specific number-parsing code out of SVGContentUtils and into the
> general nsContentUtils bucket? Or do you have any other ideas/thoughts on
> this?)

I *think* I'm ok with adding more coupling with SVG for this. There's a remote chance this animation stuff may be used in other contexts (specifically some timesheet contexts) but this seems like a pretty easy linkage to remove later if it becomes necessary.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 831098 [details] [diff] [review]
leave the number parsing code where it is

Was there something you specifically wanted feedback on?

Comment 11

[Robert Longson [:longsonr]]

Just the tighter coupling with the svg code, that's all really, unless you want to comment on anything of course.

Comment 12

[Robert Longson [:longsonr]]

An nsDependentSubstring only has 3 members doesn't it? A pointer (to the first character) a length and some flags. We're not creating any copies of the string data when we call TrimWhitespace.

Comment 13

[Robert Longson [:longsonr]]

Attachment: Address a couple of review comments

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #8)
> Trim is not a member of nsAString (looks like you need an nsAutoString or
> something for Trim)

Ah, right - looks like it's only on nsString for some reason. OK, I guess we can't use it after all.

> We're not creating any copies of the string data when we call TrimWhitespace.

Right, I wasn't worried about that, and the TrimWhitespace impl does look good to me. Just wanted to share code if possible. Sorry for the red herring.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #7)
> See comment 0. There are reftest failures without. Basically strtod is very
> slightly more accurate than parseNumber so when parsing 0.29 it returns
> 0.29000000001 whereas parseNumber returns 0.2899999997 so the discrete scale
> call makes it 0.28

So, a few thoughts on this:
 (a) For off-by-epsilon float-conversion issues, I'd rather add a fudge factor to the test than add one to the code. This failure here is just a case of us setting the threshold *barely* above 29, which won't have any visible effects when an animation is actually playing. (and which is a class of problems that's impossible to avoid in general)  (The existing fudge factor in the code there is for a different class of problem, IIRC, which makes us skip a value entirely rather than just reach it a microsecond too late.)

Anyway -- the test passes with your patch & without this code-fudge-factor if I change it to seek to 29.001 instead off 29.

 (b) I suspect we should fix ParseNumber to be more like PR_strtod, which might fix this particular testcase without the need for a fudge factor. In particular, it looks like PR_strtod just treats the decimal point as part of the exponent, and ignores it until the very end which appears to affect behavior, at least from my testing in GDB. For example, PR_strtod ends up parsing ".12" as:
  (1.0 * 10 + 2.0) / 100
whereas ParseNumber currently parses it more like this:
  (1.0 / 10 + 2.0 / 100)
In my GDB scratchpad and a quick local C++ testcase, the former expression evaluates to exactly 0.12, whereas the latter is 0.11999999999999998.

If you're up for fixing (b), that would be awesome -- though I think that should probably happen in its own patch (possibly in its own bug).

Comment 16

[Daniel Holbert [:dholbert]]

("the test" being anim-discrete-values-2.svg -- that's the test that fails on my system with your patch, at least)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 831532 [details] [diff] [review]
Address a couple of review comments

Sorry for the delays here -- this is quite a lot of code, quite rewritten, and it's a bit dense/complex (and parsers are notoriously tricky & sources of security issues), so I'm erring on the side of being meticulous.

Having said that, this generally looks great. Any commenting you want to add would be appreciated, to make the density/complexity a bit easier to skim through.

Here's my next batch of feedback. (still haven't gotten through the whole thing)
(I'm interested in your thoughts on comment 15, too.)
=====================================================

>+++ b/content/smil/nsSMILParserUtils.cpp
> inline bool
> IsSpace(const PRUnichar c)
> {
>   return (c == 0x9 || c == 0xA || c == 0xD || c == 0x20);

Now that we have SVGContentUtils.h included here, we don't need this function. Let's drop it and replace it with IsSVGWhitespace().

>+ParseMetric(RangedPtr<const PRUnichar>& aIter,
>+            const RangedPtr<const PRUnichar>& aEnd,
>+            uint32_t& aMultiplier)

Maybe name this "ParseClockMetric" or "ParseTimeMetric"?

('metric' on its own is a bit of a generic term, and note that this file has a lot of non-clock-related code, so the "clock" nature of this method isn't automatically implied.)

>+bool
>+nsSMILParserUtils::ParseKeySplines(const nsAString& aSpec,
>+                                   FallibleTArray<nsSMILKeySpline>& aKeySplines)
>+{
>+  nsCharSeparatedTokenizerTemplate<IsSpace> controlPointTokenizer(aSpec, ';');
>+  while(controlPointTokenizer.hasMoreTokens()) {

nit: insert space between "while" and "(".

Same goes for "switch(", twice in this file. (nsSMILParserUtils.cpp)

>+ParseClockValue(RangedPtr<const PRUnichar>& aIter,
>+                const RangedPtr<const PRUnichar>& aEnd,
>+                nsSMILTimeValue* aResult)
> {

Add a comment above this method definition with a link to http://www.w3.org/TR/SVG/animate.html#ClockValueSyntax

(The parsing strategy is a bit tricky to understand without that chunk of spec text handy).

>+  do {
>+    switch(*iter) {
>+    case ':':
>+       if (clockType == FULL_CLOCK_VALUE) {
>+         return false;
>+       }
>+       ++clockType;
>+       break;
[etc]

Add a comment above this ^ loop saying something like "First, scan over the whole value to determine what type of clock value to parse it as".

>+    case 'e':
>+    case 'E':
>+    case '-':
>+    case '+':
>+      // Exclude anything invalid that number parsing might otherwise allow.
>+      return false;

s/invalid/invalid (for clock values)/

(with newlines as appropriate, since this will probably push us over 80 chars)

>+      if (iter != aEnd &&
>+          (*iter != '.' || !SVGContentUtils::ParseNumber(iter, aEnd, fraction))) {

line too long -- rewrap.

>+      aResult->SetMillis((double)hours * MSEC_PER_HOUR + minutes * MSEC_PER_MIN +
>+                         NS_round((seconds + fraction) * MSEC_PER_SEC));

Same here. (Might help to declare a local helper-var "millis" which you then pass to SetMilis.)

Also: "(double)hours" should really be "(nsSMILTime)hours", i think.  (nsSMILTime is int64_t, and it's the type that SetMillis takes.)  Otherwise, you'll have floating-point math going on outside of the NS_round call, which isn't good.


>+      if (!ParseMetric(iter, aEnd, multiplier)) {
>+        return false;
>+      }
>+      aResult->SetMillis(NS_round((timecount + fraction) * multiplier));
>+      aIter = iter;
>+      return true;

I think we can just "return iter == aEnd;" here instead of true, and then the caller won't need to bother with that check.  (This is already implicitly done for ParseClockValue's other "return true" case -- there's an early-fail-return for "if iter == aEnd" just before that.)

The same applies to the ParseOffsetValue() impl too, I think.

>--- a/content/smil/nsSMILParserUtils.h
>+   * This method can actually parses an offset value as defined in the
>+   * SMIL Animation specification.
[...]

s/This method can actually parses an offset value/Parses an offset value/

(Looks like the extra words there are leftovers from the old header-comment, before you split this method up)

Comment 18

[Robert Longson [:longsonr]]

Attachment: address more review comments

The NumberParser change seems to fix the failing reftest at least on Windows.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 832899 [details] [diff] [review]
address more review comments

(In reply to Robert Longson from comment #18)
> The NumberParser change seems to fix the failing reftest at least on Windows.

Awesome!

Here's my next (probably second-to-last) batch of comments.

>+++ b/content/smil/nsSMILParserUtils.cpp
>+bool
>+ParseClockValue(RangedPtr<const PRUnichar>& aIter,
>+                const RangedPtr<const PRUnichar>& aEnd,
>+                nsSMILTimeValue* aResult)
[...]
>+      aResult->SetMillis((nsSMILTime)hours * MSEC_PER_HOUR +
>+                         minutes * MSEC_PER_MIN +
>+                         NS_round((seconds + fraction) * MSEC_PER_SEC));

The "seconds + fraction" addition there makes us do some unnecessary floating-point addition (and hence unnecessary precision-loss).

Let's just scale 'seconds' and 'fraction' independently, and then add them after we've rounded 'fraction'. -- i.e., split up that last line into these two lines:
                           seconds * MSEC_PER_SEC +
                           NS_round(fraction * MSEC_PER_SEC));

>+ParseOffsetValue(RangedPtr<const PRUnichar>& aIter,
>+                 const RangedPtr<const PRUnichar>& aEnd,
>+                 nsSMILTimeValue* aResult)
[...]
>+  int32_t sign = 1;
>+
>+  switch (*iter) {
>+  case '+':
>+    ++iter;
>+    break;
>+  case '-':
>+    sign = -1;
>+    ++iter;
>+    break;
>+  }

This could be shortened slightly (removing one "++iter" and one "break") if you reversed the case-order and used fall-through.

But really, this is doing the same thing as a chunk of code that you already have twice in SVGContentUtils::ParseNumber & ParseInteger, so I'd prefer that we just make all three spots share code, using a new SVGContentUtils method. (This could happen in a followup bug, if you want to punt on it for now.) I'm imagining something like:
  void ParseOptionalSign(RangedPtr<const PRUnichar>& aIter,
                         const RangedPtr<const PRUnichar>& aEnd,
                         int32_t& aSignMultiplier)

>+  while (iter != aEnd) {
>+    if (!IsSVGWhitespace(*iter)) {
>+      break;
>+    }
>+    ++iter;
>+  }

The first four lines there can be condensed to just
  while (iter != aEnd && IsSVGWhitespace(*iter)) {

BUT: let's split out this code into a helper-function, e.g. "SkipWhitespace()", since we have this pattern (or the single-line "while" condition variant) duplicated in at least four places in this file now -- namely:
 - ParseOffsetValue
 - ParseOptionalOffset
 - The first loop in TrimWhitespace (though that function deals with nsAString::const_iterator instead of RangedPtr, but it'd probably be worth switching over to RangedPtr for consistency anyway?)
 - CheckForNegativeNumber

This helper function should probably live up at the top of this file, above ParseColon, since that's around where the old function "SkipBeginWsp" lived, and this would be its new incarnation.

>+bool
> ParseAccessKey(const nsAString& aSpec, nsSMILTimeValueSpecParams& aResult)
[...]
>-  nsresult rv = ParseOptionalOffset(Substring(start, end), result);
>-  if (NS_FAILED(rv))
>-    return rv;
>-
>-  aResult = result;
>-
>-  return NS_OK;
>+  if (ParseOptionalOffset(iter, end, &result.mOffset) && iter == end) {
>+    aResult = result;
>+    return true;
>+  }
>+  return false;
> }

nit: So, Brian structured a lot of this code (including this function) to follow a pattern where the error cases or special cases are generally early-returns, and the normal "success" code-path takes the final return.

Let's preserve that pattern here.  So, flip the logic in that "if" check. (and swap the "return false" with the if-body).

>@@ -334,521 +409,330 @@ ParseElementBaseTimeValueSpec(const nsAS
>-  nsresult rv = ParseOptionalOffset(Substring(tokenEnd, specEnd), result);
>-  if (NS_SUCCEEDED(rv)) {
>+  if (ParseOptionalOffset(iter, end, &result.mOffset) && iter == end) {
>     aResult = result;
>+    return true;
>   }
> 
>-  return rv;
>+  return false;
> }

Same here. (end of ParseElementBaseTimeValueSpec)

>+bool
>+MoveToNextToken(RangedPtr<const PRUnichar>& aIter,
>+                const RangedPtr<const PRUnichar>& aEnd,
>+                bool aBreakOnDot)
> {
>-  const PRUnichar* tokenEnd = aStr.BeginReading();
>-  const PRUnichar* const end = aStr.EndReading();
>+  bool escapePresent = false;
>   bool escape = false;

The variable-naming here is confusing. Maybe name these bools "isAnyCharEscaped" vs. "isCurCharEscaped"? (I think those are basically what these are tracking.)

Also: Please add a header-comment here about the return value. (Generally, we've got boolean return values indicating whether the parsing is successful, whereas here it seems to indicate whether there's an escaped character anywhere. It's worth stating that explicitly; otherwise it looks like this function is requiring escape characters in order to "succeed", which is quite confusing.)

>+  while (aIter != aEnd) {
>+    if (IsSVGWhitespace(*aIter)) {
>+      return escapePresent;
>     }

Collapse to:
   // (Stop when we hit end-of-string or whitespace)
   while (aIter != aEnd && !IsSVGWhitespace(*aIter) {

When we break out of the loop, we hit the final "return escapePresent" anyway.

>+    if (!escape) {
>+      switch (*aIter) {
>+      case '.':
>+        if (aBreakOnDot) {
>+          return escapePresent;
>+        }
>+        break;
>+      case '+':
>+      case '-':
>+        return escapePresent;

Let's convert this switch() to "if" statements. There aren't too many values that we're switching over, anyway, so switch isn't really buying us any perf here -- and using "if" will give us the option to replace these duplicate "return" statements with "break" statements, so we can then take the single final "return escapePresent;" at the end of the method.

>+already_AddRefed<nsIAtom>
>+ConvertUnescapedTokenToAtom(const nsAString& aToken)
> {
>+  // Whether the token is an id-ref or event-symbol it should be a valid NCName
>+  if (NS_FAILED(nsContentUtils::CheckQName(aToken, false)))
>+    return nullptr;
>+  return do_GetAtom(aToken);
>+}
>+    
>+already_AddRefed<nsIAtom>
>+ConvertTokenToAtom(const nsAString& aToken,
>+                   bool aUnescapeToken)
>+{
>+  // Unescaping involves making a copy of the string which we'd like to avoid if possible
>+  if (aUnescapeToken) {

The bulk of this function is inside of this clause.

Let's flip the logic here (handling the !aUnescapeToken case first), to save on indentation here and also to avoid unnecessarily modifying the blame of the whole chunk of code inside this clause. (I know we don't care too much about modifying hg blame; but if it's easy (which I think is the case here), we might as well avoid it, for smaller/more understandable patches and for easier hg archeology.)

>+    nsAutoString token(aToken);
>+    const PRUnichar* read = token.BeginReading();
>+    const PRUnichar* const end = token.EndReading();
>+    PRUnichar* write = token.BeginWriting();

These should probably be RangedPtr<> now (particularly since we're writing into a string here, so running out of bounds would be very bad).  That doesn't have to happen here, though, particularly since this chunk of code isn't changing (if you de-indent it, per my previous comment).

So, maybe file a followup on making these RangedPtr?

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> >+bool
> >+MoveToNextToken(RangedPtr<const PRUnichar>& aIter,
> >+                const RangedPtr<const PRUnichar>& aEnd,
> >+                bool aBreakOnDot)
> > {
[...]
> Also: Please add a header-comment here about the return value [...]

...er, by "header-comment" I of course meant "comment above this function" (since it isn't listed in any header) :)

Comment 21

[Robert Longson [:longsonr]]

Attachment: address more review comments

nsSMILParserUtils::TrimWhitespace can be removed once we have the same whitespace characters in SVG and HTML. This will be part of SVG 2, Cameron has already raised it as an issue in the w3c bug tracker.

nsSMILParserUtils::CheckForNegativeNumber should be moved into nsSMILCSSValueType.cpp and fixed/removed as part of bug 501188 I can add a note to that bug but I don't really want to restructure it in this one.

So I've left both of these as BeginReading/EndReading

ConvertTokenToAtom could change as part of a followup but it's complicated, existing code and it works so should probably go in its own bug.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #21)
> So I've left both of these as BeginReading/EndReading

Sounds good.

> ConvertTokenToAtom could change as part of a followup but it's complicated,
> existing code and it works so should probably go in its own bug.

Agreed. (mind filing a followup on that?)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8334464 [details] [diff] [review]
address more review comments

>+++ b/content/smil/nsSMILParserUtils.cpp
>+ConvertTokenToAtom(const nsAString& aToken,
>+                   bool aUnescapeToken)
[...]
>+  token.SetLength(write - token.BeginReading());

Let's change this to call "Truncate" instead of "SetLength".

(Truncate is just a wrapper for SetLength, with an assertion (and a semantic implication for human readers) that we're not extending the string beyond its current length -- and that's nice here, since we definitely shouldn't be extending the string.)

>-  Unescape(token);
[...]
>-  if (token.IsEmpty())
>-    return NS_ERROR_FAILURE;
>+  if (start == end) {
>+    return false;

So previously, the string "\" would get unescaped to "", and then take this empty-string early-return.  Now, I think that string will end up giving us an empty-string atom (via ConvertTokenToAtom), and we'll set result.mDependentElemID to that empty-string atom. And that's bad.

Best way to address this is probably to add a |token.IsEmpty| check w/ early-return (returning null), near the end of the ConvertTokenToAtom() impl, just before the final return.

(This all applies to the atom for the second token, too, for the "element-name.event-symbol" clause. I think that atom could end up empty, too, but that'll also be fixed by the IsEmpty check that I'm suggesting above.)

>+  RangedPtr<const PRUnichar> iter(start);
>+  bool requiresUnescaping;
>+  MoveToNextToken(iter, end, true, requiresUnescaping);

Rename "iter" to "tokenEnd" or something like that, to be clearer about what it represents in this function (and to make it clear how it differs from |start|, which you also advance in this function).

>+    int32_t repeatValue;

Move this declaration into the only clause that uses it -- the REPEAT_PREFIX clause.

>   // We've reached the end of the token, so we should now be either looking at
>   // a '+', '-', or the end.
>-  const PRUnichar* specEnd = aSpec.EndReading();
>-  SkipBeginWsp(tokenEnd, specEnd);
>-
>-  nsresult rv = ParseOptionalOffset(Substring(tokenEnd, specEnd), result);
>-  if (NS_SUCCEEDED(rv)) {
>-    aResult = result;
>+  if (!ParseOptionalOffset(iter, end, &result.mOffset) || iter != end) {
>+    return false;

Since the SkipBeginWsp is no longer explicit here (it's now inside of ParseOptionalOffset()) -- tweak the comment to make it clearer that whitespace is OK here. e.g. replace:
  // a '+', '-', or the end.
...with...
  // a '+', '-' (possibly with whitespace before it), or the end.

> class SMILValueParser : public nsSMILParserUtils::GenericValueParser
> {
[...]
>-  virtual nsresult Parse(const nsAString& aValueStr) {
>+  virtual bool Parse(const nsAString& aValueStr) {

Add a MOZ_OVERRIDE annotation (before the opening curly-brace), while you're here.

(This family of classes should all be NS_STACK_CLASS, too -- I filed bug 941333 on doing that, but feel free to fix it here if you'd like.)

>   // wallclock type
>-  else if (StringBeginsWith(spec, WALLCLOCK_PREFIX)) {
>-    rv = NS_ERROR_NOT_IMPLEMENTED;
>+  if (StringBeginsWith(spec, WALLCLOCK_PREFIX)) {
>+    return true;

I'm pretty sure this "return true" wants to be replaced with:
  return false; // Wallclock times not implemented


>+++ b/content/smil/nsSMILParserUtils.h
>-   * @return NS_OK if aSpec was successfully parsed as a valid clock value
>-   * (according to aFlags), an error code otherwise.
>+   * @param aResult  The parsed result. [OUT]
>    */
[...]
>+  static bool ParseClockValue(const nsAString& aSpec,
>+                              nsSMILTimeValue* aResult);

Two things about this documentation:

(1) Let's not drop the @return documentation entirely here -- add that back, just saying e.g.
  * @return true if parsing succeeds, false otherwise.

(2) Please update the documentation for this function to state that, when this function succeeds, aResult is guaranteed to be a non-negative, definite time value (i.e. return true from IsDefinite). (it's non-negative because you have a first-pass scan for "-" characters (while counting ":" characters) and you bail if you find one, and it's definite because you only set the outparam via SetMillis).

(The guarantees in (2) are important to some callers -- see comments below for nsSMILTimedElement::SetMin & others.)

>+++ b/content/smil/nsSMILTimedElement.cpp
>+    // mSimpleDur should never be unresolved. ParseClockValue will either set
>+    // duration to resolved/indefinite/media or will return a failure code.
>+    NS_ABORT_IF_FALSE(duration.IsResolved(),
>+      "Setting unresolved simple duration");
>+  }
> 
>   mSimpleDur = duration;
>   UpdateCurrentInterval();

This comment (which the patch is currently just reindenting) is no longer quite correct. In particular, ParseClockValue doesn't handle indefinite or media anymore (it *only* returns definite time values now), and it doesn't return a "failure code" anymore -- just a bool.

ALSO: I think this comment & assertion are really supposed to be at the top-level scope (where they were originally) -- they're supposed to be a sanity-check on |duration| *right* before we commit its value to mSimpleDur. In your patch right now, they only sanity-check it in one particular control-flow, and not in the other.

So, please move this comment/assertion up one level of scope (after the curly-brace) and reword to adapt to the ParseClockValue() changes.

> nsSMILTimedElement::SetMin(const nsAString& aMinSpec)
> {
[...]
>+    if (!nsSMILParserUtils::ParseClockValue(min, &duration) ||
>+        !duration.IsDefinite() ||
>+        duration.GetMillis() < 0L) {
>+      mMin.SetMillis(0L);
>+      return NS_ERROR_FAILURE;
>+    }

You don't need the "duration.IsDefinite()" or the < 0 checks anymore -- your changes to ParseClockValue mean we can assume that both of those checks are satisfied.

(Maybe worth converting those into an assertion, though)

> nsSMILTimedElement::SetMax(const nsAString& aMaxSpec)
>+    if (!nsSMILParserUtils::ParseClockValue(max, &duration) ||
>+        !duration.IsResolved() ||
>+        duration.GetMillis() <= 0L) {
>+      mMax.SetIndefinite();
>+      return NS_ERROR_FAILURE;

The above almost applies here (in SetMax), too, except we *do* need a zero-check in this case. (since the "max" attribute can't be 0)  But we can drop the IsDefinite check ( / convert it to an assertion), at least.

(Technically the SVG spec says "min" also can't be 0, but I think that's a spec bug; see http://lists.w3.org/Archives/Public/www-svg/2013Nov/0126.html )

> nsresult
> nsSMILTimedElement::SetRepeatDur(const nsAString& aRepeatDurSpec)
> {
>+    if (!nsSMILParserUtils::ParseClockValue(repeatDur, &duration) ||
>+        !duration.IsResolved()) {

No need for the IsResolved check here. I think parseClockValue already was guaranteed to return something resolved, and with your simplifications it's definitely guaranteed (since it can only set its outparam to a definite time-value (via SetMillis)).

>diff --git a/content/svg/content/src/SVGContentUtils.h b/content/svg/content/src/SVGContentUtils.h
>+  /**
>+   * Parses the sign (+ or -) of a number and moves aIter to the next
>+   * character if a sign is found.
>+   * @param aSignMultiplier -1 if the sign is negative otherwise 1
>+   */
>+  static inline bool
>+  ParseOptionalSign(mozilla::RangedPtr<const PRUnichar>& aIter,
>+                    const mozilla::RangedPtr<const PRUnichar>& aEnd,
>+                    int32_t& aSignMultiplier)
>+  {

In the comment:
 s/aSignMultiplier/aSignMultiplier [outparam]/
...or something like that, to make that outparam-ness clearer in the documentation.

And a brief explanation of the return value, too, to make the callers' early-returns for ParseOptionalSign()-failure make more sense. Maybe:
   * @return false if we hit the end of the string (i.e. if aIter is initially
   *         at aEnd, or if we reach aEnd right after the sign character).

>+++ b/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp
> SVGMotionSMILAnimationFunction::SetKeyPoints(const nsAString& aKeyPoints,
>                                              nsAttrValue& aResult)
> {
[...]
>-  if (NS_SUCCEEDED(rv) && mKeyPoints.Length() < 1)
>-    rv = NS_ERROR_FAILURE;
>-
>-  if (NS_FAILED(rv)) {
>-    mKeyPoints.Clear();
>+  if (nsSMILParserUtils::ParseSemicolonDelimitedProgressList(aKeyPoints, false,
>+                                                             mKeyPoints) &&
>+      mKeyPoints.Length() >= 1) {
>+    mHasChanged = true;
>   }
> 
>   return NS_OK;

Previously, this function would return NS_ERROR_FAILURE if parsing failed. But with your patch, it always returns NS_OK.

I think we need to preserve the old "return NS_ERROR_FAILURE" behavior on parse failures, though, right? Otherwise we won't be able to report parse errors.

r=me with the above addressed. (Thanks again for taking this on and for being patient with my piecewise review! :) )

Comment 24

[Robert Longson [:longsonr]]

Attachment: updated to pass reftests and with MOZ_STACK_CLASS

I had to change numberParser to make it more accurate with floats as the divide at the end strategy introduces new reftest failures. All numberParsing works on doubles internally and then converts at the end. There are a few other tweaks too.

Comment 25

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=79f60f2953fb

Comment 26

[Robert Longson [:longsonr]]

Attachment: without indenting change

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8336932 [details] [diff] [review]
without indenting change

>+template<class floatType>
>+bool
>+SVGContentUtils::ParseNumber(RangedPtr<const PRUnichar>& aIter,
>+                             const RangedPtr<const PRUnichar>& aEnd,
>+                             floatType& aValue)
>+{
>+  double value;
>+  if (!::ParseNumber(aIter, aEnd, value)) {
>     return false;
>   }
>-  aIter = iter;
>-  aValue = value;
>+  floatType floatValue = floatType(value);
>+  if (!NS_finite(floatValue)) {
>+    return false;
>+  }
>+  aValue = floatValue;
>   return true;

Minor bug here -- when we take the NS_finite failure case, we will have *incorrectly* updated the outparam |aIter|, even though we're returning false. (because ::ParseNumber() will have already updated |aIter|, before we realize that we're going to fail.).

I think you just need to make a local copy of |aIter| here, and pass that *copy* to ::ParseNumber(). And then only set |aIter| at the end, in the same spot where you set |aValue|.

(It surprises me a bit that this doesn't break any of our tests... Maybe just because we usually bail out entirely when we hit a ParseNumber failure, for most attributes? If you can think of a test that would break as a result of this bug, that would be great, but I also don't want to gate this review / this patch on that.)

Anyway: r=me with that fixed.

Comment 28

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34c84bda8feb

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34c84bda8feb