Last Comment Bug 785017 - Use NS_GetStaticAtom in svg content where appropriate
: Use NS_GetStaticAtom in svg content where appropriate
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Robert Longson
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-23 02:20 PDT by Robert Longson
Modified: 2012-08-25 09:44 PDT (History)
1 user (show)
longsonr: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.40 KB, patch)
2012-08-23 09:37 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Robert Longson 2012-08-23 02:20:43 PDT

    
Comment 1 Robert Longson 2012-08-23 02:22:10 PDT
The do_GetAtom issue from Bug 783915 comment 11 issue occurs in various places in svg content.
Comment 2 Robert Longson 2012-08-23 09:37:07 PDT
Created attachment 654663 [details] [diff] [review]
patch
Comment 3 Daniel Holbert [:dholbert] 2012-08-23 11:01:30 PDT
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 2012-08-24 00:30:17 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c20d760a5581
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:04:05 PDT
https://hg.mozilla.org/mozilla-central/rev/8e3e0ae18274
Comment 7 Daniel Holbert [:dholbert] 2012-08-25 09:44:29 PDT
(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)

Note You need to log in before you can comment on or make changes to this bug.