Closed Bug 1265722 Opened 4 years ago Closed 4 years ago

Decouple fronts from actors in inspector.

Categories

(DevTools :: Inspector, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.2 - May 23
Tracking Status
firefox49 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(6 files, 10 obsolete files)

31.11 KB, patch
jryans
: review+
Details | Diff | Splinter Review
8.74 KB, patch
jryans
: review+
Details | Diff | Splinter Review
74.29 KB, patch
jryans
: review+
Details | Diff | Splinter Review
21.27 KB, patch
jryans
: review+
Details | Diff | Splinter Review
1.41 KB, patch
jryans
: review+
Details | Diff | Splinter Review
102.50 KB, patch
jryans
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 1265429
Blocks: 1263289
Severity: normal → enhancement
Whiteboard: [devtools-html]
Assignee: nobody → ejpbruel
Attachment #8743375 - Flags: review?(jryans)
Attached the wrong patch here too, like a not smart person.
Attachment #8743375 - Attachment is obsolete: true
Attachment #8743375 - Flags: review?(jryans)
Attachment #8743379 - Flags: review?(jryans)
Attachment #8743382 - Flags: review?(jryans)
This patch is a big one, but the changes are almost entirely mechanical.
Attachment #8743383 - Flags: review?(jryans)
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?
Flags: needinfo?(jryans)
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.
Attachment #8743379 - Attachment is obsolete: true
Attachment #8743379 - Flags: review?(jryans)
Attachment #8743744 - Flags: review?(jryans)
Attachment #8743382 - Attachment is obsolete: true
Attachment #8743382 - Flags: review?(jryans)
Attachment #8743745 - Flags: review?(jryans)
Attachment #8743745 - Attachment is patch: true
Attachment #8743745 - Attachment mime type: text/x-patch → text/plain
Attachment #8743383 - Attachment is obsolete: true
Attachment #8743383 - Flags: review?(jryans)
Attachment #8743746 - Flags: review?(jryans)
Attachment #8743750 - Flags: review?(jryans)
Attachment #8743751 - Flags: review?(jryans)
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.
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.
Flags: needinfo?(jryans)
(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.
(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.
Flags: needinfo?(jryans)
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.
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jimb)
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.
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?
(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.
(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 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 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.
Attachment #8743746 - Flags: review?(jryans) → review+
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?
Flags: needinfo?(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.
Flags: needinfo?(ejpbruel)
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
Attachment #8743750 - Flags: review?(jryans) → review+
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.
Attachment #8743751 - Flags: review?(jryans) → review+
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
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.
Depends on: 1268458
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).
Attachment #8743750 - Attachment is obsolete: true
Comment on attachment 8743751 [details] [diff] [review]
Decouple CustomHighlighterFront from HighlighterFront.

Ditto for this patch.
Attachment #8743751 - Attachment is obsolete: true
Blocks: 1265730
No longer blocks: 1265730
Depends on: 1265730
(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.
Try push for the patch to decouple NodeFront from NodeActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c2c272f1c60
No longer depends on: 1265730
Depends on: 1268568
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.
Attachment #8743744 - Attachment is obsolete: true
Attachment #8747735 - Flags: review?(jryans)
Ditto for this patch.
Attachment #8743745 - Attachment is obsolete: true
Attachment #8747737 - Flags: review?(jryans)
And this one.
Attachment #8743746 - Attachment is obsolete: true
Attachment #8747738 - Flags: review?(jryans)
This patch also removes the last references to the Fronts from the server code, as explained earlier.
Attachment #8749550 - Flags: review?(jryans)
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
Attachment #8749550 - Flags: review?(jryans) → feedback+
(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.
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.
Attachment #8750717 - Flags: review?(jryans)
As discussed with jryans over irc, this patch removes the .js extensions from the requires in those files that already landed.
Attachment #8750720 - Flags: review?(jryans)
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 on attachment 8749550 [details] [diff] [review]
Decouple InspectorFront from InspectorActor.

Any particular reason why this is f+ and not r+?
Attachment #8749550 - Flags: review?(jryans)
Comment on attachment 8750720 [details] [diff] [review]
Remove .js extensions from requires.

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

Great, thank you!
Attachment #8750720 - Flags: review?(jryans) → review+
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?
Attachment #8750717 - Flags: review?(jryans) → review+
(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 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.
Attachment #8749550 - Flags: review?(jryans)
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.
Attachment #8749550 - Flags: feedback+ → review+
Iteration: 49.1 - May 9 → 49.2 - May 23
Try push for the patch to decouple NodeFront from NodeActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83385913457f
Due to general incompetence, I ended up attaching the wrong patch to this review.
Attachment #8750717 - Attachment is obsolete: true
Attachment #8751260 - Flags: review?(jryans)
Attachment #8751260 - Attachment is patch: true
Attachment #8751260 - Attachment mime type: text/x-patch → text/plain
Duplicate of this bug: 1270186
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
Try push for the patch to decouple NodeListFront from NodeListActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba23c1f942c
More rebase issues. Here's another try push for the NodeActor patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=574360456c5e
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
Rebased try push for the NodeActorList patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b74313c6b2c3
Keywords: leave-open
Try push for the patch to decouple WalkerFront from WalkerActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=406919e25545
Try push for the patch to decouple InspectorFront from InspectorActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2780204be1d
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
Try push for the patch to move the fronts to the shared directory:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1392b88c3afe
More test failures for the InspectorActor patch. Here's yet another try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a96bb1f127e
New try push for the patch to move the fronts to the shared directory:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffaec5a574ae
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
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
Flags: needinfo?(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.
Flags: needinfo?(ejpbruel)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/c11343f54755
https://hg.mozilla.org/mozilla-central/rev/c480fcee4fc0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1274493
Blocks: 1277706
No longer blocks: 1263289
Blocks: 1263289
Whiteboard: [devtools-html]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.