bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Does nsSVGMarkerElement need to override GetAttr?

RESOLVED FIXED in mozilla21



5 years ago
5 years ago


(Reporter: bz, Assigned: jwatt)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(2 attachments)

It looks like ever since the fix for bug 395616 this override hasn't been needed, unless I'm missing something.  Can we just remove it?

Note that this is one of the main blockers towards devirtualizing GetAttr, as far as I can tell.
Flags: needinfo?(longsonr)
At the moment it does. <marker> elements have an orient attribute (http://www.w3.org/TR/SVG/painting.html#MarkerElement). This can either be an angle which we model with nsSVGAngle or the special value auto. 

The GetAttr implementation allows the marker orient to be serialised as auto when necessary.

What we could do is create a SVGOrient class that is derived from nsSVGAngle but also allows auto to be stored/serialised.
Flags: needinfo?(longsonr)

Comment 2

5 years ago
I think that's what we should do.
But in nsSVGMarkerElement::ParseAttribute if aName == orient and aValue == "auto" we set aResult to the aValue string.

Which means that in that case the nsAttrValue just stores the string "auto", not an nsSVGAngle, no?  So the normal GetAttr on Element seems like it should do the right thing...

Am I just missing something?
Flags: needinfo?(longsonr)
Ah yes, I think you're right.
Flags: needinfo?(longsonr)

Comment 5

5 years ago
FWIW I have a build running to pass over the mochitests/reftests. We seem to have sufficient coverage of 'auto'.

Comment 6

5 years ago
It passed fine, BTW.

Comment 7

5 years ago
Created attachment 707543 [details] [diff] [review]

Heck, here's the patch.
Attachment #707543 - Flags: review?


5 years ago
Attachment #707543 - Flags: review? → review?(longsonr)
Attachment #707543 - Flags: review?(longsonr) → review+
Assignee: nobody → jwatt
OS: Mac OS X → All
Hardware: x86 → All

Comment 8

5 years ago
Created attachment 708031 [details] [diff] [review]
Add mochitest coverage

Actually it seems that we don't explicitly test this now that I look more closely at the tests. Here's a patch to make sure we do.
Attachment #708031 - Flags: review?(longsonr)
Attachment #708031 - Flags: review?(longsonr) → review+
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.