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.

Depends on bug 877413 for testing so that we can register resource://test/ urls with our loader.

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.

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.

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.

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.

Yeah, let's throw and see if it causes problems.

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.

https://hg.mozilla.org/integration/fx-team/rev/21075f02a219 https://hg.mozilla.org/integration/fx-team/rev/5c76c23df424

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.

Backed out the bad commit message in https://hg.mozilla.org/integration/fx-team/rev/deff9b455b24

(In reply to Nick Fitzgerald [:fitzgen] from comment #7) > I'm so happy (๑╹ڡ╹)╭ ～ ♡ Yeah, I like the new system too.

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