Closed Bug 418158 Opened 14 years ago Closed 14 years ago

nsIDOMSVGComponentTransferFunctionElement has non-DOM method on it

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: roc)

Details

Attachments

(2 files, 1 obsolete file)

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.)
Flags: blocking1.9?
Assignee: nobody → roc
Attached patch fix (obsolete) — Splinter Review
Attachment #304111 - Flags: superreview?(pavlov)
Attachment #304111 - Flags: review?
Attachment #304111 - Flags: review? → review?(longsonr)
Whiteboard: [needs review]
Attached patch oops, rev uuidSplinter Review
Attachment #304113 - Flags: superreview?(pavlov)
Attachment #304113 - Flags: review?(longsonr)
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*.
Why is that dangerous? We still get a valid pointer to nsISupports.
Attachment #304111 - Attachment is obsolete: true
Attachment #304111 - Flags: superreview?(pavlov)
Attachment #304111 - Flags: review?(longsonr)
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.
(The whole point of QueryInterface is to give you the *right* pointer, not just any valid nsISupports pointer.)
Attachment #304113 - Flags: superreview?(pavlov) → superreview+
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?
Attachment #304113 - Flags: review?(longsonr) → review+
I think overkill since these are only used in one place in this file only.
Whiteboard: [needs review]
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.
Attachment #304113 - Flags: approval1.9?
Attached patch updated patchSplinter Review
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.
Attachment #304194 - Flags: superreview+
Attachment #304194 - Flags: review+
Attachment #304194 - Flags: approval1.9?
Comment on attachment 304113 [details] [diff] [review]
oops, rev uuid

clearing nom (this patch is out of date, yes?)
Attachment #304113 - Flags: approval1.9?
Comment on attachment 304194 [details] [diff] [review]
updated patch

a=beltzner for 1.9
Attachment #304194 - Flags: approval1.9? → approval1.9+
Yes, that other patch is obsolete. Thanks for the approval.
checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.