Closed Bug 1450323 Opened 2 years ago Closed 2 years ago

Debugger: Update Pause Points

Categories

(DevTools :: Debugger, enhancement, P2)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(1 file)

Pause Points went to nightly 3/25/2017. Since then, we updated some of the pause point locations [1] and tightened the mochitests [2].

[1]: https://github.com/devtools-html/debugger.html/pull/5777
[2]: https://github.com/devtools-html/debugger.html/pull/5808

It'd be nice to revisit a couple early assumptions:

1. pause points should be a list
2. we should resume if a pause point is not found

---

### Pause points should be a list

The first version had the type signature:

> type PausePoint = {
>   location: { line: number, column: number }
>   types: { breakpoint: boolean, stepOver: boolean }
> }
> type PausePoints = PausePoint[];

The new version will have the signature: 

> type PausePoints = {
>   line: {
>     column: { break: boolean, step: boolean }
>   }
> }

The new type has some advantages:

1. it is more compact
2. step does not imply step over, it could be in, out...
3. it is easier to find a pause point at a line,colum 
4. It is easier to check if a line has points


### We should resume if a pause point is not found

The first version assumed that if a location did not have a pause point, we did not want to stop there. This approach biased towards fewer steps. I think going forward, we should go in the other direction and bias toward pausing if we're not sure.
Assignee: nobody → jlaster
Priority: -- → P2
Comment on attachment 8964002 [details]
Bug 1450323 - Debugger: Update Pause Points.

https://reviewboard.mozilla.org/r/232826/#review238532

::: devtools/client/debugger/new/parser-worker.js:21134
(Diff revision 3)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
>  
> -const isControlFlow = node => t.isForStatement(node) || t.isWhileStatement(node) || t.isIfStatement(node) || t.isSwitchCase(node) || t.isSwitchStatement(node);
>  
> -const isAssignment = node => t.isVariableDeclarator(node) || t.isAssignmentExpression(node);
> +const isControlFlow = node =>

This work will be separately reviewed here:
https://github.com/devtools-html/debugger.html/pull/5836

the GH pr has proper unit tests.

The rationale for the changes here are:
1. the change to the data structure
2. the advent of empty pause points which note places we do *not* want to pause at
Comment on attachment 8964002 [details]
Bug 1450323 - Debugger: Update Pause Points.

https://reviewboard.mozilla.org/r/232826/#review240748

The server side of this looks good. I'll look at the client side on GitHub.
Comment on attachment 8964002 [details]
Bug 1450323 - Debugger: Update Pause Points.

https://reviewboard.mozilla.org/r/232826/#review241104

I approved the client-side changes on GitHub. Looks good.
Attachment #8964002 - Flags: review?(jimb) → review+
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2219f409ada9
Debugger: Update Pause Points (v37). r=jimb
I'm treating this commit as release 37 because it included the client code for pause points which might warrant a beta fix

https://github.com/devtools-html/debugger.html/compare/release-36...release-37
https://hg.mozilla.org/mozilla-central/rev/2219f409ada9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.