243536 - Complete, improve error checking, and reduce footprint of nsSVGTransformList.cpp

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Opening to give myself a logical place to locate files relating to the
implementation of the Consolidate, CreateSVGTransformFromMatrix,
InsertItemBefore and ReplaceItem methods of the nsSVGTransformList class.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: Draft Patch for Consolidate

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Draft Patch for CreateSVGTransformFromMatrix

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Draft Patch for InsertItemBefore

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: Draft Patch for ReplaceItem

Comment 5

[Jonathan Watt [:jwatt]]

Alex, I thought I'd put this stuff into bugzilla rather than keep on filling up
your mailbox with code. I'll try and read up on XPCOM to better understand when
I should be adding references to objects. Meanwhile any comments would be welcome. 

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

after assigning to out params, you need NS_ADDREF(*_retval) or something like this

could you attach patches as real patches, not as a text file containing the new
fucntion? (e.g. as produced by cvs diff -pu6 > patchfilename.diff)

Comment 7

[Jonathan Watt [:jwatt]]

For the moment I'm using text files to make it easy to quickly see what I'm
doing. The code is very rough and certainly isn't ready to be used as a patch
yet. Once it is I'll get round to the creating a patch side of things. See the
existing file at
http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGTransformList.cpp#410

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: Draft Patch for Consolidate

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: Patch for nsSVGSVGElement::CreateSVGTransformFromMatrix()

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: Patch for nsSVGSVGElement::CreateSVGTransformFromMatrix()

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: Patch for nsSVGSVGElement::CreateSVGTransformFromMatrix()

Sorry, stupid mistake in last file.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: Patch for CreateSVGTransformFromMatrix and Consolidate

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: Patch for nsSVGSVGElement::CreateSVGTransformFromMatrix()

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: Patch for CreateSVGTransformFromMatrix and Consolidate

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 148685 [details] [diff] [review]
Patch for nsSVGSVGElement::CreateSVGTransformFromMatrix()

moved to bug 244292

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 148688 [details] [diff] [review]
Patch for CreateSVGTransformFromMatrix and Consolidate

Needs more work, although the code for Consolidate() will still give a rough
idea of the proposed implementation.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: rough patch

This patch hasn't been properly debugged, but I'm looking for an informal
review of my basic approach.

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: patch addressing points brought up on irc

Comment 20

[Alex Fritze]

Comment on attachment 155420 [details] [diff] [review]
patch addressing points brought up on irc

r=afri with the following changes:

>Index: mozilla/dom/public/idl/svg/nsIDOMSVGTransformList.idl
[...]
> [scriptable, uuid(df41474c-a4f8-4ec3-ae79-4342e6f56d8e)]
> interface nsIDOMSVGTransformList : nsISupports
>-{ 
>+{

stray whitespace change

>Index: mozilla/content/svg/content/src/nsSVGTransformList.cpp
>===================================================================
[...]
>+// Should check in params for our implementation?
>+// nsCOMPtr<nsSVGTransform> transform = do_QueryInterface(newItem);
>+// if (!transform)
>+//   return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
>+// will this allow the removal of: if (!newItem) return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
>+

I'd prefer not to have this comment here, as this is a much wider issue and
doesn't only affect this file. At some point we will probably introduce more
decomed types (like e.g. nsSVGContextProvider) and this issue will
automatically be addessed as a side-effect.

[...]
>+nsSVGTransformList::Create(const nsAString& aValue, nsISVGValue** aResult)
> {
[...]
> }

This function is unused, so you can remove it.

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: final patch

Patch addressing above requests.

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: TESTCASE

Here's a testcase to show everything works fine.

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: diff for forgotten nsSVGTransformList.h

Sorry tor!

Comment 24

[tor]

Checked in.