Closed Bug 1790912 Opened 5 months ago Closed 4 months ago

Consider moving js/ductwork/debugger to devtools/platform

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
107 Branch
Tracking Status
firefox107 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.

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

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:

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.

Assignee: nobody → tcampbell

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

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

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.

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.

Attachment #9294983 - Attachment description: WIP: Bug 1790912 - Fix lint issues in js/ductwork/debugger → Bug 1790912 - Fix lint issues in js/ductwork/debugger. r?ochameau
Attachment #9294984 - Attachment description: WIP: Bug 1790912 - Migrate js/ductwork/debugger to devtools/platform → Bug 1790912 - Migrate js/ductwork/debugger to devtools/platform. r?ochameau
Severity: -- → N/A
Priority: -- → P2
Pushed by tcampbell@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.