1375809 - [dt-addon] remove dependency between XPIProvider.jsm and devtools

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

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 1

[Patrick Brosset <:pbro>]

DevTools bug triage (filter on CLIMBING SHOES).

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1375809 - use events to communicate between XPIProvider.jsm and ToolboxProcess.jsm;

As DevTools are moving to be an addon and out of mozilla-central, we should remove the
coupling between the mozilla-central code and the DevTools code.

Instead of directly loading and calling the ToolboxProcess, XPIProvider.jsm now uses
events to interact with BrowserToolboxProcess.

Review commit: https://reviewboard.mozilla.org/r/152192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152192/

Comment 3

[Andrew Swan [:aswan]]

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!

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 5

[Julian Descottes [:jdescottes]]

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 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8880867 [details]
Bug 1375809 - use events to communicate between XPIProvider.jsm and ToolboxProcess.jsm;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152192/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fceeb41c167e
use events to communicate between XPIProvider.jsm and ToolboxProcess.jsm;r=aswan,jryans

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fceeb41c167e