1247203 - Cleanup devtools/client/main relation against gDevTools/gDevToolsBrowser

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

This cleanup of gDevTools.jsm highlighted the complexity behind the main module (devtools/client/main).
It also highlighted some cycle dependencies between this module, gDevTools and gDevToolsBrowser.

The main module can most likely be merged into gDevToolsBrowser, or the other way around. But the complexity comes to the initialization codepath. The code from main module has to be executed in order for devtools to work as expected. Some code relied on the fact that loading gDevTools.jsm was also loading the main module. This is no longer true, if you don't use gDevToolsBrowser.

We should also cut the cycle dependencies if possible.

Comment 1

[Alexandre Poirot [:ochameau]]

devtools/client/framework/toolbox-process-window.js
will benefit from such cleanup.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: register tools and themes in the sorted order - v1

That's going to be important for bug 1248603 (dynamic menu insertion).
It also makes a step forward this cleanup.

I'll keep posting many small patches in this bug until we reach something great.

I have already a long patch queue of various tweaks...

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Stop exporting useless symbols of of devtools/client/main - v1

Note that this patch is based on top of bug 1248603.
I do two things here:
* remove useless symbols exported by main module.
Main module is special as any of its symbol will be also exported by Loader.jsm's `devtools`/`loader` object.
Various addons are using TargetFactory, but none are using defaultTools/defaultThemes. (looked at mxr addons)
I'm not sure about Toolbox, so I kept it.
* pull definitons instead of main in /devtools/
As main should just be about exporting these magic globals and nothing more.
I have another patch to do that completely.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Emit devtools-loaded from the loader - v1

devtools-loaded is more a Loader event.
I think it is clearer to keep both devtools-loader and devtools-unloaded events fired by Loader.jsm

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Remove useless call to main(). r=jryans

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8724955 [details] [diff] [review]
Remove useless call to main(). r=jryans

This is redundant with:
  main(id) {
    if (this._mainid) {
      return;
    }
    ...

I have another patch to clarify the main module story.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Move telemetry ping from gDevTools to gDevToolsBrowser - v1

I don't know why that ended up on gDevTools,
but this is really meant to be part of gDevToolsBrowser!

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Do not hardcode main module path in Loader.jsm - v1

The main module path isn't part of Loader.jsm
as each user of Loader.jsm may have a different main module.
So, just reuse whatever was the main module when reloading!

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: wip patch v1

The is the main patch, I still need to bake it.
But that may give you an insight for all these patches I just posted!

Comment 10

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4052bfa0426

Comment 11

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

Comment on attachment 8724953 [details] [diff] [review]
Stop exporting useless symbols of of devtools/client/main - v1

Review of attachment 8724953 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/main.js
@@ -12,5 @@
>  // start registering all default tools and themes: create menuitems, keys, emit
>  // related events.
>  gDevTools.registerDefaults();
>  
> -// Re-export for backwards compatibility, but we should probably the

I assume add-ons did not use these?

Comment 12

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

Comment on attachment 8724954 [details] [diff] [review]
Emit devtools-loaded from the loader - v1

Review of attachment 8724954 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/Loader.jsm
@@ +244,5 @@
>        XPCOMUtils.defineLazyGetter(this, key, () => this._main[key]);
>      });
> +
> +    var events = this.require("sdk/system/events");
> +    events.emit("devtools-loaded", {});

Looks like Developer Toolbar is the only listener for these events?

Comment 13

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

Comment on attachment 8724960 [details] [diff] [review]
Move telemetry ping from gDevTools to gDevToolsBrowser - v1

Review of attachment 8724960 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/framework/devtools.js
@@ +488,1 @@
>  

Nit: remove extra blank line

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Comment on attachment 8724953 [details] [diff] [review]
> Stop exporting useless symbols of of devtools/client/main - v1
> 
> Review of attachment 8724953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/main.js
> @@ -12,5 @@
> >  // start registering all default tools and themes: create menuitems, keys, emit
> >  // related events.
> >  gDevTools.registerDefaults();
> >  
> > -// Re-export for backwards compatibility, but we should probably the
> 
> I assume add-ons did not use these?

I looked at mxr addons and haven't found any usage of defaultTools/defaultThemes. I tried to look for Tools, but it is a bit harder. With some assumptions on the way to import/use it, I haven't found any usage either.

Comment 15

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> Comment on attachment 8724954 [details] [diff] [review]
> Emit devtools-loaded from the loader - v1
> 
> Review of attachment 8724954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/shared/Loader.jsm
> @@ +244,5 @@
> >        XPCOMUtils.defineLazyGetter(this, key, () => this._main[key]);
> >      });
> > +
> > +    var events = this.require("sdk/system/events");
> > +    events.emit("devtools-loaded", {});
> 
> Looks like Developer Toolbar is the only listener for these events?

In mozilla-central, yes. It can most likely be done differently.

Otherwise I see one usage in addons, in firebug:
  https://mxr.mozilla.org/addons/source/1843/content/firebug/firefox/browserCommands.js#88
to disable some builtin shortcuts.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: Move telemetry ping from gDevTools to gDevToolsBrowser - v2

Removed empty line.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: register tools and themes in the sorted order - v2

Rebase.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: wip patch v2

Rebase.

Comment 19

[Alexandre Poirot [:ochameau]]

These patches are based on top of bug 1233463. Only the two last one have modifications related to it.

Comment 20

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cda13a9587c

Comment 21

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afd76dd9fdf1

Comment 22

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01a2bf5f0587

Comment 23

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/81484938ac9917f182edd39e7fa837ae58b0e0e2
Bug 1247203 - Stop exporting useless symbols of of devtools/client/main. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/a668838de9dcb7c3ca77144e903c988443329bf9
Bug 1247203 - Emit devtools-loaded from the loader. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/80ad10b22bcc3412a05d2c4ef88722fea7b9ea3b
Bug 1247203 - Remove useless call to main(). r=jryans

https://hg.mozilla.org/integration/fx-team/rev/3b8417be6c87458bf84166b282b155019bc834a4
Bug 1247203 - Move telemetry ping from gDevTools to gDevToolsBrowser. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/bfd61742bd5952153c5fc97c6ff9ae22e6594f3e
Bug 1247203 - Do not hardcode main module path in Loader.jsm. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/36f76c141709a89c42718b8efa9c8e272ef549c7
Bug 1247203 - Register tools and themes in the sorted order. r=jryans

Comment 24

[Greg Tatum [:gregtatum]]

Attachment: Register tools and themes in the sorted order.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81484938ac99
https://hg.mozilla.org/mozilla-central/rev/a668838de9dc
https://hg.mozilla.org/mozilla-central/rev/80ad10b22bcc
https://hg.mozilla.org/mozilla-central/rev/3b8417be6c87
https://hg.mozilla.org/mozilla-central/rev/bfd61742bd59
https://hg.mozilla.org/mozilla-central/rev/36f76c141709

Comment 26

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

Filter on TEAPOT-SPLINES.

Comment 27

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

rebase

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

I think it is much better with this patch.
Using loader.main() makes it much more explicit we are spawing the main modules at these precise callsites.
But I keep devtools/client/main.js just for the exports used within Loader.jsm.
The main module is really devtools-browser.

There might still be some cleanup to do within devtools.js and devtools-browser.js,
especially at the end of these files where we startup and register cleanup callbacks.
Like moving these codes into functions instead of being in top level scope.
There might still be some confusion between devtools and devtools-browser?
Like, I'm not sure devtools.js has to register a module loader unload listener itself?

Feel free to highlight potential cleanups for this bug or followups!
I've been hacking on this for too long and don't clearly see what is confusing.

Comment 29

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

Comment on attachment 8733311 [details] [diff] [review]
patch v4

Review of attachment 8733311 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/framework/devtools-browser.js
@@ +896,5 @@
>    }
>  }
>  
> +// Watch for module loader unload. Fires when the tools are reloaded.
> +const unloadObserver = {

Can you use the SDK unload module[1] instead?

var { when: unload } = require("sdk/system/unload");

That way less of the unload details are exposed.

[1]: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/system/unload.js#19

::: devtools/client/framework/devtools.js
@@ -480,5 @@
>     * All browser windows have been closed, tidy up remaining objects.
>     */
>    destroy: function() {
>      Services.obs.removeObserver(this.destroy, "quit-application");
> -    Services.obs.removeObserver(this._teardown, "devtools-unloaded");

Can you remove "devtools-unloaded" from developer-toolbar.js too?  Then we could stop emitting it from Loader.jsm.  (Maybe you already did that in some other patch, I can't remember...)

::: devtools/client/main.js
@@ +18,5 @@
>    get: () => require("devtools/client/framework/target").TargetFactory
>  });
>  
> +// Load the main browser module
> +require("devtools/client/framework/devtools-browser");

I think this is the part I wonder about the most...  Especially for cases where I don't want to register connections to browser UI:

1. Some non-Firefox app
2. Displaying the toolbox UI from child process

(I realize these are fairly minor use cases at the moment.)

In these cases, I just want DevTools itself to be ready (default tools registered, etc.) without any of the browser <-> DevTools connections.

So, are we saying this file is only for "DevTools inside Firefox", or should it for "starting DevTools" and there is a second, separate step to load connections with the browser UI?

Comment 30

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> Comment on attachment 8733311 [details] [diff] [review]
> patch v4
> 
> Review of attachment 8733311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/framework/devtools-browser.js
> @@ +896,5 @@
> >    }
> >  }
> >  
> > +// Watch for module loader unload. Fires when the tools are reloaded.
> > +const unloadObserver = {
> 
> Can you use the SDK unload module[1] instead?

Oh I thought it was working only when using a real addon, but looks like it should also work in our usage.

> ::: devtools/client/main.js
> @@ +18,5 @@
> >    get: () => require("devtools/client/framework/target").TargetFactory
> >  });
> >  
> > +// Load the main browser module
> > +require("devtools/client/framework/devtools-browser");
> 
> I think this is the part I wonder about the most...  Especially for cases
> where I don't want to register connections to browser UI:
> 
> 1. Some non-Firefox app
> 2. Displaying the toolbox UI from child process
> 
> (I realize these are fairly minor use cases at the moment.)

You could have added:
 3. spawning actors in child processes.

That's a non issue. "main" module is nothing. We could even get rid of it.
It is only used in two ways:
 - special module that defines what to export out of Loader.jsm (TargetFactory and Toolbox have to be exported on Loader.jsm's devtools exported symbol)
 - as a pure virtual metaphor to say, hey this is the "main" module that we should be running first

At the end, it is up to you to use Loader.jsm and do:
  loader.main("whatever/you/want")
or even just:
  loader.require("whatever/you/want"); if you don't care about Loader.jsm exported symbols.

So. This is not an issue for your scenarios 1. and 2.
But as the goal of this bug is to clarify things, may be I should do something.

It may be good to just get rid of "main". May be Loader.jsm should inline the export of TargetFactory/Toolbox in it and remove completely the support of main module.
I did this at some point in my patch, but then I decided to keep it.
Then, we would do loader.main(/devtools/client/main) but loader.main(/devtools/client/devtools-browser).


> So, are we saying this file is only for "DevTools inside Firefox",

To clarify. Yes main.js is really related to the one instance of Loader.jsm that ends up being used by addon in the parent process. So it is natural to hardcode devtools-browser.

> or should
> it for "starting DevTools" and there is a second, separate step to load
> connections with the browser UI?

To clarify again. No. Starting devtools can be done in various different ways, like in child process for actors.

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

See comments 30. Feel free to request event more cleanups!

I'll verify that on monday, but this may have introduced the leak reported in this try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d3374d9dd7e
  (can be related to developer-toolbar change)

Comment 32

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1183d5474563

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

> ::: devtools/client/framework/devtools.js
> @@ -480,5 @@
> >     * All browser windows have been closed, tidy up remaining objects.
> >     */
> >    destroy: function() {
> >      Services.obs.removeObserver(this.destroy, "quit-application");
> > -    Services.obs.removeObserver(this._teardown, "devtools-unloaded");
> 
> Can you remove "devtools-unloaded" from developer-toolbar.js too?  Then we
> could stop emitting it from Loader.jsm.  (Maybe you already did that in some
> other patch, I can't remember...)

Hum. Actually. Do you mind if I keep that for a followup?
It seems to be introducing the gcli leak.

See next attachment, I pulled out the devtools-unloaded removal that I suggest to keep for another bug dedicated to just that.

Comment 34

[Alexandre Poirot [:ochameau]]

Attachment: Unregister gcli via unload module instead of custom events.

Potential followup.

Comment 35

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0fe49bfda5d

Comment 36

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

Comment on attachment 8736010 [details] [diff] [review]
patch v6

Review of attachment 8736010 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, seems fine.  Please do file the follow up!

Comment 37

[Alexandre Poirot [:ochameau]]

Comment on attachment 8736012 [details] [diff] [review]
Unregister gcli via unload module instead of custom events.

Opened bug 1261092 as followup.

Comment 38

[Alexandre Poirot [:ochameau]]

Ready to land one fxteam reopens.

Comment 39

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/6b710da110000b8d4b0116281fa319ab78879fbe
Bug 1247203 - Cleanup relations between main client module and devtools-browser. r=jryans

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b710da11000