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.
9 years ago
Created attachment 327065 [details] [diff] [review] 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.
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.
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.
Those classes would only be used in fields of data structures that hang around --- primarily frames and elements.
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?
Created attachment 351527 [details] [diff] [review] 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.
Created attachment 351836 [details] [diff] [review] 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.
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.
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.
9 years ago
I'll take this.
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).
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.
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.
I see. :)
Created attachment 387738 [details] [diff] [review] 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.
Created attachment 387970 [details] [diff] [review] patch removing stack instances Patch updated to tip and now passing SVG reftests and mochitests.
Comment on attachment 387970 [details] [diff] [review] patch removing stack instances Pushed http://hg.mozilla.org/mozilla-central/rev/941a73f2fc21
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
Created attachment 390200 [details] [diff] [review] [checked in] patch removing stack instances Pushed http://hg.mozilla.org/mozilla-central/rev/684dadc0726e
bug 602759 removed what was left.