835219 - Does nsSVGMarkerElement need to override GetAttr?

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Robert Longson [: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.

Comment 2

[Jonathan Watt [:jwatt]]

I think that's what we should do.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Robert Longson [:longsonr]]

Ah yes, I think you're right.

Comment 5

[Jonathan Watt [:jwatt]]

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

Comment 6

[Jonathan Watt [:jwatt]]

It passed fine, BTW.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Heck, here's the patch.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: 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.

Comment 9

[Jonathan Watt [:jwatt]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4207c7cf71a8
http://hg.mozilla.org/integration/mozilla-inbound/rev/220a6ba702cb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4207c7cf71a8
https://hg.mozilla.org/mozilla-central/rev/220a6ba702cb