Open Bug 361920 Opened 13 years ago Updated 5 years ago

Incorrect unit conversion for SVGLength created through createSVGLength

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

REOPENED

People

(Reporter: sharparrow1, Unassigned)

Details

Attachments

(1 file, 4 obsolete files)

(Originally reported in m.d.t.svg)

len = document.getElementById("svg").createSVGLength();

len.newValueSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_MM, "30");
 -> len.unitType = 7
 -> len.value = 30 (Should be about 8)
 -> len.valueInSpecifiedUnits = 30
 -> len.valueAsString = 30mm

len.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_PX);
 -> len.unitType = 5
 -> len.value = 30 (Should be about 8)
 -> len.valueInSpecifiedUnits = 30 (Should be about 8)
 -> len.valueAsString = 30px (Should be about 8px)

Of course, the exact correct number depends on your screen's dpi.

This test causes the following warning to show up:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGLength.cpp&rev=1.23&mark=519#519

The basic fix is to just assign a context into lengths in the createSVGLength implementation.  I don't know if it's worth fixing on the branch, because there is an easy workaround, which is to use pixelUnitToMillimeterX to do conversions yourself.  But it is an easy and safe fix.

I think that unit conversion in SVG should be cleaned up, though. The code could be greatly simplified by just using a single float to store the pixel/mm ratio rather than having all these contexts.  Since the dpi is the same in the x and y directions and it is always constant for any device, it shouldn't be so complicated.
createSVGLength still uses the classic nsSVGLength rather than the simplified nsSVGLength2.  There's also work in progress in simplifying the coord context stuff, bug 353172.

Attached patch patch (obsolete) — Splinter Review
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #363587 - Flags: superreview?(roc)
Attachment #363587 - Flags: review?(roc)
Attached patch updated patch (obsolete) — Splinter Review
Make logic more obvious by creating an IsOutermostSVGElement method. Also fixes incorrect logic for foreignObject in previous patch.
Attachment #363587 - Attachment is obsolete: true
Attachment #363608 - Flags: superreview?(roc)
Attachment #363608 - Flags: review?(roc)
Attachment #363587 - Flags: superreview?(roc)
Attachment #363587 - Flags: review?(roc)
I think we need cycle collecting goo for DOMSVGLength and DOMSVGNumber. One of these could be stored as a JS property on its SVG element and then I think we'd have a cycle.

Why don't we just make GetCtx return 'this' for the outermost <svg>?
Attached patch updated patch (obsolete) — Splinter Review
(In reply to comment #4)
> I think we need cycle collecting goo for DOMSVGLength and DOMSVGNumber. One of
> these could be stored as a JS property on its SVG element and then I think we'd
> have a cycle.

I've done that for DOMSVGLength which you're right did need it but DOMSVGNumber doesn't have a mSVGElement member so I don't think there's a cycle. DOMSVGAngle wasn't converted by the cycle collection bug.

> 
> Why don't we just make GetCtx return 'this' for the outermost <svg>?

It's more efficient my way as you have to check for having an outermost SVG element in most cases anyway before you call GetCtx (where you now check for an outermost SVG element again). I've changed it per your suggestion so you can see what it looks like but I think it was better the way I had it.
Attachment #363608 - Attachment is obsolete: true
Attachment #363611 - Flags: superreview?(roc)
Attachment #363611 - Flags: review?(roc)
Attachment #363608 - Flags: superreview?(roc)
Attachment #363608 - Flags: review?(roc)
(In reply to comment #5)
> (In reply to comment #4)
> > I think we need cycle collecting goo for DOMSVGLength and DOMSVGNumber. One of
> > these could be stored as a JS property on its SVG element and then I think
> > we'd have a cycle.
> 
> I've done that for DOMSVGLength which you're right did need it but DOMSVGNumber
> doesn't have a mSVGElement member so I don't think there's a cycle. DOMSVGAngle
> wasn't converted by the cycle collection bug.

Of course, good call.

> > Why don't we just make GetCtx return 'this' for the outermost <svg>?
> 
> It's more efficient my way as you have to check for having an outermost SVG
> element in most cases anyway before you call GetCtx (where you now check for an
> outermost SVG element again). I've changed it per your suggestion so you can
> see what it looks like but I think it was better the way I had it.

Why not make GetCtx virtual, and override it in nsSVGSVGElement to avoid the tag check and the casts? Then in nsSVGSVGElement::GetViewboxToViewportTransform and nsSVGSVGElement::GetLength you could just call GetCtx and if it returns 'this', do the mViewportWidth/Height thing.
Attached patch updated patch (obsolete) — Splinter Review
Attachment #363616 - Attachment description: upd → updated patch
Attachment #363616 - Attachment is patch: true
Attachment #363616 - Flags: superreview?(roc)
Attachment #363616 - Flags: review?(roc)
Comment on attachment 363616 [details] [diff] [review]
updated patch

(In reply to comment #6)
> 
> Why not make GetCtx virtual, and override it in nsSVGSVGElement to avoid the
> tag check and the casts? Then in nsSVGSVGElement::GetViewboxToViewportTransform
> and nsSVGSVGElement::GetLength you could just call GetCtx and if it returns
> 'this', do the mViewportWidth/Height thing.

Great idea, done.
Attachment #363611 - Attachment is obsolete: true
Attachment #363611 - Flags: superreview?(roc)
Attachment #363611 - Flags: review?(roc)
Attachment #363616 - Flags: superreview?(roc)
Attachment #363616 - Flags: superreview+
Attachment #363616 - Flags: review?(roc)
Attachment #363616 - Flags: review+
checked in http://hg.mozilla.org/mozilla-central/rev/288d7890c276
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 413975
Blocks: 342513
Correct me if I'm wrong, but with this change there is no longer an nsISVGLength type exposed to javascript which means you can't add anything to an nsSVGAnimatedLengthList.
Oops meant nsSVGLengthList not the animated one
Additionally CreateSVGNumber is the only way to create a new SVGNumber in scripts. It needs to return something that inherits from nsISVGValue so it can properly notify an nsSVGNumberList part of an nsSVGAnimatedNumberList that is an attribute of an element.
(In reply to comment #11)
> Oops meant nsSVGLengthList not the animated one

I've not made any change to nsSVGLengthList. It still uses nsSVGLength for it's elements and nsSVGLength still inherits from nsISVGLength.

(In reply to comment #12)
> Additionally CreateSVGNumber is the only way to create a new SVGNumber in
> scripts. It needs to return something that inherits from nsISVGValue so it can
> properly notify an nsSVGNumberList part of an nsSVGAnimatedNumberList that is
> an attribute of an element.

The number returned by CreateSVGNumber can't be contained in an nsSVGAnimatedNumberList directly. If you copy it into such a list it will become stored in a nsSVGNumber element instead.

Feel free to provide counterexamples in a new bug if you still disagree.
Actually all this stuff could do with additional mochitests even if all they do is show it is working.
nsSVGLengthList doesn't have any way to create a new element except when parsing a string(for SetAttribute). And the string parsing routine erases the entire list. So if you were in javascript and wanted to add a new IDOMSVGLength to the list after string parsing you would call createSVGLength on an SVG element and then call AppendItem on the list. But AppendItem will now fail.
I think you are right. I thought it was a value copy but it seems to be just a reference transfer. Adding and implementing the interface with its one method should fix it. Should be a new bug though.
Attachment #363616 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs landing]
Patch backed out.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [needs landing]
QA Contact: ian → general
No longer blocks: 413975
Assignee: longsonr → nobody
No longer blocks: 342513
You need to log in before you can comment on or make changes to this bug.