Closed Bug 835219 Opened 7 years ago Closed 7 years ago

Does nsSVGMarkerElement need to override GetAttr?

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bzbarsky, Assigned: jwatt)

References

Details

Attachments

(2 files)

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)
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)
FWIW I have a build running to pass over the mochitests/reftests. We seem to have sufficient coverage of 'auto'.
It passed fine, BTW.
Attached patch patchSplinter Review
Heck, here's the patch.
Attachment #707543 - Flags: review?
Attachment #707543 - Flags: review? → review?(longsonr)
Attachment #707543 - Flags: review?(longsonr) → review+
Assignee: nobody → jwatt
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 836050
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+
https://hg.mozilla.org/mozilla-central/rev/4207c7cf71a8
https://hg.mozilla.org/mozilla-central/rev/220a6ba702cb
Status: NEW → RESOLVED
Closed: 7 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.