Closed Bug 906249 Opened 8 years ago Closed 8 years ago

The number of lines in onResume is TOO DAMN HIGH


(DevTools :: Debugger, defect, P3)



(Not tracked)

Firefox 26


(Reporter: fitzgen, Assigned: fitzgen)




(1 file)

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!
Hear, hear. This part of GDB is a monstrosity, too.
Just shuffling code around, not functionality changes here or anything.
Attachment #792339 - Flags: review?(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.
Priority: -- → P3
Ping? The dependency for this bug landed in fx-team, would love to have this code clean up as well.
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+
And thank you for the cleanup!
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.