Closed
Bug 1265722
Opened 9 years ago
Closed 9 years ago
Decouple fronts from actors in inspector.
Categories
(DevTools :: Inspector, enhancement, P1)
DevTools
Inspector
Tracking
(firefox49 fixed)
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
Details
Attachments
(6 files, 10 obsolete files)
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•9 years ago
|
Severity: normal → enhancement
Whiteboard: [devtools-html]
Assignee | ||
Comment 1•9 years ago
|
||
Assignee: nobody → ejpbruel
Attachment #8743375 -
Flags: review?(jryans)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8743382 -
Flags: review?(jryans)
Assignee | ||
Comment 4•9 years ago
|
||
This patch is a big one, but the changes are almost entirely mechanical.
Attachment #8743383 -
Flags: review?(jryans)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8743382 -
Attachment is obsolete: true
Attachment #8743382 -
Flags: review?(jryans)
Attachment #8743745 -
Flags: review?(jryans)
Assignee | ||
Updated•9 years ago
|
Attachment #8743745 -
Attachment is patch: true
Attachment #8743745 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8743383 -
Attachment is obsolete: true
Attachment #8743383 -
Flags: review?(jryans)
Attachment #8743746 -
Flags: review?(jryans)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8743750 -
Flags: review?(jryans)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8743751 -
Flags: review?(jryans)
Assignee | ||
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
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)
Attachment #8743744 -
Flags: review?(jryans) → review+
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•9 years ago
|
||
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?
Attachment #8743745 -
Flags: review?(jryans) → review+
Comment 19•9 years ago
|
||
(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)
Flags: needinfo?(jimb)
Assignee | ||
Comment 24•9 years ago
|
||
(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+
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Comment 27•9 years ago
|
||
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.
Assignee | ||
Comment 28•9 years ago
|
||
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
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8743751 [details] [diff] [review]
Decouple CustomHighlighterFront from HighlighterFront.
Ditto for this patch.
Attachment #8743751 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 30•9 years ago
|
||
(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.
Assignee | ||
Comment 31•9 years ago
|
||
Try push for the patch to decouple NodeFront from NodeActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c2c272f1c60
Assignee | ||
Comment 32•9 years ago
|
||
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)
Assignee | ||
Comment 33•9 years ago
|
||
Ditto for this patch.
Attachment #8743745 -
Attachment is obsolete: true
Attachment #8747737 -
Flags: review?(jryans)
Assignee | ||
Comment 34•9 years ago
|
||
And this one.
Attachment #8743746 -
Attachment is obsolete: true
Attachment #8747738 -
Flags: review?(jryans)
Attachment #8747735 -
Flags: review?(jryans) → review+
Attachment #8747737 -
Flags: review?(jryans) → review+
Attachment #8747738 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 35•9 years ago
|
||
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.
Assignee | ||
Comment 38•9 years ago
|
||
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)
Assignee | ||
Comment 39•9 years ago
|
||
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)
Comment 40•9 years ago
|
||
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.
Assignee | ||
Comment 41•9 years ago
|
||
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+
Updated•9 years ago
|
Iteration: 49.1 - May 9 → 49.2 - May 23
Assignee | ||
Comment 47•9 years ago
|
||
Try push for the patch to decouple NodeFront from NodeActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83385913457f
Assignee | ||
Comment 48•9 years ago
|
||
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
Attachment #8751260 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 50•9 years ago
|
||
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
Assignee | ||
Comment 51•9 years ago
|
||
Try push for the patch to decouple NodeListFront from NodeListActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba23c1f942c
Assignee | ||
Comment 52•9 years ago
|
||
More rebase issues. Here's another try push for the NodeActor patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=574360456c5e
Assignee | ||
Comment 53•9 years ago
|
||
And also for the NodeListActor patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fb9e032d1ea
Assignee | ||
Comment 54•9 years ago
|
||
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
Assignee | ||
Comment 55•9 years ago
|
||
Rebased try push for the NodeActorList patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b74313c6b2c3
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 58•9 years ago
|
||
Try push for the patch to decouple WalkerFront from WalkerActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=406919e25545
Assignee | ||
Comment 59•9 years ago
|
||
Try push for the patch to decouple InspectorFront from InspectorActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2780204be1d
Assignee | ||
Comment 61•9 years ago
|
||
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
Assignee | ||
Comment 62•9 years ago
|
||
Try push for the patch to move the fronts to the shared directory:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1392b88c3afe
Assignee | ||
Comment 63•9 years ago
|
||
More test failures for the InspectorActor patch. Here's yet another try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a96bb1f127e
Comment 64•9 years ago
|
||
bugherder |
Assignee | ||
Comment 66•9 years ago
|
||
New try push for the patch to move the fronts to the shared directory:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffaec5a574ae
Assignee | ||
Comment 67•9 years ago
|
||
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•9 years ago
|
||
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)
Assignee | ||
Comment 70•9 years ago
|
||
(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)
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 73•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c11343f54755
https://hg.mozilla.org/mozilla-central/rev/c480fcee4fc0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 74•9 years ago
|
||
bugherder |
Updated•8 years ago
|
Whiteboard: [devtools-html]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•