1222047 - Mainstream front creation via DebuggerClient and Target

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

# Developer story:
------------------

We keep adding:
 * fields like inspector, highlighter, performance,
 * init methods like initInspector, initPerformance,
 * destroy methods like destoryInspector, destroyPerformance.
 * manualy add calls to these init and destroy methods from initialization and destruction codepath of the toolbox
all in toolbox.js.

That doesn't scale!

Like actors on the server, we should have an easy way to manage :
 * initialisation,
 * destruction,
 * retrieval 
of client/fronts for any actor. That, without having to hack so many garbage into toolbox.js!

Offering such abstraction is going to:
 + help us restrict actor specifics to the actor files
 + ease playing with new actors from anywhere
 + keep us sane


# Proposal:
-----------

What about exposing something like:

  getFront(name) => promise of the front

Or, with some more magic:

  fronts.`name` => promise of the front

Also, I wouldn't expose that on toolbox. I think it would be handy in some other cases where we don't have a toolbox to expose that via `target` or `client` objects.

Then we would have to still declare the Fronts somehow, like what we are doing for Actors. But that will just be a list of module paths with all necessary arguments that may help implementing all that.

I would first do that for a new kind of front that I need to access. (Here the Preference front) Then, I think it is still doable to rework existing usages as there isn't that many usages of toolbox.`front` attributes in our codebase. Obvious inspector is going to be painful because of all these races it has on destruction, but it would be worth it!


# What is wrong today in our code:
----------------------------------

This won't scale:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#283
Neither that:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1943
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1952

There is too many actor specifics here:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1788
Even worse here:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1814
Too many performance specifics in toolbox:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#2046

We do things similar in WebIDE for other actors, but that ends up only be accessible to WebIDE codebase whereas it could be useful anywhere:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/webide/modules/app-manager.js#559

Comment 1

[Alexandre Poirot [:ochameau]]

f? r? a?

Jordan, I think you raised this subject a while back, it was for perf tools if I remember correctly. Does this proposal makes any sense?

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Sounds good! Do you have something in mind where we have a utility that takes possibly a toolbox or target and returns the associated front? `getFront(target, "performance")`

There are some caveats of the toolbox actor code, atleast for performance that I'm aware of. We start that up immediately (so we can get a console.profile() event if the performance tools aren't yet opened), and if we do see that event, as the performance tool starts up, we cache events to dump to the tool once it's opened.[0] I think this'll reduce a lot of toolbox boilerplate, but we still may have code-specific startup stuff in toolbox for the time being.

[0] onPerformanceFrontEvent http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#2098

Comment 3

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

Overall, it sounds good to me!

(In reply to Alexandre Poirot [:ochameau] from comment #0)
> Also, I wouldn't expose that on toolbox. I think it would be handy in some
> other cases where we don't have a toolbox to expose that via `target` or
> `client` objects.

I would think the `client` is the most natural location, since there are place like in WebIDE where we want to access the fronts without a target or toolbox.

> Then we would have to still declare the Fronts somehow, like what we are
> doing for Actors. But that will just be a list of module paths with all
> necessary arguments that may help implementing all that.

I am not sure what this means, can you provide more detail?

Would you also plan to remove existing one-off front caches, like `getDeviceFront`[1], since there would now be a new general path?

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/device.js#103

Comment 4

[Patrick Brosset <:pbro>]

This would help bug 1265798. In this bug we are adding a new actor that exposes the list of all CSS properties that a debuggee supports (which obviously will depend on the engine that currently runs the content page).
We need this list of properties to be loaded once and then we need to access it in different places of the code. Some of these places don't have access to a toolbox or target.
For instance: RuleRewriter in css-parsing-utils.js only deals with StyleRuleFront objects. 
So, if from this object we can get a client, and from it get the CssPropertiesFront, that would be super useful.
It would avoid having to store this thing in the inspector, or somewhere, and then pass it around in a lot of places in the code, via function arguments, just to finally use it in the RuleRewriter.

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Overall, it sounds good to me!
> 
> (In reply to Alexandre Poirot [:ochameau] from comment #0)
> > Also, I wouldn't expose that on toolbox. I think it would be handy in some
> > other cases where we don't have a toolbox to expose that via `target` or
> > `client` objects.
> 
> I would think the `client` is the most natural location, since there are
> place like in WebIDE where we want to access the fronts without a target or
> toolbox.

Yes, client looks like a good placeholder we are already using in many places, like front caches used in various existing fronts like device.

> 
> > Then we would have to still declare the Fronts somehow, like what we are
> > doing for Actors. But that will just be a list of module paths with all
> > necessary arguments that may help implementing all that.
> 
> I am not sure what this means, can you provide more detail?

We would need a way to declare a mapping and some setup/cleanup code.
We already do have something for actors:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/main.js#377-424
This registerModule function.

We would need something somewhat similar in order to:
- designate where the module lives for each front "id"
- define a setup method that is required for some fronts like inspector, performance or css-properties
- may be also a cleanup/destroy method?
- also some flags to say if it must be created on toolbox/tool creation or if that can be done on-demand

Compared to server/main.js, we may do something declarative instead:
const fronts = {
  inspector: {
    module: "devtools/shared/fronts/inspector",
    setupOnStartup: true,
    setup: "initInspector"
  },
  device: {
    module: "devtools/shared/fronts/devices"
  },
  ...
};
Or have an API and use it to allow addons to use this mechanism.
We would use the "setup" attribute to call a symbol exported by the module, if the front need a custom intialization process. We may have some challenges to solve for some fronts depending on other ones.


> 
> Would you also plan to remove existing one-off front caches, like
> `getDeviceFront`[1], since there would now be a new general path?
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/device.
> js#103

Yes! that was my very first goal. It may be a first good iteration, to just get rid of these very simple usecases that are lazy created and do not require any complex setup/destroy.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: wip v1

Here is a sketch refactoring just the preference and device fronts (simple usecase).
The main issue I have is that we have to pass a "form" object.
Which is either listTabs response for root actors like preferences and device,
or getTab response for inspector, console and all tab actors.
I wish we could cache this response on DebuggerClient and TabClient to make it easier,
but that may be hard. I started storing listTabs response on target, via target.root,
but it may be better to find placeholders in the client galaxy.

Comment 7

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

I am a bit behind on other tasks, so I haven't been able to get to this yet.  Hopefully I can tomorrow, sorry for the delay.

Comment 8

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

Comment on attachment 8795748 [details] [diff] [review]
wip v1

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

Hmm.  With this patch, you're covering the simple case of fronts for the "top level" actors like device and preference, as you stated.  This got to me think about what this problem looks like for more complex actors with children, etc.  In (most of...?) those cases though, we are using protocol.js to describe the actor that owns the child actors.

This is relevant because protocol.js handles the front creation for the caller when an RDP method returns one or more actors.  So, in some ways, this feels like we'd be duplicating a feature we already get with protocol.js today.

Of course, at the moment, we can't make use of this protocol.js feature for things like device, preference, etc. because the root actor is not described by protocol.js, and that's the one that owns these actors.  Have we considered approaching this the more "radical" way, by converting the root actor to protocol.js (finally)?

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8795748 [details] [diff] [review]
wip v1

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

If only we could convert just the root actor, but I imagine that would imply converting all others script, console, addon? :/

::: devtools/shared/client/main.js
@@ +30,5 @@
> +    path: "devtools/shared/fronts/preference",
> +    constructor: "PreferenceFront",
> +    actorName: "preferenceActor"
> +  }
> +};

But I'm wondering, even if we do convert all of them, wouldn't we end up having the same dictionnary the this anyway? But instead of having it in the client abstraction, we would have it in a root front?

Comment 10

[Alexandre Poirot [:ochameau]]

Do you think I can pursue with this approach and see how it fits with the complex front that are mutually dependant (inspector, highlighter)?
Or should I investigate more the protocol.js convertion? Or slightly tweak what I already did?

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1222047 - Expose helper to easily instanciate Root and Tab actor fronts.

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

Comment 12

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

Comment on attachment 8912923 [details]
Bug 1222047 - Expose helper to easily instanciate Root and Tab actor fronts.

Looks reasonable!  (I only scanned it quickly.)

Comment 13

[Alexandre Poirot [:ochameau]]

Bug 1448499 and bug 1440322 recently draw issues regarding destroy of fronts, which I believe would be more easily addressed if we do that.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8912923 [details]
Bug 1222047 - Expose helper to easily instanciate Root and Tab actor fronts.

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

Comment 15

[Alexandre Poirot [:ochameau]]

Here is a rebased version. I need to take another look at the latest codebase to see if there is more to refactor.
And ensure that try is green:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=782439d6e45771456cda9d70c104680399cad97d

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1222047 - Remove unused visible/hidden events on target. r=yulia

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1222047 - Expose helper to easily instanciate global and target scoped actor fronts. r=yulia

Depends On D3314

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1222047 - Manage StyleSheets fronts via target.getFront. r=yulia

Depends On D3316

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1222047 - Manage device and preference fronts via client.mainRoot.getFront. r=yulia

Depends On D3317

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1222047 - Manage performance fronts via target.getFront. r=yulia

Depends On D3318

Comment 21

[Alexandre Poirot [:ochameau]]

With this patch queue, I get a green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3d0ddd99a58e05393fdac27fb6c33ce47f211f1&selectedJob=193819153
(only one failure related to WorkerTarget missing getTabFront)

So, the approach here is to have a DebuggerClient.getRootFront for global actors like device and preference
and TabTarget.getTabFront for target scoped actors like inspector, performance, console, ...

The front instanciation is now managed by DebuggerClient and TabTarget, as well as their destruction.
An important difference is that now, root fronts lifecycle is no longer linked to the toolbox one. Instead it is bound to DebuggerClient. So that they are only destroyed when the client closes. Fronts are registered as top level pools (everything in protocol.js inherit from Pool) and are destroyed from here:
  https://searchfox.org/mozilla-central/source/devtools/shared/client/debugger-client.js#939-940
Note that target scoped fronts lifecycle is bound to the target object, which itself is also following toolbox lifecycle, so there is no change here.

It is simple for stylesheet as there is nothing special being made on instanciation/destruction.
It is slightly more complex for preference, as we still want to clear the preferences being set by the toolbox, but no longer manually destroy the front.
It is quite straightforward for device. We just have to tweak WebIDE as creating the fronts is now asynchronous (getRootFront retursn a promise).
It is much more complex for performance as we do many things in toolbox.js on opening/close. I would like to followup on this in order to move the logic into the front rather than polluting the toolbox codebase with panel/front specifics.
It is even more complex for the inspector as changing anything to it introduce docshell leaks in mochitests. I'll keep inspector for a dedicated followup.

Comment 22

[Alexandre Poirot [:ochameau]]

Note that I just realized that the second patch includes bug 1405638...
And for some context, this work should help bug 1465635 by moving logic from toolbox back to client codebase that is meant to be converted to protocol.js fronts. (TabTarget should become a front and RootClient should be RootFront)

Comment 23

[Yulia Startsev [:yulia]]

Comment on attachment 8999885 [details]
Bug 1222047 - Remove unused visible/hidden events on target. r=yulia

Yulia Startsev [:yulia] has approved the revision.

Comment 24

[Yulia Startsev [:yulia]]

Comment on attachment 8999890 [details]
Bug 1222047 - Manage device and preference fronts via client.mainRoot.getFront. r=yulia

Yulia Startsev [:yulia] has approved the revision.

Comment 25

[Yulia Startsev [:yulia]]

Comment on attachment 8999891 [details]
Bug 1222047 - Manage performance fronts via target.getFront. r=yulia

Yulia Startsev [:yulia] has approved the revision.

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1222047 - Cache root actor form on RootClient rather than Target object. r=yulia

Depends On D3314

Comment 27

[Yulia Startsev [:yulia]]

Comment on attachment 8999887 [details]
Bug 1222047 - Expose helper to easily instanciate global and target scoped actor fronts. r=yulia

Yulia Startsev [:yulia] has approved the revision.

Comment 28

[Yulia Startsev [:yulia]]

Comment on attachment 8999888 [details]
Bug 1222047 - Manage StyleSheets fronts via target.getFront. r=yulia

Yulia Startsev [:yulia] has approved the revision.

Comment 29

[Yulia Startsev [:yulia]]

Comment on attachment 9002728 [details]
Bug 1222047 - Cache root actor form on RootClient rather than Target object. r=yulia

Yulia Startsev [:yulia] has approved the revision.

Comment 30

[Alexandre Poirot [:ochameau]]

A DAMP run just to check if it changes anything to performane: no.
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=4a30b77fdf7bc285f860c3514c4bae3be388086e&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=12&selectedTimeRange=172800

Comment 31

[Alexandre Poirot [:ochameau]]

green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=962ccb6e987c822b2725d4cc9774ba19e47c3eed

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/105a304b2922
Remove unused visible/hidden events on target. r=yulia
https://hg.mozilla.org/integration/mozilla-inbound/rev/203dbb34de04
Cache root actor form on RootClient rather than Target object. r=yulia
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47fe9721bd8
Expose helper to easily instanciate global and target scoped fronts. r=yulia
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1efefe78cb6
Manage StyleSheets fronts via target.getFront. r=yulia
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a1ea339162
Manage device and preference fronts via client.mainRoot.getFront. r=yulia
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb1726e2a62
Manage performance fronts via target.getFront. r=yulia

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/105a304b2922
https://hg.mozilla.org/mozilla-central/rev/203dbb34de04
https://hg.mozilla.org/mozilla-central/rev/b47fe9721bd8
https://hg.mozilla.org/mozilla-central/rev/b1efefe78cb6
https://hg.mozilla.org/mozilla-central/rev/d8a1ea339162
https://hg.mozilla.org/mozilla-central/rev/ecb1726e2a62