Based on bug 1050691 miss of information of Florent about actors hierarchy, I crafted a documentation about that particular topic. Drawing in one place how actors articulate and what kind of "TabActor" we do have.

Comment on attachment 8615271 [details] [diff] [review] patch v1 Panos, Ryan, I'm trying to document the root actor and "TabActor" instances we have and how they play with each others. But I hit an issue where we have: 1) "Tab actors" Console, inspector, threadActor, ... 2) "TabActor" instances BrowserTabActor, ContentActor, ChromeActor, ChildProcessActor, BrowserAddonActor. Most of them inherit from TabActor. That ends up being very misleading to talk about both TabActor's and "Tab actors". Shouldn't we rename TabActor to something less misleading? It is not longer for tabs as it targets any kind of document (being in a tab, an app, or a top level window) Or how would you designate actors in 2) list?

We've been referring to actors in the 1st list as tab-scoped actors, or child actors of the tab actor. You should mention somewhere that actors are organized in a hierarchy for easier lifecycle/memory management: once a parent is removed from the pool, its children are removed as well. I agree that TabActor is no longer an appropriate name for the base type of these actors, but I haven't thought of anything really good yet. I'll take a look at the doc tomorrow.

Comment on attachment 8615271 [details] [diff] [review] patch v1 Review of attachment 8615271 [details] [diff] [review]: ----------------------------------------------------------------- I agree that "TabActor" is now a terrible and confusing name, since it no longer means what it did. So, what's a good word for "actor that represents a single document / app / add-on"? Here are names I think we should **avoid**, because they already have too many other meanings: * Browser actor * Window actor * Tab actor * Child actor To help think of a new name, what are the properties that "TabActor" and all of it's subclasses have? They have: * is targeted to a something specific * a window * a document * a docShell * various sources * various sub-frames * various workers Here are some names that seem okay, but I don't really love any of them either: * Target actor * Document actor Once a new name is chosen, the "tab-scoped actors" like console should also be renamed to be thought of as "X-scoped actors", as in "target-scoped actors". We should probably file separate bugs to discuss and do the actual renaming, separate for the step of adding these docs, which we should add right away! The file seems to use random line wrapping in different blocks. We should probably aim for 80 char max, like in other places. ::: toolkit/devtools/server/docs/actor-hierarchy.md @@ +6,5 @@ > + > + RootActor: First one, automatically instanciated when we start connecting. > + | Mostly meant to instanciate new actors. > + | > + |--> Global actors: Actors exposing features related to the main process, It may help to say "global-scoped" here. @@ +10,5 @@ > + |--> Global actors: Actors exposing features related to the main process, > + | that are not specific to any document/app/addon. > + | A good example is the preference actor. > + | > + |--> "TabActor" (or alike): Actors meant to designate one document, tab, app, addon Super nit: Use the "\" here @@ +13,5 @@ > + | > + |--> "TabActor" (or alike): Actors meant to designate one document, tab, app, addon > + | and track its lifetime. > + | > + \--> Tab actors: Actors exposing one particular feature set, this time, It may help to say "tab-scoped" here. Soon to be replaced by a better name I hope... @@ +21,5 @@ > +## RootActor > + > +The root actor is special. It is automatically created when a client connects. > +It has a special "actorID" which is unique and is "root". > +All other actors have actorID which is computed dynamically, Nit: have **an** actorID @@ +30,5 @@ > + | > + |-- BrowserTabActor (webbrowser.js) - targets tabs living in the parent process. > + | Exposed via "onListTabs" or "getTab" requests. > + | > + |-- ContentActor (childtab.js) - targets tabs living out-of-process (e10s) or apps (on firefox OS). ContentActor is also a poor name... Maybe OOPContentActor? ChildContentActor? The file name "childtab" is actually more clear than the object name, even though it also means apps too. @@ +33,5 @@ > + | > + |-- ContentActor (childtab.js) - targets tabs living out-of-process (e10s) or apps (on firefox OS). > + | Also exposed via "onListTabs" or "getTab" requests. > + | > + |-- ChromeActor (chrome.js) - targets all ressources in the parent process of firefox (chrome documents, JSM, JS XPCom, ...) Nit: resources, XPCOM @@ +35,5 @@ > + | Also exposed via "onListTabs" or "getTab" requests. > + | > + |-- ChromeActor (chrome.js) - targets all ressources in the parent process of firefox (chrome documents, JSM, JS XPCom, ...) > + | Returned by "getProcess" request without any argument. > + | Nit: trailing whitespace @@ +40,5 @@ > + |-- ChildProcessActor (child-process.js) - targets the chrome of the child process (e10s) > + | Returned by "getProcess" request with a id argument, matching the targeted process. > + | > + \-- BrowserAddonActor (addon.js) - targets the javascript of addons > + Returned vy "listAddons" request. Nit: by @@ +41,5 @@ > + | Returned by "getProcess" request with a id argument, matching the targeted process. > + | > + \-- BrowserAddonActor (addon.js) - targets the javascript of addons > + Returned vy "listAddons" request. > + Nit: trailing whitespace @@ +54,5 @@ > +For historical reasons, these actors also handle creating the ThreadActor, used to manage breakpoints in the debugger. > +All the other tab actors are created when we access the TabActor's grip. We return the tab actors actorID in it. > +Actors inheriting from TabActor expose `attach`/`detach` requests, that allows to start/stop the ThreadActor. > + > +The tab actors have some expectation on the "TabActor": "tab-scoped actors" here and in other usages @@ +60,5 @@ > + ThreadActor instance for the given context, > + only defined once `attach` request is called, or on construction. > + - isRootActor: (historical) > + Always false, but for ChromeActor. > + Despite the attribute name, indicated to not limit to content ressources, Nit: resources

Comment on attachment 8615271 [details] [diff] [review] patch v1 Review of attachment 8615271 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/docs/actor-hierarchy.md @@ +15,5 @@ > + | and track its lifetime. > + | > + \--> Tab actors: Actors exposing one particular feature set, this time, > + specific to a given document/app/addon. > + Like console, inspector actors. You should add that other actors may extend this hierarchy by containing their own children, like LongStringActor. @@ +28,5 @@ > + > + RootActor (root.js) > + | > + |-- BrowserTabActor (webbrowser.js) - targets tabs living in the parent process. > + | Exposed via "onListTabs" or "getTab" requests. Let's be consistent here and use the requestType for every request, not the method handler name. @@ +46,5 @@ > +## "TabActor" > + > +Those are the actors exposed by the root actors which are meant to track the lifetime > +of a given context: tab, app, process or addon. It also allows to fetch the tab actors connected > +to this context. Actors like console, inspector, thread (for debugger), styleinspector, ... Nit: s/.../etc./ @@ +47,5 @@ > + > +Those are the actors exposed by the root actors which are meant to track the lifetime > +of a given context: tab, app, process or addon. It also allows to fetch the tab actors connected > +to this context. Actors like console, inspector, thread (for debugger), styleinspector, ... > +Most of them inherits from TabActor (defined in webbrowser.js) which is document centric. It automatically tracks Typo: Most of them inherit @@ +48,5 @@ > +Those are the actors exposed by the root actors which are meant to track the lifetime > +of a given context: tab, app, process or addon. It also allows to fetch the tab actors connected > +to this context. Actors like console, inspector, thread (for debugger), styleinspector, ... > +Most of them inherits from TabActor (defined in webbrowser.js) which is document centric. It automatically tracks > +the lifetime of the targeted document, but also track its iframes and allows switching the context Typo: but [it] also tracks @@ +49,5 @@ > +of a given context: tab, app, process or addon. It also allows to fetch the tab actors connected > +to this context. Actors like console, inspector, thread (for debugger), styleinspector, ... > +Most of them inherits from TabActor (defined in webbrowser.js) which is document centric. It automatically tracks > +the lifetime of the targeted document, but also track its iframes and allows switching the context > +to one of its iframe. Typo: iframes @@ +64,5 @@ > + Despite the attribute name, indicated to not limit to content ressources, > + but accept all of them, especially chrome one. > + - makeDebugger: > + Helper object with two functions to create Debugger object for the targeted context. > +In addition to this, the actors inheriting from TabActor, exposes many other attributes and events: Typo: expose @@ +80,5 @@ > + > + > +## Tab actors > + > +Each of these actors focus on providing one particular feature set, specific to one context, Typo: focuses @@ +81,5 @@ > + > +## Tab actors > + > +Each of these actors focus on providing one particular feature set, specific to one context, > +that can be a web page, an app, a top level firefox window, a process or an addon ressource. Typo: resource

Comment on attachment 8617283 [details] [diff] [review] patch v2 Review of attachment 8617283 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your comment, that helps! Here is a new patch with all the suggestions applied. I like DocumentActor for TabActor. It is precise and match what I have in mind when I think about this class. The only think is that it's not only just for one document, it can change with the iframe switching feature. But the idea is that it targets something with one or many document(s). But your suggestions doesn't cover how to call the "TabActor instances": BrowserTabActor, ContentActor, ChromeActor, ChildProcessActor, BrowserAddonActor. Most of them inherit from TabActor, but not all of them. They are all created by the RootActor. I wish we could have a significantly different way to name them, compared to the tab-scoped actors (console, inspector,...) I originally started this document to highlight these particular classes!

Comment on attachment 8617283 [details] [diff] [review] patch v2 Review of attachment 8617283 [details] [diff] [review]: ----------------------------------------------------------------- I'll add more thoughts on new names in bug 1172897. ::: toolkit/devtools/server/docs/actor-hierarchy.md @@ +27,5 @@ > + > +## RootActor > + > +The root actor is special. It is automatically created when a client connects. > +It has a special "actorID" which is unique and is "root". Nit: Use `actorID` to match the others

Comment on attachment 8617283 [details] [diff] [review] patch v2 Review of attachment 8617283 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/docs/actor-hierarchy.md @@ +42,5 @@ > + |-- ContentActor (childtab.js) > + | Targets tabs living out-of-process (e10s) or apps (on firefox OS). > + | Returned by "listTabs" or "getTab" requests. > + | > + |-- ChromeActor (chrome.js Nit: missing )

Comment on attachment 8617283 [details] [diff] [review] patch v2 Review of attachment 8617283 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/docs/actor-hierarchy.md @@ +1,5 @@ > +# How actors are organized > + > +To start with, actors are living within /toolkit/devtools/server/actors/ folder. > +They are organized in a hierarchy for easier lifecycle/memory management: > +once a parent is removed from the pool, its children are removed as well. Can you add a sentence here about how actors are lazily-loaded and constructed on-demand? @@ +35,5 @@ > + > + RootActor (root.js) > + | > + |-- BrowserTabActor (webbrowser.js) > + | Targets tabs living in the parent process. Can you be more explicit here that this actor is only present in single-process (non-e10s) Firefox? @@ +39,5 @@ > + | Targets tabs living in the parent process. > + | Returned by "listTabs" or "getTab" requests. > + | > + |-- ContentActor (childtab.js) > + | Targets tabs living out-of-process (e10s) or apps (on firefox OS). Maybe also mention RemoteBrowserTabActor here (or in a separate bullet)? @@ +42,5 @@ > + |-- ContentActor (childtab.js) > + | Targets tabs living out-of-process (e10s) or apps (on firefox OS). > + | Returned by "listTabs" or "getTab" requests. > + | > + |-- ChromeActor (chrome.js Unrelated, but wouldn't it make more sense to call the ChromeActor, MainProcessActor, for consistency with ChildProcessActor? @@ +48,5 @@ > + | (chrome documents, JSM, JS XPCOM, etc.). > + | Returned by "getProcess" request without any argument. > + | > + |-- ChildProcessActor (child-process.js) > + | Targets the chrome of the child process (e10s) Missing period. @@ +53,5 @@ > + | Returned by "getProcess" request with a id argument, > + | matching the targeted process. > + | > + \-- BrowserAddonActor (addon.js) > + Targets the javascript of addons Ditto. @@ +71,5 @@ > +created when we access the TabActor's grip. We return the tab-scoped actors > +`actorID` in it. Actors inheriting from TabActor expose `attach`/`detach` > +requests, that allows to start/stop the ThreadActor. > + > +The tab-scoped actors have some expectation on the "TabActor": Nit: more explicit than "have some expectation" would be "expect to find the following properties" @@ +78,5 @@ > + only defined once `attach` request is called, or on construction. > + - isRootActor: (historical) > + Always false, but for ChromeActor. > + Despite the attribute name, indicated to not limit to content resources, > + but accept all of them, especially chrome one. I can't parse this sentence. Do you mean something other than "Always false, except on ChromeActor, but the property is present on every actor"? It would be a good idea to explain why it is deprecated now. @@ +81,5 @@ > + Despite the attribute name, indicated to not limit to content resources, > + but accept all of them, especially chrome one. > + - makeDebugger: > + Helper object with two functions to create Debugger object for the targeted > + context. Helper *function* used to create a Debugger object (it can be configured by a configuration object with two functions). Also, reference make-debugger.js for finding more information about it. @@ +88,5 @@ > +attributes and events: > + - window: > + Reference to the window global object currently targeted. > + It can change over time if we switch context to an iframe. > + So that you shouldn't store that in a variable, but always query it. Slightly better: "It can change over time if we switch context to an iframe, so it shouldn't be stored in a variable, but always retrieved from the actor". @@ +90,5 @@ > + Reference to the window global object currently targeted. > + It can change over time if we switch context to an iframe. > + So that you shouldn't store that in a variable, but always query it. > + - windows: > + List of all globals with the document's window object, plus all iframe's one. "List of all document globals, including the main window object and all iframes." @@ +92,5 @@ > + So that you shouldn't store that in a variable, but always query it. > + - windows: > + List of all globals with the document's window object, plus all iframe's one. > + - docShell: > + DocShell reference for the targeted context Nit: missing period.

(In reply to Panos Astithas [:past] from comment #10) > Comment on attachment 8617283 [details] [diff] [review] > > ::: toolkit/devtools/server/docs/actor-hierarchy.md > @@ +1,5 @@ > > +# How actors are organized > > + > > +To start with, actors are living within /toolkit/devtools/server/actors/ folder. > > +They are organized in a hierarchy for easier lifecycle/memory management: > > +once a parent is removed from the pool, its children are removed as well. > > Can you add a sentence here about how actors are lazily-loaded and > constructed on-demand? I've just added a link to actor-registration.md which gives more insight on actor implementation. > @@ +39,5 @@ > > + | Targets tabs living in the parent process. > > + | Returned by "listTabs" or "getTab" requests. > > + | > > + |-- ContentActor (childtab.js) > > + | Targets tabs living out-of-process (e10s) or apps (on firefox OS). > > Maybe also mention RemoteBrowserTabActor here (or in a separate bullet)? Oh, good catch! ContentActor isn't returned directly by RootActor. Instead, RemoveBrowserTabActor proxifies it. > > @@ +42,5 @@ > > + |-- ContentActor (childtab.js) > > + | Targets tabs living out-of-process (e10s) or apps (on firefox OS). > > + | Returned by "listTabs" or "getTab" requests. > > + | > > + |-- ChromeActor (chrome.js > > Unrelated, but wouldn't it make more sense to call the ChromeActor, > MainProcessActor, for consistency with ChildProcessActor? Yes. Let's figure this out in bug 1172897. > @@ +78,5 @@ > > + only defined once `attach` request is called, or on construction. > > + - isRootActor: (historical) > > + Always false, but for ChromeActor. > > + Despite the attribute name, indicated to not limit to content resources, > > + but accept all of them, especially chrome one. > > I can't parse this sentence. Do you mean something other than "Always false, > except on ChromeActor, but the property is present on every actor"? > > It would be a good idea to explain why it is deprecated now. Yes. I rephrased this sentence... It isn't deprecated, it is just that the name is outdated. It still serve the same purpose as before, but doesn't indicated that it is the root actor anymore.

no try run, this is just a new md file in tree for doc.