Closed Bug 1247203 Opened 8 years ago Closed 8 years ago

Cleanup devtools/client/main relation against gDevTools/gDevToolsBrowser

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(8 files, 8 obsolete files)

4.55 KB, patch
jryans
: review+
ochameau
: checkin+
Details | Diff | Splinter Review
1.38 KB, patch
jryans
: review+
ochameau
: checkin+
Details | Diff | Splinter Review
932 bytes, patch
jryans
: review+
ochameau
: checkin+
Details | Diff | Splinter Review
1.14 KB, patch
jryans
: review+
ochameau
: checkin+
Details | Diff | Splinter Review
7.19 KB, patch
ochameau
: review+
ochameau
: checkin+
Details | Diff | Splinter Review
4.38 KB, patch
ochameau
: checkin+
Details | Diff | Splinter Review
15.65 KB, patch
jryans
: review+
Details | Diff | Splinter Review
11.33 KB, patch
jryans
: review+
Details | Diff | Splinter Review
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.
devtools/client/framework/toolbox-process-window.js
will benefit from such cleanup.
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...
Attachment #8724950 - Flags: review?(jryans)
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.
Attachment #8724953 - Flags: review?(jryans)
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
Attachment #8724954 - Flags: review?(jryans)
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.
Attachment #8724955 - Flags: review?(jryans)
I don't know why that ended up on gDevTools,
but this is really meant to be part of gDevToolsBrowser!
Attachment #8724960 - Flags: review?(jryans)
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!
Attachment #8724961 - Flags: review?(jryans)
Attached patch wip patch v1 (obsolete) — Splinter Review
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 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?
Attachment #8724953 - Flags: review?(jryans) → review+
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?
Attachment #8724954 - Flags: review?(jryans) → review+
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
Attachment #8724960 - Flags: review?(jryans) → review+
(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.
(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.
Assignee: nobody → poirot.alex
Attachment #8724960 - Attachment is obsolete: true
Rebase.
Attachment #8725690 - Flags: review?(jryans)
Attachment #8724950 - Attachment is obsolete: true
Attachment #8724950 - Flags: review?(jryans)
Attached patch wip patch v2 (obsolete) — Splinter Review
Rebase.
Attachment #8724963 - Attachment is obsolete: true
These patches are based on top of bug 1233463. Only the two last one have modifications related to it.
Depends on: 1233463
Blocks: 1248603
Keywords: leave-open
Filter on TEAPOT-SPLINES.
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Attachment #8724953 - Flags: checkin+
Attachment #8724954 - Flags: checkin+
Attachment #8724955 - Flags: checkin+
Attachment #8724961 - Flags: checkin+
Attachment #8725673 - Flags: checkin+
Attachment #8725690 - Attachment is obsolete: true
Attachment #8726812 - Flags: checkin+
Attached patch patch v3 (obsolete) — Splinter Review
rebase
Attachment #8725691 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #8733311 - Flags: review?(jryans)
Attachment #8732091 - Attachment is obsolete: true
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?
Attachment #8733311 - Flags: review?(jryans)
(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.
Attached patch patch v5Splinter Review
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)
Attachment #8734548 - Flags: review?(jryans)
Attachment #8733311 - Attachment is obsolete: true
Attached patch patch v6Splinter Review
> ::: 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.
Attachment #8736010 - Flags: review?(jryans)
Potential followup.
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!
Attachment #8736010 - Flags: review?(jryans) → review+
Blocks: 1261092
Comment on attachment 8736012 [details] [diff] [review]
Unregister gcli via unload module instead of custom events.

Opened bug 1261092 as followup.
Attachment #8736012 - Attachment is obsolete: true
Ready to land one fxteam reopens.
https://hg.mozilla.org/integration/fx-team/rev/6b710da110000b8d4b0116281fa319ab78879fbe
Bug 1247203 - Cleanup relations between main client module and devtools-browser. r=jryans
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/6b710da11000
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.