Closed Bug 1450974 Opened 2 years ago Closed 2 years ago

Refactor the thread actor's stepping hooks

Categories

(DevTools :: Debugger, enhancement)

enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jlast, Assigned: jlast)

Details

Attachments

(1 file)

It'd be nice to take some time and refactor the thread actor's stepping hooks:

1. use async
2. arrow functions
3. indentation
4. consistency
Comment on attachment 8964562 [details]
Bug 1450974 - Refactor the thread actor's stepping hooks.

https://reviewboard.mozilla.org/r/233270/#review238844


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/thread.js:419
(Diff revision 1)
> +
> +      return pauseAndRespond(frame);
>      };
>    },
>  
> -  _makeOnPop: function({ thread, pauseAndRespond, createValueGrip: createValueGripHook,
> +  _makeOnPop: function({ thread, pauseAndRespond, createValueGrip: createValueGripHook, 

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: devtools/server/actors/thread.js:465
(Diff revision 1)
>      result.originalLocation = startLocation;
>  
>      return result;
>    },
>  
> -  _makeOnStep: function({ thread, pauseAndRespond, startFrame,
> +  _makeOnStep: function({ thread, pauseAndRespond, startFrame, 

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment on attachment 8964562 [details]
Bug 1450974 - Refactor the thread actor's stepping hooks.

https://reviewboard.mozilla.org/r/233270/#review239368

::: devtools/server/actors/thread.js:380
(Diff revision 1)
> -                      source: originalLocation.originalSourceActor.form(),
> +        source: originalLocation.originalSourceActor.form(),
> -                      line: originalLocation.originalLine,
> +        line: originalLocation.originalLine,
> -                      column: originalLocation.originalColumn
> +        column: originalLocation.originalColumn
> -                    };
> +      };
> -                    Promise.resolve(onPacket(packet))
> -          .catch(error => {
> +
> +      try {

I believe changing this `.catch` to a `try` block, as it was done here, expands it to cover the call to `this.conn.send(pkt)`, whose exceptions would previously be dropped into an uncaught promise. If we're trying to avoid changing behavior here, then we might want to leave that out of the `try` block.

::: devtools/server/actors/thread.js:421
(Diff revision 1)
>    },
>  
> -  _makeOnPop: function({ thread, pauseAndRespond, createValueGrip: createValueGripHook,
> +  _makeOnPop: function({ thread, pauseAndRespond, createValueGrip: createValueGripHook, 
>                            startLocation }) {
> -    const result = function(completion) {
> -      // onPop is called with 'this' set to the current frame.
> +    // onPop is called with 'this' set to the current frame.

I feel like this comment does belong next to the call to `getFrameLocation(this)` that follows.

::: devtools/server/actors/thread.js:590
(Diff revision 1)
>     *          rejected with an error packet.
>     */
> -  _handleResumeLimit: function(request) {
> +  _handleResumeLimit: async function(request) {
>      let steppingType = request.resumeLimit.type;
>      if (!["break", "step", "next", "finish"].includes(steppingType)) {
> -      return Promise.reject({ error: "badParameterType",
> +      return Promise.reject({

Just a thought: since we're in an async function, won't throwing the object have the same effect as returning a rejected promise?
Attachment #8964562 - Flags: review?(jimb) → review+
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b256cde917
Refactor the thread actor's stepping hooks. r=jimb
https://hg.mozilla.org/mozilla-central/rev/b4b256cde917
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee: nobody → jlaster
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.