addOneTimeListener should return a promise

RESOLVED FIXED in Firefox 59

Status

P3
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: ochameau, Assigned: nchevobbe)

Tracking

unspecified
Firefox 59

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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)

Updated

a year ago
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
status-firefox57: --- → fix-optional
Priority: -- → P3
(Assignee)

Comment 1

a year ago
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 hidden (mozreview-request)
(Reporter)

Comment 3

a year ago
mozreview-review
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)
(Assignee)

Comment 4

a year ago
mozreview-review-reply
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 :)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
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).
(Reporter)

Comment 7

a year ago
mozreview-review
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+
(Assignee)

Comment 8

a year ago
mozreview-review-reply
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 :)
(Assignee)

Comment 9

a year ago
mozreview-review
Comment on attachment 8925437 [details]
Bug 1383711 - Make addOneTimeListener return a Promise; .

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

Comment 10

a year ago
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
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

6 months ago
Product: Firefox → DevTools
status-firefox57: wontfix → ---
status-firefox59: fixed → ---
You need to log in before you can comment on or make changes to this bug.