Allow actors to be loaded as addon-sdk modules

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

unspecified
Firefox 24
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 755527 [details] [diff] [review]
v1, no tests

In advance of the whole debugger server loading with the jetpack loader, it should be easy to allow specific modules to be loaded using the loader, which will let us land inspector work separately from the push to the jetpack loader on the debugger server.
Comment on attachment 755527 [details] [diff] [review]
v1, no tests

Review of attachment 755527 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/main.js
@@ +193,5 @@
> +      return;
> +    }
> +    let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +    let mod = devtools.require(id);
> +    mod.register();

I understand we can't do

  this.createRootActor = devtools.require(id).createRootActor;

because some modules just define actors but not a root actor, but I don't get why we are relying on implicit state changes by just calling

  mod.register()

Couldn't we make this more explicit by having like a shared namespace for actors and doing something like

  mod.register(actors)

and let the module define it's actors onto the shared namespace?

I don't know, maybe I am being nitpick-y but something about this implicit-ness is really rubbing me the wrong way.
(Assignee)

Comment 2

5 years ago
Depends on bug 877413 for testing so that we can register resource://test/ urls with our loader.
Depends on: 877413
(Assignee)

Comment 3

5 years ago
These modules have full access to the server API.  As long as we load modules we're open to side effects.  A required register() and unregister() pair at least makes it clear when we expect those side effects to happen.

There are some commonly-expected side effects, particularly registration of actors.  We could maybe do more to show that this is a commonly-expected side effect by having the explicit shared namespace.  But that won't prevent side effects, just give a false sense that there are none.
(Assignee)

Comment 4

5 years ago
But it might be nice to pass in the extension API object (DebuggerServer) rather than making each imported actor reimport the Debugger server.  Then down the road it would be easy to trim that object down if it's helpful.
(Assignee)

Comment 5

5 years ago
Created attachment 755642 [details] [diff] [review]
WIP 2

This version passes in a module-unique extension API object to each module's register/unregister functions.  The module should use this method to add and remove actors.  If they do so, modules that still exist after unregister() can be cleaned up by the server.

For now you can also manually add via DebuggerServer, but we could remove that once everything's modularized if we want.
Attachment #755527 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 755645 [details] [diff] [review]
WIP 2.1
Attachment #755642 - Attachment is obsolete: true
I'm so happy (๑╹ڡ╹)╭ ~ ♡
Comment on attachment 755645 [details] [diff] [review]
WIP 2.1

Review of attachment 755645 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/main.js
@@ +239,5 @@
> +   */
> +  unregisterModule: function DS_unregisterModule(id) {
> +    let mod = gRegisteredModules[id];
> +    if (!mod) {
> +      throw new Error("Tried to unregister a module that was not previously registered.");

If we throw when unregistering a non-existent module then maybe we should also throw when trying to register another module with the same id? If we prefer to be lenient then maybe not throw in either case?

If being strict doesn't cause problems with updating add-ons that register modules, then I vote for throwing in both cases. Or maybe abide by Postel's law and log a warning (so the dev can find out why it didn't work) instead of throwing.
(Assignee)

Comment 9

5 years ago
Created attachment 756716 [details] [diff] [review]
WIP 3

Yeah, let's throw and see if it causes problems.
Attachment #755645 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 877300
(Assignee)

Comment 10

5 years ago
Created attachment 759486 [details] [diff] [review]
v1
Attachment #756716 - Attachment is obsolete: true
Attachment #759486 - Flags: review?(jimb)
(Assignee)

Updated

5 years ago
Attachment #759486 - Flags: review?(past)
Attachment #759486 - Flags: review?(past) → review+
Created attachment 760309 [details] [diff] [review]
v1.00001

Rebase. Minor change to xpcshell.ini
Not obsoleting v1 because dcamp should see what's going on.
fwiw, the interdiff looks sane:

diff attachment.cgi\?id=759486 .hg/patches/dcamp-877295.patch
2c2
< # Parent e32eee2ed2f965fcfacaad6518c27586520a8dcb
---
> # Parent 855a29c9dd686ddeb5fdb485a24ca975589d445e
101c101
< @@ -155,16 +207,23 @@ var DebuggerServer = {
---
> @@ -154,16 +206,23 @@ var DebuggerServer = {
125c125
< @@ -177,16 +236,55 @@ var DebuggerServer = {
---
> @@ -176,16 +235,55 @@ var DebuggerServer = {
270c270,273
< @@ -35,16 +35,17 @@ reason = bug 821285
---
> @@ -32,16 +32,17 @@ reason = bug 821285
>  [test_frameclient-02.js]
>  [test_nativewrappers.js]
>  [test_eval-01.js]
275,277d277
<  [test_protocol_simple.js]
<  [test_protocol_longstring.js]
<  [test_protocol_children.js]
Scratch that - the problem isn't bitrot, it's patch application order.
Adding dependency on bug 866305, which is only a 'to cleanly apply xpcshell.ini' dependency.

Patch queues are 1D where dependency trees are 2D.
Depends on: 866305
Depends on: 866306
No longer depends on: 866305
Attachment #760309 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/21075f02a219
https://hg.mozilla.org/integration/fx-team/rev/5c76c23df424
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/5c76c23df424 was from a different bug, will be backing that out and relanding with the right commit message.
(Assignee)

Comment 16

5 years ago
Backed out the bad commit message in https://hg.mozilla.org/integration/fx-team/rev/deff9b455b24
https://hg.mozilla.org/mozilla-central/rev/21075f02a219
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
(Assignee)

Updated

5 years ago
No longer depends on: 877413

Comment 18

4 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> I'm so happy (๑╹ڡ╹)╭ ~ ♡

Yeah, I like the new system too.

Comment 19

4 years ago
Comment on attachment 759486 [details] [diff] [review]
v1

Review of attachment 759486 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/main.js
@@ +61,5 @@
> + * destroyed.
> + */
> +function ModuleAPI() {
> +  let activeTabActors = new Set();
> +  let activeGlobalActors = new Set();

This is great; I like the idea of having an object whose scope is exactly the lifetime of these extensions.

But: will the same actor factory function ever get passed to a ModuleAPI's addTabActor or addGlobalActor method twice? Then the first 'destroy' will remove it. I guess the checks for duplicate module ids should cover this.

It might be nice to let ModuleAPI instances *own* the factories, and just have DebuggerServer maintain a set of ModuleAPI instances. But that would require a bunch of work on CommonCreateExtraActors etc; probably a pain.

Separately: I don't really care, but aside from avoiding 'this.' everywhere, is there a reason not to make this a normal constructor, with methods on its prototypes instead of its instances? It's what people expect to see; less importantly, it's fewer objects allocated (one instance pointing to a shared prototype, versus one new closure per method per instance).
Attachment #759486 - Flags: review?(jimb) → review+
You need to log in before you can comment on or make changes to this bug.