1332965 - addEventListener shim should work with an option object given as third parameter

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Florian Quèze [:florian]]

In browser chrome mochitests, replacing:
  browser.addEventListener("load", listener, true);
with
  browser.addEventListener("load", listener, {capture: true});
breaks lots of tests.

I discovered this while working on bug 1331599.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

This turned out to be more complicated than I initially thought. The patch includes comments to explain the parts that were hard to figure out.

The version of the patch I'm attaching is green on the latest try push from bug 1331599 comment 8.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8829282 [details] [diff] [review]
Patch

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

I don't really want to add to this code if possible. Can we instead just avoid doing anything special if "once" is set? You won't get the benefit of the compatibility shims, but new code shouldn't be using them anyway.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8829282 [details] [diff] [review]
Patch

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

Florian explained to me that he's trying to do a mass rewrite of our tests to use once=true, so it's hard to avoid fixing this code.

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +646,5 @@
> +    // Handling the 'once' option without leaking a reference to our target in
> +    // EventTargetParent requires that we set the listener directly on the
> +    // target for events that won't come from a child process.
> +    // Otherwise the listener directly attached to the target will self-remove,
> +    // and the listener attached in EventTargetParent will stay around.

Rather than this code (which can easily break), I think you could do something like makeFilteringListener that removes the listener if once=true.

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch v2

Now using an approach similar to makeFilteringListener.

It's green enough on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b2e1e46ec20

Note: if a listener is first added with {once: true}, then added without {once: true}, and finally removed, the code in this patch will remove the listener added with {once: true} and the other one will remain. MDN doesn't document the expected behavior for this edge case, so I don't think it's worth worrying about it. In my opinion if someone writes new code that depends on the shims and on this edge case, that person is asking for trouble and shouldn't be surprised if the behavior isn't perfect.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8829895 [details] [diff] [review]
Patch v2

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

Looks great. Thanks!

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +660,5 @@
>        addon, CompatWarning.warnings.DOM_events);
>  
> +    let useCapture =
> +      options === true || (typeof options == "object" && options.capture) || false;
> +    if (typeof options == "object" && options.once)

Please put braces around the conditional.

@@ +674,5 @@
> +  function(addon, target, type, listener, options) {
> +    let useCapture =
> +      options === true || (typeof options == "object" && options.capture) || false;
> +
> +    if (selfRemovingListeners.has(listener))

Same here.

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7181326507e1c832265f9716e6272ddc6c96242
Bug 1332965 - addEventListener shim should work with an option object given as third parameter, r=billm.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7181326507e