Closed Bug 1383711 Opened 3 years ago Closed 2 years ago

addOneTimeListener should return a promise

Categories

(DevTools :: Debugger, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 59

People

(Reporter: ochameau, Assigned: nchevobbe)

References

Details

Attachments

(1 file)

http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#65

It would prevent having callback-style code in many places in debugger codebase, especially in tests!

Or we could switch to EventEmitter.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: -- → P3
This is not purely dependent on Bug 1403895, but since there would be a lot of things moving around, we'd better wait until it lands before tackling this bug.
Depends on: 1403895
Comment on attachment 8925437 [details]
Bug 1383711 - Make addOneTimeListener return a Promise; .

https://reviewboard.mozilla.org/r/196554/#review201792

::: devtools/shared/client/event-source.js:61
(Diff revision 1)
> -    let l = (...args) => {
> +      let l = (...args) => {
> -      this.removeListener(name, l);
> +        this.removeListener(name, l);
> +        if (listener) {
> -      listener.apply(null, args);
> +          listener.apply(null, args);
> +        }
> +        resolve(args);

Did you tried using that somewhere, like in tests?


For example, this function here:
http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg_event-listeners-01.js#42-57
would become that:

async function pauseDebuggee(aThreadClient) {
  let onPaused = gClient.addOneTimeListener("paused");

  generateMouseClickInTab(gTab, "content.document.querySelector('button')");

  let [_, packet] = await onPaused;
  is(packet.type, "paused",
    "We should now be paused.");
  is(packet.why.type, "debuggerStatement",
    "The debugger statement was hit.");

  return aThreadClient;
}

But this:
  let [_, packet] = await onPaused;
Doesn't match event-emitter's once() behavior, which resolved only the first value.

http://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.js#142

It looks like Debugger clients only use one argument, in most cases it is only the RDP packet. So may be we can stick to event-emitter behavior so that it would be easier to switch to it?
Attachment #8925437 - Flags: review?(poirot.alex)
Comment on attachment 8925437 [details]
Bug 1383711 - Make addOneTimeListener return a Promise; .

https://reviewboard.mozilla.org/r/196554/#review201792

> Did you tried using that somewhere, like in tests?
> 
> 
> For example, this function here:
> http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg_event-listeners-01.js#42-57
> would become that:
> 
> async function pauseDebuggee(aThreadClient) {
>   let onPaused = gClient.addOneTimeListener("paused");
> 
>   generateMouseClickInTab(gTab, "content.document.querySelector('button')");
> 
>   let [_, packet] = await onPaused;
>   is(packet.type, "paused",
>     "We should now be paused.");
>   is(packet.why.type, "debuggerStatement",
>     "The debugger statement was hit.");
> 
>   return aThreadClient;
> }
> 
> But this:
>   let [_, packet] = await onPaused;
> Doesn't match event-emitter's once() behavior, which resolved only the first value.
> 
> http://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.js#142
> 
> It looks like Debugger clients only use one argument, in most cases it is only the RDP packet. So may be we can stick to event-emitter behavior so that it would be easier to switch to it?

Sounds good to me.
I wanted to refactor the existing addOneTimeListener calls in follow-ups so we can handle them in chunks.
I can do a few here maybe.
Also, I'm not sure of doing it for the debugger tests (like the on ein your code example), since they are run only in the old debugger (http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/head.js#35). I'll ask the debugger team about this.

Thanks for the review alex :)
TRY push https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6ca5dca578f2b3d914d7c1baf35cb2e4f332510

I only refactored a few calls, the easiest one, and I'll file bugs to deal with other ones (in particular server tests, some of them look really messy).
Comment on attachment 8925437 [details]
Bug 1383711 - Make addOneTimeListener return a Promise; .

https://reviewboard.mozilla.org/r/196554/#review204906

Thanks a lot for this cleanup!
(You modified the old debugger, note that we may just wait for its removal instead of refactoring it)
Attachment #8925437 - Flags: review?(poirot.alex) → review+
Comment on attachment 8925437 [details]
Bug 1383711 - Make addOneTimeListener return a Promise; .

https://reviewboard.mozilla.org/r/196554/#review204906

It was only to ensure addOneTimeListener was working as expected :)
Comment on attachment 8925437 [details]
Bug 1383711 - Make addOneTimeListener return a Promise; .

https://reviewboard.mozilla.org/r/196554/#review204910
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b2d5e508af1
Make addOneTimeListener return a Promise; r=ochameau.
https://hg.mozilla.org/mozilla-central/rev/2b2d5e508af1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.