1265722 - Decouple fronts from actors in inspector.

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P1)

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple NodeFront from NodeActor.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple NodeFront from NodeActor.

Attached the wrong patch here too, like a not smart person.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple NodeListFront from NodeListActor.

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple WalkerFront from WalkerActor.

This patch is a big one, but the changes are almost entirely mechanical.

Comment 5

[Eddy Bruel [:ejpbruel]]

The inspector actor uses the highlight actor, so before I can decouple the former, I need to decouple the latter.

A minor problem is that it's not clear where I should put the fronts and the protocol specifications for the highlight actors. My plan was to put the fronts for each tool in /devtools/client/<tool>/front.js, the protocol specifications for each tool in /devtools/shared/<tool>/specs.js, and to keep the actors where they are (in /devtools/server/actors).

The highlighter actors, however, are used by multiple tools, so there is no obvious place to put them. We could do something like /devtools/client/shared/fronts.js, /devtools/shared/shared/specs.js, but I'm not a fan of overloading the name shared like that.

Ryan, what do you think we should do here?

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple NodeFront from NodeActor.

Because some actors don't really belong to a single component, I ended up adopting the following directory scheme:

* Fronts go to /devtools/client/fronts/
* Specifications go to /devtools/shared/specs/
* Actors go to /devtools/server/actors

One big advantage of this is that if you're looking for the corresponding front or spec for an actor defined in, say, inspector.js, you can easily find it, because the path names are symmetric.

I'm going to update the patches to reflect this change.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple NodeListFront from NodeListActor.

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple WalkerFront from WalkerActor.

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple HighlighterFront from HighlighterActor

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple CustomHighlighterFront from HighlighterFront.

Comment 11

[Eddy Bruel [:ejpbruel]]

The inspector actor also uses the style actor. The style actor also uses the stylesheet actors. I will have to decouple those actors first before I can decouple the inspector actor.

Comment 12

[Eddy Bruel [:ejpbruel]]

Jryans, I figured out what was causing the ordering problems I ran into.

The inspector actor is defined in inspector.js. It uses the style actor, which is defined in styles.js. The style actor uses the node actor, which is also defined in inspector.js. In addition, the inspector actor uses the highlighter actors, which are defined in highlighters.js. These also use the node actor. So, we have two cyclic dependencies.

What we should have done was define the node actor in a separate file, so inspector.js, styles.js, and highlighter.js can all require it individually. Instead, we opted to predeclare the node actor in style.js, by explicitly adding its type. protocol.js only allows this is the type does not already exist.

Here's where things get hairy. styles.js and highlighter.js both need to predeclare the node actor, but since protocol.js doesn't allow a type to be added twice, only styles.js does. As a result, highlighter.js needs to be required *after* styles.js.

Comment 13

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

(In reply to Eddy Bruel [:ejpbruel] from comment #12)
> Jryans, I figured out what was causing the ordering problems I ran into.
> 
> The inspector actor is defined in inspector.js. It uses the style actor,
> which is defined in styles.js. The style actor uses the node actor, which is
> also defined in inspector.js. In addition, the inspector actor uses the
> highlighter actors, which are defined in highlighters.js. These also use the
> node actor. So, we have two cyclic dependencies.
> 
> What we should have done was define the node actor in a separate file, so
> inspector.js, styles.js, and highlighter.js can all require it individually.
> Instead, we opted to predeclare the node actor in style.js, by explicitly
> adding its type. protocol.js only allows this is the type does not already
> exist.
> 
> Here's where things get hairy. styles.js and highlighter.js both need to
> predeclare the node actor, but since protocol.js doesn't allow a type to be
> added twice, only styles.js does. As a result, highlighter.js needs to be
> required *after* styles.js.

While breaking out the node actor to a separate file is one way to go, I think we can also remove the footgun here so that p.js is a bit more flexible here:

I believe we can change `addActorType` to allow declaring an actor type multiple times.  Specifically, at the beginning of `addActorType`, check if the type name is registered.  If it is, just return without error.  Since `addActorType` _only_ allows declaring (there are no options you can pass in), it seems safe to allow this multiple times.

Comment 14

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

(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> Created attachment 8743744 [details] [diff] [review]
> Decouple NodeFront from NodeActor.
> 
> Because some actors don't really belong to a single component, I ended up
> adopting the following directory scheme:
> 
> * Fronts go to /devtools/client/fronts/
> * Specifications go to /devtools/shared/specs/
> * Actors go to /devtools/server/actors
> 
> One big advantage of this is that if you're looking for the corresponding
> front or spec for an actor defined in, say, inspector.js, you can easily
> find it, because the path names are symmetric.
> 
> I'm going to update the patches to reflect this change.

Great, this is basically what I was hoping to see: I like that each of the 3 different modules keeps the same file name as the actor module.  That should make it easy to find the 3 related files even though they are spread around to different folders.

There's one thing that worries me about the fronts though.  By placing them in /devtools/client, they will be inaccessible to applications that do not ship the client code, such as remote devices (mainly Firefox for Android these days).  In production use, that's fine, since those devices shouldn't need to use the fronts.  But for testing, now we can't use the fronts when writing tests that run on such devices, since the fronts aren't available.

This issue is why the "main" / "legacy" client for the root actor and others is currently in the semi-strange location /devtools/shared/client/main.  It feels awkward, since the file is clearly a "client" and there's a whole /devtools/client directory, so why isn't it over there?!

I need to think a little more about this, I am not sure what the best answer is yet...  We could consider changing the build setup somewhat so that the fronts **can** live at /devtools/client/fronts like you have it, and then we'd include only that sub-directory of /devtools/client for remote devices.

Setting ni? to keep thinking.

Comment 15

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

Additional data about directories and remote devices:

* Firefox for Android production code:
  * Main files:
    * https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/mobile/android/chrome/content/RemoteDebugger.js
    * https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/mobile/android/modules/dbg-browser-actors.js
  * Modules used:
    * devtools/server/actors/root
    * devtools/server/main
    * devtools/server/actors/webbrowser

* Firefox for Android test code:
  * Main files:
    * https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/mobile/android/tests/browser/chrome/test_debugger_server.html
  * Modules used:
    * devtools/server/main

* B2G production code:
  * Main files:
    * https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/b2g/chrome/content/devtools/debugger.js
    * https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/b2g/chrome/content/devtools/hud.js
    * https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/b2g/components/DebuggerActors.js
  * Modules used:
    * devtools/server/main
    * devtools/shared/discovery/discovery
    * devtools/shared/client/main
    * devtools/shared/webconsole/utils
    * devtools/server/actors/eventlooplag
    * devtools/server/actors/performance-entries
    * devtools/server/actors/memory
    * devtools/server/actors/webbrowser
    * devtools/shared/DevToolsUtils

So, summary of this data:

* Firefox for Android only uses non-protocol.js bits and they are all server side, so there is no issue there.
* B2G uses several protocol.js fronts in production code, so it would eventually break if the new fronts aren't shipped there.  However, it's also a tier 3 product now, so I'm not sure how much this matters.

Given all this, I think the directory setup you're proposing should work.  When we get to converting memory / perf fronts, we may reach an issue with B2G which we can consider how to solve (or ignore) at that time.

Comment 16

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

(In reply to Jim Blandy :jimb from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > As far as file organization, do we want to keep the fronts in
> > /devtools/server/actors?  Would it make more sense for them to be in
> > /devtools/client/fronts or something?  Ideally it would eventually become
> > possible to use the client without any code from the /devtools/server
> > directory.
> 
> Why not devtools/shared/protocol?

:jimb, what's your reasoning for that over something like /devtools/client/fronts?  /devtools/client/fronts seems pretty natural to me, since the fronts feel like a client-side concern.  It's possible it would complicate testing of server-only build configurations that wanted to use fronts in tests, but we could make changes to support that case if we run into it.

Comment 17

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

Comment on attachment 8743744 [details] [diff] [review]
Decouple NodeFront from NodeActor.

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

::: devtools/server/actors/inspector.js
@@ +63,5 @@
>  const {Class} = require("sdk/core/heritage");
>  const {WalkerSearch} = require("devtools/server/actors/utils/walker-search");
>  const {PageStyleActor, getFontPreviewData} = require("devtools/server/actors/styles");
> +const {nodeSpec} = require("devtools/shared/specs/inspector");
> +const {NodeFront} = require("devtools/client/fronts/inspector");

Why does the actor require the front?  We should avoid that since the fronts aren't available everywhere, and it seems unused.

Comment 18

[Jim Blandy :jimb]

The protocol specs are used by both the fronts and the actors. The request and response specs are needed on the server side as well. Both sides need to require the spec file. This seems really obvious; perhaps I'm missing something?

Comment 19

[Jim Blandy :jimb]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> (In reply to Jim Blandy :jimb from comment #8)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > > As far as file organization, do we want to keep the fronts in
> > > /devtools/server/actors?  Would it make more sense for them to be in
> > > /devtools/client/fronts or something?  Ideally it would eventually become
> > > possible to use the client without any code from the /devtools/server
> > > directory.
> > 
> > Why not devtools/shared/protocol?
> 
> :jimb, what's your reasoning for that over something like
> /devtools/client/fronts?  /devtools/client/fronts seems pretty natural to
> me, since the fronts feel like a client-side concern.  It's possible it
> would complicate testing of server-only build configurations that wanted to
> use fronts in tests, but we could make changes to support that case if we
> run into it.

Oh, I see the confusion now. I meant to suggest that the *specs* be kept in a shared directory, not the *fronts*. Fronts are clearly a client-side concern. Sorry about that.

Comment 20

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

(In reply to Jim Blandy :jimb from comment #19)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> > (In reply to Jim Blandy :jimb from comment #8)
> > > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > > > As far as file organization, do we want to keep the fronts in
> > > > /devtools/server/actors?  Would it make more sense for them to be in
> > > > /devtools/client/fronts or something?  Ideally it would eventually become
> > > > possible to use the client without any code from the /devtools/server
> > > > directory.
> > > 
> > > Why not devtools/shared/protocol?
> > 
> > :jimb, what's your reasoning for that over something like
> > /devtools/client/fronts?  /devtools/client/fronts seems pretty natural to
> > me, since the fronts feel like a client-side concern.  It's possible it
> > would complicate testing of server-only build configurations that wanted to
> > use fronts in tests, but we could make changes to support that case if we
> > run into it.
> 
> Oh, I see the confusion now. I meant to suggest that the *specs* be kept in
> a shared directory, not the *fronts*. Fronts are clearly a client-side
> concern. Sorry about that.

Okay, great, that's what :ejpbruel's plan does indeed do: the specs go to /devtools/shared/specs.

Comment 21

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

Comment on attachment 8743745 [details] [diff] [review]
Decouple NodeListFront from NodeListActor.

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

::: devtools/server/actors/inspector.js
@@ +63,5 @@
>  const {Class} = require("sdk/core/heritage");
>  const {WalkerSearch} = require("devtools/server/actors/utils/walker-search");
>  const {PageStyleActor, getFontPreviewData} = require("devtools/server/actors/styles");
> +const {nodeSpec, nodeListSpec} = require("devtools/shared/specs/inspector");
> +const {NodeFront, NodeListFront} = require("devtools/client/fronts/inspector");

Why does the actor require the front?  We should avoid that since the fronts aren't available everywhere, and it seems unused.

Comment 22

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

Comment on attachment 8743746 [details] [diff] [review]
Decouple WalkerFront from WalkerActor.

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

::: devtools/server/actors/inspector.js
@@ +63,5 @@
>  const {Class} = require("sdk/core/heritage");
>  const {WalkerSearch} = require("devtools/server/actors/utils/walker-search");
>  const {PageStyleActor, getFontPreviewData} = require("devtools/server/actors/styles");
> +const {nodeSpec, nodeListSpec, walkerSpec} = require("devtools/shared/specs/inspector");
> +const {NodeFront, NodeListFront, WalkerFront} = require("devtools/client/fronts/inspector");

Why does the actor require the front?  We should avoid that since the fronts aren't available everywhere, and it seems unused.

Comment 23

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

Oh, I see now...  You have left the fronts as still being exported by the server modules, instead of updating all users of the front to require the new front module.  It seems incomplete without that.  Did you intend to do that work in separate patches?

Comment 24

[Eddy Bruel [:ejpbruel]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23)
> Oh, I see now...  You have left the fronts as still being exported by the
> server modules, instead of updating all users of the front to require the
> new front module.  It seems incomplete without that.  Did you intend to do
> that work in separate patches?

Yeah, the plan was to do that final separation in the last patch.

Comment 25

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

Comment on attachment 8743750 [details] [diff] [review]
Decouple HighlighterFront from HighlighterActor

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

::: devtools/shared/specs/highlighters.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const protocol = require("devtools/server/protocol");
> +const { Arg, Option, RetVal, types } = protocol;

Nit: Only Arg and Option appear to be used

Comment 26

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

Comment on attachment 8743751 [details] [diff] [review]
Decouple CustomHighlighterFront from HighlighterFront.

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

::: devtools/client/fronts/highlighters.js
@@ +11,5 @@
>  });
>  
>  exports.HighlighterFront = HighlighterFront;
> +
> +const CustomHighlighterFront = protocol.FrontClassWithSpec(customHighlighterSpec, {});

Is it possible to drop the second arg if it's empty?

::: devtools/shared/specs/highlighters.js
@@ +31,5 @@
> +const customHighlighterSpec = protocol.actorSpec({
> +  typeName: "customhighlighter",
> +
> +  methods: {
> +    release: {

`release` still has the method wrapper in the actor.

Comment 27

[Jim Blandy :jimb]

Continuing the directory discussion:

Nick has pointed out that, in some cases, the tests for the actors use fronts that are extensions of those generated from the specs: they call the protocol.js thing to generate a front class, but then add or override methods from that. The server tests then use these customized fronts. Such fronts probably need to live in a shared location. If the dependencies are clean, placing them in separate modules is probably the best way to encourage people to keep them that way.

Comment 28

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8743750 [details] [diff] [review]
Decouple HighlighterFront from HighlighterActor

I've moved the patches to decouple the highlighter actors into their own bug (see bug 1265722).

Comment 29

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8743751 [details] [diff] [review]
Decouple CustomHighlighterFront from HighlighterFront.

Ditto for this patch.

Comment 30

[Eddy Bruel [:ejpbruel]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23)
> Oh, I see now...  You have left the fronts as still being exported by the
> server modules, instead of updating all users of the front to require the
> new front module.  It seems incomplete without that.  Did you intend to do
> that work in separate patches?

Just to clarify: the main reason I want to do this in a final patch is because the inspector front is still defined on the server. I cannot decouple that without decoupling the highlighter actors, style actors, and stylesheet actors first. Because the inspector front also needs the corresponding fronts for these actors, we still have to pull them in here.

Once the inspector front is moved to its own file, we should be able to pull in the front definitions it requires from there, so we can remove all dependencies from the server on the fronts.

Comment 31

[Eddy Bruel [:ejpbruel]]

Try push for the patch to decouple NodeFront from NodeActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c2c272f1c60

Comment 32

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple NodeFront from NodeActor.

You already reviewed these patches, but I had to manually rebase them, so I'd like you to take another pass over them, just in case. The patches didn't really change, so this should just be a rubber stamp review.

Comment 33

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple NodeListFront from NodeListActor.

Ditto for this patch.

Comment 34

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple WalkerFront from WalkerActor.

And this one.

Comment 35

[Eddy Bruel [:ejpbruel]]

Attachment: Decouple InspectorFront from InspectorActor.

This patch also removes the last references to the Fronts from the server code, as explained earlier.

Comment 36

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

Comment on attachment 8749550 [details] [diff] [review]
Decouple InspectorFront from InspectorActor.

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

Please don't add these .js extensions...  I see one has already landed in a spec and front:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/specs/stylesheets.js#10
https://dxr.mozilla.org/mozilla-central/source/devtools/client/fronts/stylesheets.js#6-11

::: devtools/client/fronts/inspector.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  const { Ci } = require("chrome");
> +require("devtools/client/fronts/styles.js");

Nit: don't use the .js extension

Comment 37

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #36)
> Comment on attachment 8749550 [details] [diff] [review]
> Decouple InspectorFront from InspectorActor.
> 
> Review of attachment 8749550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please don't add these .js extensions...  I see one has already landed in a
> spec and front:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/specs/
> stylesheets.js#10
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/fronts/
> stylesheets.js#6-11

I have a patch to clean up these two as part of bug 1270619.

Comment 38

[Eddy Bruel [:ejpbruel]]

Attachment: Move the fronts to the shared directory.

As discussed with Nick during our standup last week, the fronts are also required by the server tests, so they should be moved to the shared directory.

Comment 39

[Eddy Bruel [:ejpbruel]]

Attachment: Remove .js extensions from requires.

As discussed with jryans over irc, this patch removes the .js extensions from the requires in those files that already landed.

Comment 40

[Patrick Brosset <:pbro>]

Could you make sure to send an email to the devtools mailing list about these changes? I was a bit surprised today after I pulled by some of the code in devtools/server/actors (I was looking for the word method and couldn't find it anymore). This is a rather big change that needs to be explained to people.
Also, maybe we have some docs here and there that explain how to write actors.
https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/protocol.js.md comes to mind, but there might be other places to be updated.

Comment 41

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8749550 [details] [diff] [review]
Decouple InspectorFront from InspectorActor.

Any particular reason why this is f+ and not r+?

Comment 42

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

Comment on attachment 8750720 [details] [diff] [review]
Remove .js extensions from requires.

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

Great, thank you!

Comment 43

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

Comment on attachment 8750717 [details] [diff] [review]
Move the fronts to the shared directory.

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

This patch does not have any moved files, did you already move them in bulk somewhere else?

Comment 44

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

(In reply to Patrick Brosset <:pbro> from comment #40)
> Also, maybe we have some docs here and there that explain how to write
> actors.
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/protocol.
> js.md comes to mind, but there might be other places to be updated.

We may also want to move that docs file to /devtools/shared or /devtools/docs, since the protocol.js module is now itself in /devtools/shared.

Comment 45

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

Comment on attachment 8749550 [details] [diff] [review]
Decouple InspectorFront from InspectorActor.

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

I set f+ to be sure the .js module IDs _inside this patch_ would get updated.

Comment 46

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

Comment on attachment 8749550 [details] [diff] [review]
Decouple InspectorFront from InspectorActor.

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

On IRC, you said you'll fix the module IDs before landing.

Comment 47

[Eddy Bruel [:ejpbruel]]

Try push for the patch to decouple NodeFront from NodeActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83385913457f

Comment 48

[Eddy Bruel [:ejpbruel]]

Attachment: Move the fronts to the shared directory.

Due to general incompetence, I ended up attaching the wrong patch to this review.

Comment 50

[Eddy Bruel [:ejpbruel]]

Try push for the patch to decouple NodeFront from NodeActor failed due to an unresolved merge conflict. Here's a new try push with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3aa7616dece

Comment 51

[Eddy Bruel [:ejpbruel]]

Try push for the patch to decouple NodeListFront from NodeListActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba23c1f942c

Comment 52

[Eddy Bruel [:ejpbruel]]

More rebase issues. Here's another try push for the NodeActor patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=574360456c5e

Comment 53

[Eddy Bruel [:ejpbruel]]

And also for the NodeListActor patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fb9e032d1ea

Comment 54

[Eddy Bruel [:ejpbruel]]

Only a few more test failures left. This try push for the NodeActor patch will hopefully be green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07a60c830f69

Comment 55

[Eddy Bruel [:ejpbruel]]

Rebased try push for the NodeActorList patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b74313c6b2c3

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3b86a9c8c852

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/26656bbb6d39

Comment 58

[Eddy Bruel [:ejpbruel]]

Try push for the patch to decouple WalkerFront from WalkerActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=406919e25545

Comment 59

[Eddy Bruel [:ejpbruel]]

Try push for the patch to decouple InspectorFront from InspectorActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2780204be1d

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e9533997fcef

Comment 61

[Eddy Bruel [:ejpbruel]]

Try push for the patch to decouple InspectorFront from InspectorActor had test failures due to some paths not having been updated. Here's a new try push with those issues addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45628a95b6d5

Comment 62

[Eddy Bruel [:ejpbruel]]

Try push for the patch to move the fronts to the shared directory:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1392b88c3afe

Comment 63

[Eddy Bruel [:ejpbruel]]

More test failures for the InspectorActor patch. Here's yet another try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a96bb1f127e

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b86a9c8c852
https://hg.mozilla.org/mozilla-central/rev/26656bbb6d39
https://hg.mozilla.org/mozilla-central/rev/e9533997fcef

Comment 65

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/92b07e6e84bf

Comment 66

[Eddy Bruel [:ejpbruel]]

New try push for the patch to move the fronts to the shared directory:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffaec5a574ae

Comment 67

[Eddy Bruel [:ejpbruel]]

Try push for the patch to move the fronts to the shared directory had test failures due to a missing path update. Here is a new try push with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0750c598f9

Comment 68

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing test_animation_actor-lifetime.html because of undefined function InspectorFront.

Backout: https://hg.mozilla.org/integration/fx-team/rev/d1a574f8a643
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=92b07e6e84bf828040ac5231eafea31c843e0472
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=9374531&repo=fx-team
10:27:40     INFO -  790 INFO TEST-START | devtools/server/tests/mochitest/test_animation_actor-lifetime.html
10:33:08     INFO -  TypeError: InspectorFront is not a function
10:33:08     INFO -  TEST-INFO | started process screentopng
10:33:09     INFO -  TEST-INFO | screentopng: exit 0
10:33:09     INFO -  791 INFO Setting up inspector and animation actors.
10:33:09     INFO -  792 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/mochitest/test_animation_actor-lifetime.html | Test timed out.
10:33:09     INFO -  reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:114:7

Comment 69

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c11343f54755

Comment 70

[Eddy Bruel [:ejpbruel]]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #68)
> Backed out for failing test_animation_actor-lifetime.html because of
> undefined function InspectorFront.
> 
> Backout: https://hg.mozilla.org/integration/fx-team/rev/d1a574f8a643
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=fx-
> team&revision=92b07e6e84bf828040ac5231eafea31c843e0472
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=9374531&repo=fx-team
> 10:27:40     INFO -  790 INFO TEST-START |
> devtools/server/tests/mochitest/test_animation_actor-lifetime.html
> 10:33:08     INFO -  TypeError: InspectorFront is not a function
> 10:33:08     INFO -  TEST-INFO | started process screentopng
> 10:33:09     INFO -  TEST-INFO | screentopng: exit 0
> 10:33:09     INFO -  791 INFO Setting up inspector and animation actors.
> 10:33:09     INFO -  792 INFO TEST-UNEXPECTED-FAIL |
> devtools/server/tests/mochitest/test_animation_actor-lifetime.html | Test
> timed out.
> 10:33:09     INFO - 
> reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:114:7

As it turns out the try push I made did not include the mochitests for the server. I made sure those tests passed locally, and relanded the patch.

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c480fcee4fc0

Comment 72

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a8518344e759

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c11343f54755
https://hg.mozilla.org/mozilla-central/rev/c480fcee4fc0

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8518344e759