613634 - NS_ERROR_XPC_NOT_ENOUGH_ARGS on rumpetroll.com (investigate if we should have optional useCapture parameter)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josh Matthews [:jdm]]

STR:
1. Visit rumpetroll.com

Expected:
Swimming around in a fun websocket-based game

Actual:
The error appears in the console, no game is present.

This works fine in chrome.

Comment 1

[Josh Matthews [:jdm]]

This is the failing line in main.js:

>document.getElementById('authorize-user-button').addEventListener('click', function(e) { 
>		app.authorize(null,null);
>		authWindow = window.open("auth.html","","width=950,height=460,menubar=no,toolbar=no,location=no,directories=no,status=no,scrollbars=yes,resizable=yes')")
>		return false;
>	});

Comment 2

[Josh Matthews [:jdm]]

This error is occurring because |addEventListener('click', function(e) {})| throws.  CCing people who might know whether this is correct behaviour or not.

Comment 3

[Boris Zbarsky [:bzbarsky]]

It's correct behavior per DOM 2 Events, and this site is assuming Webkit's behavior which doesn't follow that spec, really.

There have been proposals to change it for Web DOM Core; I don't believe we have a bug on making the third arg optional so far.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Still required in DOM3 Events: <http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-EventTarget>. I filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=11358>.

Comment 5

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

Attachment: optional capture param

I pushed this to tryserver

Comment 6

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

Apparently that kind of simple patch breaks quite a few tests.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, at least the ones in the official DOM test suites, right?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorkerMessageHandler.cpp#339

342 NS_IMETHODIMP
343 nsDOMWorkerMessageHandler::AddEventListener(const nsAString& aType,
344                                             nsIDOMEventListener* aListener,
345                                             PRBool aUseCapture,
346                                             PRBool aWantsUntrusted,
347                                             PRUint8 optional_argc)
348 {
349   // We don't support aWantsUntrusted yet.
350   NS_ENSURE_TRUE(optional_argc == 0, NS_ERROR_NOT_IMPLEMENTED);

This isn't going to fly anymore...

Comment 9

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

We may want to fix this for FF4.
If I understood Travis (from MS) correctly, IE9 may make the useCapture
optional too.

Comment 10

[Cameron McCormack (:heycam)]

As mentioned in http://www.w3.org/mid/[e-mail] we came to the conclusion that Web IDL should allow too few arguments, and coerce them from undefined, (and allow too many, and ignore the extra arguments) as the most JavaScript-y way to handle mismatched arity.  Nobody has spoken up against this proposal on the list yet.  (The change is yet to have been made to the spec.)

The result of this is that it wouldn't matter if useCapture is optional or not -- omitting it would require the missing argument to be treated as undefined and thus be converted to false.

Comment 11

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

Not sure if this really needs to *block*.

Comment 12

[Johnny Stenback (:jst)]

Agreed, we can get to this later.

Comment 13

[Frank Yan (:fryn)]

Is there a technical or spec holdup to fixing this?

We've already made the second parameter of window.getComputedStyle to be optional.

Comment 14

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

(In reply to comment #13)
> Is there a technical or spec holdup to fixing this?
No. I just need to find time to fix this.

Comment 15

[Cameron McCormack (:heycam)]

Also note that Web DOM Core and DOM 3 Events have changed to make the argument optional:

  http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#eventtarget
  http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-EventTarget-addEventListener

Comment 16

[Boris Zbarsky [:bzbarsky]]

Olli, is there anything to do here other than adding "optional" to the idl and writing tests?

Comment 17

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

I did try that [optional] approach and something broke.
The reason is, IIRC, that we already have one optional parameter, so need to
update optional_argc handling.

Comment 18

[Frank Yan (:fryn)]

(In reply to comment #17)
> I did try that [optional] approach and something broke.
> The reason is, IIRC, that we already have one optional parameter, so need to
> update optional_argc handling.

This problem only affects the nsIDOMNSEventTarget interface, which web content doesn't use, AIUI. Could we could first land a patch that makes useCapture optional only for nsIDOMEventTarget?

Comment 19

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

Web content does use addEventListener from nsIDOMNSEventTarget.
addEventListener in nsIDOMEventTarget is [noscript]

Comment 20

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

Attachment: patch

Uploaded to tryserver

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 527936 [details] [diff] [review]
patch

Maybe also test that

x.addEventListener("foo", l);
x.removeEventListener("foo, l, false);

removes the event listener?

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/f0ad024522bb

Comment 24

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMEventTarget
https://developer.mozilla.org/en/DOM/element.addEventListener

And mentioned on Firefox 6 for developers.