Closed
Bug 1265728
Opened 9 years ago
Closed 8 years ago
Decouple fronts from actors in promise debugger.
Categories
(DevTools :: Debugger, enhancement, P1)
DevTools
Debugger
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
Details
Attachments
(1 file)
19.27 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•9 years ago
|
Severity: normal → enhancement
Whiteboard: [devtools-html]
Updated•9 years ago
|
Flags: qe-verify-
Priority: -- → P2
Updated•9 years ago
|
Assignee: nobody → nfitzgerald
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Priority: P2 → P1
Comment 1•9 years ago
|
||
Depends on bug 1265722 for the introduction of the fronts and specs directories and moz.builds, etc.
Depends on: 1265722
Comment 2•9 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.
Attachment #8749857 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 3•9 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?
Attachment #8749857 -
Flags: review?(ejpbruel) → review+
Comment 4•9 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).
Updated•9 years ago
|
Iteration: 49.1 - May 9 → 49.2 - May 23
Assignee | ||
Comment 5•9 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.
Comment 6•9 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.)
Comment 7•9 years ago
|
||
Removing from release - blocked on dependency.
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
Iteration: 49.2 - May 23 → ---
Priority: P1 → P2
Assignee | ||
Comment 8•9 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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ejpbruel
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Priority: P2 → P1
Assignee | ||
Comment 9•8 years ago
|
||
Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dcc60fd8c58
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
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
Updated•8 years ago
|
Iteration: 49.3 - Jun 6 → 50.1
Comment 12•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
•