785017 - Use NS_GetStaticAtom in svg content where appropriate

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

The do_GetAtom issue from Bug 783915 comment 11 issue occurs in various places in svg content.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 654663 [details] [diff] [review]
patch

>+++ b/content/svg/content/src/SVGLength.cpp
>     char *theRest = unit;
>     if (*unit != '\0' && !IsSVGWhitespace(*unit)) {
>       while (*theRest != '\0' && !IsSVGWhitespace(*theRest)) {
>         ++theRest;
>       }
[...]
>     } else {
>       tmpUnit = nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER;
>     }

While you're touching this code -- we can drop that "if" check and its "else" block, right?  If the if-check would fail, then the while loop is going to be a no-op, and the GetUnitTypeForString() call will do the right thing (return SVG_LENGTHTYPE_NUMBER right away, since we're passing in the empty-string).

>+          if (valAtom) {
>+            rv = booleanInfo.mBooleans[i].SetBaseValueAtom(valAtom, this);
>+            if (NS_FAILED(rv)) {
>+              booleanInfo.Reset(i);
>+            } else {
>+              aResult.SetTo(valAtom);
>+              didSetResult = true;
>+            }
>+          } else {
>             booleanInfo.Reset(i);

So, this duplicates the failure-case code there (the Reset() call). I'd prefer to avoid that duplication, if possible.

This could simplify to...

  if (valAtom) {
    rv = booleanInfo.mBooleans[i].SetBaseValueAtom(valAtom, this);
  }
  if (!valAtom || NS_FAILED(rv)) {
    // everything after here remains as-is in current m-c

or alternately:
  if (valAtom && NS_SUCCEEDED(SetBaseValueAtom()) { success } else { fail }

>+++ b/content/svg/content/src/nsSVGLength2.cpp
>@@ -141,17 +142,18 @@ GetValueFromString(const nsAString &aVal
>   const char *str = value.get();
> 
>   if (NS_IsAsciiWhitespace(*str))
>     return NS_ERROR_DOM_SYNTAX_ERR;
>   

Aside: This contextual code probably wants to be using IsSVGWhitespace, not IsAsciiWhitespace... I filed bug 785140 on that, since it happens elsewhere, too.

r=me with the above (ignoring that last whitespace thing)

Comment 4

[Robert Longson [:longsonr]]

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

Comment 5

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3e0ae18274

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e3e0ae18274

Comment 7

[Daniel Holbert [:dholbert]]

(FWIW: https://hg.mozilla.org/integration/mozilla-inbound/rev/8247f7b038ce just landed for bug 783915, but it mistakenly had this bug's number in its commit message. So: anyone looking for information about that cset should instead hop over to bug 783915)