Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Description • 8 years ago

With bug 1287706 we got support for one-time listeners which would not require an extra call to removeEventListener. I think it would be kinda handy in Marionette and would save us a couple of lines in the marionette-server.

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 8 years ago

This is potentially _very_ exciting seeing as Marionette has had tonnes of bugs around failing to remove event listeners.  Increasing the importance of this bug.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 2 • 8 years ago

I talked with Andreas on IRC and he is happy to mentor this bug. So what needs to be done. First, get familiar with Marionette (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette) and run our existent unit tests for the marionette driver. If that works, make sure to read all about event handling and using addEventListener (https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener).

What we have to do on this bug is to change all of those calls, which definitely get called only once before removeEventListener() is called to unregister the formerly handler. Handlers which will be called multiple times should not be changed. All the instances of addEventListener can be found here:

https://dxr.mozilla.org/mozilla-central/search?q=path%3Atesting%2Fmarionette+addEventListener&redirect=true

	Nishanth Mane [:nishu-tryinghard]
	Comment 3 • 7 years ago

I have been reading about marionette and feel that I understand how server, client and gecko are linked. So I feel I can contribute to this bug.
 
Coming to the actual bug, I am trying to debug the events in driver.js and trying to find if it is called once. But i can see that the listener for the 
    win.removeEventListener("load", listener);
in 
    GeckoDriver.prototype.newSession"
makes a check if ev.target is win.document so i think its safe to assume that we cannot refactor this event.

And also the listener for the
    win.removeEventListener("load", listener);
in
    GeckoDriver.prototype.addFrameCloseListener
looks similar to the above scenario

Now coming to the things i am finding difficult to work on. Next i thought of debugging the event in evaluate.js and I dont have a clue on how to actually trigger that code. I guess it is probably via client side python api's?

	Nishanth Mane [:nishu-tryinghard]
	Comment 4 • 7 years ago

(In reply to Nishanth Mane [:nishu-tryinghard] from comment #3)
> I have been reading about marionette and feel that I understand how server,
> client and gecko are linked. So I feel I can contribute to this bug.
>  
> Coming to the actual bug, I am trying to debug the events in driver.js and
> trying to find if it is called once. But i can see that the listener for the 
>     win.removeEventListener("load", listener);
> in 
>     GeckoDriver.prototype.newSession"
> makes a check if ev.target is win.document so i think its safe to assume
> that we cannot refactor this event.
> 
> And also the listener for the
>     win.removeEventListener("load", listener);
> in
>     GeckoDriver.prototype.addFrameCloseListener
> looks similar to the above scenario
> 
> Now coming to the things i am finding difficult to work on. Next i thought
> of debugging the event in evaluate.js and I dont have a clue on how to
> actually trigger that code. I guess it is probably via client side python
> api's?

its win.addEventListener not win.removeEventListener

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 7 years ago

(In reply to Nishanth Mane [:nishu-tryinghard] from comment #3)
> Coming to the actual bug, I am trying to debug the events in driver.js and
> trying to find if it is called once. But i can see that the listener for the 
>     win.removeEventListener("load", listener);
> in 
>     GeckoDriver.prototype.newSession"
> makes a check if ev.target is win.document so i think its safe to assume
> that we cannot refactor this event.

We can probably not make this use {once: true}, no.

> And also the listener for the
>     win.removeEventListener("load", listener);
> in
>     GeckoDriver.prototype.addFrameCloseListener
> looks similar to the above scenario

Yes.

> Now coming to the things i am finding difficult to work on. Next i thought
> of debugging the event in evaluate.js and I dont have a clue on how to
> actually trigger that code. I guess it is probably via client side python
> api's?

self.marionette.execute_script

On another note, there are a number of cases in testing/marionette/listener.js where we can employ {once: true} that might be easier to tackle.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago

refresh() will be fixed as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1291320.  I believe this is the last remaining use of event listeners that can (easily) be replaced with {once: true}.

Thanks to nishu for making us aware of this!