Transport.js currently contains some module boilerplate code allowing it to be loaded as both an SDK module and a subscript. Once the marionette server has been converted to load transport.js as an SDK module, this boilerplate will no longer be required.

Since bug 1356237 landed, transport.js has been forked for marionette to be a JSM. So that devtools/shared/transport/transport.js is now only loaded as a module. We can now remove the boilerplate. Also transport.js in marionette can be stripped to DebuggerTransport, the only class it really uses. I saw that the rest is being maintained and refactored for nothing...

I used this as an opportunity to do some late spring cleaning around transports. And rush on using mozreview while we still can easily submit multiples changesets... A quick overview of this patch queue: 1) Cleanup marionette transport.js from DevTools ... Bug 1356237 forked both transport.js, so that marionette now maintains its own fork. It is now a JSM and uses toolkit's event-emitter. I noticed that it contains worker transport that is obviously unecessary for marionette. I also removed all other things that were not strictly necessary for it. 2) Remove loader boilerplate from transport.js. Since bug 1356237 forked transport.js, the module in devtools is now only loaded as a devtools module and so we can get rid of the module boilerplate. 3) Use loader helpers to import symbols lazily. server/mains.js uses DevToolsUtils helpers rather than loader one. I cleaned that up. We may want to go over the whole codebase and remove that helper from DevToolsUtils... https://searchfox.org/mozilla-central/search?q=DevToolsUtils.define&case=true&regexp=false&path= 4,5,6) Move {LocalDebuggerTransport,ChildDebuggerTransport,WorkerDebuggerTransport} to its own module. transports.js is a very big file with too many classes in there. So I moved all but DebuggerTransport into individual modules. My first motivation was specific to worker class... I find the following pattern extremelly confusing: if (this.isWorker) WorkerDebuggerTransport=... else WorkerDebuggerTransport=... So I two distinct class that are use explicitely from main.js. This will help loading less code on both main and worker threads!

Comment on attachment 8991522 [details] Bug 1000814 - Cleanup marionette transport.js from DevTools specifics. https://reviewboard.mozilla.org/r/256440/#review263414 Thanks, seems reasonable! :) ::: testing/marionette/transport.js:15 (Diff revision 1) > > ChromeUtils.import("resource://gre/modules/Services.jsm"); > ChromeUtils.import("resource://gre/modules/EventEmitter.jsm"); > const {StreamUtils} = > ChromeUtils.import("chrome://marionette/content/stream-utils.js", {}); > const {Packet, JSONPacket, BulkPacket} = I bet all the bulk packet features aren't used either, but that's a more invasive change...

Comment on attachment 8991523 [details] Bug 1000814 - Remove loader boilerplate from transport.js. https://reviewboard.mozilla.org/r/256442/#review263416

Comment on attachment 8991524 [details] Bug 1000814 - Use loader helpers to import symbols lazily. https://reviewboard.mozilla.org/r/256444/#review263418

Comment on attachment 8991527 [details] Bug 1000814 - Move WorkerDebuggerTransport to its own module. https://reviewboard.mozilla.org/r/256450/#review263422 Thanks! :) I agree it's clearer with separate objects.

Comment on attachment 8991526 [details] Bug 1000814 - Move ChildDebuggerTransport to its own module. https://reviewboard.mozilla.org/r/256448/#review263424

Comment on attachment 8991525 [details] Bug 1000814 - Move LocalDebuggerTransport to its own module. https://reviewboard.mozilla.org/r/256446/#review263420 ::: devtools/shared/transport/moz.build:10 (Diff revision 1) > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini'] > > DevToolsModules( > + 'local-transport.js', You could name this just `local.js` since `transport` is redundant with the directory (same for the other transport patches). Whatever you prefer is fine.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9) > Comment on attachment 8991522 [details] > Bug 1000814 - Cleanup marionette transport.js from DevTools specifics. > > https://reviewboard.mozilla.org/r/256440/#review263414 > > Thanks, seems reasonable! :) > > ::: testing/marionette/transport.js:15 > (Diff revision 1) > > > > ChromeUtils.import("resource://gre/modules/Services.jsm"); > > ChromeUtils.import("resource://gre/modules/EventEmitter.jsm"); > > const {StreamUtils} = > > ChromeUtils.import("chrome://marionette/content/stream-utils.js", {}); > > const {Packet, JSONPacket, BulkPacket} = > > I bet all the bulk packet features aren't used either, but that's a more > invasive change... Oh really? That's unfortunate as it could probably help use being faster on data-intensive requests. Isn't that still used by old-perf/memory? May be the recent screenshot feature could benefit from this... (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14) > Comment on attachment 8991525 [details] > Bug 1000814 - Move LocalDebuggerTransport to its own module. > > https://reviewboard.mozilla.org/r/256446/#review263420 > > ::: devtools/shared/transport/moz.build:10 > (Diff revision 1) > > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini'] > > > > DevToolsModules( > > + 'local-transport.js', > > You could name this just `local.js` since `transport` is redundant with the > directory (same for the other transport patches). Whatever you prefer is > fine. Yes... I was wondering about that, I was hesitent to name them without the -transport suffix and thought that it would be hard to distinguish all the files in this folder (distinguish from stream-utils and packets). I made the call to use the suffix when I saw WebSocketDebuggerTransport having this prefix.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d88d4667a344 Cleanup marionette transport.js from DevTools specifics. r=jryans https://hg.mozilla.org/integration/autoland/rev/f635e3ea68ac Remove loader boilerplate from transport.js. r=jryans https://hg.mozilla.org/integration/autoland/rev/7d62f2231644 Use loader helpers to import symbols lazily. r=jryans https://hg.mozilla.org/integration/autoland/rev/5f1ec5e68971 Move LocalDebuggerTransport to its own module. r=jryans https://hg.mozilla.org/integration/autoland/rev/a2de573ca18c Move ChildDebuggerTransport to its own module. r=jryans https://hg.mozilla.org/integration/autoland/rev/3433315f30bd Move WorkerDebuggerTransport to its own module. r=jryans

https://hg.mozilla.org/mozilla-central/rev/d88d4667a344 https://hg.mozilla.org/mozilla-central/rev/f635e3ea68ac https://hg.mozilla.org/mozilla-central/rev/7d62f2231644 https://hg.mozilla.org/mozilla-central/rev/5f1ec5e68971 https://hg.mozilla.org/mozilla-central/rev/a2de573ca18c https://hg.mozilla.org/mozilla-central/rev/3433315f30bd