The number of lines in onResume is TOO DAMN HIGH

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
Firefox 26
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

4 years ago
Hear, hear. This part of GDB is a monstrosity, too.
Created attachment 792339 [details] [diff] [review]
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
Attachment #792339 - Flags: review?(jimb)

Comment 3

4 years ago
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.
Priority: -- → P3
Ping? The dependency for this bug landed in fx-team, would love to have this code clean up as well.

Comment 5

4 years ago
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.
Attachment #792339 - Flags: review?(jimb) → review+

Comment 6

4 years ago
And thank you for the cleanup!
https://hg.mozilla.org/integration/fx-team/rev/20b0a55f4f17
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/20b0a55f4f17
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.