1468384 - Debugger: Add Debugger.Script.isModule

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tom S [:evilpie]]

Attachment: Add Debugger.Script.isModule

Comment 1

[Nicolas B. Pierron [:nbp]]

Tom, should you ask Jim for review?

Comment 2

[Tom S [:evilpie]]

Jason Laster said that the devtools team would like to take this, I think.

Comment 3

[Nicolas B. Pierron [:nbp]]

(based on comment 2)

Comment 4

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1468384 - Add an .isModule getter to Debugger.Script.

Review commit: https://reviewboard.mozilla.org/r/255530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255530/

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8990457 [details]
Bug 1468384 - Add an .isModule getter to Debugger.Script.

https://reviewboard.mozilla.org/r/255530/#review262540

This needs a few more things, I hope minor.

- a line or two of documentation in js/src/doc/Debugger/Debugger.Script.md

- tests for what this should return in function and direct-eval code within a module.

- Please make the test use the onNewScript hook rather than the onDebuggerStatement hook. It happens earlier, so more chance to catch badness. You can avoid the `g.eval()` in isModule-01 by doing `g.parseModule("...");` instead.


Make sure that a test failure actually counts as a failure; onNewScript is special in that it can't throw exceptions, but they should be reported and logged as test failures anyway. If not, it's a bug (which can be filed and added to our backlog--not pressing).

Comment 6

[Logan Smyth [:loganfsmyth]]

Comment on attachment 8990457 [details]
Bug 1468384 - Add an .isModule getter to Debugger.Script.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255530/diff/1-2/

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8990457 [details]
Bug 1468384 - Add an .isModule getter to Debugger.Script.

https://reviewboard.mozilla.org/r/255530/#review265218

r=me. Thanks!

::: js/src/jit-test/tests/debug/Script-isModule-01.js:11
(Diff revision 2)
> +    assertEq(script.isModule, true);
> +};
> +const m = g.parseModule("");
> +m.declarationInstantiation();
> +m.evaluation();
> +

Assert that the onNewScript hook actually hit.

Comment 8

[Logan Smyth [:loganfsmyth]]

Comment on attachment 8990457 [details]
Bug 1468384 - Add an .isModule getter to Debugger.Script.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255530/diff/2-3/

Comment 9

[Tom S [:evilpie]]

Should this be landed?

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1468384 - Add Debugger.Script.isModule. r=jorendorff

Comment 11

[Jason Orendorff [:jorendorff]]

r?jimb because Phabricator wants a review and doesn't want me to review a patch I posted.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 9007588 [details]
Bug 1468384 - Add Debugger.Script.isModule. r=jorendorff

Jim Blandy :jimb has approved the revision.

Comment 13

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5f877b96bfbd32bd463f57c93e38fb1b21f1fa

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/309721897c1c
Add Debugger.Script.isModule. r=jimb

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/309721897c1c