Eddy Bruel [:ejpbruel] Assignee
	Description • 8 years ago

      No description provided.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 1 • 8 years ago

Depends on bug 1265722 for the introduction of the fronts and specs directories and moz.builds, etc.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 2 • 8 years ago

This passes all promises actor related tests locally, but will need to be
rebased on top of moving fronts into devtools/shared because of requires from
devtools/server/actors to devtools/client/fronts.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 3 • 8 years ago

Comment on attachment 8749857 [details] [diff] [review]
Decouple PromisesFront from PromisesActor

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

LGTM

::: devtools/client/debugger/test/mochitest/browser_dbg_promises-allocation-stack.js
@@ +9,5 @@
>  
>  "use strict";
>  
>  const TAB_URL = EXAMPLE_URL + "doc_promise-get-allocation-stack.html";
> +const PromisesFront = require("devtools/client/fronts/promises");

Assuming you will either update this to /devtools/shared, or will move everything there in a future patch, as per your comments during the standup last friday.

::: devtools/shared/specs/object-actor.js
@@ +5,5 @@
> +
> +const { types } = require("devtools/server/protocol");
> +
> +// Teach protocol.js how to deal with legacy actor types
> +types.addType("ObjectActor", {

Why not just put this /devtools/shared/specs/promises.js?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 4 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #3)
> ::: devtools/shared/specs/object-actor.js
> @@ +5,5 @@
> > +
> > +const { types } = require("devtools/server/protocol");
> > +
> > +// Teach protocol.js how to deal with legacy actor types
> > +types.addType("ObjectActor", {
> 
> Why not just put this /devtools/shared/specs/promises.js?

Because if anything else ends up using the object actor too, then they can just require this module rather than require the promise spec (which wouldn't make sense).

	Eddy Bruel [:ejpbruel] Assignee
	Comment 5 • 8 years ago

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #4)
> (In reply to Eddy Bruel [:ejpbruel] from comment #3)
> > ::: devtools/shared/specs/object-actor.js
> > @@ +5,5 @@
> > > +
> > > +const { types } = require("devtools/server/protocol");
> > > +
> > > +// Teach protocol.js how to deal with legacy actor types
> > > +types.addType("ObjectActor", {
> > 
> > Why not just put this /devtools/shared/specs/promises.js?
> 
> Because if anything else ends up using the object actor too, then they can
> just require this module rather than require the promise spec (which
> wouldn't make sense).

True, but that's not the case right now. We can always do it when required. As it stands, moving this to a separate file even though it's only ever required here only serves to make things harder to read.

That's just my opinion, of course. I hate fighting over issues that are unimportant in the bigger picture, so I'll leave the final decision up to your judgement.

	Jim Blandy :jimb
	Comment 6 • 8 years ago

Well, it's not "fighting" -- it's "coming to a shared understanding". :D

I think Nick's right in this case. The ObjectActor type is something that's logically independent of the promises code. While splitting code across lots of files can be a pain, making it clear that two things are actually not at all entangled can also be helpful to readers. (I find it so, anyway.)

	Marco Mucci [:MarcoM]
	Comment 7 • 8 years ago

Removing from release - blocked on dependency.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 8 • 8 years ago

(In reply to Jim Blandy :jimb from comment #6)
> Well, it's not "fighting" -- it's "coming to a shared understanding". :D
> 
> I think Nick's right in this case. The ObjectActor type is something that's
> logically independent of the promises code. While splitting code across lots
> of files can be a pain, making it clear that two things are actually not at
> all entangled can also be helpful to readers. (I find it so, anyway.)

Well, although I agree with that argument, when taken to its logical extreme, you can end up with hundreds of files containing only 10 lines of code, which actually hampers understanding more than it helps, in my experience. The same is true for the opposite extreme, where you put everything in a single file.

Personally, I would prefer it if there were some threshold where we don't move code into a separate file unless:

1. The code is required by more than one file (not the case here)
2. The code is logically distinct, AND takes up more than n lines.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 9 • 8 years ago

Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dcc60fd8c58

	Eddy Bruel [:ejpbruel] Assignee
	Comment 10 • 8 years ago

Try push failed due to TBPL failures. New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1555f2b6f701

	Pulsebot
	Comment 11 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/d6e0d9a0319f
Decouple PromisesFront from PromisesActor; r=ejpbruel

	Carsten Book [:Tomcat]
	Comment 12 • 8 years ago
bugherder

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