Closed Bug 991797 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: vporof, Assigned: jlong)

References

Details

Attachments

(1 file, 7 obsolete files)

No description provided.
Summary: Improve code legibility in the Debugger by using Task.async where it makes sense → Improve code legibility in the Debugger frontend by using Task.async where it makes sense
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8401432 - Flags: review?(past)
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).
Depends on: 887923
What Brandon says sounds reasonable to me. Victor are you OK with me postponing the review until that bug lands?
Of course.
Comment on attachment 8401432 [details] [diff] [review]
v1

I'm going to need to rebase this anyway.
Attachment #8401432 - Attachment is obsolete: true
Attachment #8401432 - Flags: review?(past)
Attached patch v2 (rebased) (obsolete) — Splinter Review
Attached patch v2.1 (obsolete) — Splinter Review
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.
Assignee: vporof → jlong
Attached patch v2 (rebased again) (obsolete) — Splinter Review
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.
Attachment #8412669 - Attachment is obsolete: true
Attachment #8414984 - Flags: review?(vporof)
Attached patch v2 (rebased again) (obsolete) — Splinter Review
Oops, my patch had lost some of your changes in debugger-controller.js from the previous patch. Included in here, all tests passing.
Attachment #8414984 - Attachment is obsolete: true
Attachment #8414984 - Flags: review?(vporof)
Attachment #8415463 - Flags: review?(vporof)
Depends on: 995252
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.
Attachment #8415463 - Flags: review?(vporof) → review+
> 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
Is there still something preventing this from landing? If so, it might be a good idea to add the blocking bug.
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.
Attached patch v3 (obsolete) — Splinter Review
Finalized rebased patch with victor's 2.1 patch merged in with some fixes for tests
Attachment #8412861 - Attachment is obsolete: true
Attachment #8415463 - Attachment is obsolete: true
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.
Attached patch v3 (obsolete) — Splinter Review
Thanks, fixed!
Attachment #8425065 - Attachment is obsolete: true
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.
Keywords: checkin-needed
Attached patch v3Splinter Review
Forgot to add commit description
Attachment #8425552 - Attachment is obsolete: true
(In reply to James Long (:jlongster) from comment #17)
> Try is looking good here.

This vexes me. I'm cautiously happy about it though!
(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.
https://hg.mozilla.org/mozilla-central/rev/fbc736d91427
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Just curious -- the commit on fx-team came through as:

commit 2dd3993d4d7b7ffc309b2ecb22f139a3306ee126
Author: Wes Kocher <wkocher@mozilla.com>
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`.
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.