502234 - python-xpcom "optional" arguments are never allowed e.g. nsIHTMLCanvas.drawImage()

Status: RESOLVED INVALID

(Other Applications Graveyard :: PyXPCOM, defect)

Description

[lkcl]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.14) Gecko/20080404 Iceweasel/2.0.0.14 (Debian-2.0.0.14-2)
Build Identifier: 

nsiHTMLCanvas.drawImage takes zero arguments, according to the IDL file.
actually, it takes three, five or nine arguments.

unfortunately, python-xpcom goes, "oh, the IDL file says that there are zero arguments, therefore you can get stuffed".



Reproducible: Always

Steps to Reproduce:
1) download and install pyjamas from http://pyjs.org
2) install python-xpcom and python-hulahop
3) cd examples/addonsgallery
4) python AddonsGallery.py
5) click on "Canvas" tab.
6) note ****-looking screen and note constant console error output

Actual Results:  
impossible to draw images in canvas.

Expected Results:  
being able to draw images in canvas.

this is similar to #502109

Comment 1

[Benjamin Smedberg]

I'm pretty sure you're talking about nsIDOMRenderingContext2D. The drawImage method is declared with no parameters in IDL, and the code walks back into the JS call context to find out the arguments that were passed in.

I'm inclined to say this is WONTFIX, but I'll let the DOM peers make that call... perhaps it's possible to represent this using [optional] arguments now.

Comment 2

[lkcl]

(In reply to comment #1)
> I'm pretty sure you're talking about nsIDOMRenderingContext2D. The drawImage
> method is declared with no parameters in IDL,

 yes, that's it.

> and the code walks back into the
> JS call context to find out the arguments that were passed in.

 what JS call context?  this is python bindings direct to the COM interface.  unless i misunderstand how things work [from a technical perspective, i'd be interested to know if there _was_ a JS call context, it would help me to understand what's going on]

 i'm looking at the python-xpcom source code in order to confirm this (it's very obtuse) but the number of parameters on this drawImage function is definitely reported as ZERO - exactly as shown in the IDL file.


> I'm inclined to say this is WONTFIX, but I'll let the DOM peers make that
> call... perhaps it's possible to represent this using [optional] arguments now.

 that would be good.  because if you set this bug as WONTFIX then i am very sorry but i would just have to reopen another bug.  this is not a complete showstopper (thank god) like the XMLHttpRequest one was, and for that, thank god, the default (likewise completely inaccessible and non-optional) async argument is set to "True" so it was ok.  not very acceptable, but ok.

in this case, however, the number of arguments that python-xpcom sees is: zero.

therefore, you can only do this:

    context.drawImage()

you CANNOT do:

    context.drawImage(image, x, y)


which is completely fricking useless.  what's the point of having a drawImage function if you can't pass it the bloody image that you want to be drawn?

so - WONTFIX will ... not be very well received, shall we say.

all 9 arguments being optional would be a good workaround, here, requiring the least work.

l.

Comment 3

[lkcl]

ok - i'm looking at this function, in content/canvas/src/nsCanvasRenderingContext2D.cpp

NS_IMETHODIMP
nsCanvasRenderingContext2D::DrawImage()

and i notice that yes, there is something about a JSContext.

in a python XPCOM based call, i have to emphasise and ask this question:

_what_ JSContext?

how in python-xpcom do i "get" this JSContext, and how would i throw arguments onto it?

seriously - does python-xpcom put the arguments onto JSContext?  is that how python-xpcom works?

ok.

rrright.  looking at this:
NS_IMETHODIMP
nsXMLHttpRequest::Open(const nsACString& method, const nsACString& url)

i see that there is a way to merge arguments.  so, you have two that are non-optional and then JSContext gets some more arguments, argc argv blah blah.

it's just that someone decided, in drawImage(), to be a bit lazy, and use JSContext arg-getting for absolutely all the arguments, rather than making the first three mandatory.

ok.  i'll take a look at making the first three arguments mandatory.  that will not be particularly good but better than the rubbish current arrangement where drawImage has absolutely no arguments from python-xpcom.

Comment 4

[(not currently active) Ted Mielczarek]

I believe bsmedberg's point is that the DrawImage method, as written, will *only* work when called from JavaScript, because it expects to find a JSContext on the stack.

And yes, the peers of that module could WONTFIX this bug, meaning they don't care if that method is callable from other XPCOM implementations, but that's up to them to decide.

Comment 5

[lkcl]

Attachment: makes 3 args the default, optionally uses JSContext to get 5 and 9 args

3 args DrawImage(image, dx, dy) so that python-xpcom has a chance of getting _something_, otherwise python-xpcom DrawImage can only take zero arguments.

this is the ONLY way to get python-xpcom to have access to images in the canvas2d context.

accompanied by patch to IDL file

Comment 6

[lkcl]

Attachment: patch to IDL file to make drawImage take 3 args rather than 0

accompanies patch to implementation of DrawImage

Comment 7

[lkcl]

(In reply to comment #4)
> I believe bsmedberg's point is that the DrawImage method, as written, will
> *only* work when called from JavaScript, because it expects to find a JSContext
> on the stack.

 not any more.  patch makes the three arguments mandatory, then works out if there were any more and if so uses them.

 i followed the style of XMLHTTPRequest Open function implementation, cutting-and-pasting the code which does "optional" JSContext rather than the bloody-minded (and stupid) mandatory use of JSContext.
 
> And yes, the peers of that module could WONTFIX this bug, meaning they don't
> care if that method is callable from other XPCOM implementations, but that's up
> to them to decide.

ya.  thus alienating users of the other XPCOM implementations.  great.  that would go down well.

Comment 8

[Benjamin Smedberg]

Comment on attachment 386816 [details] [diff] [review]
makes 3 args the default, optionally uses JSContext to get 5 and 9 args

Please attach the entire patch together as one. Then set review?[e-mail] because he's the module owner of this code.

Also whenever you change an IDL interface you need to generate a new random UUID at the top.

I tend to think this patch isn't worth taking however, and that it's ok that this method is only usable from JS. Note that native C++ can't call this method either!

Comment 9

[lkcl]

(In reply to comment #8)
> (From update of attachment 386816 [details] [diff] [review])
> Please attach the entire patch together as one.

 [ i'll try - i downloaded the debian package 1.9.0.11 and am seriously short on space to make copies and do diff -ru. ]

> Then set
> review?[e-mail] because he's the module owner of this code.

 ahh, ok.
 
> Also whenever you change an IDL interface you need to generate a new random
> UUID at the top.

 ohh. first time i've done xul-hacking so, surpriise, things i don't know :)  thank you.
 
> I tend to think this patch isn't worth taking however, and that it's ok that
> this method is only usable from JS.

 sorry for putting it like this, but i would have to say that that's a bit ****.  i've _just_ started using python-xpcom and was very excited to have managed to make the pyjamas python desktop widget set API use XUL, and now you're saying that if i make improvements to get things to work, that people might want to use, that i and any of the pyjamas desktop users can... well... get stuffed, basically?

please tell me that you're not recommending that.

i'd really rather not have to send a message to the europython mailing list telling people that they needn't bother trying to use pyjamas-desktop for SVG Canvas because the mozilla team are refusing to accept improvements and patches.

i trust that that's not the intent.

 
> Note that native C++ can't call this method
> either!

well with the patch, it could be called, could it not?

so that would be at least two systems that would be opened up, to be able to call this method.

Comment 10

[lkcl]

Attachment: diff -u with 3 non-optional args to DrawImage and rest optional. IDL uuid changed too

following ben's advice, have attached as a single attachment, added uuid-gen'd uuid.

the patch adds absolutely critical functionality where DrawImage can now be accessed from python (or native c++).  it's not perfect, as the other optional arguments cannot still be accessed, but it is better than nothing at all.

Comment 11

[lkcl]

(In reply to comment #8)


> I tend to think this patch isn't worth taking however, and that it's ok that
> this method is only usable from JS.

 perhaps i should explain, ben: the target audience for the usage of this is _not_ using JS.  at all.  that's the whole point.

 please take a look at http://pyjs.org and also at http://pyjd.org for the background.

 pyjamas is a python-to-javascript compiler.  so, you can create desktop-like applications that run in a web browser, in python.  including 2D Canvas:

http://pyjs.org/examples/addonsgallery/output/AddonsGallery.html#Canvas

 pyjamas-desktop REMOVES the javascript dependency ENTIRELY from the equation, and, using python bindings to the DOM (such as those found in python-xpcom) you can do EXACTLY the same application as a desktop application with ZERO javascript.

 ... except, it sounds like you're advocating that if the python DOM bindings are broken, that users of pyjamas-desktop can go take a hike.  i apologise if i have the wrong impression, from what you're saying.

 i trust that that's not what you are recommending.  i trust that you would welcome patches to improve xulrunner so that it can be opened up to other environments.  i trust that you will welcome opportunities to expand the reach of mozilla technology to more users.  i trust that you will encourage your peers to the same.

l.

Comment 12

[Walter Prins]

I for one am interested in playing with this.  As an aside, there's this gradual move towards the browser becoming as it were a lightweight OS (or at least more of an application platform/framework than it currently is), and a "don't care about anything but JS" attitude is consequently not very helpful if we agree that goal has merit.

Comment 13

[Steve Holden]

Speaking as someone who was only just exposed to pyjamas I'd really like to be able to use all rendering capabilities from within the library.

I'm unfamiliar with all the ins and outs of this issue, and I could understand reluctance to accept the patch if there was a negative impact on the native JavaScript platform. Refusing to accommodate the patch otherwise would be sad: pyjamas could actually be very good advertising for Mozilla projects.

It would be good if the pyXPCom bindings could have a future with support where necessary. C'mon, guys, play nice ;-)

Comment 14

[Boris Zbarsky [:bzbarsky]]

That patch is wrong; if nothing else elements other than <html:img> can be passed to drawImage in the current code.  The patch breaks that.

Comment 15

[lkcl]

original code:
           vvvvvvvvvvv
nsCOMPtr<nsIDOMElement> imgElt;
           ^^^^^^^^^^^
    if (!ConvertJSValToXPCObject(getter_AddRefs(imgElt),
                                 NS_GET_IID(nsIDOMElement),
                                 ctx, argv[0]))

yep, well spotted boris - lemme update...

Comment 16

[lkcl]

(In reply to comment #14)
> That patch is wrong; if nothing else elements other than <html:img> can be
> passed to drawImage in the current code.  The patch breaks that.

ok - what does the W3C specification say on this?  is the fact the DOMElement
can be passed in a bug?  or is it in fact correct?

i'll make the necessary changes - which will be reflected in the IDL file -
and you (the mozilla developers) can decide which one is correct.

Comment 17

[lkcl]

Attachment: version of same patch using nsIDOMElement not nsIDOMHTMLImageElement

necessarily contrary to the specification
http://www.w3.org/TR/2009/WD-html5-20090212/the-canvas-element.html

the lowest common denominator element type (DOMElement) between those two elements that _are_ in the specification (DOMImageElement and DOMCanvasElement) is put into the IDL file.

presumably CairoSurfaceFromElement and DrawImageSecurityCheck all make things hunky-dory.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Bug 459452 might help here. Then we could use [optional] in the .idl and C++
implementation could check parameter count. Not sure how python-xpcom handles
optional parameters.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> presumably CairoSurfaceFromElement and DrawImageSecurityCheck all make things
> hunky-dory.

Yeah, they should.

Comment 20

[lkcl]

(In reply to comment #18)
> Bug 459452 might help here. Then we could use [optional] in the .idl and C++
> implementation could check parameter count.

 woOOoo, looks scary.  and great!

> Not sure how python-xpcom handles
> optional parameters.

 hmm, have to try it out.  if python-xpcom falls over in a heap, that's bad.  if any [optional_argc] functions require an extra argument (_argc) that's klunky but acceptable.

 one thing i was told by neil (on mozilla.tech.dev.xpcom) was that one reason why [optional] hasn't been used is because you can't set defaults.  defaults are 0, NULL and FALSE.  clearly, for async on XmlHttpRequest.open, that's bad, because it would change prior behaviour.  however, the patch in #459452 uses the number of _argc to detect things and changes async to TRUE, which is good!

+  if (!_argc) {
+    // No optional arguments were passed in. Default async to true.
+    async = PR_TRUE;
   }

previously that wasn't possible.

i still like the MS way of solving this one, though.  got optional function params?  make another function, call it function1.  got more optional params?  make another function, call it function2.  repeat until no more optional params.

this technique has solved countless IDL-related issues for MS for the past... 18 years.  and the IDL technology in mozilla is from exactly the same place that MS got it from: DCE/RPC.  so, exactly same problem.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The Microsoft solution is not acceptable for web-exposed interfaces, as I already said in the newsgroup thread....

Comment 22

[lkcl]

ahh, ok, thanks boris - sorry, i was out-of-date.

Comment 23

[Gour]

Hi!

Although not fully aware of the technical consequences involved in the acceptance of lkcl's patch, as new pyjamas-(desktop) user i'd like to be able to take advantage of all the functionality offered by the pyjamas project and I wonder how it could not affect Mozilla projects positively.

Or the solution is to put a hand-brake on open-source projects and force people to depend on proprietary solutions. :-/

Please, kick this bug forward and help extend pyjamas' functionality.


Sincerely,
Gour

Comment 24

[Nochum Sossonko [:Natch]]

One way or another this bug should be resolved. cc'ing the module owner as well...

Comment 25

[Nochum Sossonko [:Natch]]

Oh, and the patch in 459452 actually lists this method as one of its' examples iiuc...

Comment 26

[lkcl]

(In reply to comment #25)
> Oh, and the patch in 459452 actually lists this method as one of its' examples
> iiuc...

yeah, it does :)  and, very helpfully, points out that we should be running
into difficulties with "ScrollIntoView" as well, because there will be a missing
[non-]optional 2nd argument, there.

i'm reluctant to say i like #459452.  the above patch falls into the "good enough to be getting on with" category, and #459452 solves the "general" problem, but in a way that i think will carry you down a path that's further and further away from the DCE/RPC origins on which DCOM (and therefore xpcom) are based. .... but, hey, if you're comfortable with that, go for it.

but, whilst making that decision, might i ask if this very simple patch could be looked at first, as DrawImage is quite literally the only function, out of thousands, that pyjamas-desktop cannot use.  all the others have turned out to be either absolutely perfect or perfectly acceptable.

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 386882 [details] [diff] [review]
version of same patch using nsIDOMElement not nsIDOMHTMLImageElement

>diff -ru xulrunner-1.9.0.11.orig/content/canvas/src/nsCanvasRenderingContext2D.cpp xulrunner-1.9.0.11/content/canvas/src/nsCanvasRenderingContext2D.cpp

>+nsCanvasRenderingContext2D::DrawImage(nsIDOMElement*aImage,

Space before *.

>+    nsCOMPtr<nsIDOMElement> imgElt = do_QueryInterface(aImage);

No need for this QI.

>+        #define GET_ARG(dest,whicharg) \
>+            do { if (!ConvertJSValToDouble(dest, ctx, whicharg)) { rv = NS_ERROR_INVALID_ARG; goto FINISH; } } while (0)

Keep the define completely to the left.

Please attach a diff -w so I can review the code you reindented.

Comment 28

[lkcl]

(In reply to comment #27)
> (From update of attachment 386882 [details] [diff] [review])
> >diff -ru xulrunner-1.9.0.11.orig/content/canvas/src/nsCanvasRenderingContext2D.cpp xulrunner-1.9.0.11/content/canvas/src/nsCanvasRenderingContext2D.cpp
> 
> >+nsCanvasRenderingContext2D::DrawImage(nsIDOMElement*aImage,
> 
> Space before *.
 
 ack.

> >+    nsCOMPtr<nsIDOMElement> imgElt = do_QueryInterface(aImage);
> 
> No need for this QI.

 oh? goood.
 
> >+        #define GET_ARG(dest,whicharg) \
> >+            do { if (!ConvertJSValToDouble(dest, ctx, whicharg)) { rv = NS_ERROR_INVALID_ARG; goto FINISH; } } while (0)
> 
> Keep the define completely to the left.

 ah.. ah... whoops, sorry, peter, yes that appears to be a
 
> Please attach a diff -w so I can review the code you reindented.

 ack.  ahh, i see what you mean.  looks better [less].

Comment 29

[lkcl]

Attachment: answering review comments; also is diff -ruw format as requested

* deals with review requests on 386882 kindly made by peter
* is in diff -ruw format as requested
* tested and still works using python-xpcom, with pyjamas-desktop
  xulrunner backend, using the port of mozilla 2d canvas tutorial
  to pure python
* also tested the javascript by running the same mozilla 2d canvas
  test, compiled to pure javascript, in the same xulrunner backend
  but only as a web browser (i.e. not running any python).
  still works.

... always a good idea to actually test what's being submitted, duh :)

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 389121 [details] [diff] [review]
answering review comments; also is diff -ruw format as requested

> diff -ruw xulrunner-1.9.0.11.orig/content/canvas/src/nsCanvasRenderingContext2D.cpp xulrunner-1.9.0.11/content/canvas/src/nsCanvasRenderingContext2D.cpp

>     nsAXPCNativeCallContext *ncc = nsnull;
>     rv = nsContentUtils::XPConnect()->
>         GetCurrentNativeCallContext(&ncc);
>-    NS_ENSURE_SUCCESS(rv, rv);
> 
>-    if (!ncc)
>-        return NS_ERROR_FAILURE;
>+    double sx = 0, sy = 0, sw, sh;
>+    double dw, dh;
> 
>+    dw = sw = (double) imgWidth;
>+    dh = sh = (double) imgHeight;

Move this before the call to GetCurrentNativeCallContext.

>+
>+    if (NS_SUCCEEDED(rv) && ncc) {
>     JSContext *ctx = nsnull;
> 
>     rv = ncc->GetJSContext(&ctx);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+        if (NS_SUCCEEDED(rv) && ctx) {

Why did you change this?

>     if (argc == 3) {
>-        GET_ARG(&dx, argv[1]);
>-        GET_ARG(&dy, argv[2]);
>+                //GET_ARG(&dx, argv[1]); /* already got */
>+                //GET_ARG(&dy, argv[2]); /* already got */
>         sx = sy = 0.0;
>         dw = sw = (double) imgWidth;
>         dh = sh = (double) imgHeight;

Delete this block (you're doing double work now).

>     } else if (argc == 5) {
>-        GET_ARG(&dx, argv[1]);
>-        GET_ARG(&dy, argv[2]);
>+                //GET_ARG(&dx, argv[1]); /* already got */
>+                //GET_ARG(&dy, argv[2]); /* already got */

Delete instead of commenting out.

>         sx = sy = 0.0;
>         sw = (double) imgWidth;
>         sh = (double) imgHeight;

Delete (you're doing double work now).

>     } else if (argc == 9) {
>-        GET_ARG(&sx, argv[1]);
>-        GET_ARG(&sy, argv[2]);
>+                double tmpdx;
>+                double tmpdy;
>+                sx = dx;
>+                sy = dy;
>+                //GET_ARG(&sx, argv[1]); /* already got */
>+                //GET_ARG(&sy, argv[2]); /* already got */

Delete instead of commenting out.

>         GET_ARG(&sw, argv[3]);
>         GET_ARG(&sh, argv[4]);
>-        GET_ARG(&dx, argv[5]);
>-        GET_ARG(&dy, argv[6]);
>+                GET_ARG(&tmpdx, argv[5]);
>+                GET_ARG(&tmpdy, argv[6]);

Given that this has a different order than the other calls I'd name the arguments arg1 and arg2, keep the local dx and dy variables, set dx to arg1 and dy to arg2 where you set the other variables to their initial values, and set sx to arg1 and sy to arg2 here. I think we should also make the type for arg1 and arg2 be double in idl, given that we eventually want doubles here anyway (and add a comment explaining that in the idl).

>diff -ruw xulrunner-1.9.0.11.orig/dom/public/idl/canvas/nsIDOMCanvasRenderingContext2D.idl xulrunner-1.9.0.11/dom/public/idl/canvas/nsIDOMCanvasRenderingContext2D.idl

>+//interface nsIDOMHTMLImageElement;

Delete instead of commenting out.

>+  /* https://bugzilla.mozilla.org/show_bug.cgi?id=502234#c14
>+     use nsIDOMElement here not nsIDOMHTMLImageElement
>
>+   */

Replace that comment with one saying that this function accepts HTMLImageElement, HTMLCanvasElement or HTMLVideoElement.

Comment 31

[Peter Van der Beken [:peterv]]

And please remove changes that aren't relevant to this bug from your patches before attaching them.

Comment 32

[lkcl]

(In reply to comment #30)
> (From update of attachment 389121 [details] [diff] [review])
> > diff -ruw xulrunner-1.9.0.11.orig/content/canvas/src/nsCanvasRenderingContext2D.cpp xulrunner-1.9.0.11/content/canvas/src/nsCanvasRenderingContext2D.cpp
> 
> >     nsAXPCNativeCallContext *ncc = nsnull;
> >     rv = nsContentUtils::XPConnect()->
> >         GetCurrentNativeCallContext(&ncc);
> >-    NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-    if (!ncc)
> >-        return NS_ERROR_FAILURE;
> >+    double sx = 0, sy = 0, sw, sh;
> >+    double dw, dh;
> > 
> >+    dw = sw = (double) imgWidth;
> >+    dh = sh = (double) imgHeight;
> 
> Move this before the call to GetCurrentNativeCallContext. 

 ack.  oh yes i see why.  gcncc and then check it, dw=sw=... breaks that up.


> >+
> >+    if (NS_SUCCEEDED(rv) && ncc) {
> >     JSContext *ctx = nsnull;
> > 
> >     rv = ncc->GetJSContext(&ctx);
> >-    NS_ENSURE_SUCCESS(rv, rv);
> >+        if (NS_SUCCEEDED(rv) && ctx) {
> 
> Why did you change this?

 look at the code from which it was cut/paste - nsXMLHttpRequest.cpp's Open function.  this change is the whole reason - the key to - why this patch is being made.

 walk it through from the non-JS angle.  python-xpcom calls into this function.   there _is_ no JSContext.  the ncc->GetJSContext(&ctx) will _always_ fail, when called from python-xpcom, and the previous code will _always_ bomb out, thus terminating any possibility for drawImage to be useable.  thus defeating the purpose of making the patch.

 walk it through from the JS angle.  JS calls into this function.  there's a JS context.  the first 3 args are passed in already, so they can be ignored (and as you've noted, the 3 args case can do "nothing".  5 args, do something, 9 args do something - all other cases, bomb out.

 summary: it's _the_ way to ensure that the function can be accessed and is useful from both JS and non-JS.

 another way to do this would be:

 rv = ncc->GetJSContext(&ctx);
 if (!NS_SUCCEEDED(rv) || ctx == NULL) {
    /* we're in non-JS context: assume xpcom, and go for the 3-args option */
    argc = 3;
    argv = NULL;
 }

 and then the if argc==3,5,9 block could be left at its present indent level.

 whiiiich.... i will do.  because that will make things more like the
 original code, before-patch.

> >     if (argc == 3) {
> >-        GET_ARG(&dx, argv[1]);
> >-        GET_ARG(&dy, argv[2]);
> >+                //GET_ARG(&dx, argv[1]); /* already got */
> >+                //GET_ARG(&dy, argv[2]); /* already got */
> >         sx = sy = 0.0;
> >         dw = sw = (double) imgWidth;
> >         dh = sh = (double) imgHeight;
> 
> Delete this block (you're doing double work now).

 ack - but you still need the if test, so that 3-args is accepted.
 so i'll put a comment in, to that effect.  otherwise it's a bit "huh??"

> >     } else if (argc == 5) {
> >-        GET_ARG(&dx, argv[1]);
> >-        GET_ARG(&dy, argv[2]);
> >+                //GET_ARG(&dx, argv[1]); /* already got */
> >+                //GET_ARG(&dy, argv[2]); /* already got */
> 
> Delete instead of commenting out.

 ack.

> >         sx = sy = 0.0;
> >         sw = (double) imgWidth;
> >         sh = (double) imgHeight;
> 
> Delete (you're doing double work now).

 huh.  the _original_ code was already doing double work.  i think.  ahhh, no, it was me adding it in above this group of if statements.

 ok... let's see what you think of it with the mods before i say any more.
 
> >     } else if (argc == 9) {
> >-        GET_ARG(&sx, argv[1]);
> >-        GET_ARG(&sy, argv[2]);
> >+                double tmpdx;
> >+                double tmpdy;
> >+                sx = dx;
> >+                sy = dy;
> >+                //GET_ARG(&sx, argv[1]); /* already got */
> >+                //GET_ARG(&sy, argv[2]); /* already got */
> 
> Delete instead of commenting out.

 gone.  just wanted to make a point.  made now.
 
> >         GET_ARG(&sw, argv[3]);
> >         GET_ARG(&sh, argv[4]);
> >-        GET_ARG(&dx, argv[5]);
> >-        GET_ARG(&dy, argv[6]);
> >+                GET_ARG(&tmpdx, argv[5]);
> >+                GET_ARG(&tmpdy, argv[6]);
> 
> Given that this has a different order than the other calls I'd name the
> arguments arg1 and arg2, keep the local dx and dy variables, set dx to arg1 and
> dy to arg2 where you set the other variables to their initial values, and set
> sx to arg1 and sy to arg2 here. 

 my brain has melted. :)

 ok i think i got it.  looks clearer, too.

> I think we should also make the type for arg1
> and arg2 be double in idl, given that we eventually want doubles here anyway
> (and add a comment explaining that in the idl).

 ack.
 
> >diff -ruw xulrunner-1.9.0.11.orig/dom/public/idl/canvas/nsIDOMCanvasRenderingContext2D.idl xulrunner-1.9.0.11/dom/public/idl/canvas/nsIDOMCanvasRenderingContext2D.idl
> 
> >+//interface nsIDOMHTMLImageElement;
> 
> Delete instead of commenting out.

 ack.
 
> >+  /* https://bugzilla.mozilla.org/show_bug.cgi?id=502234#c14
> >+     use nsIDOMElement here not nsIDOMHTMLImageElement
> >
> >+   */
> 
> Replace that comment with one saying that this function accepts
> HTMLImageElement, HTMLCanvasElement or HTMLVideoElement.

 ok.  done.

 patch on its way....

Comment 33

[lkcl]

(In reply to comment #31)
> And please remove changes that aren't relevant to this bug from your patches
> before attaching them.

 whoops, ah, drat - thank you for tolerating that lot.  that wasn't supposed to be in there - i'm using the debian-patched code and extracting the relevant bits that aren't ... never mind.  i'll try to make sure i pay attention :)

Comment 34

[lkcl]

Attachment: alternative much simpler clearer version

peter this is a cleaner clearer version, you choose.  the idea of arg1, arg2 is what clinched it.

Comment 35

[lkcl]

Attachment: previous version answering review comments

sorry peter something went wrong with submit of patch answering last review comments, resubmitting here. this is the "not-so-clean" version which strictly addresses your review request comments.  the second "clean" version is after i thought some more about what you'd asked.

Comment 36

[Peter Van der Beken [:peterv]]

Comment on attachment 390876 [details] [diff] [review]
alternative much simpler clearer version

>diff -ruw xulrunner-1.9.0.11.orig/content/canvas/src/nsCanvasRenderingContext2D.cpp xulrunner-1.9.0.11/content/canvas/src/nsCanvasRenderingContext2D.cpp

>+    if (NS_SUCCEEDED(rv) && ncc) {
> 
>     rv = ncc->GetJSContext(&ctx);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    PRUint32 argc;
>-    jsval *argv = nsnull;
>+        if (NS_SUCCEEDED(rv) && ctx) {
> 
>     ncc->GetArgc(&argc);
>     ncc->GetArgvPtr(&argv);
> 
>-    // we always need at least an image and a dx,dy
>-    if (argc < 3)
>-        return NS_ERROR_INVALID_ARG;
>-
>     JSAutoRequest ar(ctx);

The JSAutoRequest has to be alive during the calls to JS API functions in GET_ARG, so you should put one in the |if (argc == 5)| block and one in the |if (argc == 9)| block (and remove this one). Once you do that I'll r+, so please attach both a regular diff and a diff -w of the patch next time (so I can check the whitespace changes).

Comment 37

[Peter Van der Beken [:peterv]]

You'll also have to change the content/canvas/test/test_2d.drawImage.wrongtype.html test. See attachment 342670 [details] [diff] [review] in bug 459452.

Comment 38

[lkcl]

(In reply to comment #36)
> (From update of attachment 390876 [details] [diff] [review])
> >diff -ruw xulrunner-1.9.0.11.orig/content/canvas/src/nsCanvasRenderingContext2D.cpp xulrunner-1.9.0.11/content/canvas/src/nsCanvasRenderingContext2D.cpp
> >-    // we always need at least an image and a dx,dy
> >-    if (argc < 3)
> >-        return NS_ERROR_INVALID_ARG;
> >-
> >     JSAutoRequest ar(ctx);
> 
> The JSAutoRequest has to be alive during the calls to JS API functions in
> GET_ARG, so you should put one in the |if (argc == 5)| block and one in the |if
> (argc == 9)| block (and remove this one).

 ohh.  i'm glad you know about that!

> Once you do that I'll r+, so please
> attach both a regular diff and a diff -w of the patch next time (so I can check
> the whitespace changes).

 ack.

 haven't got a... ahhh, as in i can just copy what's there? whew.

Comment 39

[lkcl]

Attachment: update

Comment 40

[lkcl]

Attachment: update

Comment 41

[lkcl]

ok sorry didn't add any explanation because i was in transit.  i added something that we forgot about, that is in https://bug459452.bugzilla.mozilla.org/attachment.cgi?id=342670 - a test for the imgElt being NULL.  that matches up with the test which i also modified.  but - the rest of the test mods are there because of the change to the way that the param analysis works, which are inappropriate for this patch, so i left them as-is.

Comment 42

[Peter Van der Beken [:peterv]]

I think after bug 459452 was fixed the way to fix this would be in pyxpcom itself, adding support for optional arguments and optional_argc. From a quick look at the pyxpcom code it looks like it might be in VariantUtils.cpp in PyXPCOM_InterfaceVariantHelper::Init, but I don't really know anything about pyxpcom. Maybe Mark knows.

Comment 43

[lkcl]

(In reply to comment #42)
> I think after bug 459452 was fixed the way to fix this would be in pyxpcom
> itself, adding support for optional arguments and optional_argc. From a quick
> look at the pyxpcom code it looks like it might be in VariantUtils.cpp in
> PyXPCOM_InterfaceVariantHelper::Init, but I don't really know anything about
> pyxpcom.

 well if you didn't know anything about pyxpcom, why in god's name was
 #459452 allowed to be added??

 advice:  back it out.  now.

Comment 44

[Peter Van der Beken [:peterv]]

(In reply to comment #43)
>  well if you didn't know anything about pyxpcom, why in god's name was
>  #459452 allowed to be added??

Because pyxpcom is not the main scripting bridge for Mozilla, XPConnect is. There is no reason that the same fix can't be applied to pyxpcom, we just need someone (preferably with pyxpcom knowledge) to do it.

Comment 45

[Peter Van der Beken [:peterv]]

CC'ing Todd too, maybe he can help out.

Comment 46

[lkcl]

(In reply to comment #44)
> (In reply to comment #43)
> >  well if you didn't know anything about pyxpcom, why in god's name was
> >  #459452 allowed to be added??
> 
> Because pyxpcom is not the main scripting bridge for Mozilla, XPConnect is.
> There is no reason that the same fix can't be applied to pyxpcom, we just need
> someone (preferably with pyxpcom knowledge) to do it.

 yes - and in the mean-time, if you can't find someone, then that
 patch will end up in a major release of xulrunner, and when someone
 comes to compiling python/xpcom, it will break, won't it?

 if i compile python/xpcom, right now, does it work, yes or no?

 if the answer's no, then i trust that this entire issue is listed as
 a critical blocker on a xulrunner release because otherwise there are
 going to be some users of pyjamas desktop who are going to be extremely
 **** off when xulrunner filters through to debian, ubuntu, gentoo,
 archlinux etc.

 the nightmare we're going to have, alerting all the distributions that
 that particular [stupid, ill-conceived] patch must be patched out as part
 of a release, i don't even want to think about it.

 not to mention all the other projects using python/xpcom, such as miro.
 i don't believe they're using xpcom DOM features to the same extent as
 pyjamas desktop, but if they even try, with things the way they are,
 they'd be just as hosed.
 
 you guys have _responsibilities_ now, you can't just go randomly throwing
 functionality out the window: these APIs have to remain stable or at least
 functional.

 l.

Comment 47

[Peter Van der Beken [:peterv]]

(In reply to comment #46)
>  if i compile python/xpcom, right now, does it work, yes or no?

Why wouldn't it work? And why don't you try, since you have an interest in it? The situation before or after that patch should be exactly the same, you can't call certain XPCOM APIs from python. You couldn't before because you couldn't pass arguments, you can't now because they have optional arguments. The only thing that has changed is where it needs to be patched. I am sorry that we made you spend time on a patch that eventually became obsolete, but even if we'd taken it we'd now be in the same spot.

>  [stupid, ill-conceived] patch

The patch is not stupid and ill-conceived, it was the right way to fix this problem.

If you want to be constructive you should try to get the maintainers of the PyXPCOM bridge to fix this bug or try to patch it there yourself. Complaining to me is hardly going to be effective, I'm not a contributor to the PyXPCOM bridge and I don't have an interest in it (even though I would like for this issue to be fixed).

Comment 48

[Todd Whiteman]

Right - bug 459452 certainly looks like the correct way to go.

As long as there is a way for Python to query if the XPCOM method supports optional arguments (I think there already is an "IsOptional" method for querying parameters), then it shouldn't be too difficult to fix in pyxpcom.

Comment 49

[lkcl]

(In reply to comment #48)
> Right - bug 459452 certainly looks like the correct way to go.
> 
> As long as there is a way for Python to query if the XPCOM method supports
> optional arguments (I think there already is an "IsOptional" method for
> querying parameters), then it shouldn't be too difficult to fix in pyxpcom.

 "shouldn't be too difficult".  right.  has the work _been_ done?  no.

 so, the next release screws up python xpcom, yes or no?  yes.

 so why is #459452 not in a separate branch, pending this "not too difficult work" actually being done?

Comment 50

[lkcl]

(In reply to comment #47)
> (In reply to comment #46)
> >  if i compile python/xpcom, right now, does it work, yes or no?
> 
> Why wouldn't it work? And why don't you try,

 pay me some money, and i will.

> since you have an interest in it?

 i have an interest in expanding the reach of xulrunner technology into
 other dynamic languages, such as python, providing the *full* DOM
 functionality.

 the mozilla foundation have made it absolutely bluntly clear that they
 have a sole and exclusive and pathological interest in making speed,
 speed, speed the absolute top 100% be-all and end-all priority, with
 absolutely everything else placed at zero importance.

 on the OLPC mailing list, it was made absolutely clear, by one of the
 top people working for the mozilla foundation, that the mozilla foundation
 has absolutely no interest in helping people who want to use the xulrunner
 API via dynamic languages such as python; that it is abandoning them
 and that they are entirely on their own.

 [ and i'm supposed to be happy about this, peter ?  why would it
   come as a surprise to you that i'm pretty **** off at having
   to consider maintaining separate branches of a multi-million-line
   MAJOR free software project, just so that pyjamas-desktop users
   have a complete and fullly supported xulrunner API to work with??
   
   you gotta be xxxxing kidding me to think that that's something
   i would be happy with, right?  
  ]

 so as a result of the conflict of these two goals, i anticipate my advice
 and expertise being totally ignored on this issue, and i'm not in
 the slightest bit interested in having my time wasted.

 i deleted the xulrunner tree i was keeping around, because my time is
 being wasted.

 if you're going to make decisions which mess up people's projects,
 you're on your own...

 ... until i get some communicating indicating that there is an interest
 in implementing coclasses, and providing the proper infrastructure and
 documentation to educate people about the ways to do "fast" as well as
 "easy" binding, in order to have both highly optimised code suitable for
 c++ as well as the easily readable code suitable for dynamic programming
 languages.

 or, that the stupid patch - and i am not going to apologise for calling
 #459452 stupid, or call it anything other than what it is (and you'll find
 out that it's a stupid decision, but you're not going to listen to me
 anyway so i will speak unreservedly and freely and make no apologies for it)
 - is placed into a branch, pending completion of this "not too difficult work"
 getting python-xpcom working with it.

 l.

Comment 51

[lkcl]

(In reply to comment #47)
> (In reply to comment #46)
> >  if i compile python/xpcom, right now, does it work, yes or no?
> 
> Why wouldn't it work? And why don't you try, since you have an interest in it?

 peter: think through the implications of what's been done.

 * for the first time ever in the world, a project (pyjamas-desktop)
   begins to actually make comprehensive and full use of the python
   DOM bindings to XPCOM.  not just a few bits and pieces here, just
   to "throw up a webuhh paguhh" but actually hammers the ENTIRE range
   of DOM functions in a way that makes xpcomext look like a toy.

 * on top of python xpcom bindings, pyjamas desktop turns xulrunner
   into a powerful platform-independent Desktop Widget Set that rivals
   qt4 and gtk2.

 * _just_ at the point where this is demonstrated, proven, and functional
   (bar this one issue #502234), and _just_ at the point where people are
   beginning to deploy pyjamas-desktop applications LIVE, someone goes and
   creates - WITHOUT consulting the people affected and WITHOUT due
   consideration - a stupid patch #459452 which completely destroys any
   opportunity to use xulrunner as a pyjamas-desktop back-end.

 * then, you ask me, "well, why don't you do some work and fix it?"

 well the answer's no.  and the reason is because you didn't damn well ask
 me in advance if it was okay, and, when i quite by accident found out that
 the stupid patch #459452 was being put in place,  i said "this patch
 causes problems" you've completely ignored my input.

 so if you expect me to be nice and reasonable, in light of my input being
 completely ignored, and in light of expectations that i should shoulder the
 workload to fix problems caused by lack of consideration, technical ignorance
 and lack of foresight, you can absolutely forget it.

 on the other hand, if the mozilla foundation is willing to reverse the stupid
 policy of focussing on "absolute speed to the maximum extent and screw any
 project that's relying on existing APIs", and is willing to _listen_ to
 people who are relying on existing APIs, then i'll spend _my_ time seeing
 if i can dig up some resources from somewhere.

 god knows they won't me my own personal resources, with $50,000 debt
 and income about 1/2 to 1/3 of the average IT employee, i got better
 things to do than deal with problems caused by mozilla foundation stupid
 decisions.

Comment 52

[Boris Zbarsky [:bzbarsky]]

[e-mail], stop with the personal abuse.  It's not acceptable.

1)  You seem to have some fundamental misunderstandings about the status of python-xpcom.  It was never supported by the Mozilla Foundation, nor ever promoted by it.  It is in the mozilla core tree, and interested parties can use it as it fits their needs, but that does not imply a support commitment.  We obviously try to not break consumers if we can avoid it.  But you seem to be under the impression that someone owes you something.  That's just not the case.  The code is there.  It works as well as it ever has.  You are welcome to use it or not as it fits your needs.  You are welcome to improve it as you choose to do so, and share your improvements with others (or not).

2)  You seem to also misunderstand the patch in bug 459452 at a fundamental level.  What it did is change from having C++ methods directly poke at the JS stack to look for information to instead declaring the information the methods take in IDL and moving the JS spec inspection into the JS to C++ binding layer.  What this means is that now it is possible to make a similar change to the Python to C++ binding layer and get those methods working from Python.  Before that change, there was no way whatsoever to make those methods work from python (or from C++, for that matter!).

You seem to think that a more general solution would have allowed the Python binding layer to work "automatically" without any additional work.  That's entirely possible, at some cost to other parts of the system (including compatibility with existing deployed web code, note).  It was judged that a bit of extra work on the Python binding to allow it to call methods that it could never call before but now can was a smaller price to pay than having to have web pages change their code (the latter is essentially impossible).

Now as I see it, you have several choices.  You can keep up with the "people owe me" attitude or you can acknowledge that Peter's fix actually made it possible for the Python binding to be extended to more methods.  You can take your marbles and go home, or you can work on getting the Python binding thus extended (whether yourself, by trying to convince Todd or someone else to work on it, by paying someone to work on it, whatever).  This is the whole point of open source: you're empowered to fix the things that you need fixed, in a wide variety of ways.

To answer your salient question, though:

> so, the next release screws up python xpcom, yes or no?  

Not anymore than it was already screwed up.

Now, you were interested in working on this bug before (by changing the C++ code to be somewhat more python-friendly); that can still be done as desired, or you could make the python binding layer just solve this problem for all python consumers once and for all like the JS binding layer now does.  Or you could pick up your marbles and walk off in a huff, like I said.  It's really up to you, but the random rants and personal abuse are really not acceptable.

Comment 53

[Boris Zbarsky [:bzbarsky]]

And just to be clear, pyxpcom is in the mozilla source repository largely as a convenience for the people who work on it and use it.  It's not there because the Mozilla Corporation promises to support it or anything.  The same is true of various other modules (javaxpcom comes to mind, but there are various others).

In the past, pyxpcom has largely been maintained by volunteer contributors and ActiveState employees, as far as I can tell.

Comment 54

[Benjamin Smedberg]

Actually, pyxpcom was moved out of mozilla-central to its own repository to make its status clear and make it easier to maintain, it's now located at http://hg.mozilla.org/pyxpcom

Comment 55

[lkcl]

(In reply to comment #52)
> [e-mail], stop with the personal abuse.  It's not acceptable.

 if you look carefully at what i've written, there _should_ be absolutely no
 personal criticisms.

 i have a lot of practice, as you may well be aware, at criticising _decisions_
 rather than people.

 so, if you read what i've written (again - carefully) and deem it to be
 "personal abuse", i think you'll find that that connection is nowhere but
 inside your own head.

 the difference being that i use phrases "that is a stupid bit of code",
 not "the PERSON who wrote that code is stupid".

 if i'm wrong about that, and have made the mistake of using personal
 pronouns, anywhere, i'm deeply sorry.


> it as it fits their needs, but that does not imply a support commitment.  We
> obviously try to not break consumers if we can avoid it.

 well, you're just about to.


> 2)  You seem to also misunderstand the patch in bug 459452 at a fundamental
> level.

 yes.  without consulting those people relying on it as to whether they
 would like to be forced to spend time fixing the python interface.

 which is _why_ i have requested - repeatedly - that #459452 go into a branch
 pending finding the people willing to actually do that.

 rather than them effectively being blackmailed into doing the work.

> To answer your salient question, though:
> 
> > so, the next release screws up python xpcom, yes or no?  
> 
> Not anymore than it was already screwed up.

 wrong.

 the only flaw found which is intolerable - unbelievably - is this one in nsiHTMLCanvas.drawImage.

 that may be hard to believe, but it is a testament to the fantastic work done (so far) and to the incredibly far-sighted design of xpcom that absolutely, absolutely everything works - _except_ this one function.

 there are some cases - other places where optional args are used (and don't work in python) such as XMLHttpRequest.open, where it would be "nice" to be able to set the optional arguments (such as username, password and sync/async) BUT, the thing is that the defaults of NULL, NULL and async=True are  "good enough".

 so i'm not making this up - but what's there, right now, is quite literally only broken in _one_ semi-important way, and slightly broken in a not-really-that-vital second way, and THAT's IT.

 out of several thousand functions and properties!

 however: right now, what you're doing, **** _everything_ up.  XMLHttpRequest? gone.  out the window.  HTMLCanvas.drawImage?  gone.

 so will you _please_, pretty please with sugar on top, create a nice branch, put this one teeeny little patch in which will give us HTMLcanvas.drawImage with three fixed arguments (image, x and y) which is "good enough" for most people's purposes, and then ASK if there is anyone out there who would like to work on python xpcom and javaxpcom etc.

 because right now, you're effectively blackmailing people such as myself, the pyjamas desktop users, activestate and others into fixing some code by *deliberately* breaking what is otherwise pretty much useable and useful.

and it might be the case that we simply don't have the time, money or the same level of expertise and knowledge that you do.  certainly, the pyjamas desktop users, they're python-heads not expert c++ programmers, come on!  these are _users_, and you're letting them down.

Comment 56

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 57

[Todd Whiteman]

Attachment: Provide support for optional params to/from Python XPCOM

This adds support to and from Python for XPCOM optional parameters.

A good example for testing to C++ from Python (if you've got a firefox/xulrunner instance) is to call the blocklistService from Python:

from xpcom import components as Components
blockSvc = Components.classes["@mozilla.org/extensions/blocklist;1"].createInstance(Components.interfaces.nsIBlocklistService)
print blockSvc.isAddonBlocklisted("[e-mail]", "")

Comment 58

[Mook (work)]

Comment on attachment 551193 [details] [diff] [review]
Provide support for optional params to/from Python XPCOM

Review of attachment 551193 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/client/__init__.py
@@ +74,4 @@
>          param_name = "Param%d" % (param_no,)
>          param_default = ""
>          if not param.hidden_indicator and param.IsIn() and not param.IsDipper():
> +            if param.IsOptional() or param.IsOut() or used_default: # If the param is "inout", provide a useful default for the "in" direction.

nit: might be time to move the comment to its own line.

::: xpcom/src/VariantUtils.cpp
@@ +924,5 @@
>  	PRBool have_set_auto; 
>  };
>  
> +static void ProcessPythonTypeDescriptors(PythonTypeDescriptor *pdescs, int num,
> +					 int &min_num_params, int &max_num_params)

consider using pointers instead of reference for the last two args, to make it clearer that they will get modified during the call.

@@ +1307,5 @@
>  			ns_v.val.d = PyFloat_AsDouble(val_use);
>  			break;
>  		  case nsXPTType::T_BOOL:
> +			if (val==Py_None) {
> +				ns_v.val.b = 0;

Isn't that a PRBool (and therefore should be PR_FALSE)?

::: xpcom/test/test_optional_params.py
@@ +18,5 @@
> +# Portions created by the Initial Developer are Copyright (C) 2011
> +# the Initial Developer. All Rights Reserved.
> +#
> +# Contributor(s):
> +#   Mook <[e-mail]>

I don't think I wrote this file.  That or I had some really got Tequila and forgot about it and the drinking part...

Comment 59

[Todd Whiteman]

Checked in with Mook's suggestions:
http://hg.mozilla.org/pyxpcom/rev/73d7e4f52d02

Comment 60

[Todd Whiteman]

Actually, this was the following checkin instead:
http://hg.mozilla.org/pyxpcom/rev/7f16f6d87730

Comment 61

[lkcl]

this is still an outstanding (unfixed) issue in python-xpcom.  the multiple optional
arguments simply don't work.  specifically:

* using the 2-option version of XMLHttpRequest.open now actually causes the
  application to completely lock up!  not even a segfault is issued.
  we are now forced to utilise the 5-arg version, putting in explicit values
  True, '' and '' for what _should_ be default arguments

* using the 3-option version, 5-option version or 9-option version of
  Canvas.DrawImage all result in "INVALID ARGUMENT".

the original patch that i created at least enabled the 3-argument version to work.
we're now 2 and a half years on from the date when the original patch was created,
yet the "approved" patch doesn't even do the job it was supposed to do, _and_ you
didn't listen to my advice!

if you'd listened to my advice and enabled the 3-argument version in the first
place then the "approved" patch which didn't do the damn job would have made
absolutely no difference in its complete ineffectiveness.

Comment 62

[Boris Zbarsky [:bzbarsky]]

The original patch was wrong.  The spec requires that the arguments be passed as floats, not doubles.  The difference is trivially detectable; they have different rounding behavior.

You're welcome to fix things however you want on the pyxpcom end, but you're not welcome to break the API in the process for other consumers....

Comment 63

[lkcl]

(In reply to Boris Zbarsky (:bz) from comment #62)
> The original patch was wrong. 

 whatwhatwhat? :)  it was?  bugger!

> The spec requires that the arguments be
> passed as floats, not doubles.  The difference is trivially detectable; they
> have different rounding behavior.

 achh, my mistake - i didn't notice.
 
> You're welcome to fix things however you want on the pyxpcom end, but you're
> not welcome to break the API in the process for other consumers....

 heck, that makes perfect sense to me. ha! *excited again* - opportunity to fix
 things.  i'll grab the source code and have another go at it.

 thanks boris.

Comment 64

[lkcl]

yep, you're right, boris: the spec was changed (inconsistently as it turns out) -
it made sense at the time because all the internal params are doubles.

ahh... looking at the code here there's no way that the original patch can
be applied, as the function's prototype is now this:

nsCanvasRenderingContext2D::DrawImage(nsIDOMElement *imgElt, float a1,
                                      float a2, float a3, float a4, float a5,
                                      float a6, float a7, float a8,
                                      PRUint8 optional_argc)

which doesn't explain why there's the "invalid argument" response to this function call.

ok, so this bug's not appropriate, meaning a new one's needed, realistically.