Closed Bug 1000814 Opened 6 years ago Closed 2 years ago

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

Categories

(DevTools :: Debugger, enhancement, P3)

x86
macOS
enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: ejpbruel, Assigned: ochameau)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files)

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.
Depends on: 1083711
Blocks: dbg-server
Component: Developer Tools → Developer Tools: Debugger
Product: Firefox → DevTools
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...
Assignee: nobody → poirot.alex
Depends on: 1356237
Blocks: 1473828
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!
Severity: normal → enhancement
Priority: -- → P3
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...
Attachment #8991522 - Flags: review?(jryans) → review+
Comment on attachment 8991523 [details]
Bug 1000814 - Remove loader boilerplate from transport.js.

https://reviewboard.mozilla.org/r/256442/#review263416
Attachment #8991523 - Flags: review?(jryans) → review+
Comment on attachment 8991524 [details]
Bug 1000814 - Use loader helpers to import symbols lazily.

https://reviewboard.mozilla.org/r/256444/#review263418
Attachment #8991524 - Flags: review?(jryans) → review+
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.
Attachment #8991527 - Flags: review?(jryans) → review+
Comment on attachment 8991526 [details]
Bug 1000814 - Move ChildDebuggerTransport to its own module.

https://reviewboard.mozilla.org/r/256448/#review263424
Attachment #8991526 - Flags: review?(jryans) → review+
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.
Attachment #8991525 - Flags: review?(jryans) → review+
(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
You need to log in before you can comment on or make changes to this bug.