Closed Bug 1665716 Opened 4 years ago Closed 4 years ago

Cleanup destroy method of Debugger's main.js module

Categories

(DevTools :: Debugger, enhancement, P3)

Product:
DevTools
Component:
Debugger
Type:
enhancement

Tracking

(firefox87 fixed)

Status:
RESOLVED FIXED
Milestone:
87 Branch
Tracking Flags:
Tracking Status
firefox87 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1665716 - [devtools] Remove unused traits attribute passed around debugger frontend.
(deleted), text/x-phabricator-request
Details
Bug 1665716 - [devtools] initialize and destroy ReactDOM from the same module.
(deleted), text/x-phabricator-request
Details
Bug 1665716 - [devtools] initialize and destroy source queue from the same module.
(deleted), text/x-phabricator-request
Details
Bug 1665716 - [devtools] initialize and destroy debugger workers from the same module.
(deleted), text/x-phabricator-request
Details
Bug 1665716 - [devtools] Merge client/index.js into main.js.
(deleted), text/x-phabricator-request
Details
Bug 1665716 - [devtools] Get rid of unused connection object.
(deleted), text/x-phabricator-request
Details
Bug 1665716 - [devtools] Listen to all resources only from one place in the debugger frontend.
(deleted), text/x-phabricator-request
Details

This destroy method contains various cleanup that are probably meant to be down in sub component:
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/main.js#28-32

See Julian's review comment:
https://phabricator.services.mozilla.com/D90427#inline-513228

followup: this destroy method is actually a bit wild :)
Apart from the "onDisconnect" that you added which mirrors perfectly the onConnect from "bootstrap", the rest of the content feels like it comes from nowhere. unmountRoot() and teardownWorkers() would fitt nicely in your new onDisconnect I think, because they mirror bootstrapApp and bootstrapWorkers (unmountRoot & bootstrapApp are on a slightly different level of abstraction, but I don't want to make things too complex).

For sourceQueue.clear() it's a bit weirder. The sourceQueue is initialized from setupEvents which is called in onConnect, so it could also make sense to have it in your disconnect?

Severity: -- → S3
Priority: -- → P3

If we need trait, we can access it from the targetFront directly.

Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b270b7095db3 [devtools] Remove unused traits attribute passed around debugger frontend. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/c7649862f44f [devtools] initialize and destroy ReactDOM from the same module. r=bomsy,nchevobbe https://hg.mozilla.org/integration/autoland/rev/f50924deab4f [devtools] initialize and destroy source queue from the same module. r=bomsy,nchevobbe https://hg.mozilla.org/integration/autoland/rev/c94dcb558f2a [devtools] initialize and destroy debugger workers from the same module. r=bomsy,nchevobbe https://hg.mozilla.org/integration/autoland/rev/949606f2079c [devtools] Merge client/index.js into main.js. r=bomsy,nchevobbe https://hg.mozilla.org/integration/autoland/rev/a9e0c9b685a4 [devtools] Get rid of unused connection object. r=bomsy,nchevobbe https://hg.mozilla.org/integration/autoland/rev/42133a1bb1e5 [devtools] Listen to all resources only from one place in the debugger frontend. r=bomsy,nchevobbe
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: