Closed
Bug 1265720
Opened 8 years ago
Closed 8 years ago
Decouple fronts from actors in gcli.
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, enhancement, P1)
DevTools Graveyard
Graphic Commandline and Toolbar
Tracking
(firefox50 fixed)
RESOLVED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
Details
Attachments
(1 file)
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•8 years ago
|
Severity: normal → enhancement
Whiteboard: [devtools-html]
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ejpbruel
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8753341 -
Flags: review?(jryans)
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
Comment 2•8 years ago
|
||
Comment on attachment 8753341 [details] [diff] [review] Bug 1265720 - Decouple fronts from actors in gcli. Review of attachment 8753341 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/commandline/test/helpers.js @@ +466,5 @@ > }); > > // Send a message to add the commands to the content process > const front = yield GcliFront.create(options.target); > + yield front._testOnlyAddItemsByModule(MOCK_COMMANDS_URI); What's the thinking in s/_a/A/ here (and s/_r/R/ below) is the '_' a problem, or is it just that it looks strange? The point was that it would look strange, because I didn't want people thinking they could use this in non-test code. That said, I'm not sure that it's important, other than a potential backwards compat thing. ::: devtools/shared/fronts/gcli.js @@ +15,5 @@ > + this.actorID = tabForm.gcliActor; > + > + // XXX: This is the first actor type in its hierarchy to use the protocol > + // library, so we're going to self-own on the client side for now. > + this.manage(this); I have a feeling that we might not need to do this any more, is that right?
Comment on attachment 8753341 [details] [diff] [review] Bug 1265720 - Decouple fronts from actors in gcli. Review of attachment 8753341 [details] [diff] [review]: ----------------------------------------------------------------- This seem okay overall, but I'd like to see it again without the method name changes to be sure. (Or alternatively let me know why you changed them!) :) ::: devtools/client/commandline/test/helpers.js @@ +466,5 @@ > }); > > // Send a message to add the commands to the content process > const front = yield GcliFront.create(options.target); > + yield front._testOnlyAddItemsByModule(MOCK_COMMANDS_URI); I think I agree with :jwalker that we should keep these decoupling patches to just what's necessary to achieve the goal. There's no reason to change RDP method names in this process. It's true it's _probably_ okay to change these specific ones, but let's leave as-is unless there's a real reason to change them. ::: devtools/shared/fronts/gcli.js @@ +15,5 @@ > + this.actorID = tabForm.gcliActor; > + > + // XXX: This is the first actor type in its hierarchy to use the protocol > + // library, so we're going to self-own on the client side for now. > + this.manage(this); AFAIK, this is still required for all fronts that are first in a tree of fronts to use protocol.js (all the "major" actors), just like this existing comment states. I haven't tested recently to see if it has changed though... https://dxr.mozilla.org/mozilla-central/search?q=this.manage(this)&redirect=false&case=true
Attachment #8753341 -
Flags: review?(jryans) → feedback+
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3) > Comment on attachment 8753341 [details] [diff] [review] > Bug 1265720 - Decouple fronts from actors in gcli. > > Review of attachment 8753341 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seem okay overall, but I'd like to see it again without the method name > changes to be sure. (Or alternatively let me know why you changed them!) :) > > ::: devtools/client/commandline/test/helpers.js > @@ +466,5 @@ > > }); > > > > // Send a message to add the commands to the content process > > const front = yield GcliFront.create(options.target); > > + yield front._testOnlyAddItemsByModule(MOCK_COMMANDS_URI); > > I think I agree with :jwalker that we should keep these decoupling patches > to just what's necessary to achieve the goal. > > There's no reason to change RDP method names in this process. It's true > it's _probably_ okay to change these specific ones, but let's leave as-is > unless there's a real reason to change them. > > ::: devtools/shared/fronts/gcli.js > @@ +15,5 @@ > > + this.actorID = tabForm.gcliActor; > > + > > + // XXX: This is the first actor type in its hierarchy to use the protocol > > + // library, so we're going to self-own on the client side for now. > > + this.manage(this); > > AFAIK, this is still required for all fronts that are first in a tree of > fronts to use protocol.js (all the "major" actors), just like this existing > comment states. I haven't tested recently to see if it has changed though... > > https://dxr.mozilla.org/mozilla-central/search?q=this. > manage(this)&redirect=false&case=true The reason I changed the method names is because with earlier patches, I got eslint errors when moving methods that don't use camelCase into their own file. Apparently, eslint requires us to always use camelCase for method names. Is that correct? In any case, I wanted to avoid these errors here. I didn't actually check whether they occurred for this patch as well, but I assumed they would. Given the above, let me know what you'd like me to do here.
Flags: needinfo?(jryans)
(In reply to Eddy Bruel [:ejpbruel] from comment #4) > The reason I changed the method names is because with earlier patches, I got > eslint errors when moving methods that don't use camelCase into their own > file. Apparently, eslint requires us to always use camelCase for method > names. Is that correct? Yes, that's how we have configured it currently[1]. Does that mean you modified RDP method names in other patches? I can't tell from your comment you mean already made similar changes in the past. If they are not test-only methods, this could be bad since it would break compatibility. > In any case, I wanted to avoid these errors here. I didn't actually check > whether they occurred for this patch as well, but I assumed they would. It would probably be good to start by checking if they do indeed actually show up as errors. If they do, then for *this specific case* it seems okay because these are test only methods, and we don't currently run tests across different RDP versions. In the general case though (for production RDP methods), we should generally not make a change to the method name. To appease ESLint while keeping the current names, you can use ESLint comments[2] to disable that rule temporarily for the line or block involved. [1]: https://dxr.mozilla.org/mozilla-central/rev/46fe2115d46a5bb40523b8466341d8f9a26e1bdf/devtools/.eslintrc#82-83 [2]: http://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) > (In reply to Eddy Bruel [:ejpbruel] from comment #4) > > In any case, I wanted to avoid these errors here. I didn't actually check > > whether they occurred for this patch as well, but I assumed they would. > > It would probably be good to start by checking if they do indeed actually > show up as errors. If they do, then for *this specific case* it seems okay > because these are test only methods, and we don't currently run tests across > different RDP versions. To clarify, I mean that "it seems okay" to change the names for this specific case if there are in fact ESLint errors.
Updated•8 years ago
|
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Updated•8 years ago
|
Iteration: 49.3 - Jun 6 → 50.1
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) > > (In reply to Eddy Bruel [:ejpbruel] from comment #4) > > > In any case, I wanted to avoid these errors here. I didn't actually check > > > whether they occurred for this patch as well, but I assumed they would. > > > > It would probably be good to start by checking if they do indeed actually > > show up as errors. If they do, then for *this specific case* it seems okay > > because these are test only methods, and we don't currently run tests across > > different RDP versions. > > To clarify, I mean that "it seems okay" to change the names for this > specific case if there are in fact ESLint errors. There are in fact ESLint errors. When I change the names of those methods back to their original non-camel-cased names, I get the following error: 18:5 error Identifier '_testOnly_addItemsByModule' is not in camel case camelcase 23:5 error Identifier '_testOnly_removeItemsByModule' is not in camel case camelcase Given that that's case, could you please rereview?
Assignee | ||
Updated•8 years ago
|
Attachment #8753341 -
Flags: review?(jryans)
Comment on attachment 8753341 [details] [diff] [review] Bug 1265720 - Decouple fronts from actors in gcli. Review of attachment 8753341 [details] [diff] [review]: ----------------------------------------------------------------- Okay, works for me then. ::: devtools/client/commandline/test/helpers.js @@ +466,5 @@ > }); > > // Send a message to add the commands to the content process > const front = yield GcliFront.create(options.target); > + yield front._testOnlyAddItemsByModule(MOCK_COMMANDS_URI); I think I agree with :jwalker that we should keep these decoupling patches to just what's necessary to achieve the goal. There's no reason to change RDP method names in this process. It's true it's _probably_ okay to change these specific ones, but let's leave as-is unless there's a real reason to change them. ::: devtools/shared/fronts/gcli.js @@ +15,5 @@ > + this.actorID = tabForm.gcliActor; > + > + // XXX: This is the first actor type in its hierarchy to use the protocol > + // library, so we're going to self-own on the client side for now. > + this.manage(this); AFAIK, this is still required for all fronts that are first in a tree of fronts to use protocol.js (all the "major" actors), just like this existing comment states. I haven't tested recently to see if it has changed though... https://dxr.mozilla.org/mozilla-central/search?q=this.manage(this)&redirect=false&case=true
Attachment #8753341 -
Flags: review?(jryans)
Attachment #8753341 -
Flags: review+
Attachment #8753341 -
Flags: feedback+
Assignee | ||
Comment 9•8 years ago
|
||
Try push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=681dc44b6d74
Comment 10•8 years ago
|
||
Pushed by ejpbruel@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/f1c88261bd73 Decouple fronts from actors in gcli;r=jryans
Followup to fix eslint so you don't have to get backed out over two missing whitespaces :) https://hg.mozilla.org/integration/fx-team/rev/4d03f2c0d717
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #11) > Followup to fix eslint so you don't have to get backed out over two missing > whitespaces :) > https://hg.mozilla.org/integration/fx-team/rev/4d03f2c0d717 Great! Thank you Wes!
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1c88261bd73 https://hg.mozilla.org/mozilla-central/rev/4d03f2c0d717
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
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•