899006 - Refactor tab/tablist actors out of webbrowser.js and split addBrowserActors

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

webbrowser.js contains the browser root actor, as well as BrowserTabList and BrowserTabActor.

Other applications may be using BrowserTabActor, but want their own implementation of the root actor.

I propose we add a method to DebuggerServer called "addRemoteActors", which loads any actors that are accessible remotely. Then we change addBrowserActors() to call addRemoteActors(), so we don't break existing code.

Then, with the root actor in its own file, we can load it in addBrowserActors(). BrowserTabList/BrowserTabActor can be loaded in addRemoteActors().

How does this sound to you?

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

In case it wasn't already clear from comment 0, my goal is to have another app like Thunderbird to be able to call addRemoteActors(), so that any new remote features are available without code changes in the Thunderbird debugger glue cdode.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Actually the root actor is in toolkit/devtools/server/actors/root.js. It is supposed to contain the protocol details that don't change among products/embeddings, which should be providing their own overrides of the stuff in webbrowser.js. In other words, if a different root actor is used, the protocol client library in dbg-client.js won't know how to communicate with it.

It's not clear to me what exactly you want to achieve. You mention a new method "addRemoteActors", which is supposed to "load any actors that are accessible remotely". But every actor is accessible remotely (over the remote protocol), so I don't understand what you mean here.

Are you suggesting we move more things from webbrowser.js to root.js? If so, which parts specifically?

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Panos Astithas [:past] from comment #2)
> Actually the root actor is in toolkit/devtools/server/actors/root.js. It is
> supposed to contain the protocol details that don't change among
> products/embeddings, which should be providing their own overrides of the
> stuff in webbrowser.js. In other words, if a different root actor is used,
> the protocol client library in dbg-client.js won't know how to communicate
> with it.
I should have written this differently. I meant that webbrowser.js contains the createRootActor function for the browser, but it also contains BrowserTabActor and BrowserTabList, which are subclassed or used in other places, for example Thunderbird.

I would like to separate this so that the createRootActor() function is somewhere else, so that an app like Thunderbird can load BrowserTabActor without loading the createRootActor function.


> 
> It's not clear to me what exactly you want to achieve. You mention a new
> method "addRemoteActors", which is supposed to "load any actors that are
> accessible remotely". But every actor is accessible remotely (over the
> remote protocol), so I don't understand what you mean here.
Ok, I was assuming this is not the case since it also loads the inspector actor, which is for now at least not yet fully remotable. In that case an alternate proposal:

1. Move webbrowser.js' createRootActor() to a separate file (maybe in browser/ ?)
2. Leave BrowserTabActor/BrowserTabList in webbrowser.js
3. Rename addBrowserActors() to addToolkitActors()
4a. Where ever existing code calls addBrowserActors(), add a line to load the file
    containing createRootActor().
4b. Alternatively to 4a, make addBrowserActors() load the createRootActor file and
    then call addToolkitActors().

I hope this makes my intentions a little more clear, sorry for being unclear!

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

This is about what I am aiming for. File locations and details are up for discussion of course, therefore just requesting feedback.

As I believe :past is away for a few days, anyone else is welcome to comment too.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 785534 [details] [diff] [review]
Fix - v1

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

I don't really see the need for this change, unless of course I'm missing something.

createRootActor() is a required function for bootstrapping the server, but in every current embedding overriding it in the product-specific actors works fine. Perhaps it could be argued that the method used to override that function is not that common since it is defined in the global object, but I'm perfectly fine with that.

You also included sendShutdownEvent() in this new browser-specific actor file, but it doesn't have to be, since it is only used by tests. An embedding can choose to not set it in its createRootActor() as long as it doesn't get used by that embedding's tests.

In the same line of thinking, allAppShellDOMWindows() doesn't seem to fit better in main.js compared to webbrowser.js.

As for the addBrowserActors/addToolkitActors split, I'm not philosophically against it, but from what I've seen so far it's not necessary (yet?), and if we ever come to that, I'd like to try to fix it first or use the current method of actor overriding.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Panos Astithas [:past] from comment #5)


> As for the addBrowserActors/addToolkitActors split, I'm not philosophically
> against it, but from what I've seen so far it's not necessary (yet?), and if
> we ever come to that, I'd like to try to fix it first or use the current
> method of actor overriding.
Let me start with this, as it was the main reason for opening the bug. I think it is necessary, because if you guys add a new actor type, right now you add it both in b2g's shell.js and the current addBrowserActors. But what about Thunderbird? Or maybe some xulrunner app?

Thunderbird could just call addBrowserActors, but as in its current form it will already contain a createRootActor implementation which uses BrowserTabList, which uses Firefox-specific code (i.e. gBrowser). Of course I can hack around by first calling addBrowserActors and then replaceing createRootActor.

It bothers me that something in toolkit, which is meant to be product independent, uses code that is product specific. Which is why I added the second part: 

> createRootActor() is a required function for bootstrapping the server, but
> in every current embedding overriding it in the product-specific actors
> works fine. Perhaps it could be argued that the method used to override that
> function is not that common since it is defined in the global object, but
> I'm perfectly fine with that.
Yes, overriding the createRootActor by a product-specific actor is what I think is wrong. To be strict, BrowserTabList/BrowserTabActor could even be seen as browser/ specific, but as its being used as a base class by different products, I decided to keep them there.


> 
> You also included sendShutdownEvent() in this new browser-specific actor
> file, but it doesn't have to be, since it is only used by tests. An
> embedding can choose to not set it in its createRootActor() as long as it
> doesn't get used by that embedding's tests.
Not sure I understand. Its being used by Firefox tests, which is why its in browser/ where it makes sense, right? A different product that uses the toolkit/ server might not need it and therefore its not used.

Maybe its rather a path issue. I was aiming for browser/devtools/dbg-browser-actors.js to be the createRootActor implementation for Firefox only.

> 
> In the same line of thinking, allAppShellDOMWindows() doesn't seem to fit
> better in main.js compared to webbrowser.js.
main.js? I've put it into DevToolsUtils so it can be used by dbg-browser-actors and webbrowser.js without duplicate code?

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

I get the impression that you read too much into the toolkit/browser directory dichotomy, which is something that will change soon as we are about to move all devtools code in a top level directory. Similarly, the special-casing of how actors are loaded in b2g was only ever meant to be temporary and is very close to being eliminated as the last blocker is now under review.

With that in mind I'd like you to consider all devtools backend code as cross-product code and simply help us fix any cases where that property doesn't hold. Taste-related issues aside (like how createRootActor is overridden or where allAppShellDOMWindows should reside) I think the existing model works well for all our current embeddings, as you have most excellently demonstrated with your work on Thunderbird! Nothing will live in the source directory under toolkit/ in the near future, so perhaps your perception of the code split will change as well.

(In reply to Philipp Kewisch [:Fallen] (unavailable until September 5th) from comment #6)
> (In reply to Panos Astithas [:past] from comment #5)
> > In the same line of thinking, allAppShellDOMWindows() doesn't seem to fit
> > better in main.js compared to webbrowser.js.
> main.js? I've put it into DevToolsUtils so it can be used by
> dbg-browser-actors and webbrowser.js without duplicate code?

Sorry, I should have been more clear: I don't think this function belongs in a utility library, as it is a core element of the server's architecture, but I can see how it could be defined in the server's main definition file (main.js), which in your patch is accomplished by loading the utility library there.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Oops.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

It might just be a naming problem afterall, but without a change here the best way to go forward for Thunderbird is to call DebuggerServer.addBrowserActors(), then load the mail actors (which overwrite createRootActor) and overwrite DebuggerServer.chromeWindowType to be mail:3pane.

It may indeed be a matter of taste, but to me it seems wrong to call a function named addBrowserActors() from a non-browser app and then overwrite everything that needs to be replaced. It would make more sense to me to have a function like addToolkitActors (or maybe addCommonActors) that loads the really cross product stuff, and then put the specifics like createRootActor and setting the window type into the actual product. This is independent of the matter that everything is going into /devtools, as I believe the directory move doesn't change the fact that things should be separated.

Anyway, long story short, I don't think this issue is big enough to argue over. I'm fine with WONTFIXing this bug, I've made adjustments to the Thunderbird patches accordingly. Thanks for your input!