334400 - Create SVG transform list lazily

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: lazy transform list creation, remove duplicate code

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 218734 [details] [diff] [review]
lazy transform list creation, remove duplicate code

>+  if (!mTransforms) {
>+    *_retval = parentCTM;
>+    NS_ADDREF(*_retval);
>+  }

Missing return.


>+  if (!mTransforms) {
>+    *_retval = parentScreenCTM;
>+    NS_ADDREF(*_retval);
>+  }

And here.


nsSVGGraphicElement::GetTransform(nsIDOMSVGAnimatedTransformList * *aTransform)
> {
>+  if (!mTransforms)
>+    CreateTransformList();

Check rv here and at other call points (see below). E.g. could encounter OOM.


>+nsSVGGraphicElement::GetLocalTransform()

Can you call this GetLocalTransformMatrix to be clear it's a matrix we're getting?


>+{
>+  if (!mTransforms)
>+    return nsnull;
>+
>+  nsCOMPtr<nsIDOMSVGMatrix> localTM;
>+
>+  nsCOMPtr<nsIDOMSVGTransformList> transforms;
>+  mTransforms->GetAnimVal(getter_AddRefs(transforms));
>+  NS_ASSERTION(transforms, "null transform list");
>+
>+  PRUint32 numberOfItems;
>+  transforms->GetNumberOfItems(&numberOfItems);
>+  if (numberOfItems > 0)
>+    transforms->GetConsolidationMatrix(getter_AddRefs(localTM));

Can you just fix GetConsolidationMatrix to return nsnull if there are no transforms in the list?

Also please check GetConsolidationMatrix's rv (and the rv's of other function calls in this function) and return an appropriate rv from this function (obviously checking at the call points as necessary) to protect against OOM etc.


>+nsSVGGraphicElement::CreateTransformList()

Please also make this function return an rv. Your code assumes that it will succeed and just uses mTransforms after calling it.


>   nsCOMPtr<nsIDOMSVGMatrix> canvasTM;
> 
>+  nsSVGGraphicElement *element =
>+    NS_STATIC_CAST(nsSVGGraphicElement*, mContent);
>+  nsCOMPtr<nsIDOMSVGMatrix> localTM = element->GetLocalTransform();

I'd prefer the declaration of canvasTM come after rather than before this.

Comment 3

[tor]

Attachment: address review comments

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 219001 [details] [diff] [review]
address review comments

>+  if (!mTransforms && NS_FAILED(CreateTransformList()))
>+      return NS_ERROR_OUT_OF_MEMORY;

Too much indentation here


> NS_IMETHODIMP nsSVGTransformList::GetConsolidationMatrix(nsIDOMSVGMatrix **_retval)
> {
>   *_retval = nsnull;
>   PRInt32 count = mTransforms.Count();
> 
>+  if (!count)
>+    return NS_OK;
>+

A transform list with a single item in it is also a common case. Rather than burdening all calls with the cost of multiplication, an early return for count==1 would be preferable.

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 219001 [details] [diff] [review]
address review comments

Oop, no, r-. You still need to check whether GetConsolidationMatrix returned null in many of its call points. E.g.

>   transforms->GetConsolidationMatrix(getter_AddRefs(matrix));
>   return parentCTM->Multiply(matrix, _retval);  // addrefs, so we don't
> }

Comment 6

[tor]

Attachment: another...

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 219005 [details] [diff] [review]
another...

In nsSVGTransform::Consolidate:

>+  if (!conmatrix) {
>+    rv = NS_NewSVGMatrix(getter_AddRefs(conmatrix));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

You don't need this since there are early returns for count==0 and count==1 above.


> nsSVGGradientFrame::GetGradientTransform(nsIDOMSVGMatrix **aGradientTransform,
> ...
>-  trans->GetConsolidationMatrix(getter_AddRefs(gradientTransform));
>+  nsresult rv = trans->GetConsolidationMatrix(getter_AddRefs(gradientTransform));
>+  NS_ENSURE_SUCCESS(rv, rv);

You don't need to check rv since you're checking gradientTransform. If we returned a failure code we must null out the out param (if we didn't (we do) it would be a bug).


> nsSVGPatternFrame::GetPatternTransform(nsIDOMSVGMatrix **aPatternTransform)
> ...
>-    lTrans->GetConsolidationMatrix(aPatternTransform);
>+    nsresult rv = lTrans->GetConsolidationMatrix(aPatternTransform);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (!*aPatternTransform) {
>+      rv = NS_NewSVGMatrix(aPatternTransform);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }

Same here. No need to check rv.

Comment 8

[tor]

Attachment: last pass (hopefully)

Comment 9

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

Can't the tails of nsSVGGraphicElement::GetCTM/GetScreenCTM be shared in a helper, so you don't have to add code twice?

Other than that, it looks good.

I think a good project would be to make nsSVGMatrix just be a wrapper around a Thebes gfxMatrix, and for internal SVG use, just work with gfxMatrix (passing const gfxMatrix& or gfxMatrix* as in/out parameters, and returning gfxMatrix results). Then instead of returning null to represent the identity matrix you could just return one directly, or have a static identity matrix that you point to in such situations.

Comment 10

[tor]

Attachment: create a new helper

While non-cairo-gfx is still a buildable configuration, I won't be adding hard dependencies on Thebes in the SVG code.  Switching to cairo use directly in the interim (cairo_matrix_t in this case) I'm more comfortable with, and is the approach I'm using for reworking path storage.

Comment 11

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

Fair enough.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 219058 [details] [diff] [review]
create a new helper

r=jwatt

Comment 13

[Jesse Ruderman]

Fix was checked in 2006-04-20 08:24.  Shouldn't this be marked as fixed?