1265720 - Decouple fronts from actors in gcli.

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, enhancement, P1)

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Bug 1265720 - Decouple fronts from actors in gcli.

Comment 2

[Joe Walker [:jwalker]]

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 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 4

[Eddy Bruel [:ejpbruel]]

(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.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 7

[Eddy Bruel [:ejpbruel]]

(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?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 9

[Eddy Bruel [:ejpbruel]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f1c88261bd73
Decouple fronts from actors in gcli;r=jryans

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Eddy Bruel [:ejpbruel]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1c88261bd73
https://hg.mozilla.org/mozilla-central/rev/4d03f2c0d717