906249 - The number of lines in onResume is TOO DAMN HIGH

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Follow up to bug 878307, which makes onResume even bigger. Over 200 lines. With closure definitions all over the place. This is way too big to be able to follow properly, you can't even see where variables were defined.

Time to refactor and make it smaller!

Comment 1

[Jim Blandy :jimb]

Hear, hear. This part of GDB is a monstrosity, too.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-906249-onResume-too-damn-high.patch

Just shuffling code around, not functionality changes here or anything.

https://tbpl.mozilla.org/?tree=Try&rev=88da3888e52a

Comment 3

[Jim Blandy :jimb]

Total side note: it occurred to me that synchronize is essentially the same as Task.jsm, except that the latter will handle resolutions in any order, whereas synchronize only handles them in a stack-like order.

I don't know what the pros and cons are in practice. It might be worth experimenting to find out.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ping? The dependency for this bug landed in fx-team, would love to have this code clean up as well.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 792339 [details] [diff] [review]
bug-906249-onResume-too-damn-high.patch

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

::: toolkit/devtools/server/actors/script.js
@@ +733,5 @@
> +
> +    this.dbg.getNewestFrame().pop(aRequest.completionValue);
> +    let packet = this._resumed();
> +    this._popThreadPause();
> +    return { type: "resumeLimit", frameFinished: aRequest.forceCompletion };

(Not really part of this bug, but... could we just drop all this except for the "notImplemented" error return, and do that unconditionally? The code is just wrong --- grabbing completionValue directly out of the packet doesn't deal with grips, etc. etc.)

@@ +835,5 @@
> +  _makeSteppingHooks: function TA__makeSteppingHooks(aStartLocation) {
> +    // Bind these methods and state because some of the hooks are called
> +    // with 'this' set to the current frame. Rather than repeating the
> +    // binding in each _makeOnX method, just do it once here and pass it
> +    // in to each function.

This makes me wonder if there shouldn't be an object with these properties, whose methods (when bound) can serve as the hooks. But that should be a different patch, if it is actually a good idea.

Comment 6

[Jim Blandy :jimb]

And thank you for the cleanup!

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20b0a55f4f17