435356 - Stop using nsIDOMSVGMatrix internally

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Right now SVG uses XPCOM for some basic internal types like rect and matrix. We have deCOM versions of these types with pretty decent APIS. Cleaning this up would shrink the code, make it more readable, and probably improve performance.

Comment 1

[Craig Topper]

Attachment: Work in progress patch that converts nsIDOMSVGMatrix to gfxMatrix

This patch converts many uses of nsIDOMSVGMatrix to gfxMatrix. It's work in progress and likely won't build. Just want to make sure I'm heading the right direction.

Comment 2

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

Basically looks great.

Use const gfxMatrix& instead of const gfxMatrix*.

GetTMIncludingOffset should just return the matrix.

Wherever 'null' means 'identity', don't bother with validity bits, just return the matrix.

I assume you're going to rename the *Thebes() functions to their base names at some point.

Comment 3

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

One thing I just realized is that gfxMatrix and gfxRect use doubles internally whereas SVG currently uses floats.

For storage in frames and elements, we probably should use floats to save memory. The best way to do this might be to have nsSVGPackedMatrix and nsSVGPackedRect classes that store floats internally, have constructors that take a gfxMatrix/gfxRect, and have an Unpack method that returns a gfxMatrix/gfxRect.

Comment 4

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

Those classes would only be used in fields of data structures that hang around --- primarily frames and elements.

Comment 5

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

Craig, I don't know if you're still working on this, but can you post your latest patch here so if you can't carry it on, someone else can build on your work?

Comment 6

[Arpad Borsos [:Swatinem]]

Attachment: Step1: Cleanup

I have cleaned up Craigs patch to make it compile and also included a few suggestions from comment #2.

Unfortunately the patch fails 4 svg reftests and I don't have the slightest idea why that is. Getting the hang of this XPCOM outparam business is quite hard.

Also I'm not sure if I have enough time to work on this bug right now, I'm kind of occupied with exams.

Comment 7

[Craig Topper]

Attachment: More complete patch than the original one I posted

Here are the changes that have been sitting in my tree. It should be more complete than the original patch I submitted.

Comment 8

[Craig Topper]

I seem to be having a problem getting the reftests to run without crashing the browser completely so I'm not sure of the health of the patch I just posted.

Comment 9

[Arpad Borsos [:Swatinem]]

Assigning back to Craig. Nice to see you back.
Hope you can also integrate some of my changes, like getting rid of the outparam in GetViewboxToViewportTransform.

Comment 10

[Ryo Onodera]

I'll take this.

Comment 11

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

See bug 488314 which converts GetCanvasTM to return gfxMatrix.

Comment 12

[Ryo Onodera]

(In reply to comment #11)
> See bug 488314 which converts GetCanvasTM to return gfxMatrix.

I see.

Comment 13

[Jonathan Watt [:jwatt]]

I cleaned up our internal usage of nsIDOMSVGRect and nsSVGRect in bug 495938, so we now only use gfxRect (there was never anything for rect in patches on this bug BTW).

Comment 14

[Ryo Onodera]

The mentioned bug is now commited and fixed. So, is it time to work on this? Can you assign this to me? Or should I still wait for Bug 488314? Well, I've almost forgot what this bug is for. Sorry for being lazy.

Comment 15

[Jonathan Watt [:jwatt]]

Hi Ryo. You're not being lazy at all. Your patches on other bugs have been great. :-) For this bug though we have a deadline of the end of this month, so I'd rather take this one and work on it myself. Sorry for not marking it as so before, and thanks for your offer to work on it.

Comment 16

[Ryo Onodera]

I see. :)

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: patch - filters broken

I've dug myself into a bit of a hole. This patch is not easy to split up into smaller patches, and somehow I've managed to breaks filters. Nevertheless, up for review since it's pretty much there, and hopefully the filter fix is fairly small and easy to review afterwards.

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: patch removing stack instances

Patch updated to tip and now passing SVG reftests and mochitests.

Comment 19

[Jonathan Watt [:jwatt]]

Comment on attachment 387970 [details] [diff] [review]
patch removing stack instances

Pushed http://hg.mozilla.org/mozilla-central/rev/941a73f2fc21

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 387970 [details] [diff] [review]
patch removing stack instances

Backed out due to nsSVGMatrix and nsTArray_base leaks. E.g. see http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1248209054.1248213673.22667.gz

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: [checked in] patch removing stack instances

Pushed http://hg.mozilla.org/mozilla-central/rev/684dadc0726e

Comment 22

[Robert Longson [:longsonr]]

bug 602759 removed what was left.