705584 - Improve/increase use of nsCharSeparatedTokenizerTemplate in nsSMILParserUtils

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

I was just looking through nsSMILParserUtils, and I noticed a few things about its tokenizing:

 (1) nsSMILParserUtils::ParseValuesGeneric uses nsCharSeparatedTokenizer, when it
      should be using nsCharSeparatedTokenizerTemplate<IsSVGWhitespace> instead.

 (2) At first glance, it looks like 
     nsSMILParserUtils::ParseSemicolonDelimitedProgressList does ad-hoc tokenizing,
     and it could benefit from using nsCharSeparatedTokenizerTemplate. 
     (or if it can't for some reason, we should add a comment saying why it can't)

Comment 1

[Robert Longson [:longsonr]]

Attachment: token.txt

Comment 2

[Daniel Holbert [:dholbert]]

Thanks for picking this up! 

> nsresult
> nsSMILParserUtils::ParseSemicolonDelimitedProgressList(const nsAString& aSpec,
>                                                        bool aNonDecreasing,
>                                                        nsTArray<double>& aArray)
> {
>   nsresult rv = NS_OK;

This variable is now unused, after your patch, and it triggers a FAIL_ON_WARNINGS compile failure on my machine. (It should trigger a build error on MSVC, too, if you build with ac_add_options --enable-warnings-as-errors, which you proably want to add to your mozconfig if it's not there already.)

So: remove that variable.

More coming later; this is all I've got from my "does it build" and quick-skim sanity-check. :)

Comment 3

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 798444 [details] [diff] [review]
address review comment

>diff --git a/content/smil/nsSMILParserUtils.cpp b/content/smil/nsSMILParserUtils.cpp
> nsSMILParserUtils::ParseSemicolonDelimitedProgressList(const nsAString& aSpec,
>                                                        bool aNonDecreasing,
>                                                        nsTArray<double>& aArray)
> {
[...]
>+  while (tokenizer.hasMoreTokens()) {
>+    NS_ConvertUTF16toUTF8 utf8Token(tokenizer.nextToken());
>+    const char *token = utf8Token.get();
>+    if (*token == '\0') {
>+      return NS_ERROR_FAILURE;
>+    }

Add a comment, for readability, like we have at
 http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGIntegerPair.cpp#41
e.g. 
   // empty string (e.g. two ';' in a row),

(At first, I read this as checking whether we'd hit the end of the source-string; I didn't immediately know that each utf8Token was null-terminated. A comment clears this up and makes the check clearer.)

r=me with that. Thanks!

Comment 5

[Robert Longson [:longsonr]]

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

Comment 6

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/809ec333407e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/809ec333407e