Stop using nsIDOMSVGMatrix internally

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: roc, Assigned: jwatt)

Tracking

({arch})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.
Whiteboard: [good first bug]

Comment 1

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.
Assignee: nobody → arpad.borsos
Attachment #327065 - Attachment is obsolete: true
Status: NEW → ASSIGNED

Comment 7

9 years ago
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

9 years ago
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.
Assignee: arpad.borsos → craig.topper
Assignee: craig.topper → nobody

Comment 10

8 years ago
I'll take this.
See bug 488314 which converts GetCanvasTM to return gfxMatrix.
Depends on: 488314

Comment 12

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

I see.
(Assignee)

Comment 13

8 years ago
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).
Summary: Convert SVG internal usage of nsIDOMSVGMatrix and nsIDOMSVGRect to gfxMatrix/gfxRect → Stop using nsIDOMSVGMatrix internally

Comment 14

8 years ago
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.
(Assignee)

Comment 15

8 years ago
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.
Assignee: nobody → jwatt

Comment 16

8 years ago
I see. :)
(Assignee)

Updated

8 years ago
Depends on: 498275
(Assignee)

Comment 17

8 years ago
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.
Attachment #387738 - Flags: review?(longsonr)
(Assignee)

Comment 18

8 years ago
Created attachment 387970 [details] [diff] [review]
patch removing stack instances

Patch updated to tip and now passing SVG reftests and mochitests.
Attachment #387738 - Attachment is obsolete: true
Attachment #387970 - Flags: review?(longsonr)
Attachment #387738 - Flags: review?(longsonr)

Updated

8 years ago
Attachment #387970 - Flags: review?(longsonr) → review+

Updated

8 years ago
Whiteboard: [good first bug]
(Assignee)

Comment 19

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

Pushed http://hg.mozilla.org/mozilla-central/rev/941a73f2fc21
Attachment #387970 - Attachment description: patch → [checked in] patch removing stack instances
(Assignee)

Comment 20

8 years ago
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
Attachment #387970 - Attachment description: [checked in] patch removing stack instances → patch removing stack instances
(Assignee)

Comment 21

8 years ago
Created attachment 390200 [details] [diff] [review]
[checked in] patch removing stack instances

Pushed http://hg.mozilla.org/mozilla-central/rev/684dadc0726e
Attachment #387970 - Attachment is obsolete: true

Updated

8 years ago
Depends on: 508247
(Assignee)

Updated

8 years ago
Depends on: 517393

Updated

8 years ago
Depends on: 530268

Updated

7 years ago
Depends on: 578309

Updated

7 years ago
No longer depends on: 578309
(Assignee)

Updated

6 years ago
Keywords: arch

Updated

6 years ago
Depends on: 602759

Comment 22

6 years ago
bug 602759 removed what was left.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.