All users were logged out of Bugzilla on October 13th, 2018

getConsolidationMatrix() returns null

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: sky, Assigned: longsonr)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061204 GranParadiso/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061204 GranParadiso/3.0a1

for all transform objects t, t.baseVal.getConsolidationMatrix() should return a matrix of type SVGMatrix.  Gran Paradiso returns null instead.

Reproducible: Always

Steps to Reproduce:
1. load http://www.columbia.edu/~sky/svg/consolidationmatrix.svg
2. first alert shows return value of getConsolidationMatrix()

Actual Results:  
null
Should return "[Object SVGMatrix]"

Expected Results:  
[Object SVGMatrix]
Should return "[Object SVGMatrix]"

Comment 1

12 years ago
It will return a matrix, except in the case of having no explicit transform set (internal optimization inadvertently exposed to DOM).

(Reporter)

Comment 2

12 years ago
(In reply to comment #1)
> It will return a matrix, except in the case of having no explicit transform set
> (internal optimization inadvertently exposed to DOM).

I get a null here, too (when there's a transform matrix in the parent):
http://www.columbia.edu/~sky/svg/consolidationmatrix2.svg
but not when it's set on the element itself:
http://www.columbia.edu/~sky/svg/consolidationmatrix3.svg

Comment 3

12 years ago
(In reply to comment #2)
> I get a null here, too (when there's a transform matrix in the parent):
> http://www.columbia.edu/~sky/svg/consolidationmatrix2.svg
> but not when it's set on the element itself:
> http://www.columbia.edu/~sky/svg/consolidationmatrix3.svg

That's expected, as getConsolidationMatrix() only acts on that transform list, without regard to where it is in the document hierarchy.

Note that getConsolidationMatrix() is a moz-internal method, not part of the standard SVG DOM, and was only accidentally exposed to scripting.  We will be rectifying at least the last bit (script availability) shortly, so you should rework your code to use standard DOM methods.
(Assignee)

Comment 4

12 years ago
Created attachment 248506 [details] [diff] [review]
something like this perhaps
Attachment #248506 - Flags: review?(tor)
(Assignee)

Comment 5

12 years ago
Comment on attachment 248506 [details] [diff] [review]
something like this perhaps

Tor is away
Attachment #248506 - Flags: review?(tor) → review?(jwatt)
Comment on attachment 248506 [details] [diff] [review]
something like this perhaps

I think the return value should be |static already_AddRefed<nsIDOMSVGMatrix>| like nsSVGUtils::GetViewBoxTransform. (In fact, come to think of it, maybe this method would be better as a member of nsSVGUtils, although I don't really mind either way.) Then you can have more readable code such as:

  nsCOMPtr<nsIDOMSVGMatrix> matrix =
    nsSVGTransformList::GetConsolidationMatrix(transforms);

Other than that, looks good to me.
Attachment #248506 - Flags: review?(jwatt) → review+
(Assignee)

Comment 7

12 years ago
Created attachment 249123 [details] [diff] [review]
rev idl ID and change to using return value

(In reply to comment #6)
> (From update of attachment 248506 [details] [diff] [review] [edit])
> I think the return value should be |static already_AddRefed<nsIDOMSVGMatrix>|
> like nsSVGUtils::GetViewBoxTransform. (In fact, come to think of it, maybe this
> method would be better as a member of nsSVGUtils, although I don't really mind
> either way.) Then you can have more readable code such as:
> 
>   nsCOMPtr<nsIDOMSVGMatrix> matrix =
>     nsSVGTransformList::GetConsolidationMatrix(transforms);
> 
> Other than that, looks good to me.
> 

I left the method in nsSVGTransformList but have changed it to returning a value. I do hope I've put ADDREFs in the right place(s). Checking in the debugger seems to indicate this is so.
Assignee: general → longsonr
Attachment #248506 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Attachment #249123 - Flags: superreview?(roc)
(Assignee)

Updated

12 years ago
Attachment #249123 - Flags: superreview?(roc)
(Assignee)

Comment 8

12 years ago
Created attachment 249134 [details] [diff] [review]
addref correction
Attachment #249123 - Attachment is obsolete: true
Attachment #249134 - Flags: superreview?(roc)
Comment on attachment 249134 [details] [diff] [review]
addref correction

>-  nsCOMPtr<nsIDOMSVGMatrix> localTM;
>-  rv = transforms->GetConsolidationMatrix(getter_AddRefs(localTM));
>-  NS_ENSURE_SUCCESS(rv, nsnull);
>+  nsCOMPtr<nsIDOMSVGMatrix> localTM =
>+    nsSVGTransformList::GetConsolidationMatrix(transforms);
>+  if (!localTM)
>+    return nsnull;
> 
>   nsIDOMSVGMatrix *retval = localTM.get();
>   NS_IF_ADDREF(retval);
>   return retval;
> }

No need to do this dance BTW. Just return directly from GetConsolidationMatrix and forget about the following six lines.
(Assignee)

Comment 10

12 years ago
Created attachment 249235 [details] [diff] [review]
address additional review comment

(In reply to comment #9)
> (From update of attachment 249134 [details] [diff] [review] [edit])
> >-  nsCOMPtr<nsIDOMSVGMatrix> localTM;
> >-  rv = transforms->GetConsolidationMatrix(getter_AddRefs(localTM));
> >-  NS_ENSURE_SUCCESS(rv, nsnull);
> >+  nsCOMPtr<nsIDOMSVGMatrix> localTM =
> >+    nsSVGTransformList::GetConsolidationMatrix(transforms);
> >+  if (!localTM)
> >+    return nsnull;
> > 
> >   nsIDOMSVGMatrix *retval = localTM.get();
> >   NS_IF_ADDREF(retval);
> >   return retval;
> > }
> 
> No need to do this dance BTW. Just return directly from GetConsolidationMatrix
> and forget about the following six lines.
> 

Done.
Attachment #249134 - Attachment is obsolete: true
Attachment #249235 - Flags: superreview?(roc)
Attachment #249134 - Flags: superreview?(roc)
I'm confused. What bug is actually being fixed here?
(Assignee)

Comment 12

12 years ago
(In reply to comment #11)
> I'm confused. What bug is actually being fixed here?
> 

As per the last paragraph of comment 3 we're removing getConsolidationMatrix from the SVG DOM.
Comment on attachment 249235 [details] [diff] [review]
address additional review comment

+  nsIDOMSVGMatrix *_retval = conmatrix;
+  NS_ADDREF(_retval);
+  return _retval;

Use
  nsIDOMSVGMatrix *_retval = nsnull;
  conmatrix.swap(_retval);
  return _retval;
to avoid the extra addref.

A deCOMtaminated GetItem would be nice here...
Attachment #249235 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 14

12 years ago
Created attachment 250330 [details] [diff] [review]
patch for checkin
(Assignee)

Comment 15

12 years ago
(In reply to comment #14)
> Created an attachment (id=250330) [details]

Checked in. 

getConsolidationMatrix is no longer accessible from the SVG DOM.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.