Closed
Bug 1273183
Opened 9 years ago
Closed 9 years ago
Install a temporary add-on via remote debugging
Categories
(DevTools :: Shared Components, defect)
DevTools
Shared Components
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: kumar, Assigned: kumar)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54304/
Attachment #8754903 -
Flags: review?(jryans)
Assignee | ||
Comment 2•9 years ago
|
||
Sorry for the trigger finger, hold up on this. I need to add a test for invalid file paths.
Assignee | ||
Updated•9 years ago
|
Attachment #8754903 -
Flags: review?(jryans)
Assignee | ||
Comment 3•9 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/
Attachment #8754903 -
Flags: review?(jryans)
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
Attachment #8754903 -
Flags: review?(jryans)
Assignee | ||
Comment 5•9 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/
Attachment #8754903 -
Attachment description: MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r?jryans → MozReview Request: Bug 1273183 - Install a temporary add-on via remote debugging. r=jryans
Attachment #8754903 -
Flags: review?(jryans)
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
Attachment #8754903 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 7•9 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/
Assignee | ||
Comment 8•9 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/
Assignee | ||
Comment 9•9 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).
Assignee | ||
Comment 10•9 years ago
|
||
fixed up some legit Windows bustage
Assignee | ||
Comment 11•9 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/
Assignee | ||
Comment 12•9 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/
Blocks: 1243460
Assignee | ||
Comment 13•9 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
Keywords: checkin-needed
Comment 14•9 years ago
|
||
Keywords: checkin-needed
Comment 15•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•