Closed
Bug 1375809
Opened 7 years ago
Closed 7 years ago
[dt-addon] remove dependency between XPIProvider.jsm and devtools
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
Details
Attachments
(1 file)
As DevTools are moving to an addon, and we later be maintained outside of m-c, we should remove all hardcoded dependencies between mozilla-central code and DevTools classes. XPIProvider.jsm is communicating with DevTools to get the BrowserToolboxProcess. We should either use the DevToolsShim, that will stay in mozilla-central, or use events to communicate between the XPIProvider and the BrowserToolboxProcess.
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8880867 [details] Bug 1375809 - use events to communicate between XPIProvider.jsm and ToolboxProcess.jsm; https://reviewboard.mozilla.org/r/152192/#review157276 I just looked at the XPIProvider.jsm bits but looks great to me, thanks!
Attachment #8880867 -
Flags: review?(aswan) → review+
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8880867 [details] Bug 1375809 - use events to communicate between XPIProvider.jsm and ToolboxProcess.jsm; https://reviewboard.mozilla.org/r/152192/#review157378 Thanks, seems reasonable overall! :) ::: devtools/client/framework/ToolboxProcess.jsm:298 (Diff revision 1) > + * @param {DebuggerServerConnection} connection > + * The connection that was opened or closed. > + */ > + _onConnectionChange: function (evt, what, connection) { > + let wrappedJSObject = { what, connection }; > + Services.obs.notifyObservers({ wrappedJSObject }, "toolbox-connection-change"); Do you actually need to wrap it an extra object? What about: ``` Services.obs.notifyObservers({ what, connection }, "toolbox-connection-change"); ``` It was hard to tell from a quick search what works and what doesn't. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4295 (Diff revision 1) > logger.warn("Error loading bootstrap.js for " + aId, e); > } > > - // Only access BrowserToolboxProcess if ToolboxProcess.jsm has been > - // initialized as otherwise, when it will be initialized, all addons' > - // globals will be added anyways > + // Notify the BrowserToolboxProcess that a new addon has been loaded. > + let wrappedJSObject = { id: aId, options: { global: activeAddon.bootstrapScope }}; > + Services.obs.notifyObservers({ wrappedJSObject }, "toolbox-update-addon-options"); Similarly here, I am not sure you actually need the extra object...? So, worth removing if it's possible to do so.
Attachment #8880867 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 5•7 years ago
|
||
Thanks for the reviews! Green try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c209e40d039074c13b92b88759a457e20c3a5b4c (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4) > Comment on attachment 8880867 [details] > Bug 1375809 - use events to communicate between XPIProvider.jsm and > ToolboxProcess.jsm; > > https://reviewboard.mozilla.org/r/152192/#review157378 > > Thanks, seems reasonable overall! :) > > ::: devtools/client/framework/ToolboxProcess.jsm:298 > (Diff revision 1) > > + * @param {DebuggerServerConnection} connection > > + * The connection that was opened or closed. > > + */ > > + _onConnectionChange: function (evt, what, connection) { > > + let wrappedJSObject = { what, connection }; > > + Services.obs.notifyObservers({ wrappedJSObject }, "toolbox-connection-change"); > > Do you actually need to wrap it an extra object? I am afraid we have to do that. I'm not super familiar with XPCOM components, but I suppose Services.obs is one, which means that the JS objects that we pass are translated to XPConnect wrappers. So we have to go through this wrappedJSObject to be able to retrieve the object in the observer. If we were passing a string, I believe it would be automatically wrapped, but objects need a special treatment it seems. The only related documentation I found were: - https://developer.mozilla.org/en/docs/wrappedJSObject - https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService (which just says that the interface exposed by Services.obs is an XPCOM component)
Comment hidden (mozreview-request) |
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fceeb41c167e use events to communicate between XPIProvider.jsm and ToolboxProcess.jsm;r=aswan,jryans
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fceeb41c167e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•