929011 - Simplify nsSVGDataParser

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: parser2.txt

The data parser is used for parsing paths and transforms. At the moment it makes a utf8 copy of the string it's given thereby using 50% more memory to parse. This converts parsing to parsing the UTF16 string directly.

Implementation notes...

The string to be parsed is now passed in the constructor so that the iterators can be initialised there.

Methods generally return true/false rather than an nsresult. Some that can't fail have benn made void.

SkipWsp() replaces

while (IsTokenWspStarter()) {
  ENSURE_MATCHED(MatchWsp());
}

since MatchWsp() could never fail if preceded by a IsTokenWspStarter() test.

Similarly for SkipCommaWsp()

GetNextToken is replaced by direct reading of the token and the various token iterators and classifiers (mInputPos, mTokenPos, mTokenType) have been collapsed into a single mTokenPos iterator

Comment 1

[Robert Longson [:longsonr]]

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

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 819818 [details] [diff] [review]
parser2.txt

Review feedback, part 1:

>+++ b/content/svg/content/src/SVGContentUtils.h
>   /**
>    * Parse a number of the form:
>    * number ::= integer ([Ee] integer)? | [+-]? [0-9]* "." [0-9]+ ([Ee] integer)?
>    * Parsing fails if the number cannot be represented by a floatType.
>-   * Anything after the number is returned in aLeftOver.
>    */
>   template<class floatType>
>   static bool
>-  ParseNumber(const nsAString& aString, floatType& aValue,
>-              nsAString& aLeftOver);
>+  ParseNumber(mozilla::RangedPtr<const PRUnichar>& aIter,
>+              const mozilla::RangedPtr<const PRUnichar>& aEnd,
>+              floatType& aValue);

Please update the documentation here, at least to mention how |aIter| will be updated, since that's a new thing. (Ideally it'd be great to have snazzy @param/@return documentation, but I'm also OK keeping it prose for now, too.)

>diff --git a/content/svg/content/src/SVGLength.cpp b/content/svg/content/src/SVGLength.cpp
>+  mozilla::RangedPtr<const PRUnichar> iter(aString.Data(), aString.Length());
>+  const mozilla::RangedPtr<const PRUnichar> end(aString.Data() + aString.Length(),
>+                                                aString.Data(), aString.Length());

Firstly: Drop the "mozilla::" prefix in this file & this patch's other .cpp files. (And instead, add "using namespace mozilla;" at the top, if it's not there already.)

Secondly: Hmm... It's a bit ugly to have this boilerplate all over the place, with several copies of Data() / Length(), some of which are added, some of which are args. :-/  Not easy to read, and very easy to subtly get wrong.

Could we abstract this into helper-functions in SVGContentUtils? Something like:
 RangedPtr<const PRUnichar> iter =
   SVGContentUtils::GetStartRangedPtr(aString);
 const RangedPtr<const PRUnichar> end =
   SVGContentUtils::GetEndRangedPtr(aString);

That would be much more foolproof & much easier to read, IMHO.

(These might even belong as methods on the nsAString API, ultimately.  But for now, SVGContentUtils iswould be a reasonable place for them, since they'll benefit every client of the updated SVGContentUtils::ParseNumber API.)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 819818 [details] [diff] [review]
parser2.txt

Review feedback, part 2:

> void nsSVGDataParser::GetNextToken()
> {
>-  mTokenPos = mInputPos;
>-  mTokenVal = *mInputPos;
>-  
>-  switch (mTokenVal) {
[...]
>+  if (mTokenPos != mEnd) {
>+    ++mTokenPos;
>   }

This method needs a better name. The current name is misleading on two levels:
 (1) "Get" is incorrect -- it returns void and has no outparams, so it's not a "getter" in any sense.
 (2) "Token" is incorrect -- it doesn't do any tokenizing. It just increments a pointer by 1 -- not not up to any "next token", for any common definition of "token".

(This gripe might actually apply to every usage of "Token" in this family of classes, actually... I'm not sure yet.)

So. How about we rename this to something like "SkipChar()" or "AdvancePosition()" or something like that?

>+bool nsSVGDataParser::IsTokenWspStarter() const
> {
[...]
>+  return mTokenPos != mEnd && IsSVGWhitespace(*mTokenPos);

I'm not a huge fan of this function-name, either... "WspStarter" is a weird descriptive word. (and "token" is problematic here, too -- a "token" should *not* start with whitespace, by definition -- at least, according to the normal definition of token that I'm familiar with.)

BUT: We don't need to come up with a better name, because as it turns out, this method only has one caller (in SkipWsp()). So the method can just be inlined into its caller.

SO: let's just drop this method entirely and move its contents directly into SkipWsp(). :)

>+++ b/content/svg/content/src/nsSVGDataParser.h
>   // helpers
>   void GetNextToken();
>-  void RewindTo(const char* aPos);
>-  virtual nsresult Match()=0;
>+  virtual bool Match()=0;

Might be worth separating out "Match()" into its own section in this header-file (i.e. maybe move it before the "// helpers" comment) since it's special & different from the rest of the functions in this chunk. (It's special because it's the thing that actually does the interesting parsing work, and (unlike everything else) it's explicitly not implemented in this parent class.)

>+  void SkipWsp(); 
                   ^
Delete end-of-line whitespace.

Comment 4

[Robert Longson [:longsonr]]

Attachment: address review comments so far

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 821114 [details] [diff] [review]
address review comments so far

>+++ b/content/svg/content/src/SVGPointList.cpp
>@@ -55,33 +55,37 @@ SVGPointList::SetValueFromString(const n
>+    const nsAString& token = tokenizer.nextToken();
>+    mozilla::RangedPtr<const PRUnichar> iter(token.Data(), token.Length());
>+    const mozilla::RangedPtr<const PRUnichar> end(token.Data() + token.Length(),
>+                                                  token.Data(), token.Length());

Looks like this spot could benefit from for the "drop mozilla::" treatment, as well as the "use GetStartRangedPtr & GetEndRangedPtr" treatment.

>+++ b/content/svg/content/src/nsSVGDataParser.cpp
[...]
>+nsSVGDataParser::nsSVGDataParser(const nsAString& aValue)
>+  : mTokenPos(aValue.Data(), aValue.Length()),
>+    mEnd(aValue.Data() + aValue.Length(), aValue.Data(),
>+         aValue.Length())
> {

(Seems like we might as well call GetStartRangedPtr/GetEndRangedPtr here as well (to init mTokenPos & mEnd) for brevity, consistency, & a more-foolproof argument list)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 821114 [details] [diff] [review]
address review comments so far

[calling previous comment "Review Feedback, Part 3"]

Review Feedback, Part 4:

So, this updated patch gives me this link error (on Ubuntu 13.10, with GCC 4.8 :-/  (I'll bet Try probably hits this as well):
{
/usr/bin/ld.gold.real: error: obj/toolkit/library/../../content/svg/content/src/nsSVGAngle.o: requires dynamic R_X86_64_PC32 reloc against '_ZN15SVGContentUtils17GetStartRangedPtrERK18nsAString_internal' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: obj/toolkit/library/../../content/svg/content/src/nsSVGLength2.o: requires dynamic R_X86_64_PC32 reloc against '_ZN15SVGContentUtils17GetStartRangedPtrERK18nsAString_internal' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: obj/toolkit/library/../../content/svg/content/src/nsSVGNumber2.o: requires dynamic R_X86_64_PC32 reloc against '_ZN15SVGContentUtils17GetStartRangedPtrERK18nsAString_internal' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: obj/toolkit/library/../../content/svg/content/src/SVGContentUtils.o: requires dynamic R_X86_64_PC32 reloc against '_ZN15SVGContentUtils17GetStartRangedPtrERK18nsAString_internal' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: obj/toolkit/library/../../content/svg/content/src/SVGContentUtils.o: requires dynamic R_X86_64_PC32 reloc against '_ZN15SVGContentUtils17GetStartRangedPtrERK18nsAString_internal' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: obj/toolkit/library/../../content/svg/content/src/SVGContentUtils.o: requires dynamic R_X86_64_PC32 reloc against '_ZN15SVGContentUtils17GetStartRangedPtrERK18nsAString_internal' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: obj/toolkit/library/../../content/svg/content/src/SVGLength.o: requires dynamic R_X86_64_PC32 reloc against '_ZN15SVGContentUtils17GetStartRangedPtrERK18nsAString_internal' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: read-only segment has dynamic relocations
/mozilla/content/svg/content/src/nsSVGAngle.cpp:102: error: undefined reference to 'SVGContentUtils::GetStartRangedPtr(nsAString_internal const&)'
/mozilla/content/svg/content/src/nsSVGAngle.cpp:104: error: undefined reference to 'SVGContentUtils::GetEndRangedPtr(nsAString_internal const&)'
/mozilla/content/svg/content/src/nsSVGLength2.cpp:130: error: undefined reference to 'SVGContentUtils::GetStartRangedPtr(nsAString_internal const&)'
/mozilla/content/svg/content/src/nsSVGLength2.cpp:132: error: undefined reference to 'SVGContentUtils::GetEndRangedPtr(nsAString_internal const&)'
/mozilla/content/svg/content/src/nsSVGNumber2.cpp:57: error: undefined reference to 'SVGContentUtils::GetStartRangedPtr(nsAString_internal const&)'
/mozilla/content/svg/content/src/nsSVGNumber2.cpp:59: error: undefined reference to 'SVGContentUtils::GetEndRangedPtr(nsAString_internal const&)'
/mozilla/content/svg/content/src/SVGContentUtils.cpp:517: error: undefined reference to 'SVGContentUtils::GetStartRangedPtr(nsAString_internal const&)'
/mozilla/content/svg/content/src/SVGContentUtils.cpp:518: error: undefined reference to 'SVGContentUtils::GetEndRangedPtr(nsAString_internal const&)'
}

Not sure what's going on there, but we need to sort it out.

>+++ b/content/svg/content/src/SVGTransformListParser.h
>+  SVGTransformListParser(const nsAString& aValue)
>+    : nsSVGDataParser(aValue) {}
>+  
  ^^ drop whitespace-on-blank-line

> private:
>-  nsTArray<nsSVGTransform> mTransforms;
>+  virtual bool Match() MOZ_OVERRIDE;
>+
>+  FallibleTArray<nsSVGTransform> mTransforms;
> 
>   // helpers
>-  virtual nsresult Match() MOZ_OVERRIDE;
>+  bool MatchNumberArguments(float *aResult,
>+                            uint32_t aMaxNum,
>+                            uint32_t *aParsedNum);
> 

Let's keep methods & data separated; so, keep mTransforms before "Match()" there (or bump mTransforms to the end; I think that's the more common convention. But either way, methods/data/methods/data intermixing is confusing & worth avoiding.

(The same applies to nsSVGDataParser.h & its member-vars vs. Match(), too.)

>+++ b/content/svg/content/src/nsSVGDataParser.cpp
>+bool nsSVGDataParser::MatchNumber(float& aX)
> {
>+  RangedPtr<const PRUnichar> iter(mTokenPos);
>   
>+  if (SVGContentUtils::ParseNumber(iter, mEnd, aX)) {
>+    mTokenPos = iter;
>+    return true;
>   }
>+  return false;
>+}

This made me realize that (with this patch) SVGContentUtils::ParseNumber is unclear about what happens to its outparams (particularly aIter) when it fails. (The documentation doesn't say, and really we just end up leaving it wherever it was when we fail.)

I'd rather we clearly define this, probably to say the outparams are only modified if we succeed, which we can easily achieve by just using a local variable for the number & iter inside of ParseNumber, and only setting the outparams to those variables when we're sure we're going to succeed. So the last few lines of ::ParseNumber would then no longer be...
  return NS_finite(aValue);
...but instead would be something like:
  if (!NS_finite(value)) {
    return false;
  }

  // If we get here, we were successful! Set our outparams.
  aIter = iter;
  aValue = value;
  return true;
(with a corresponding documentation-tweak)

Then, the nsSVGDataParser::MatchNumber() impl quoted above can do away with its own 'iter' variable and logic, since it can rely on the now-well-defined ParseNumber nehavior.  It can just become a one-liner, directly returning SVGContentUtils::ParseNumber(mTokenPos, mEnd, aX).

>+void nsSVGDataParser::SkipCommaWsp()
>+{
>+  SkipWsp();
>+  if (mTokenPos != mEnd && *mTokenPos == ',') {
>+    ++mTokenPos;
>+    SkipWsp();
[...]
>+void nsSVGDataParser::SkipWsp()
> {
>+  while (mTokenPos != mEnd && IsSVGWhitespace(*mTokenPos)) {
>+    ++mTokenPos;
>+  }

Observation: We could shave off a few redundant "mTokenPos != mEnd" comparisons in SkipCommaWsp and in the callers of both of these methods by making them return false if they hit the end of the string. (This would mean e.g. SkipCommaWsp wouldn't explicitly need the first half of its "if" check.)

Maybe worth a followup?

(It's also fine as-is (and maybe more readable as-is, at the cost of a little redundancy).)

Comment 7

[Daniel Holbert [:dholbert]]

(Ah -- the build error turns out to be easy to fix -- we're just missing a "SVGContentUtils::" prefix on the implementations of GetStartRangedPtr and GetEndRangedPtr. Thank goodness. :))

Comment 8

[Robert Longson [:longsonr]]

I've fixed that in my local copy already, I was writing it as we were having our conversation but it takes 15 minutes for me to link so I just posted what I had.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 821114 [details] [diff] [review]
address review comments so far

Comments on nsSVGPathDataParser:

>+++ b/content/svg/content/src/nsSVGPathDataParser.cpp
>-nsresult nsSVGPathDataParser::Match()
>+bool nsSVGPathDataParser::Match()
> {
>   return MatchSvgPath();
> }

High-level nit: as long as you're changing all of these function signatures in the .cpp file, it'd be nice to put the return type on its own line, per current mozilla C++ coding convention. But not a huge deal if you don't get to that.

Also, possible followup bug: we might as well just merge MatchSvgPath() into this one-liner Match() function. (There's real point in having nsFooParser::Match() be a one liner that invokes MatchFoo(); it's already clear that FooParser::Match should be the place where we Match Foo.) (I think this basically applies to the corresponding transform parser function, too.)

>+bool nsSVGPathDataParser::HasAbsCoords() const
> {
>+  return toupper(*mTokenPos) == *mTokenPos;
>+}

So this new function "HasAbsCoords" is named a bit vaguely. (unclear *what* 'has abs coords' (and where)).

Maybe rename this "IsTokenAbsCoordCommand()"? (Matching the other IsToken* function names.) (I'm disregarding the "token" naming issue for now, since per IRC convo we're not going to do that here.)

>+bool nsSVGPathDataParser::IsTokenNumberStarter() const
>+{
>+  return mTokenPos != mEnd && 
                               ^
Delete end-of-line whitespace -'

>+bool nsSVGPathDataParser::MatchSvgPath()
>+{
>+  SkipWsp();
>+
>+  if (IsTokenSubPathStarter()) {
>+    if (!MatchSubPaths()) {
>+      return false;
>+    }
>+  }
>+
>+  SkipWsp();
>+
>+  return true;
>+}

Why do we want the IsTokenSubPathStarter() check there?

It looks to me like, if we have an invalid path (one that doesn't start with "m" or "M"), that check makes us end up returning true from this function, which seems wrong.  In contrast, if we were to remove this IsTokenSubPathStarter() check, we'd return false (I think), which makes more sense to me.

On the other side of this coin: I think MatchMoveto() has a (currently) never-failing and hence of unnecessary IsTokenSubPathStarter() check. (never-failing because all of the callers, like this (indirect) one, check IsTokenSubPathStarter() before they call it, at least right now.)  I'm not sure if that call is useful, either (maybe it will be useful, if we can remove the one here?)

>+bool nsSVGPathDataParser::MatchSubPaths()
>+{
[...]
>+ while (true) {
>+    RangedPtr<const PRUnichar> saved(mTokenPos);
>+
>+    SkipWsp();
>+
>+    if (IsTokenSubPathStarter()) {
>+      if (!MatchSubPath()) {
>+        return false;
>+      }
>+    } else {
>+      mTokenPos = saved;
>+      return true;
>+    }
>+  }
>+}

(nit: "while" needs 1 more space of indentation there)

I don't think the "IsTokenSubPathStarter()" check is useful here, either.  In fact, if it's even possible it could fail here, then that's bad, because then we'd loop forever and never return, right? (So hopefully it's guaranteed to return true in this spot... :))

>+bool nsSVGPathDataParser::MatchMoveto()
>+{
[...]
>+  bool absCoords = HasAbsCoords();
>+  
  ^^
Delete whitespace on blank line.

>+bool nsSVGPathDataParser::MatchMovetoArgSeq(bool absCoords)
[...]
>+  if (IsTokenLinetoArgSeqStarter()) {
>+    return MatchLinetoArgSeq(absCoords);

Why do we have Lineto function calls inside of this Moveto function? Probably worth at least calling it out with an explanatory comment, assuming it's OK.

>+bool nsSVGPathDataParser::MatchHorizontalLinetoArgSeq(bool absCoords)
>+{
>+  while (true) {
>+    float x;
>+    if (!MatchNumber(x)) {
>+      return false;
>+    }
[...]
>+    RangedPtr<const PRUnichar> saved(mTokenPos);
>+
>+    SkipCommaWsp();
>+
>+    if (!IsTokenNumberStarter()) {
>+      mTokenPos = saved;
>+      return true;
>+    }
>+  }

Why do we bother with "saved" and that IsTokenNumberStarter() check here? It seems like that only lets us roll back to a position before some comma/whitespace, and I'm not sure what problem that prevents.

If we add a toplevel bool that tracks whether we parsed at least one number successfully, we could just return that bool in the "MatchNumber" failure-case, I think (and then we'd only need that one return clause).

>+bool nsSVGPathDataParser::MatchSubPathElement()
>+{
>+  switch (tolower(*mTokenPos)) {
>+    case 'z':
>+      return MatchClosePath();
>+    case 'l':
>+      return MatchLineto();
>+    case 'h':

So the cascade of calls behind each of these Match$COMMAND() functions make up the bulk of this file, with a lot of duplicated logic, it seems.  It would be great to condense those (and de-duplicate the logic) using templates (or maybe macros), so we don't have to audit the same logic over and over, per-function-set.

If you don't mind doing that in this bug here, it'd make me feel a lot more confident about reviewing this. (Since it's easier to audit one copy of logic rather than many.)
  return MatchCommand<'l', MatchCoordPair, StoreLineTo>();
or possibly
  return MatchCommand<'l', MatchCoordPair, StoreLineTo,
                       IsTokenNumberStarter>();
(where those args are "command", "function to call to match

>+bool
>+nsSVGPathDataParserToInternal::Parse()
> {
>   mPathSegList->Clear();
>+  return nsSVGPathDataParser::Parse();
> }

Hmm. Can we drop this override?  It's kind of a footgun [1] and it looks like it's basically unnecessary [2].

[1] It's overriding a non-virtual superclass method, which I think means we can't use MOZ_OVERRIDE to enforce that we haven't made a typo in the function-signature. (and which also means we have to be careful not to call this via a parent-class pointer, which we don't do right now but we could conceivably do in the future)

[2] It only has two callers; one of which passes a just-initialized (already clear) SVGPathData, and one of which doesn't but could (by calling Clear() just before) (or alternately we could move the Clear() call to the constructor, or to Match() maybe -- that's where it the equivalent call lives in SVGTransformListParser. Possibly both of those Clear() calls want to move to their constructor.)

Anyway -- assuming you agree about this, maybe file a followup to remove this? (or here is good too)

> class nsSVGPathDataParserToInternal : public nsSVGPathDataParser
> {
> public:

This is the only subclass of nsSVGPathDataParser, and AFAICT we only ever instantiate the subclass (the "ToInternal" one), so it's not really clear why we bother with making these two classes.  Is there a good reason??  Maybe it's a historical artifact? (If there is a reason, it'd be great to have a comment explaining the division, if you understand it. Or if not, maybe file a followup on merging this into the base class?)

>+  nsSVGPathDataParserToInternal(const nsAString& aValue,
>+                                mozilla::SVGPathData* aList)
>+    : nsSVGPathDataParser(aValue),
>+      mPathSegList(aList)
>   {}

Assert that mPathSegList is non-null in the constructor here (and per my comment above RE Parse(), either assert that it IsEmpty() here, or in the followup for parse-removal)

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> Also, possible followup bug: we might as well just merge MatchSvgPath() into
> this one-liner Match() function. (There's real point in having
> nsFooParser::Match() be a one liner that invokes MatchFoo();

(Sorry, I meant "there's _no_ real point")

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 821114 [details] [diff] [review]
address review comments so far

Comments on SVGTransformListParser:

>+bool
> SVGTransformListParser::MatchTransformList()
> {
>-  MatchWsp();
>+  SkipWsp();
> 
>   if (IsTokenTransformStarter()) {
>-    ENSURE_MATCHED(MatchTransforms());
>+    if (!MatchTransforms()) {

Similar to the path data parser, I'm unclear on what good the IsTokenTransformStarter() check here does. It has the useful feature of meaning we'll accept a whitespace-only string as valid (i.e. we'll return true), but it also looks to me like we'd return true for a string like "IAmNotATransform".

Though I might not've thought through all the implications, it seems like this function should be structured more like:
  SkipWsp(); // Skip leading whitespace
  if (DidWeHitTheEnd()) {
    return true; // accept whitespace-only strings
  }
  if (!MatchTransforms()) {
    return false; // We have something non-wsp, and it's not a transform. BAIL
  }
  SkipWsp(); // Skip trailing whitespace
 
  return DidWeHitTheEnd(); // bail if there's more bogus stuff 

(Note that this doesn't require any IsTokenTransformStarter() calls.)
(I suspect we'd want something like that structure for the analogous path function, too....?)

>+bool
> SVGTransformListParser::MatchTransforms()
> {
[...]
>     if (IsTokenTransformStarter()) {
>+      if (!MatchTransform()) {
>+        return false;
>+      }
>     } else {
>+      mTokenPos = saved;
>       break;

Can we just bump the if (!MatchTransform) check up one level (replacing IsTokenTransformStarter)? It seems redundant right now.  But if it's necessary, a brief comment might help clarify the structure.

(Side note: if we do end being able to drop IsTokenTransformStarter() here and also above in MatchTransformList, then we can (happily) rid of that function altogether, which means we can also drop the "aAdvancePos" arg for GetTransformToken and just assume that it's true, since that's the only value it ever gets.)

>+bool
> SVGTransformListParser::GetTransformToken(nsIAtom** aKeyAtom,
>                                           bool aAdvancePos)
> {
>+  if (mTokenPos == mEnd || !isalpha(*mTokenPos)) {
>+    return false;
>   }
> 
>-  nsresult rv = NS_OK;
>+  RangedPtr<const PRUnichar> iter(mTokenPos);
>+  do {
>+    ++iter;
>+  } while (iter != mEnd && isalpha(*iter));

You can simplify the error-check here a bit.
 (a) Move it *after* the loop
 (b) convert the loop to just be "while"
 (c) Change the early-return condition to "iter == mTokenPos"

So it ends up:
  RangedPtr<const PRUnichar> iter(mTokenPos);
  while (iter != mEnd && isalpha(*iter)) {
    ++iter;
  } 
  if (iter == mTokenPos) {
    // iter didn't advance --> no alphabetic characters at mTokenPos. Fail.
    return false;
  }

This will remove duplicate code for mEnd/isalpha checking, and it's IMHO a simpler (one-part instead of two-part) way to think about the early-return.


>+  const nsAString& transform = Substring(mTokenPos.get(), iter.get());
>+  *aKeyAtom = NS_NewAtom(transform).get();

Optimization follow-up-bug opportunity: so in the pathological case here where someone types a gazillion-character-long "aaaaaaa....aaaaa" in a transform string, we'll currently scan to the end of all of those a's, attempt to allocate an atom, and then do an equality-check, which is kind of wasteful since none of our transform commands are anywhere near that long. We could bail out much sooner (and avoid that silly allocation).  The longest transform command is only 9 characters long ("translate"), so we could bail out of the loop once we've advanced 10 positions. (or maybe 20 for good measure) (We have to allow ourselves to go *at least* 1 step too far, since we need to make sure that the string e.g. "translateaaaa" fails to match. If stopped after 9 characters, we'd mistakenly match. Though maybe it doesn't matter because we'd blow up on the "aaa" after that anyway.)

>+  *aKeyAtom = NS_NewAtom(transform).get();
[...]
>+  return *aKeyAtom != nullptr;

The NS_NewAtom API says it never returns null:
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIAtom.idl#122
so no need to null-check it here -- just return true.

>-nsresult
>+bool
> SVGTransformListParser::MatchNumberArguments(float *aResult,
>                                              uint32_t aMaxNum,
>                                              uint32_t *aParsedNum)

So, I know I agreed to "renaming can come later" on IRC. :) But please (here or in a followup):
 s/aParsedNum/aParsedCount/
(and a corresponding s/aMaxNum/aMaxCount/ I guess?)

Since this is a number parsing function, aParsedNum *really* sounds like "the number that I parsed for you", which it most definitely is not. :)

Comment 12

[Robert Longson [:longsonr]]

Actually we should be using NS_GetStaticAtom here instead as these are all known atoms.

Comment 13

[Daniel Holbert [:dholbert]]

Even better.  That should address the allocation aspect of my optimization-followup suggestion, which is probably the most important part.

Comment 14

[Robert Longson [:longsonr]]

Attachment: parser2.txt

I've done most things except templating. The code is different now so that doesn't make so much sense I think.

I've renamed everything too as it became so radically different there didn't seem much point not to in the end.

Comment 15

[Robert Longson [:longsonr]]

Attachment: updated

update to tip and fix test failures

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 822786 [details] [diff] [review]
updated

(I partially reviewed the previous patch version on a flight just now, and then switched to this one for some more of this review. I diffed the patches and I think all of my comments here apply, but apologies if any of this is now obsolete)

BTW: excellent work here; it's *so* great to see all this old code getting condensed & simplified. :)

>+++ b/content/svg/content/src/SVGContentUtils.cpp
>+  bool finite = NS_finite(aValue);
>+  if (finite) {
>+    aIter = iter;
>+  }
>+  return finite;
> }

We should treat aValue the same as aIter here (i.e. only set it on success return conditions, from a local var named e.g. 'value') (I had this in my sample code in comment 6, but didn't call it out - sorry for being unclear on that.)

(With that changing, let's do away with the 'finite' var and just have an early-return for the !NS_finite(value) case, followed by outparam-setting and return true.)

>+++ b/content/svg/content/src/nsSVGDataParser.cpp
>+bool
>+nsSVGDataParser::SkipCommaWsp()
> {
>+  if (SkipWsp()) {
>+    if (*mIter == ',') {
>+      ++mIter;
>+      return SkipWsp();
>+    }
>+    return true;
>   }
>+  return false;

Maybe consider flattening out the nestedness here (clearer control flow IMHO, but what you have is fine too). If you like, maybe something like:

  if (!SkipWsp()) {
    return false; // hit end of string during run of wsp
  }
  if (*mIter != ',') {
    return true; // non-comma/wsp character after a run of wsp
  }
  ++mIter; // skip comma
  return SkipWsp();

>+bool
>+nsSVGDataParser::SkipWsp()
> {
[...]
>+  while (mIter != mEnd && IsSVGWhitespace(*mIter)) {
>+    ++mIter;
>+  }
>+  return mIter != mEnd;

The mIter != mEnd check at the end there is redundant; let's restructure to avoid that. The following structure avoids this extra check (albeit with a few extra lines from splitting up your "while" condition and return statement):

  while (mIter != mEnd) {
    if (!IsSVGWhitespace(*mIter)) {
      return true;
    }
    ++mIter;
  }
  return false;

(the idea being "return true as soon as we hit a non-whitespace character (or false if we hit the end first)")

Also: please document (even just with a one-line comment) the meaning of the bool return value of these two Skip() functions

>+++ b/content/svg/content/src/SVGTransformListParser.cpp
>+SVGTransformListParser::ParseTransforms()
> {
[...]
>+  SkipWsp();
>+
>+  while (mIter != mEnd) {
[...]
>+    SkipWsp();
>+  }

It looks like this structure is equivalent to just:
 while (SkipWsp()) {
   ...
 }

Please simplify to that.

>+bool
>+SVGTransformListParser::ParseTransform()
>+{
[...]
>+  if (keyAtom == nullptr || !SkipWsp()) {
>+    return false;
>+  }

s/keyAtom == nullptr/!keyAtom/

>+SVGTransformListParser::ParseArguments(float* aResult,
>+                                       uint32_t aMaxCount,
>+                                       uint32_t* aParsedCount)
> {
>+  if (!SkipWsp()) {
>+    return false;
>   }

I'm pretty sure ParseArguments doesn't need this initial SkipWsp(). (ParseArguments gets called, indirectly, by ParseTransform(), which did a SkipWsp() before its Parse$COMMAND calls (and those calls invoke ParseArguments right away).

So I think we should convert this check to a MOZ_ASSERT.

>+  SkipWsp();
>+
>+  while (mIter != mEnd && *mIter != ')' && *aParsedCount < aMaxCount) {
[...]
>+    SkipWsp();
>+  }

Similar to above, this can be condensed by removing both of those SkipWsp() calls, and replacing the mEnd check with SkipWsp() in the loop condition.

>+  if (mIter != mEnd && *mIter == ')') {
>+    ++mIter;
>+    return true;
>+  }

I'd like to avoid checking for that ')' char in two separate places. Let's move this entire "if" block (without the 'end' check) up to be the first thing that happens in the loop, and drop the paren-check from the loop condition.

(And then the "*aParsedCount < aMaxCount" check will also need to be converted into an explicit early-return clause in the loop-body, right after that the paren early-return clause, I think.)

>+++ b/content/svg/content/src/SVGTransformListParser.h
> class nsIAtom;
> 
> namespace mozilla {

SVGTransformListParser.h doesn't need this nsIAtom forward-declare -- it doesn't use nsIAtom at all.  Let's drop that, while you're here.

>+++ b/content/svg/content/src/nsSVGPathDataParser.cpp
>+bool
>+nsSVGPathDataParser::ParseMoveto()
> {
>+  if (mIter == mEnd || (*mIter != 'm' && *mIter != 'M')) {
>+    return false;
>   }

Looks to me like the callers already ensure that mIter != mEnd.

So, let's convert that to an assertion (unless I'm missing something).

>+  SkipWsp();
>+
>+  // Per SVG 1.1 Section 8.3.2
>+  // If a moveto is followed by multiple pairs of coordinates,
>+  // the subsequent pairs are treated as implicit lineto commands
>+  ParseLineto(absCoords);
>+  return true;

(Ah, thanks for that explanatory comment)

But: do we really want to return true even if ParseLineTo fails? (e.g. say we have an odd number of coordinates, so ParseLineTo ends up failing on a straggling single-coord at the end. Presumably that should cause a parse failure, right?)

I think we really want something like:
  if (!SkipWsp()) {
    return true;
  }
  if (isalpha(*mIter)) {
    return true;
  }
  return ParseLineto(absCoords);

...to allow us to treat "real" parse failures as failures, while not penalizing ourselves when there's just another full path command. (which of course would fail to parse as a line)

>+bool
>+nsSVGPathDataParser::ParseLineto(bool absCoords)
> {
[...]
>+    if (NS_FAILED(mPathSegList->AppendSeg(
>+                   absCoords ? PATHSEG_LINETO_ABS : PATHSEG_LINETO_REL, x, y))) {

Over 80 chars. Maybe wrap "x" to next line? Or use a helper-var to store the PATHSEG_LINETO_* value (whose value would be set up above this "if" check).

(This actually applies to most of the post-AppendSeg lines in this patch.)

I'm also happy to ignore this and deal with it in a followup, in the interests of getting the rest of this patch landed.

>+    SkipWsp();
>+
>+    if (mIter != mEnd && isalpha(*mIter)) {

Let SkipWsp do the end-check for you. i.e.: 

  if (SkipWsp() && isalpha(*mIter)) {

(This chunk of code is repeated in most of the Parse$PATH_COMMAND functions, it looks like; please fix it in the various spot.)

>+  while (true) {
>+    float x, y, x1, y1, x2, y2;

nit: bump "x, y" to the end of this list, for consistency with the ordering you use in the two other places where these variables are listed in this function.

(This applies to nearly all of the Parse*Curveto functions, as well as ParseEllipticalArc.)

> bool
> nsSVGArcConverter::GetNextSegment(gfxPoint *cp1, gfxPoint *cp2, gfxPoint *to)
> {
>   if (mSegIndex == mNumSegs) {
>     return false;
>   }
>-  
>+ 
  ^
This change is replacing two space characters with one, on an empty line. :) Either go all the way (0 spaces) or just revert this part. :)

>+++ b/content/svg/content/src/nsSVGPathDataParser.h
>+  nsSVGPathDataParser(const nsAString& aValue,
>+                      mozilla::SVGPathData* aList)
>+    : nsSVGDataParser(aValue),
>+      mPathSegList(aList)
>+  {}

This still needs an assertion that mPathSegList is non-null.

>+  mozilla::SVGPathData *mPathSegList;

(While we're at it, declare mPathSegList as type
  mozilla::SVGPathData* const
to enforce that we don't retarget the pointer elsewhere.)

Comment 17

[Robert Longson [:longsonr]]

Attachment: address more review comments

Comment 18

[Robert Longson [:longsonr]]

Attachment: Make robust agains being given unicode and fix try failures

Comment 19

[Robert Longson [:longsonr]]

Attachment: fix typo in ToUpper

Comment 20

[Robert Longson [:longsonr]]

Seems to fix bug 792058 too.

Comment 21

[Robert Longson [:longsonr]]

Unless of course something else fixed it in the meantime. I haven't tried a trunk build without bug 929011 on bug 792058

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 822874 [details] [diff] [review]
fix typo in ToUpper

>+++ b/content/svg/content/src/nsSVGPathDataParser.cpp
>+bool
>+nsSVGPathDataParser::ParseSubPathElements()
>+{
>+  while (SkipWsp()) {
>+    PRUnichar command = *mIter;
>+
>+    RangedPtr<const PRUnichar> saved(mIter);
>+
>+    // Upper case commands have absolute co-ordinates,
>+    // lower case commands have relative co-ordinates.
>+    bool absCoords = ToUpper(command) == command;
>+
>+    ++mIter;
[...]
>+    switch (ToUpper(command)) {

Let's store the result of ToUpper in a local var, rather than running ToUpper twice here.

>+      default:
>+        mIter = saved;
>+        // Start of a new command
>+        return IsAlpha(*mIter);
>+    }

What's going on with "saved" and this default case? Isn't 'saved' pointing to the letter (command) that we're already switching across here?  What's the use in checking whether it IsAlpha() when we've already determined that it's not any of our recognized commands?

I suspect this clause is really letting us return gracefully when we hit a new "M" (the start of a new sub-path), right? If so, we should make that explicit with an early-return check before we even advance mIter; then we can do away with 'saved' and make the default case an actual unrecognized-character-return-false case.

Other than that, this is r=me, but I'll just mark feedback+ for the moment because I'm interested to see how this ^ sorts out. Thanks!

Comment 23

[Robert Longson [:longsonr]]

> What's going on with "saved" and this default case? Isn't 'saved' pointing
> to the letter (command) that we're already switching across here?  What's
> the use in checking whether it IsAlpha() when we've already determined that
> it's not any of our recognized commands?

It's either invalid or a move.

> 
> I suspect this clause is really letting us return gracefully when we hit a
> new "M" (the start of a new sub-path), right? If so, we should make that
> explicit with an early-return check before we even advance mIter; then we
> can do away with 'saved' and make the default case an actual
> unrecognized-character-return-false case.

I suppose, that means this method now knows what a move is so we're testing for moves in two places.

Comment 24

[Robert Longson [:longsonr]]

Attachment: it looks like this

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #23)
> I suppose, that means this method now knows what a move is so we're testing
> for moves in two places.

Yeah, good point... This new way still feels a bit cleaner to me, I think, though.

But if you prefer the previous patch's strategy, I'd be OK with that as well, with a code-comment better-explaining what's going on with the default case there, and with the default case just being "return true;" instead of bothering with IsAlpha. (since the impending ParseMoveTo is going to reject any non-alpha characters anyway as being non-"M".)

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 823643 [details] [diff] [review]
it looks like this

r=me on this, anyway. Thanks!

Comment 27

[Robert Longson [:longsonr]]

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

Comment 28

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6165af87e7

Comment 29

[Carsten Book [:Tomcat]]

(In reply to Robert Longson from comment #28)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6165af87e7

Hi,

had to back this out since it caused B2g Build Bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=29837652&tree=Mozilla-Inbound

Comment 30

[Robert Longson [:longsonr]]

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

Comment 31

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf5c8adaf14

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fcf5c8adaf14