The number of lines in onResume is TOO DAMN HIGH

RESOLVED FIXED in Firefox 26


6 years ago
7 months ago


(Reporter: fitzgen, Assigned: fitzgen)


Firefox 26

Firefox Tracking Flags

(Not tracked)



(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

6 years ago
Hear, hear. This part of GDB is a monstrosity, too.
Created attachment 792339 [details] [diff] [review]

Just shuffling code around, not functionality changes here or anything.
Attachment #792339 - Flags: review?(jimb)

Comment 3

6 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

5 years ago
Comment on attachment 792339 [details] [diff] [review]

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

5 years ago
And thank you for the cleanup!
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26


7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.