991797 - Improve code legibility in the Debugger frontend by using Task.async where it makes sense

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Brandon Benvie [:benvie]]

Currently, Task.jsm is using addon-sdk promises. Bug 887923 changes it to use Promise.jsm and it landed and consequently was backed out earlier this week. Hopefully it will re-land very soon. When it does, it might complicate that patch because it'll introduce Promise.jsm promises in code that hasn't been updated to use them.

It would be inadvisable to land this prior to bug 887923 landing, because then converting the debugger client to Promise.jsm promises would become a blocker for it, and it's really important that it get landed ASAP (so there isn't more people inadvertently introducing synchronous promises whenever they use Task.jsm).

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

What Brandon says sounds reasonable to me. Victor are you OK with me postponing the review until that bug lands?

Comment 4

[Victor Porof [:vporof][:vp]]

Of course.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8401432 [details] [diff] [review]
v1

I'm going to need to rebase this anyway.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v2 (rebased)

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v2.1

This patch (to be applied on top of v2) also tries to switch onFrames in debugger-controller.js to not be crazy anymore. However, a few tests started failing. James, as discussed, feel free to take a look at these patches and/or take the bug if you feel like it.

Comment 8

[James Long (:jlongster)]

Attachment: v2 (rebased again)

This is the v2 patch rebased again with a few more tweaks to get tests passing. This requires bug 995252 to be landed first because I already was already switching some stuff to Task.async there and had already fixed some tests. I rebased the v2 patch including those changes.

Thought I'd put this up here now though in case you want to go ahead and review it. I just had to tweak a few places to get tests passing again.

Comment 9

[James Long (:jlongster)]

Attachment: v2 (rebased again)

Oops, my patch had lost some of your changes in debugger-controller.js from the previous patch. Included in here, all tests passing.

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8415463 [details] [diff] [review]
v2 (rebased again)

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

::: browser/devtools/debugger/debugger-controller.js
@@ +43,5 @@
>    // When a breakpoint has been added or removed on the debugger server.
>    BREAKPOINT_ADDED: "Debugger:BreakpointAdded",
>    BREAKPOINT_REMOVED: "Debugger:BreakpointRemoved",
>  
>    // When a breakpoint has been shown or hidden in the source editor.

This comment isn't 100% accurate now.

@@ +50,1 @@
>    BREAKPOINT_HIDDEN: "Debugger:BreakpointHidden",

This is now weird. Breakpoint hidden where? It'd be a good idea to make this more specific, and even add the symmetric BREAKPOINT_REMOVED_IN_WHEREVER event.

@@ +1677,5 @@
> +      // Notify that event listeners were fetched and shown in the view,
> +      // and callback to resume the active thread if necessary.
> +      window.emit(EVENTS.EVENT_LISTENERS_FETCHED);
> +      aCallback && aCallback();
> +    }.bind(this)));

Here's a question: do we still need .bind(this) when using Task.async? I can't remember. I can't even remember if it was me who wrote the code or not.

::: browser/devtools/debugger/test/browser_dbg_conditional-breakpoints-01.js
@@ +45,5 @@
>        .then(() => resumeAndTestNoBreakpoint())
> +      .then(() => {
> +        return promise.all([
> +          reloadActiveTab(gPanel, gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_EDITOR, 13),
> +          waitForDebuggerEvents(gPanel, gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_PANE, 13)

This is correct, but it may be nicer to write it like this:

return promise.all([
  reloadActiveTab(gPanel),
  gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_EDITOR, 13),
  gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_PANE, 13)
]);

..since we're waiting for more than one event after reloading. Just a nit/thought.

::: browser/devtools/debugger/test/browser_dbg_location-changes-04-breakpoint.js
@@ +103,5 @@
>      waitForDebuggerEvents(gPanel, gDebugger.EVENTS.NEW_SOURCE),
>      waitForDebuggerEvents(gPanel, gDebugger.EVENTS.SOURCES_ADDED),
>      waitForDebuggerEvents(gPanel, gDebugger.EVENTS.SOURCE_SHOWN),
> +    reloadActiveTab(gPanel, gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_EDITOR),
> +    waitForDebuggerEvents(gPanel, gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_PANE)

Ditto here.

::: browser/devtools/debugger/test/browser_dbg_server-conditional-bp-01.js
@@ +40,5 @@
>        .then(() => resumeAndTestNoBreakpoint())
> +      .then(() => {
> +        return promise.all([
> +          reloadActiveTab(gPanel, gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_EDITOR, 13),
> +          waitForDebuggerEvents(gPanel, gDebugger.EVENTS.BREAKPOINT_SHOWN_IN_PANE, 13)

And here.

Comment 11

[James Long (:jlongster)]

> Here's a question: do we still need .bind(this) when using Task.async? I can't remember. I can't even remember if it was me who wrote the code or not.

I think we do. I don't think we need it when wrapping class methods, but that one is creating a callback, so it needs to bind `this`.

> This is correct, but it may be nicer to write it like this: ...

It looks like you can't do that. I changed it and my tests time out. `reloadActiveTab` requires an event: https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/test/head.js#L438

Comment 12

[Victor Porof [:vporof][:vp]]

Is there still something preventing this from landing? If so, it might be a good idea to add the blocking bug.

Comment 13

[James Long (:jlongster)]

It took a while to fix bug 995252, and I'm back on this now. The initial patch is passing on try, but working on patch #2. A test is timing out with the second patch applied and I'm still working on it.

Comment 14

[James Long (:jlongster)]

Attachment: v3

Finalized rebased patch with victor's 2.1 patch merged in with some fixes for tests

Comment 15

[Victor Porof [:vporof][:vp]]

Comment on attachment 8425065 [details] [diff] [review]
v3

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

::: browser/devtools/debugger/debugger-controller.js
@@ +2084,3 @@
>      }
> +    let breakpointClient = yield addedPromise;
> +    var promise = breakpointClient.setCondition(gThreadClient, aCondition);

Nit: this should probably be a let.

Comment 16

[James Long (:jlongster)]

Attachment: v3

Thanks, fixed!

Comment 17

[James Long (:jlongster)]

Try is looking good here: https://tbpl.mozilla.org/?tree=Try&rev=d77c663362ca. A few weirdnesses, but doesn't look like anything related to this.

Comment 18

[James Long (:jlongster)]

Attachment: v3

Forgot to add commit description

Comment 19

[Victor Porof [:vporof][:vp]]

(In reply to James Long (:jlongster) from comment #17)
> Try is looking good here.

This vexes me. I'm cautiously happy about it though!

Comment 20

[James Long (:jlongster)]

(In reply to Victor Porof [:vporof][:vp] from comment #19)
> 
> This vexes me. I'm cautiously happy about it though!

Me too. There are a few uncaught errors that are printed out during the test, but not many. I believe those errors existed before this, and we are just seeing them more prominently now with our better promises. I say that because I've seen the exact same errors before while toying with lots of stuff. I know I should have recorded a test case, but there were too many things going on.

I'd like to land this and then file a follow-up bug to deal with the uncaught exceptions printed during tests. There are only 2 of them.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/fbc736d91427

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[James Long (:jlongster)]

Just curious -- the commit on fx-team came through as:

commit 2dd3993d4d7b7ffc309b2ecb22f139a3306ee126
Author: Wes Kocher <[e-mail]>
Date:   Tue May 20 15:55:39 2014 -0700

    Bug 991797 - convert most of the debugger frontend to use Task.jsm and fix discovered async errors r=victorporof

Usually the commits have my name attached to it, what happened here? I only care because this breaks things like `git blame`.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

It looks like your patch didn't have proper mercurial headers and Wes didn't notice it when landing, so mercurial just used his defaults.