1000814 - Remove module boilerplate from transport.js once marionette server loads it as an SDK module

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[Eddy Bruel [:ejpbruel]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

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

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1000814 - Cleanup marionette transport.js from DevTools specifics.

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

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1000814 - Remove loader boilerplate from transport.js.

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

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1000814 - Use loader helpers to import symbols lazily.

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

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1000814 - Move LocalDebuggerTransport to its own module.

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

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1000814 - Move ChildDebuggerTransport to its own module.

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

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1000814 - Move WorkerDebuggerTransport to its own module.

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

Comment 8

[Alexandre Poirot [:ochameau]]

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 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8991523 [details]
Bug 1000814 - Remove loader boilerplate from transport.js.

https://reviewboard.mozilla.org/r/256442/#review263416

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8991524 [details]
Bug 1000814 - Use loader helpers to import symbols lazily.

https://reviewboard.mozilla.org/r/256444/#review263418

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8991526 [details]
Bug 1000814 - Move ChildDebuggerTransport to its own module.

https://reviewboard.mozilla.org/r/256448/#review263424

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 15

[Alexandre Poirot [:ochameau]]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Noemi Erli[:noemi_erli]]

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