Use NS_GetStaticAtom in svg content where appropriate

RESOLVED FIXED in mozilla17

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
The do_GetAtom issue from Bug 783915 comment 11 issue occurs in various places in svg content.
Assignee: nobody → longsonr
(Assignee)

Updated

5 years ago
Summary: Use NS_GetStaticAtom in svg content where → Use NS_GetStaticAtom in svg content where appropriate
(Assignee)

Comment 2

5 years ago
Created attachment 654663 [details] [diff] [review]
patch
Attachment #654663 - Flags: review?(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)
Attachment #654663 - Flags: review?(dholbert) → review+
(Assignee)

Comment 4

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c20d760a5581
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3e0ae18274
Flags: in-testsuite-
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/8e3e0ae18274
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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)
You need to log in before you can comment on or make changes to this bug.