418158 - nsIDOMSVGComponentTransferFunctionElement has non-DOM method on it

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The nsIDOMSVGComponentTransferFunctionElement interface has a non-DOM [noscript] method stuck on it.  Given that the DOM C++ interfaces are generally frozen (although these aren't marked as such), it seems like this doesn't belong:

    [noscript] void GenerateLookupTable(out PRUint8 aTable);

It looks like this interface is new in 1.9; would be good to get it right before we ship.

It's not even clear to me why it needs to be virtual.  The code that calls it could just static_cast to nsSVGComponentTransferFunctionElement.  (It would be nice to avoid the chain of QueryInterface calls as well.)

Comment 1

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

Attachment: fix

Comment 2

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

Attachment: oops, rev uuid

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 304113 [details] [diff] [review]
oops, rev uuid

>+   // nsISupports is an ambiguous base of nsSVGFE so we have to work
>+   // around that
>+   if ( aIID.Equals(NS_GET_IID(nsSVGComponentTransferFunctionElement)) )
>+     foundInterface = static_cast<nsIContent*>(this);

This seems dangerous -- it assumes that casting from nsSVGComponentTransferFunctionElement to nsIContent doesn't offset the pointer.  Probably safer to cast to void* and from there to nsISupports*.

Comment 4

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

Why is that dangerous? We still get a valid pointer to nsISupports.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

But the caller assumes it's a valid pointer to nsSVGComponentTransferFunctionElement (as it should):

+    nsRefPtr<nsSVGComponentTransferFunctionElement> child;
+    CallQueryInterface(GetChildAt(k),
+            (nsSVGComponentTransferFunctionElement**)getter_AddRefs(child));

If the class changed such that the nsIContent base class subobject weren't at offset zero, that would break.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(The whole point of QueryInterface is to give you the *right* pointer, not just any valid nsISupports pointer.)

Comment 7

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

ok

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 304113 [details] [diff] [review]
oops, rev uuid

>   NS_DECL_NSIDOMSVGFEFUNCGELEMENT
> 
>+  virtual PRInt32 GetChannel() { return 1; }
>+

I think it would be nicer if nsSVGFEComponentTransferElementBase had an enum with the various channels something like

enum Channels { R, G, B, A };

Then return one of these in each GetChannel implementation.

r=longsonr with that and dbaron's changes.

Up to you but you could fill in the tables array using the enum too.

> +  PRUint8* tables[] = { tableR, tableG, tableB, tableA };

might become

PRUint8* tables[4];
tables[R] = tableR;
...

overkill?

Comment 9

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

I think overkill since these are only used in one place in this file only.

Comment 10

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

Comment on attachment 304113 [details] [diff] [review]
oops, rev uuid

Removes a non-scriptable method from DOM interfaces. A simple fix for an API issue. Won't affect any users.

Comment 11

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

Attachment: updated patch

Updated for dbaron's comment so I don't forget it.

This patch removes a nonscriptable method from an SVG DOM interface. It's a simple change we should make now to avoid polluting the new interface before we ship it. No users can be affected.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304113 [details] [diff] [review]
oops, rev uuid

clearing nom (this patch is out of date, yes?)

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304194 [details] [diff] [review]
updated patch

a=beltzner for 1.9

Comment 14

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

Yes, that other patch is obsolete. Thanks for the approval.

Comment 15

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

checked in.

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 304194 [details] [diff] [review]
updated patch

>    // nsISupports is an ambiguous base of nsSVGFE so we have to work
>    // around that

For what it's worth, these comments are sort of misleading as well.  Not sure if it's worth fixing, though.  I'd maybe say:

  // Give the caller a pointer to the type they QueryInterfaced for.