Last Comment Bug 435356 - Stop using nsIDOMSVGMatrix internally
: Stop using nsIDOMSVGMatrix internally
Status: RESOLVED FIXED
: arch
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on: 488314 498275 508247 517393 530268 602759
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-22 20:29 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2011-09-26 09:53 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work in progress patch that converts nsIDOMSVGMatrix to gfxMatrix (147.04 KB, patch)
2008-06-26 20:57 PDT, Craig Topper
no flags Details | Diff | Review
Step1: Cleanup (90.02 KB, patch)
2008-12-05 02:13 PST, Arpad Borsos [:Swatinem]
no flags Details | Diff | Review
More complete patch than the original one I posted (194.18 KB, patch)
2008-12-07 18:18 PST, Craig Topper
no flags Details | Diff | Review
patch - filters broken (107.83 KB, patch)
2009-07-09 14:47 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch removing stack instances (108.61 KB, patch)
2009-07-10 16:00 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
longsonr: review+
Details | Diff | Review
[checked in] patch removing stack instances (108.56 KB, patch)
2009-07-23 04:40 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-05-22 20:29:49 PDT
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 2008-06-26 20:57:46 PDT
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.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-26 21:30:05 PDT
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) (Exited; email my personal email if necessary) 2008-06-26 21:36:10 PDT
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) (Exited; email my personal email if necessary) 2008-06-26 21:36:55 PDT
Those classes would only be used in fields of data structures that hang around --- primarily frames and elements.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-09-23 16:42:10 PDT
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] 2008-12-05 02:13:49 PST
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.
Comment 7 Craig Topper 2008-12-07 18:18:47 PST
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.
Comment 8 Craig Topper 2008-12-07 18:20:11 PST
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] 2008-12-08 02:04:58 PST
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 2009-04-27 09:23:48 PDT
I'll take this.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-27 13:27:31 PDT
See bug 488314 which converts GetCanvasTM to return gfxMatrix.
Comment 12 Ryo Onodera 2009-04-27 13:36:16 PDT
(In reply to comment #11)
> See bug 488314 which converts GetCanvasTM to return gfxMatrix.

I see.
Comment 13 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-06-11 13:48:54 PDT
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 2009-06-13 05:39:51 PDT
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] (Away Jun. 27 - Jul. 13) 2009-06-13 08:59:49 PDT
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 2009-06-13 09:39:36 PDT
I see. :)
Comment 17 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-07-09 14:47:11 PDT
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.
Comment 18 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-07-10 16:00:55 PDT
Created attachment 387970 [details] [diff] [review]
patch removing stack instances

Patch updated to tip and now passing SVG reftests and mochitests.
Comment 19 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-07-21 13:45:37 PDT
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] (Away Jun. 27 - Jul. 13) 2009-07-21 16:04:25 PDT
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] (Away Jun. 27 - Jul. 13) 2009-07-23 04:40:56 PDT
Created attachment 390200 [details] [diff] [review]
[checked in] patch removing stack instances

Pushed http://hg.mozilla.org/mozilla-central/rev/684dadc0726e
Comment 22 Robert Longson 2011-09-26 09:53:36 PDT
bug 602759 removed what was left.

Note You need to log in before you can comment on or make changes to this bug.