# 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

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?

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

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

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.

(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.

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.

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 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 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?

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 on attachment 8912923 [details] Bug 1222047 - Expose helper to easily instanciate Root and Tab actor fronts. Looks reasonable! (I only scanned it quickly.)

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

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

Depends On D3314

Depends On D3316

Depends On D3317

Depends On D3318

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.

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 on attachment 8999885 [details] Bug 1222047 - Remove unused visible/hidden events on target. r=yulia Yulia Startsev [:yulia] has approved the revision.

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 on attachment 8999891 [details] Bug 1222047 - Manage performance fronts via target.getFront. r=yulia Yulia Startsev [:yulia] has approved the revision.

Depends On D3314

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 on attachment 8999888 [details] Bug 1222047 - Manage StyleSheets fronts via target.getFront. r=yulia Yulia Startsev [:yulia] has approved the revision.

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.

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

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

Pushed by apoirot@mozilla.com: 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

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