1790912 - Consider moving js/ductwork/debugger to devtools/platform

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Ted Campbell [:tcampbell]]

The js/ductwork directory is an oddball that is really gecko/toolkit code that uses the JS public API. The actual code is very straightforward, and no one currently on SpiderMonkey has any particular expertise on it. I suggest we hand it over to the devtools/platform directory beside the nsIJSInspector interface.

Over the years we have backed off on the extreme dynamism of gecko components, and having the IJSDebugger as an implementation detail of devtools seems more appropriate these days.

The C++ part of the debugger interface would remain in spidermonkey since it is quite coupled to internals.

Comment 1

[Ted Campbell [:tcampbell]]

(For transparency, my motivation is to clean up js/ module ownership and responsibilities a bit)

Comment 2

[Alexandre Poirot [:ochameau]]

Sounds good. Please consider moving as-is.

But I've always wonder the added value of jsdebugger.jsm and IJSDebugger.
It is nice as it allows to add the Debugger to any given global (IJSDebugger) and jsdebugger.jsm allows to polyfill Debugger API.

But I was wondering, for IJSDebugger, if that would have been better to:

• simply expose Debugger on backstagepass ?
• expose it via Sandbox and wantGlobalProperties ?
• use WebIDL/IDL to have it exposed as a standard class on privileged globals.

And for jsdebugger.jsm, it is definitely a good call for action to move this to DevTools.
It would help be more concious of the polyfills, which can easily be missed!
We might better integrate this into the DevTools codebase and move that within devtools/server as a followup.

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1790912 - Fix lint issues in js/ductwork/debugger. r?ochameau

When we move this out of the js/ directory, a number of lint exceptions stop
applying so we should just fix the issues now.

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1790912 - Migrate js/ductwork/debugger to devtools/platform. r?ochameau

This gecko integration code is better managed with the other devtools platform
hook code. To simplify resolving the JSM and avoiding issues around restricted
use of eval in chrome, I also reset the DIST_SUBDIR config and run the xpcshell
tests without a firefox-appdir.

Depends on D157521

Comment 5

[Ted Campbell [:tcampbell]]

There is some horribleness I ran into around toolkit vs browser. I also see pre-existing android issues in Bug 685068. That said, but setting the devtools/platform tests to not use browser mode (DIST_SUBDIR=browser / firefox-appdir=browser) I seem to have it working. I've fired off a few more tests to confirm.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1790912 - Use globalThis instead of this with jsdebugger.jsm. r?jandem!,ochameau!

Use 'globalThis' instead of 'this' when trying to attach a debugger to the
current global to avoid subtle footguns with the varied definitions of 'this'.
The debugger interface needs a true GlobalObject so this is much clearer. In
particular, this is a problem in test_nativewrappers.js when the test runs in
strict mode since the 'this' in the test function is no long implicitly the
global.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbc6349cdd60
Use globalThis instead of this with jsdebugger.jsm. r=jandem,ochameau
https://hg.mozilla.org/integration/autoland/rev/a968eaecfc47
Fix lint issues in js/ductwork/debugger. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/38f540397664
Migrate js/ductwork/debugger to devtools/platform. r=ochameau

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/dbc6349cdd60
https://hg.mozilla.org/mozilla-central/rev/a968eaecfc47
https://hg.mozilla.org/mozilla-central/rev/38f540397664