340835 - Allow new-world SVG data types to be stored in nsAttrValue

Status: RESOLVED DUPLICATE of bug 629200

(Core :: SVG, defect)

Description

[tor]

The previous heavyweight SVG data types stored a nsISupports based object in nsAttrValue with the nsISVGValue interface.  We would like to do the same for the redone versions to allow us to avoid always storing the string of an attribute value.  As the new data types are members of the parent element rather than dynamically allocated, we need to store a pointer to this in nsAttrValue.  Jonas, is it a safe assumption that nsAttrValues will not outlive the corresponding element, or do they have sharing or other complications?

Comment 1

[tor]

Attachment: store new svg data types in nsAttrValue

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 224870 [details] [diff] [review]
store new svg data types in nsAttrValue

Yes, that is a safe assumption. The nsAttrValue lifetime is decided by the nsAttrAndChildArray, which has the same lifetime as the element (the array dies first).

However, why do we need two svg types? Could nsSVGValue2 replace nsISVGValue entierly, or at least in nsAttrValue?

r=me

Comment 3

[tor]

Eventually we want to reach one svg type, but at the moment we have a mix in the tree.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 224870 [details] [diff] [review]
store new svg data types in nsAttrValue

Ok, cool

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This will make nsSVGLength2 grow one vtable pointer. I don't like that.

If we always know the element and attribute name for the nsAttrValue we're looking at, nsAttrValue doesn't need anything other than the type "I'm the SVG data!" and then we can get ask the SVG element to give us the string value from its internal data.

Comment 6

[tor]

Attachment: alternate approach by overloading GetAttr

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 226159 [details] [diff] [review]
alternate approach by overloading GetAttr

I don't like this. First of all I think it'll break cloning, and there are probably other things that go directly to the nsAttrAndChildArray. Mutation events is another thing that comes to mind.

In general we've gone down the path of unserializable nsAttrValues before, and it was very nice to get rid of that.

Could you subclass nsSVGLength2 specifically for when they need to be put in nsAttrValue?

Comment 8

[tor]

Another approach would be to add the new types directly to nsAttrValue, avoiding the need to add a virtual method.

We need to decide whether it's worthwhile doing this mild normalization of attributes, which gives us a benefit in terms of memory footprint, but breaks DOM round-tripping of attribute values.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> We need to decide whether it's worthwhile doing this mild normalization of
> attributes, which gives us a benefit in terms of memory footprint, but breaks
> DOM round-tripping of attribute values.

This is really an independent question. I think you should take it up with the SVG WG.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, I guess it's not independent in the sense that if the WG says "you have to be able to round-trip 100% accurately" then we don't need to resolve this bug.

Comment 12

[Jonathan Watt [:jwatt]]

Under nsSVGElement::DidChangeXxx 
we currently serialize the internal structure, then pass the serialization to SettAttr, which parses it, then repopulates the internal structures. That's pretty bad, especially in the case of list types such as SVGAnimatedLengthList and SVGAnimatedPathSegList. I'd really like to have an nsAttrValue type that, when the internal objects change, we can tell "throw away the string and, if someone asks for it later, serialize on demand".

I think that we do still want to keep the original strings unless the SVG DOM structure is changed, since content authors don't like attributes to be normalized unnecessarily.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we do something very similar for style attributes. For the exact same reason.

Comment 14

[Jonathan Watt [:jwatt]]

Thanks for the tip.