Kumar McMillan [:kumar] Assignee
	Description • 8 years ago

We have a button called 'install temporary add-on' on about:debugging. I'd like to execute this action from a client connected to Firefox via remote debugging.

Main use case: the web-ext command line tool [1] will execute the reload() function from a remote debugger but as of bug 1269889 it will need to first install the add-on temporarily.

[1] https://github.com/mozilla/web-ext

	Kumar McMillan [:kumar] Assignee
	Comment 1 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/54304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54304/

	Kumar McMillan [:kumar] Assignee
	Comment 2 • 8 years ago

Sorry for the trigger finger, hold up on this. I need to add a test for invalid file paths.

	Kumar McMillan [:kumar] Assignee
	Comment 3 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54304/diff/1-2/

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 4 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

https://reviewboard.mozilla.org/r/54304/#review51024

It's quite close, but a few transformations left to make.  Thanks for working on this!

::: devtools/server/actors/addons.js:10
(Diff revision 2)
> +
> +const {Cc, Ci, Cu, CC} = require("chrome");
> +const {AddonManager} = require("resource://gre/modules/AddonManager.jsm");
> +const protocol = require("devtools/shared/protocol");
> +const {method, Arg, Option, RetVal} = protocol;
> +Cu.import("resource://gre/modules/FileUtils.jsm");

Single argument `Cu.import` is bad for DevTools code because it actually creates a global in all modules, instead of being contained to just this one.

You can actually use our `require` with JSMs, so something like:

const {FileUtils} = require("resource://gre/modules/FileUtils.jsm");

For Task.jsm, we just land a move to our own copy of this as part of our conversion to HTML, please use:

const {Task} = require("devtools/shared/task");

(You'll need to rebase to have this file.)

::: devtools/server/actors/addons.js:13
(Diff revision 2)
> +exports.register = handle => {
> +  handle.addGlobalActor(AddonsActor, "addonsActor");
> +};
> +
> +exports.unregister = handle => handle.removeGlobalActor(AddonsActor);

These register / unregister exports should not be needed anymore.

::: devtools/server/actors/addons.js:19
(Diff revision 2)
> +  handle.addGlobalActor(AddonsActor, "addonsActor");
> +};
> +
> +exports.unregister = handle => handle.removeGlobalActor(AddonsActor);
> +
> +const AddonsActor = protocol.ActorClass({

We happen to be in the process of breaking up protocol.js actors so that the method specification is kept separately from the actual actor code.  Since you're adding something new, it would be great to have it separated from the beginning!  Unfortunately this new style is not yet documented... :( Sorry about that.

It's a small transformation:

1. Create `devtools/shared/specs/addons.js`
2. Look at other specs there for examples, but this file would call `generateActorSpec` passing the `typeName` and `methods` with the `request` / `response` pairs
3. Remove the `method` wrappers and `typeName` from here
4. You would require your new spec and pass it to `ActorClassWithSpec`[1].

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/styles.js#51

::: devtools/server/actors/addons.js:41
(Diff revision 2)
> +    // return new BrowserAddonActor(this.conn, addon);
> +
> +    // Return a pseudo add-on object that a calling client can work
> +    // with. Provide a flag that the client can use to detect when it
> +    // gets upgraded to a real actor object.
> +    return {id: addon.id, actor: false};

Nit: use `{ id: addon.id, actor: false };` since that's the style of request / response below.

::: devtools/server/docs/protocol.js.md:65
(Diff revision 2)
>  Here's the front for the HelloActor:
>  
>      let HelloFront = protocol.FrontClass(HelloActor, {
>        initialize: function(client, form) {
>          protocol.Front.prototype.initialize.call(this, client, form);
> +        // This will guarantee that your instance is managed in the pool.

It's slightly more nuanced than *always* needing to do `this.manage(this)`.  So, here's the story:  We have the root actor, which tells you about all other actors.  But the root actor itself does not currently use protocol.js.  So, the fronts for actors that are _direct children_ of the root actor are the first to use protocol.js for their tree of actors, and so they do need `this.manage(this)` in the front.

However, if those protocol.js actors return their own child actors, then the fronts for the children don't need this line as in this example[1].

Anyway, it's definitely very confusing, so it's great to document the two cases in here!

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/fronts/styles.js#96

::: devtools/server/docs/protocol.js.md:267
(Diff revision 2)
>      });
>  
>      let ChildFront = protocol.FrontClass(ChildActor, {
>          initialize: function(client, form) {
>              protocol.Front.prototype.initialize.call(this, client, form);
> +            this.manage(this);

Since this seems to be a child example, it should not be needed here.

::: devtools/server/tests/unit/test_addons_actor.js:19
(Diff revision 2)
> +startupAddonsManager();
> +
> +const protocol = require("devtools/shared/protocol");
> +const AddonsActor = require("devtools/server/actors/addons").AddonsActor;
> +
> +const AddonsFront = protocol.FrontClass(AddonsActor, {

We typically pre-create the fronts in their own shared module.  This step would also use the new spec module I mentioned above.

You should create devtools/shared/fronts/addons.js, and use import the spec into there[1].

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/fronts/styles.js#28

	Kumar McMillan [:kumar] Assignee
	Comment 5 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54304/diff/2-3/

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 6 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

https://reviewboard.mozilla.org/r/54304/#review51322

Overall, it looks good to go, just a few small things.  Thanks for working on this!

::: devtools/server/actors/addons.js:19
(Diff revision 3)
> +
> +  initialize: function (conn) {
> +    protocol.Actor.prototype.initialize.call(this, conn);
> +  },
> +
> +  installTemporaryAddon: Task.async(function* (addonPath) {

I suppose it's obvious, but supplying a path like this only makes sense when client and server are on the same machine.

We do also have a bulk data API that could stream (for example) the entire add-on to the server.  (It was originally added for installing apps on b2g over the protocol.)  This could be interesting for Fennec add-ons for example.  We can always add something like this later if it's interesting, let me know if it is!

::: devtools/server/docs/protocol.js.md:9
(Diff revision 3)
>  A Simple Hello World
>  --------------------
>  
>  Here's a simple Hello World actor.  It is a global actor (not associated with a given browser tab).
> +It has two parts: a spec and an implementation. The spec would go somewhere like
> +`devtools/shared/specs/hell-world.js` and would look like:

Nit: hello, though it may feel like hell at times... :)

::: devtools/server/docs/protocol.js.md:73
(Diff revision 3)
>  
>  Now we can create a client side object.  We call these *front* objects.
>  
>  Here's the front for the HelloActor:
>  
>      let HelloFront = protocol.FrontClass(HelloActor, {

Probably good to update the front to pass in the spec as well.

::: devtools/server/docs/protocol.js.md:324
(Diff revision 3)
> -        }
>      });
>  
> -    let ChildFront = protocol.FrontClass(ChildActor, {
> +    exports.ChildActor = ChildActor;
> +
> +    const ChildFront = protocol.FrontClass(ChildActor, {

Probably good to update the front to pass in the spec as well.

::: devtools/server/docs/protocol.js.md:414
(Diff revision 3)
>      types.addActorType("childActor");
>  
>      ...
>  
> -    changeC: method(function(newC) {
> -        c = newC;
> +    // spec:
> +    methos: {

Nit: methods

::: devtools/shared/fronts/addons.js:11
(Diff revision 3)
> +const {addonsSpec} = require("devtools/shared/specs/addons");
> +const protocol = require("devtools/shared/protocol");
> +
> +const AddonsFront = protocol.FrontClassWithSpec(addonsSpec, {
> +  initialize: function(client, form) {
> +    protocol.Front.prototype.initialize.call(this, client, form);

We tend to pass the entire "target" form into fronts like this, and the front peels off the correct key[1].

So, here you might do something like:

  initialize: function(client, form) {
    protocol.Front.prototype.initialize.call(this, client);
    this.actorID = form.addonsActor;
    this.manage(this);
  }
  
and the caller would:

  new AddonsFront(client, root)
  
or something.

Not totally sure which pattern is actually more clear...  but this is one we seem to use for the moment.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/fronts/storage.js#27

	Kumar McMillan [:kumar] Assignee
	Comment 7 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54304/diff/3-4/

	Kumar McMillan [:kumar] Assignee
	Comment 8 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54304/diff/4-5/

	Kumar McMillan [:kumar] Assignee
	Comment 9 • 8 years ago

https://reviewboard.mozilla.org/r/54304/#review51322

> I suppose it's obvious, but supplying a path like this only makes sense when client and server are on the same machine.
> 
> We do also have a bulk data API that could stream (for example) the entire add-on to the server.  (It was originally added for installing apps on b2g over the protocol.)  This could be interesting for Fennec add-ons for example.  We can always add something like this later if it's interesting, let me know if it is!

Oh, huh. That definitely sounds useful for Fennec. I could try following up with a patch for this. On Fennec you could start the process by pushing the add-on source to /data on device before issuing an install (as a workaround).

	Kumar McMillan [:kumar] Assignee
	Comment 10 • 8 years ago

fixed up some legit Windows bustage

	Kumar McMillan [:kumar] Assignee
	Comment 11 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54304/diff/5-6/

	Kumar McMillan [:kumar] Assignee
	Comment 12 • 8 years ago

Comment on attachment 8754903 [details]
MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54304/diff/6-7/

	Kumar McMillan [:kumar] Assignee
	Comment 13 • 8 years ago

The Try run looks ok. I think it just has intermittent failures. https://treeherder.mozilla.org/#/jobs?repo=try&revision=220f147966fc&selectedJob=21362151

	Pulsebot
	Comment 14 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/4e09a535b6ae

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

https://hg.mozilla.org/mozilla-central/rev/4e09a535b6ae