1172897 - Rename TabActor to something better!

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

While writing documentation in bug 1171416, it appeared that TabActor class name, defined in webbrowser.js was misleading. As well as some of its subclasses like ContentActor.
Depending on bug 1171416 discussions, we are going to try to agree on a better name and rename these classes.
That, to have a simplier documentation and ease grokking our codebase!

Comment 1

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

From discussion in bug 1171416, so far Alex and I agreed that:

* TabActor should be renamed DocumentActor (see bug 1171416 comment 7)

This leaves the names of the things that implement the DocumentActor interface.  Here are my proposed names:

* BrowserTabActor -> ParentBrowserActor

It's the document actor for a browser in the parent process.

* ContentActor -> ChildBrowserActor

It's the document actor for a browser in the child process (tabs / apps).

* ChromeActor -> ParentProcessActor

It's the document actor for the entire parent process.

* ChildProcessActor (no change)

It's the document actor for the entire child process.

* BrowserAddonActor -> AddonActor

It's the document actor an add-on.

So, that's my thinking.  We could potentially give all of them the suffix "DocumentActor" instead of just "Actor", to emphasize that the implement the DocumentActor interface (especially for the ones that do not inherit from DocumentActor).

Comment 2

[Alexandre Poirot [:ochameau]]

I think it's better without repeating DocumentActor in each name.
I'm wondering if BrowserTabActor/ContentActor should now be names ParentTabActor/ChildTabActor.
It can feel misleading as we are trying to get rid of that, but that would match the words being used in the platform code.
I'm suggesting alternatives for this, as "browser" for me means more than one tab. For me it's Firefox. BrowserTabActor today, is meant to be used against a <xul:browser> element, while ContentActor is used with a <html:iframe>. I thought about suggesting FrameActor, but that sounds very ambiguous.
At the end, I think I can live with ParentBrowserActor/ChildBrowserActor as I don't have any better name to suggest.

All other names looks good to me!

Comment 3

[Alexandre Poirot [:ochameau]]

And so, if you had to put a title to describe all these actors, what would you say?
 Child of the root actors?
 Target(ed) actors?
 Context actors?

Comment 4

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

Flagging a few others who may want to join the bike shed party.

Comment 5

[Jim Blandy :jimb]

I think the best way to approach a renaming like this is to look at the big picture and consider the whole ontology at once, just as Alex and Ryan are doing here. It's never just about one name, but about how all the names involved relate to each other and make a reasonable taxonomy.

I'm surprised at the choice of "document" for the key noun. Isn't it the case that, as the tab navigates, we switch from one document to another? I would expect a "document actor" to be associated with a specific document.

Is there any name for the thing whose identity remains unchanged as we navigate from one document to another? I thought that was a tab.

All that said, I emphatically *do not* object to whatever you and Alex think is good.

Comment 6

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

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> I think it's better without repeating DocumentActor in each name.
> I'm wondering if BrowserTabActor/ContentActor should now be names
> ParentTabActor/ChildTabActor.
> It can feel misleading as we are trying to get rid of that, but that would
> match the words being used in the platform code.
> I'm suggesting alternatives for this, as "browser" for me means more than
> one tab. For me it's Firefox. BrowserTabActor today, is meant to be used
> against a <xul:browser> element, while ContentActor is used with a
> <html:iframe>. I thought about suggesting FrameActor, but that sounds very
> ambiguous.

Tab seems confusing for ChildTabActor, since it can also be an app, so that isn't really a tab.

You're right that "browser" is tricky too, is it an entire browser with tabs, or just one <xul:browser>?

Maybe just these two should include DocumentActor in their names, as in: ParentDocumentActor and ChildDocumentActor?

Or, another thought is we could create both a ChildTabActor and a ChildAppActor, for naming clarity.  They would be the same object as today's ContentActor, just with two names to clearly state where they can be used.  If we did this, then ParentTabActor would fit the pattern I think.

It's hard to pick. :)

Comment 7

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

(In reply to Jim Blandy :jimb from comment #5)
> I'm surprised at the choice of "document" for the key noun. Isn't it the
> case that, as the tab navigates, we switch from one document to another? I
> would expect a "document actor" to be associated with a specific document.
> 
> Is there any name for the thing whose identity remains unchanged as we
> navigate from one document to another? I thought that was a tab.

I mostly chose it since it felt like it was (a) reasonably close to the intended meaning and (b) did not have too many alternative definitions (like e.g. "browser" does).

The current name of "tab actor" gets a bit muddied since there are many things implement its interface (apps, addons, etc.) which aren't usually thought of as tabs.

But yes, "document" does give up some precision when we're naming something than can navigate.  Maybe the Gecko terminology "docShell" is the better word?

Comment 8

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

I think another good change would be to move all these "document" actors (or whatever word we use choose) to a separate folder from the "regular" actors, and also rename the files to match the new names of the objects they contain.

Maybe something like toolkit/devtools/server/documents?

Comment 9

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

I thought WorkerActor would break the strict "document-ness" of TabActor and friends, but it looks like it is global-scoped, but not inheriting from TabActor, so that's good. On the other hand I wonder if we can avoid the imprecision of DocumentActor, by following the HTML spec. The spec's term for what TabActor contains is browsing context:

https://html.spec.whatwg.org/multipage/browsers.html#browsing-context

However, ContextActor is rather ambiguous and confusingly enough threadActor.actorPrefix == "context" (I think its original type was ContextActor), but BrowsingContextActor doesn't sound so bad, does it?

If we go with that, we could have the following transition:

TabActor -> BrowsingContextActor (abstract class)
BrowserTabActor -> MainContextActor (single-process class - non-e10s)
RemoteBrowserTabActor -> ParentContextActor (main process class - e10s)
ContentActor -> ChildContextActor (child process class - non-e10s)

Comment 10

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

In general, I think your idea makes sense, but there are some finer points I am unsure on.

(In reply to Panos Astithas [:past] from comment #9)
> TabActor -> BrowsingContextActor (abstract class)
> BrowserTabActor -> MainContextActor (single-process class - non-e10s)

Is "Main" really enough to convey the meaning here?  There are enough kinds of context actors, so what's the "main" one?  I am thinking of future readers here.  The name does not signal "single-process class - non-e10s" to me.  Somehow we need to say "browsing context that resides entirely in the main process"...  "SingleProcessContextActor"?

> RemoteBrowserTabActor -> ParentContextActor (main process class - e10s)

Ah yes, I had skipped over this shim class in my naming...  It's nice to have this be named "parent" and the other end "child" like you have it, since it matches conventions outside of DevTools too.

> ContentActor -> ChildContextActor (child process class - non-e10s)

What would you purpose for:

* ChromeActor
* ChildProcessActor
* BrowserAddonActor

under your scheme?

Comment 11

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> (In reply to Panos Astithas [:past] from comment #9)
> > TabActor -> BrowsingContextActor (abstract class)
> > BrowserTabActor -> MainContextActor (single-process class - non-e10s)
> 
> Is "Main" really enough to convey the meaning here?  There are enough kinds
> of context actors, so what's the "main" one?  I am thinking of future
> readers here.  The name does not signal "single-process class - non-e10s" to
> me.  Somehow we need to say "browsing context that resides entirely in the
> main process"...  "SingleProcessContextActor"?

It is definitely not as clear as SingleProcessContextActor (which was also my first thought), but we have been using the term Main Process in our UI (WebIDE and the Connect screen), so I think it shouldn't be that foreign to most people. My other concern is that SingleProcessContextActor sounds close enough to ParentProcessActor and ChildProcessActor, which might indicate that they are part of the same group, when they are not (even though the term Process is there). But I am not too fussed about this particular name.

> What would you purpose for:
> 
> * ChromeActor
> * ChildProcessActor
> * BrowserAddonActor
> 
> under your scheme?

I like the ones you proposed in comment 1.

Comment 12

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

I'd like to move this forward.  As we revisit these actors for Fission, it would be great to finally have some better names to talk about each one.

Overall, I like using the word "context".  Panos had a good scheme in comment 9.

I'll use the User Story field on this bug to capture my proposed renamings, so we can adjust in case I've left any out, etc.

Alex, what do you think about my latest proposal here?

Comment 13

[Alexandre Poirot [:ochameau]]

(Commenting on User Story)
> Rename the various actors for a "context" using the following scheme.  While
> "ContextActor" does make some of them a bit long, it's very helpful to
> clearly identify them as "different" from "regular" actors via a common term.

I still get the feeling that context is vague. And this isn't something we use anywhere in mozilla codebase (TBH, we now start using it in servo), nor in devtools jargon. When opening this bug again, I immediately thought about "document" again. But then re-read Jim's comment and internaly said, ahhh yes that's very true...
Then, thought about "frame", but that doesn't work well for Window/Chrome actors.

And finally I got "target" in mind, which seems to work fine. You are attaching to particular targets.
It matches the target object we use on client from where we attach/detach.
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#441
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#679
And this is where we listen for various events from TabActor.
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#525-566
This is also where we handle specifics for WebExtensionParentActor:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#422-431
And WorkerActor ones:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#836-918

Now it is a trade, we either matches Spec naming or our client naming.
If we go for the spec, we may want look into client side namings.

But it sounds important to discuss what kind of changes you expect to happen on the client side.
As the more I look into target.js and its callsites, the more I see direct translation of all these actors into the client side. It looks like, over time, we kind of lost the direct mapping of these actors into a clear client/front. We diffused TabActor client between target.js, debugger-client.js and tab-client.js.
A good example is that TabTarget is used as a kind of client/front class for most actors.
Whereas actors use inheritance to help segregating specifics into child classes, TabTarget just contains all specifics in one class. We may benefit from implementing independant classes like WorkerTarget and use inheritance.
Then, we may end up with one client-side class per actor and have the same naming, inheritance on the client side.

Otherwise, we may sort these actors in groups:
* TabActor and all its subclasses:
  TabActor, ContentActor, WindowActor, ChromeActor, WebExtensionChildActor.
* Parent process proxies:
  BrowserTabActor
  This isn't an actor really, it doesn't implement any methods. It mostly help getting the form from content process.
  It looks more like an helper class used by RootActor to implement getTab/listTabs.
* Classes trying to implement TabActor interface but not inherithing from it:
  WorkerActor, BrowserAddonActor
* WebExtensionParentActor:
  It requires its own group as it is between a parent process proxy and a class trying to implement TabActor interface.
  It feel very hacky and may be better with its own Target class, like worker, rather than hacking things into TabActor.

Comment 14

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

Targets are certainly a mess, I agree!  I think we should be open to big changes there.

Looking at target today, it isn't exactly a client of the thing being targeted.  Instead, it holds the tab-client (or similar) as the `activeTab` field.  So we have something like:

TabTarget
|_ activeTab: TabClient <-> TabActor

Things get much stranger for things like WebExtensions where you'd have:

TabTarget
|_ activeTab: TabClient <-> WebExtensionChildActor

The interface that targets implement today is convenient as a way of passing around some helpers as well as a connection, it's entry point client, etc.

So, let's see, some points I agree with:

* We should blow up the targets and remake them using inheritance
* We should consider merging the target object with the main client for the thing being targeted, as its currently a random separation of work

Coming back around to naming though, which is the focus of this bug...  I originally liked "context" precisely because it has no existing meaning in our code base.  All the other words mean any of 10 different things.  The concept however is definitely related to targets: we are trying to name the "things that can be targeted by the tools".

If we just keep rolling with the word "target", what does it look like for naming...?

Rename ContentActor to ChildBrowsingTargetActor
Create ChildBrowsingTargetFront (extends a new BrowsingTargetFront) which are reimagined versions of TabTarget but with inheritance

Is something like that what you mean?

Comment 15

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> Targets are certainly a mess, I agree!  I think we should be open to big
> changes there.
> 
> Looking at target today, it isn't exactly a client of the thing being
> targeted.  Instead, it holds the tab-client (or similar) as the `activeTab`
> field.  So we have something like:
> 
> TabTarget
> |_ activeTab: TabClient <-> TabActor
> 
> Things get much stranger for things like WebExtensions where you'd have:
> 
> TabTarget
> |_ activeTab: TabClient <-> WebExtensionChildActor
> 
> The interface that targets implement today is convenient as a way of passing
> around some helpers as well as a connection, it's entry point client, etc.
> 
> So, let's see, some points I agree with:
> 
> * We should blow up the targets and remake them using inheritance
> * We should consider merging the target object with the main client for the
> thing being targeted, as its currently a random separation of work
> 
> Coming back around to naming though, which is the focus of this bug...  I
> originally liked "context" precisely because it has no existing meaning in
> our code base.

That may be the point from where I'm diverging.
I think I'm simply expecting to follow the common rules of protocol.js,
where, for each FooActor, we have a fooSpec and a FooFront.
So I'm expecting to have similar pattern for these actors.
(That is the reason why I'm trying to put BrowserTabActor in a special category as it isn't really an actor, but rather an helper class)

>  All the other words mean any of 10 different things.  The
> concept however is definitely related to targets: we are trying to name the
> "things that can be targeted by the tools".
> 
> If we just keep rolling with the word "target", what does it look like for
> naming...?
>
> Rename ContentActor to ChildBrowsingTargetActor
> Create ChildBrowsingTargetFront (extends a new BrowsingTargetFront) which
> are reimagined versions of TabTarget but with inheritance
> 
> Is something like that what you mean?

TBH, I didn't thought about actual names for everything.
I mostly wanted to know where we are going from client side post-fission.
I'm expecting target.js to change and have something to easily go through different targets.
We could as well draw a client-side refactoring where we would align BrowserContext naming into the client APIs,
rather than reusing Target naming into the server APIs.
I think I would be happy either way.

But yes, I had something like this in mind.

Comment 16

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

I am not committed to word "context" in particular; "target" may also be okay.  For me, one of the key goals here is to label all "things that can be targeted" with the same word to clearly mark them as different from other regular actors.

I'll keep thinking about the precise names for each one.

I agree with your points about the "helper" classes being a bit different.  "Proxy" feels like a good word for them:

* BrowserTabActor
* WebExtensionParentActor plus its ProxyChildActor

Comment 17

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

I have folded in the latest round of feedback.

What do you think?

Comment 18

[Alexandre Poirot [:ochameau]]

(Commenting on User Story)
> Real Actors
> -----------
> 
> * TabActor -> BrowsingContextTargetActor

Long, but explicit.
I imagine you didn't choose TargetActor as we would easily expect all the actors to be inheriting from it...
May be the folder layout could help sorting these things out:
actors/targets:
  * browsingcontexttarget.js
  * browsercontexttarget:
    * frametarget.js
    * chromewindow.js
    *...
  * workertarget.js
  * ...
(I'm not sure we want to repeat target in all file names?)

> * ContentActor -> FrameTargetActor

Unfortunately, it doesn't access any frame.
In the content process we don't have access to the frame (in term of <xul:browser><html:iframe>).
Instead we have the "chromeGlobal" from where we pull the message manager and the docShell.
We could associate chromeGlobal to the frame scripts as it is their global, but I'm not sure we want to convey here.
But overall, yes, it does allow debugging a frame from the content process perspective.

> * WorkerActor -> WorkerTargetActor

Sounds great.

> * WindowActor -> ChromeWindowTargetActor

Looks great.
This is a great idea to specify "chrome" for the window!

> * ChromeActor -> ParentProcessTargetActor

There is a notion of chrome windows that we can debug from this actor.
As-is we could think it is a bit like the RootActor and it would allow debugging anything like the workers.
But I think it is fine. We may insist about this in docs or file header comment.
It would be great to put the small description of actor you wrote here into each actor's header!

> * ChildProcessActor -> ChildProcessTargetActor.

This one allows exposing workers but doesn't target any window.
So the duality of ParentProcessTargetActor and ChildProcessTargetActor may confuse.
But again, the folder hiearchy and docs can help mitigate that.

> * BrowserAddonActor -> AddonTargetActor

Sounds good.

> * WebExtensionChildActor -> WebExtensionTargetActor

Looks good.

> Proxy Actors
> ------------
> 
> * BrowserTabActor -> FrameProxyActor

nit: What about FrameActorProxy to really insist in saying it is a proxy rather than an actor?

> * WebExtensionParentActor and ProxyChildActor -> WebExtensionProxyActor

Do you want to merge them? See bug 1302702 comment 57 and 58 for more context.
As for FrameProxyActor, I think it is important to not end with Actor for proxies as they aren't actors.
At the end ProxyChildActor is just an helper/internal to WebExtensionParentActor,
its name doesn't have to be very specific and it shouldn't be perfect as it isn't exposed.

So either way, I think it is fine here.

> Targets
> -------
> 
> We should remove the existing client-side targets:
> 
> * TabTarget
> * WorkerTarget
> 
> and create "TargetFronts" for all "TargetActors" using inheritance to share
> common functions, similar to actor side:

I imagine we will learn from Yulia's experiment on the actor side to see how inheritance fits with protocol.js
But yes, I'm convinced we will have to do something to simplify access to all targets for fission.
TabTarget is clearly made of too many historical things and convey too many things at ones to match fission needs.

Comment 19

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

(In reply to Alexandre Poirot [:ochameau] from comment #18)
> > * BrowserTabActor -> FrameProxyActor
> 
> nit: What about FrameActorProxy to really insist in saying it is a proxy
> rather than an actor?

Sure, that seems reasonable.  I made this change.

> > * WebExtensionParentActor and ProxyChildActor -> WebExtensionProxyActor
> 
> Do you want to merge them? See bug 1302702 comment 57 and 58 for more
> context.
> As for FrameProxyActor, I think it is important to not end with Actor for
> proxies as they aren't actors.
> At the end ProxyChildActor is just an helper/internal to
> WebExtensionParentActor,
> its name doesn't have to be very specific and it shouldn't be perfect as it
> isn't exposed.

Also renamed to end with Proxy.  Yes, maybe they should be merged... though I haven't decided if I want to take that on right now.  We'll see. :)

Comment 20

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

Moved the WebExtension proxy parts to a follow up (bug 1466733) as I can't quite decide what's best, and there's already quite a lot to review here.

Comment 21

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

Attachment: Bug 1172897 - Rename TabActor to BrowsingContextTargetActor.

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

Comment 22

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

Attachment: Bug 1172897 - Rename ContentActor to FrameTargetActor.

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

Comment 23

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

Attachment: Bug 1172897 - Extract BrowserTabActor to new file.

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

Comment 24

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

Attachment: Bug 1172897 - Remove deprecated eager actor construction path.

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

Comment 25

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

Attachment: Bug 1172897 - Rename BrowserTabActor to FrameTargetActorProxy.

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

Comment 26

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

Attachment: Bug 1172897 - Extract service worker actors to new module.

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

Comment 27

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

Attachment: Bug 1172897 - Rename WorkerActor to WorkerTargetActor.

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

Comment 28

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

Attachment: Bug 1172897 - Rename WindowActor to ChromeWindowTargetActor.

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

Comment 29

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

Attachment: Bug 1172897 - Rename ChromeActor to ParentProcessTargetActor.

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

Comment 30

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

Attachment: Bug 1172897 - Rename ChildProcessActor to ContentProcessTargetActor.

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

Comment 31

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

Attachment: Bug 1172897 - Move DevTools addon modules to subdir.

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

Comment 32

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

Attachment: Bug 1172897 - Rename BrowserAddonActor to AddonTargetActor.

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

Comment 33

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

Attachment: Bug 1172897 - Rename WebExtensionChildActor to WebExtensionTargetActor.

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

Comment 34

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03fec6bca7b39bd3a31f9de054725e1eee83468e

Comment 35

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

Comment on attachment 8983271 [details]
Bug 1172897 - Rename TabActor to BrowsingContextTargetActor.

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

Comment 36

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

Comment on attachment 8983272 [details]
Bug 1172897 - Rename ContentActor to FrameTargetActor.

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

Comment 37

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

Comment on attachment 8983273 [details]
Bug 1172897 - Extract BrowserTabActor to new file.

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

Comment 38

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

Comment on attachment 8983274 [details]
Bug 1172897 - Remove deprecated eager actor construction path.

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

Comment 39

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

Comment on attachment 8983275 [details]
Bug 1172897 - Rename BrowserTabActor to FrameTargetActorProxy.

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

Comment 40

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

Comment on attachment 8983276 [details]
Bug 1172897 - Extract service worker actors to new module.

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

Comment 41

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

Comment on attachment 8983277 [details]
Bug 1172897 - Rename WorkerActor to WorkerTargetActor.

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

Comment 42

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

Comment on attachment 8983278 [details]
Bug 1172897 - Rename WindowActor to ChromeWindowTargetActor.

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

Comment 43

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

Comment on attachment 8983279 [details]
Bug 1172897 - Rename ChromeActor to ParentProcessTargetActor.

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

Comment 44

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

Comment on attachment 8983280 [details]
Bug 1172897 - Rename ChildProcessActor to ContentProcessTargetActor.

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

Comment 45

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

Comment on attachment 8983281 [details]
Bug 1172897 - Move DevTools addon modules to subdir.

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

Comment 46

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

Comment on attachment 8983282 [details]
Bug 1172897 - Rename BrowserAddonActor to AddonTargetActor.

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

Comment 47

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

Comment on attachment 8983283 [details]
Bug 1172897 - Rename WebExtensionChildActor to WebExtensionTargetActor.

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

Comment 48

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44817c2aa97b91df5ce1b4cb640189aa6c535846

Comment 49

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983271 [details]
Bug 1172897 - Rename TabActor to BrowsingContextTargetActor.

https://reviewboard.mozilla.org/r/249154/#review255790

That was quite a review to do!
Thanks a lot for having submitted split patches.

Looks good to me if you manage to have a green try and ensure you keep the history of moved files.

The new doc and namings looks clearer to me, I hope it will be for new contributors too!

::: devtools/client/framework/target-from-url.js:83
(Diff revision 2)
>        const response = await client.getProcess(id);
>        form = response.form;
>        chrome = true;
>        if (id != 0) {
> -        // Child process are not exposing tab actors and only support debugger+console
> -        isTabActor = false;
> +        // Child processes are not exposing full target actors; instead they
> +        // only support debugger and console.

I'm not sure "full target actors" is clear?
Didn't you meant to say: "browser context target actors"?

::: devtools/client/framework/target.js:315
(Diff revision 2)
>  
> -  // Tells us if the related actor implements TabActor interface
> -  // and requires to call `attach` request before being used
> -  // and `detach` during cleanup
> -  get isTabActor() {
> -    return this._isTabActor;
> +  // Tells us if the related actor implements BrowsingContextTargetActor
> +  // interface and requires to call `attach` request before being used and
> +  // `detach` during cleanup.
> +  // TODO: This flag is quite confusing, try to find a better way.
> +  // Bug 1465635 hopes to blow up these classes entirely.

Can't wait to see that happening!
It may work alongside with bug 1222047...

::: devtools/docs/backend/actor-hierarchy.md:23
(Diff revision 2)
>     |
> -   \--> "TabActor" (or alike):
> -          |    Actors meant to designate one context (document, tab, app,
> -          |    add-on, or worker) and track its lifetime.  Generally, there is
> -          |    one of these for each thing you can point a toolbox at.
> +   \--> Target actors:
> +          |    Actors that represent the main "thing" being targeted by a given
> +          |    toolbox, such as a tab, frame, worker, add-on, etc. and track its
> +          |    lifetime.  Generally, there is a target actor for each thing you
> +          |    can point a toolbox at.

I must say, the two categories "Target actors" and "Tab-scoped actors" are still mysterious described that way.

I think it would help to:
* write down actor examples for "Target actors".
* in "Tab-scoped actors", I find it confusing to repeat "targeted thing (document, tab, add-on or worker)". It may be easier to mention the relation between Tab scoped and target actors:
"Actors exposing one particular feature set, like console and inspector actors. They are (served by)|(children of) Target actors and (reuse|inspect) its target ("thing")?." (feel free to rephrase all of this again)

::: devtools/docs/backend/actor-hierarchy.md:92
(Diff revision 2)
>  ```
>  
> -## "TabActor"
> +## Target Actors
>  
>  Those are the actors exposed by the root actors which are meant to track the
> -lifetime of a given context: tab, app, process, add-on, or worker. It also
> +lifetime of a given context: tab, process, add-on, or worker. It also allows to

nit: s/context/target/?

::: devtools/docs/backend/actor-hierarchy.md:138
(Diff revision 2)
>     List of all docshells for the targeted document and all its iframes.
>   - chromeEventHandler:
>     The chrome event handler for the current context. Allows to listen to events
>     that can be missing/cancelled on this document itself.
>  
> -See TabActor documentation for events definition.
> +See BrowsingContextTargetActor documentation for more details.

This paragraph looks much clearer with the new namings! The addition about ThreadActor history is also helpful.

::: devtools/server/actors/chrome.js:25
(Diff revision 2)
>  
>  /**
> - * Creates a TabActor for debugging all the chrome content in the
> - * current process. Most of the implementation is inherited from TabActor.
> - * ChromeActor is a child of RootActor, it can be instanciated via
> - * RootActor.getProcess request.
> + * Creates a target actor for debugging all the chrome content in the current process.
> + * Most of the implementation is inherited from BrowsingContextTargetActor. ChromeActor is
> + * a child of RootActor, it can be instantiated via RootActor.getProcess request.
> + * ChromeActor exposes all tab actors via its form() request, like

tab actors?
I imagine it should be tab-scoped actors?
But I see that you opened bug 1465637...

We should at least be consistent with the doc here. Feel free to handle that in the followup.

::: devtools/server/actors/targets/browsing-context.js:105
(Diff revision 2)
>    /**
> -   * Creates a TabActor whose main goal is to manage lifetime and
> -   * expose the tab actors being registered via DebuggerServer.registerModule.
> -   * But also track the lifetime of the document being tracked.
> +   * BrowsingContextTargetActor is an abstract class used by target actors that
> +   * hold documents, such as frames, chrome windows, etc.  The term "browsing
> +   * context" is defined in the HTML spec as "an environment in which `Document`
> +   * objects are presented to the user".  In Gecko, this means a browsing context
> +   * is a `docShell`.

Should we really duplicated this between file header and here? Can't we keep it only in one place?

::: devtools/server/actors/targets/browsing-context.js:526
(Diff revision 2)
> -    // Tell the thread actor that the tab is closed, so that it may terminate
> +    // Tell the thread actor that the browsing context is closed, so that it may terminate
>      // instead of resuming the debuggee script.
>      if (this._attached) {
> -      this.threadActor._tabClosed = true;
> +      // TODO: Can the thread actor check our `_attached` state instead of keeping this
> +      // separate flag?
> +      this.threadActor._parentClosed = true;

Did you opened a followup for this?

I would really like to see the thread actor uncoupled from browsing context actor! (we may have a bug already opened...)

::: devtools/shared/specs/targets/browsing-context.js:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This is a new file instead of a rename of specs/tabs.js

Comment 50

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983272 [details]
Bug 1172897 - Rename ContentActor to FrameTargetActor.

https://reviewboard.mozilla.org/r/249156/#review255824

::: devtools/server/actors/targets/frame.js:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This file is new instead of renamed.
It sounds important to keep its history.

::: devtools/shared/specs/targets/frame.js:10
(Diff revision 2)
> +
> +const { generateActorSpec } = require("devtools/shared/protocol");
> +const { extend } = require("devtools/shared/extend");
> +const { browsingContextTargetSpecPrototype } = require("devtools/shared/specs/targets/browsing-context");
> +
> +const frameTargetSpec = generateActorSpec(extend(browsingContextTargetSpecPrototype, {

Should we followup to tweak generateActorSpec to support spec inheritance?

generateActorSpec could accept an argument specifying a spec to inherit from.

Comment 51

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983273 [details]
Bug 1172897 - Extract BrowserTabActor to new file.

https://reviewboard.mozilla.org/r/249158/#review255860

Comment 52

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983274 [details]
Bug 1172897 - Remove deprecated eager actor construction path.

https://reviewboard.mozilla.org/r/249160/#review255840

Thanks a lot for removing such prehistoric piece of code!

::: devtools/server/actors/preference.js
(Diff revision 2)
> -exports.register = function(handle) {
> -  handle.addGlobalActor(PreferenceActor, "preferenceActor");
> -};
> -
> -exports.unregister = function(handle) {
> -};

Wha. That's a very old leftover!

::: devtools/server/main.js:1242
(Diff revision 2)
> -   *        See the it's definition for more info.
> -   *
> -   * @param name string [optional]
> -   *        The name of the new request type. If this is not present, the
> -   *        actorPrefix property of the constructor prototype is used.
> -   */
> +   *        The name of the new request type.
> +   */
> +  addTabActor(actor, name) {
> +    if (typeof actor == "function") {
> +      throw Error("addTabActor no longer supports eagerly loaded actors");
> +    }

Given that we no longer support addons, I'm not sure this is any useful?

::: devtools/server/main.js:1316
(Diff revision 2)
> -   *        See the it's definition for more info.
> -   *
> -   * @param name string [optional]
> -   *        The name of the new request type. If this is not present, the
> -   *        actorPrefix property of the constructor prototype is used.
> -   */
> +   *        The name of the new request type.
> +   */
> +  addGlobalActor(actor, name) {
> +    if (typeof actor == "function") {
> +      throw Error("addGlobalActor no longer supports eagerly loaded actors");
> +    }

Same comment, I'm not sure this is useful.

Comment 53

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983275 [details]
Bug 1172897 - Rename BrowserTabActor to FrameTargetActorProxy.

https://reviewboard.mozilla.org/r/249162/#review255844

::: devtools/server/actors/common.js:128
(Diff revision 2)
>   *     some reply packet (say, a target actor grip or the "listTabs" response
>   *     form), and whose own property values are actor constructor functions, as
>   *     documented for addTabActor and addGlobalActor.
>   *
>   * @param this
> - *     The BrowserRootActor or BrowserTabActor with which the new actors will
> + *     The RootActor or BrowsingContextTargetActor with which the new actors

Good catch!
It highlights that we were lost with our actor hierarchy...

Comment 54

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983276 [details]
Bug 1172897 - Extract service worker actors to new module.

https://reviewboard.mozilla.org/r/249164/#review255862

Comment 55

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983277 [details]
Bug 1172897 - Rename WorkerActor to WorkerTargetActor.

https://reviewboard.mozilla.org/r/249166/#review255864

Comment 56

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983278 [details]
Bug 1172897 - Rename WindowActor to ChromeWindowTargetActor.

https://reviewboard.mozilla.org/r/249168/#review255848

::: devtools/server/actors/targets/chrome-window.js:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

You lost the history for this file.

Comment 57

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983279 [details]
Bug 1172897 - Rename ChromeActor to ParentProcessTargetActor.

https://reviewboard.mozilla.org/r/249170/#review255850

::: devtools/server/actors/targets/parent-process.js:14
(Diff revision 2)
> + *
> + * This actor extends BrowsingContextTargetActor.
> + * This actor is extended by WebExtensionChildActor.
> + *
> + * See devtools/docs/backend/actor-hierarchy.md for more details.
> + */

My hope is that these very short description will really help following the actor codebase, even without going through the whole doc.

Comment 58

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983280 [details]
Bug 1172897 - Rename ChildProcessActor to ContentProcessTargetActor.

https://reviewboard.mozilla.org/r/249172/#review255854

::: devtools/docs/backend/actor-hierarchy.md:85
(Diff revision 2)
>     |
> -   |-- ChildProcessActor (child-process.js)
> -   |   Targets the chrome of the child process (e10s).
> -   |   Returned by "getProcess" request with a id argument,
> -   |   matching the targeted process.
> +   |-- ContentProcessTargetActor (content-process.js)
> +   |   Targets all resources in a content process of Firefox (chrome sandboxes,
> +   |   frame scripts, documents, etc.)
> +   |   Returned by "getProcess" request with a id argument, matching the
> +   |   targeted process.

Thanks, it is clearer with your tweaks!

Comment 59

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983281 [details]
Bug 1172897 - Move DevTools addon modules to subdir.

https://reviewboard.mozilla.org/r/249174/#review255856

Even if switching to typeName everywhere may work as-is, I find it premature to switch to it just yet.

::: devtools/server/actors/addon/console.js
(Diff revision 2)
>  });
>  
> -exports.AddonConsoleActor = ActorClassWithSpec(webconsoleSpec, addonConsolePrototype);
> +exports.AddonConsoleActor = ActorClassWithSpec(addonConsoleSpec, addonConsolePrototype);
> -
> -// TODO: remove once protocol.js can handle inheritance. Bug #1450960
> -exports.AddonConsoleActor.prototype.typeName = "addonConsole";

Sorry, this is confusing as both TODO in this file refers to the wrong bug.
This good one is bug 1452920 and still opened.
We have to keep this and actorPrefix override for now.

::: devtools/server/actors/webconsole.js:313
(Diff revision 2)
>     * @private
>     * @type array
>     */
>    _webConsoleCommandsCache: null,
>  
> -  actorPrefix: "console",
> +  typeName: "console",

Please revert this, the console actor isn't converted to protocol.js yet. It still need actorPrefix.

Comment 60

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983282 [details]
Bug 1172897 - Rename BrowserAddonActor to AddonTargetActor.

https://reviewboard.mozilla.org/r/249176/#review255866

Comment 61

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983283 [details]
Bug 1172897 - Rename WebExtensionChildActor to WebExtensionTargetActor.

https://reviewboard.mozilla.org/r/249178/#review255868

Thanks again for going through such a refactoring!

Comment 62

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

Comment on attachment 8983281 [details]
Bug 1172897 - Move DevTools addon modules to subdir.

https://reviewboard.mozilla.org/r/249174/#review255856

> Please revert this, the console actor isn't converted to protocol.js yet. It still need actorPrefix.

I agree that historically, we have left `actorPrefix` for older actors and `typeName` for p.js actors.  However, it doesn't really matter whether the actor comes from p.js does it?  When adding an actor to a pool, we check both fields no matter the "style" of actor[1].

In this case, since a leaf actor (addon console) is already p.js, I thought it might be okay to change the full inheritance tree (regular console and addon console) to use `typeName` so that everyone's using the same field, even without a full p.js transition.  Do you see something that would break because of this, or do you mean prefer to only make this change as part of also doing the rest of p.js conversion for an actor?

(Since we know longer need to think about add-ons, it seems like we could rewrite `actorPrefix` with `typeName` for all actors without doing other p.js conversion steps.  Something to consider for a possible follow up.)

What do you think about this?

[1]: https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/devtools/server/actors/common.js#238

Comment 63

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

Please see comment 62.

Comment 64

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #62)
> Comment on attachment 8983281 [details]
> Bug 1172897 - Move DevTools addon modules to subdir.
> 
> https://reviewboard.mozilla.org/r/249174/#review255856
> 
> > Please revert this, the console actor isn't converted to protocol.js yet. It still need actorPrefix.
> 
> I agree that historically, we have left `actorPrefix` for older actors and
> `typeName` for p.js actors.  However, it doesn't really matter whether the
> actor comes from p.js does it?  When adding an actor to a pool, we check
> both fields no matter the "style" of actor[1].

Yes, it should not matter.

> In this case, since a leaf actor (addon console) is already p.js, I thought
> it might be okay to change the full inheritance tree (regular console and
> addon console) to use `typeName` so that everyone's using the same field,
> even without a full p.js transition.  Do you see something that would break
> because of this, or do you mean prefer to only make this change as part of
> also doing the rest of p.js conversion for an actor?

I imagine my first reaction was surprise of seeing such thing in middle of a renaming refactoring. As I said, it should work but I find it confusing to see typeName in old actor. If we get rid of actorPrefix everywhere, such move would be less confusing. As well as actorprefix override in in-progress refactoring.

> (Since we know longer need to think about add-ons, it seems like we could
> rewrite `actorPrefix` with `typeName` for all actors without doing other
> p.js conversion steps.  Something to consider for a possible follow up.)
> 
> What do you think about this?

Yes. Feel free to land as-is and create such follow-up that Nicolas and yulia would know about.

Comment 65

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

Comment on attachment 8983271 [details]
Bug 1172897 - Rename TabActor to BrowsingContextTargetActor.

https://reviewboard.mozilla.org/r/249154/#review255790

> I'm not sure "full target actors" is clear?
> Didn't you meant to say: "browser context target actors"?

Ah yes, I'll fix that, thanks.

> I must say, the two categories "Target actors" and "Tab-scoped actors" are still mysterious described that way.
> 
> I think it would help to:
> * write down actor examples for "Target actors".
> * in "Tab-scoped actors", I find it confusing to repeat "targeted thing (document, tab, add-on or worker)". It may be easier to mention the relation between Tab scoped and target actors:
> "Actors exposing one particular feature set, like console and inspector actors. They are (served by)|(children of) Target actors and (reuse|inspect) its target ("thing")?." (feel free to rephrase all of this again)

Okay, I have tried to rewrite this and include examples.

> nit: s/context/target/?

Good catch, I have made this change.

> tab actors?
> I imagine it should be tab-scoped actors?
> But I see that you opened bug 1465637...
> 
> We should at least be consistent with the doc here. Feel free to handle that in the followup.

Right, there are many places where we say "tab actors" to mean the tab-scoped ones.  I'll handle such places in the follow up.

> Should we really duplicated this between file header and here? Can't we keep it only in one place?

Haha, yeah... I couldn't decide which location was better. Anyway, I removed one of them. :)

> Did you opened a followup for this?
> 
> I would really like to see the thread actor uncoupled from browsing context actor! (we may have a bug already opened...)

Looks like you opened bug 997119 about 4 years ago. :)

> This is a new file instead of a rename of specs/tabs.js

I have tuned my cinnabar settings, so these should preserve history now.

Comment 66

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

Comment on attachment 8983272 [details]
Bug 1172897 - Rename ContentActor to FrameTargetActor.

https://reviewboard.mozilla.org/r/249156/#review255824

> Should we followup to tweak generateActorSpec to support spec inheritance?
> 
> generateActorSpec could accept an argument specifying a spec to inherit from.

Probably so!  I filed bug 1467560 for this.

Comment 67

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

Comment on attachment 8983274 [details]
Bug 1172897 - Remove deprecated eager actor construction path.

https://reviewboard.mozilla.org/r/249160/#review255840

> Given that we no longer support addons, I'm not sure this is any useful?

Okay, I'll remove it.  It was helpful while doing the conversion to spot left over problem cases, but probably not needed now.

Comment 68

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

(In reply to Alexandre Poirot [:ochameau] from comment #64)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #62)
> > (Since we know longer need to think about add-ons, it seems like we could
> > rewrite `actorPrefix` with `typeName` for all actors without doing other
> > p.js conversion steps.  Something to consider for a possible follow up.)
> > 
> > What do you think about this?
> 
> Yes. Feel free to land as-is and create such follow-up that Nicolas and
> yulia would know about.

Great!  I filed bug 1467565 for this work.

Comment 69

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

Comment on attachment 8983271 [details]
Bug 1172897 - Rename TabActor to BrowsingContextTargetActor.

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

Comment 70

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

Comment on attachment 8983272 [details]
Bug 1172897 - Rename ContentActor to FrameTargetActor.

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

Comment 71

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

Comment on attachment 8983273 [details]
Bug 1172897 - Extract BrowserTabActor to new file.

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

Comment 72

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

Comment on attachment 8983274 [details]
Bug 1172897 - Remove deprecated eager actor construction path.

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

Comment 73

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

Comment on attachment 8983275 [details]
Bug 1172897 - Rename BrowserTabActor to FrameTargetActorProxy.

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

Comment 74

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

Comment on attachment 8983276 [details]
Bug 1172897 - Extract service worker actors to new module.

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

Comment 75

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

Comment on attachment 8983277 [details]
Bug 1172897 - Rename WorkerActor to WorkerTargetActor.

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

Comment 76

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

Comment on attachment 8983278 [details]
Bug 1172897 - Rename WindowActor to ChromeWindowTargetActor.

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

Comment 77

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

Comment on attachment 8983279 [details]
Bug 1172897 - Rename ChromeActor to ParentProcessTargetActor.

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

Comment 78

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

Comment on attachment 8983280 [details]
Bug 1172897 - Rename ChildProcessActor to ContentProcessTargetActor.

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

Comment 79

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

Comment on attachment 8983281 [details]
Bug 1172897 - Move DevTools addon modules to subdir.

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

Comment 80

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

Comment on attachment 8983282 [details]
Bug 1172897 - Rename BrowserAddonActor to AddonTargetActor.

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

Comment 81

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

Comment on attachment 8983283 [details]
Bug 1172897 - Rename WebExtensionChildActor to WebExtensionTargetActor.

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

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4918c8b1dd9
Rename TabActor to BrowsingContextTargetActor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/90a8e47ce304
Rename ContentActor to FrameTargetActor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c774c6844a03
Extract BrowserTabActor to new file. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/19091d505409
Remove deprecated eager actor construction path. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/e59b5b99fbe2
Rename BrowserTabActor to FrameTargetActorProxy. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/100e22c71228
Extract service worker actors to new module. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/0a972bf5fd93
Rename WorkerActor to WorkerTargetActor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d9dfef07c688
Rename WindowActor to ChromeWindowTargetActor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/1c3f13764858
Rename ChromeActor to ParentProcessTargetActor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6ba2146c4238
Rename ChildProcessActor to ContentProcessTargetActor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/df974bbe7316
Move DevTools addon modules to subdir. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/18d2f5169579
Rename BrowserAddonActor to AddonTargetActor. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/67342f5762dd
Rename WebExtensionChildActor to WebExtensionTargetActor. r=ochameau

Comment 83

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b4918c8b1dd9
https://hg.mozilla.org/mozilla-central/rev/90a8e47ce304
https://hg.mozilla.org/mozilla-central/rev/c774c6844a03
https://hg.mozilla.org/mozilla-central/rev/19091d505409
https://hg.mozilla.org/mozilla-central/rev/e59b5b99fbe2
https://hg.mozilla.org/mozilla-central/rev/100e22c71228
https://hg.mozilla.org/mozilla-central/rev/0a972bf5fd93
https://hg.mozilla.org/mozilla-central/rev/d9dfef07c688
https://hg.mozilla.org/mozilla-central/rev/1c3f13764858
https://hg.mozilla.org/mozilla-central/rev/6ba2146c4238
https://hg.mozilla.org/mozilla-central/rev/df974bbe7316
https://hg.mozilla.org/mozilla-central/rev/18d2f5169579
https://hg.mozilla.org/mozilla-central/rev/67342f5762dd