Closed
Bug 1364075
Opened 7 years ago
Closed 7 years ago
Extract DevTools specifics from toolkit/components/processsingleton/ContentProcessSingleton.js
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ochameau, Assigned: jdescottes)
References
Details
Attachments
(1 file)
ContentProcessSingleton.js currently interacts with devtools codebase. We should refactor that and prune everything related to devtools from this components. We could possibly fork this xpcom in order to keep devtools specifics. And the from here: http://searchfox.org/mozilla-central/source/devtools/server/main.js#758 we would call loadProcessScript. We may inline content-server.jsm in it as lazy loading it wouldn't be useful anymore.
Assignee | ||
Comment 2•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c9106dd622416ccef4e01cd98c64bc3832feae8
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8872465 [details] Bug 1364075 - remove DevTools dependency in ContentProcessSingleton; Switching to f? Few things I am unsure about this: #1: I load the process script from main.js, but it feels redundant to have to send a message, just in order to pass the "prefix" info. Is there a simpler way to send this information, a way to do it at the same time as we load the script for instance? #2: in the ContentProcessSingleton, the message listener is removed on xpcom-shutdown. I created this new content script based on service-worker-child.js which doesn't take care of removing the message listener. I could still add an observer on xpcom-shutdown and remove the listener if it's necessary. Thanks.
Attachment #8872465 -
Flags: review?(poirot.alex) → feedback?(poirot.alex)
Reporter | ||
Comment 6•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #5) > #1: I load the process script from main.js, but it feels redundant to have > to send a message, just in order to pass the "prefix" info. Is there a > simpler way to send this information, a way to do it at the same time as we > load the script for instance? No, you can't send data while loading a script, but that is fine having two calls. It is only aesthetical... > #2: in the ContentProcessSingleton, the message listener is removed on > xpcom-shutdown. I created this new content script based on > service-worker-child.js which doesn't take care of removing the message > listener. I could still add an observer on xpcom-shutdown and remove the > listener if it's necessary. It isn't clear how important such removal is. I imagine message listener will be removed anyway. But I would see more value in unregistering the process script on devtools unload via Services.ppmm.removeDelayedProcessScript[1] and another message to unregister the message listener. [1] you can use Services.ppmm to simplify this
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8872465 [details] Bug 1364075 - remove DevTools dependency in ContentProcessSingleton; https://reviewboard.mozilla.org/r/143968/#review147786 ::: devtools/server/content-process-debugger-server.js:15 (Diff revision 2) > +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {}); > + > +// Only reply if we are in a real content process > +if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) { > + let {init} = Cu.import("resource://devtools/server/content-server.jsm", {}); > + init(message); You are missing the addMessageListener here. ::: devtools/server/main.js:759 (Diff revision 2) > > deferred.resolve(actor); > }); > > - mm.sendAsyncMessage("DevTools:InitDebuggerServer", { > + // Load the process script that will receive the debug:init-content-server message > + mm.loadProcessScript( You should call loadProcessScript only once to prevent loading it multiple times. Here, every time you open a browser content toolbox, a new script will be evaluated.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8872465 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #6) > (In reply to Julian Descottes [:jdescottes] from comment #5) > > > #2: in the ContentProcessSingleton, the message listener is removed on > > xpcom-shutdown. I created this new content script based on > > service-worker-child.js which doesn't take care of removing the message > > listener. I could still add an observer on xpcom-shutdown and remove the > > listener if it's necessary. > > It isn't clear how important such removal is. > I imagine message listener will be removed anyway. > But I would see more value in unregistering the process script on devtools > unload > via Services.ppmm.removeDelayedProcessScript[1] and another message to > unregister the message listener. > > [1] you can use Services.ppmm to simplify this Thanks! Still some questions. Here Services.ppmm is the GPPMM and calling removeDelayedProcessScript avoids loading the script in new content processes when they spawn. But I used a child PPMM to load the script, so I think it will only be loaded in the content process linked to this PPMM? I thought it was preferable to only load it in the content processes targeted by a browser content toolbox. Should I rather load this script "globally" using the GPPMM? And remove it on destroy() etc...
Assignee | ||
Comment 10•7 years ago
|
||
Another try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c972ee71786cef4378c296a19f2d13a8d6703766 Loading the script from the GPPMM and removing it on devtools shutdown.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #9) > Thanks! Still some questions. > > Here Services.ppmm is the GPPMM and calling removeDelayedProcessScript > avoids loading the script in new content processes when they spawn. But I > used a child PPMM to load the script, so I think it will only be loaded in > the content process linked to this PPMM? I thought it was preferable to only > load it in the content processes targeted by a browser content toolbox. True, I missed that. Yes it would be ideal to load only against the targeted process. > Should I rather load this script "globally" using the GPPMM? And remove it > on destroy() etc... Yes, I think it will be easier. We have to handle devtools unload properly, otherwise it may break when devtools add-on is going to be reloaded :/ So proper support of destroy/cleanup is more important than targetting only the right process.
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8872465 [details] Bug 1364075 - remove DevTools dependency in ContentProcessSingleton; https://reviewboard.mozilla.org/r/143968/#review148246 Looks good, thanks!
Attachment #8872465 -
Flags: review?(poirot.alex) → review+
Comment 14•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6e9b45f7ed37 remove DevTools dependency in ContentProcessSingleton;r=ochameau
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e9b45f7ed37
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•