In bug 1286281, an option was introduced to the remote debugger server to use the WebSocket protocol. There are preferences to enable this, and the --start-debugger-server command line option supports a syntax option to enable WebSocket. Implemented and described at [1]. Implement similar option to the GCLI "listen" command: - "listen" with no args - use defaults from preferences - "listen 1234" - listen on non-WebSocket port 1234 - "listen ws:1234" - listen on WebSocket port 1234 - "listen ws:" - listen on WebSocket, use port from preferences

[good first bug] whiteboard -> keyword mass change

Hi, I'd like to work on this bug. Can I get some direction on how to get started?

(In reply to sj from comment #2) > Hi, I'd like to work on this bug. Can I get some direction on how to get > started? Thanks for your interest! The first thing you need to do is to checkout the Firefox sources and build them. Please follow the hacking guides here: https://wiki.mozilla.org/DevTools/Hacking https://developer.mozilla.org/en-US/docs/Tools/Contributing Then you can have a look at the actual bug. Look how the GCLI command works when it's starting the debugger server: http://searchfox.org/mozilla-central/source/devtools/shared/gcli/commands/listen.js And make it accept the same arguments as the --start-debugger-server option on the Firefox command line. That's implemented here: http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.js#133-199 Good luck and let me know if you need any help!

Comment on attachment 8786746 [details] Bug 1295080 Add WebSocket option to GCLI 'listen' command http://pasteboard.co/fhyKuM3ep.png This is the error I get on one of the tests.

Comment on attachment 8786745 [details] Bug 1295080 - Add WebSocket option to GCLI 'listen' command https://reviewboard.mozilla.org/r/75660/#review73902 ::: devtools/client/commandline/test/browser_cmd_listen.js:41 (Diff revision 2) > setup: function () { > return helpers.setInput(options, "listen"); > }, > check: { > input: "listen", > - hints: " [port]", > + hints: " [portOrPath]", There's not reason why the parameter name should be "portOrPath". The user can specify only the "port" here, plus the WebSocket option. ::: devtools/shared/gcli/commands/listen.js:36 (Diff revision 2) > debuggerServer.allowChromeProcess = !l10n.hiddenByChromePref(); > return debuggerServer; > }); > > exports.items = [ > - { > + { Don't add whitespace here. ::: devtools/shared/gcli/commands/listen.js:47 (Diff revision 2) > params: [ > { > - name: "port", > - type: "number", > + name: "portOrPath", > + type: "string", > get defaultValue() { > return Services.prefs.getIntPref("devtools.debugger.chrome-debugging-port"); You need to return string here, and somehow take into account the remote-websocket pref. ::: devtools/shared/gcli/commands/listen.js:61 (Diff revision 2) > } > > - listener.portOrPath = args.port; > + let webSocket = false; > + let defaultPort = Services.prefs.getIntPref("devtools.debugger.remote-port"); > + let portOrPath = args.portOrPath; > + if (portOrPath === true) { The value is never "true" here. The argument is a string, possibly with a default value.

Joe, what do you think is the best way how to add bool options to the "listen" command syntax? The obvious choice is to copy the "ws:port" syntax from the --start-debugger-server, but I don't like it very much. It should be something that makes a better use of the GCLI autocomplete and hints. Also, it's likely that more options like this will be added in near future. Now it's "websocket", and soon it'll be "chrome-rdp". What about: - "listen" - use defaults - "listen 123" - specify port - "listen 123 websocket" - specify WS - "listen websocket" - WS on default port - "listen 123 chrome-rdp" - start Chrome RDP server

GCLI would work fine with all of the options you give except for "listen websocket", because it would interpret "websocket" as an attempt to set the port. Instead, you could do "listen --protocol websocket". The config would look like this: params: [ { name: "port", type: "number", get defaultValue() { return Services.prefs.getIntPref("devtools.debugger.chrome-debugging-port"); }, description: l10n.lookup("listenPortDesc"), }, { name: "protocol", defaultValue: "mozilla-rdp", type: { name: "selection", data: [ "mozilla-rdp", "websocket", "chrome-rdp" ], }, description: l10n.lookup("listenProtocolDesc"), }, ],

Comment on attachment 8788201 [details] Bug 1295080 - Add WebSocket option to GCLI 'listen' command using two params - port and protocol https://reviewboard.mozilla.org/r/76768/#review75034 This is starting to look really good, thanks for working on this! There are a few small remaining issues, and after they are fixed, we are done. Also, right now, this review has two patches - please squash them into one next time you submit. I started a try build that will build Firefox and run the tests - let's see how it goes. I expect one eslint failure, but other than that, I hope it will be OK. ::: devtools/client/commandline/test/browser_cmd_listen.js:5 (Diff revision 1) > /* Any copyright is dedicated to the Public Domain. > * http://creativecommons.org/publicdomain/zero/1.0/ */ > > // Tests that the listen/unlisten commands work as they should. > +const Services = require("Services"); The "Services" module doesn't need to be require'd here - after all, it's already used in this test file before your change. ::: devtools/client/commandline/test/browser_cmd_listen.js:47 (Diff revision 1) > - hints: " [portOrPath]", > + hints: " [port] [protocol]", > markup: "VVVVVV", > status: "VALID" > }, > exec: { > - output: "Listening on port 6080" > + output: "Listening on port "+Services.prefs.getIntPref("devtools.debugger.remote-port") Use spaces around the "+" operator: "first" + variable + "second". Otherwise, eslint will report error here. ::: devtools/shared/gcli/commands/listen.js:56 (Diff revision 1) > + { > + name: "protocol", > + defaultValue: "mozilla-rdp", > + type: { > + name: "selection", > + data: [ "mozilla-rdp", "websocket", "chrome-rdp" ], Don't include "chrome-rdp" in this list now. It's not supported at this moment. But it's nice to know that this change is future-proof and adding new option will be easy. ::: devtools/shared/gcli/commands/listen.js:68 (Diff revision 1) > if (!listener) { > throw new Error(l10n.lookup("listenDisabledOutput")); > } > > - let webSocket = false; > - let defaultPort = Services.prefs.getIntPref("devtools.debugger.remote-port"); > + let webSocket = Services.prefs.getBoolPref("devtools.debugger.remote-websocket"); > + if(args.protocol === "websocket") If the websocket pref is set to "true", then the "mozilla-rdp" option should reset the webSocket variable back to false. Make it possible to override the pref in both directions.

Comment on attachment 8786745 [details] Bug 1295080 - Add WebSocket option to GCLI 'listen' command https://reviewboard.mozilla.org/r/75660/#review75412 ::: devtools/client/commandline/test/browser_cmd_listen.js (Diff revision 4) > /* Any copyright is dedicated to the Public Domain. > * http://creativecommons.org/publicdomain/zero/1.0/ */ > > // Tests that the listen/unlisten commands work as they should. > - Nit: There is a deleted line here - a leftover from adding and removing the Services import. Please add it back, so that the patch doesn't contain unnecessary whitespace changes. ::: devtools/shared/gcli/commands/listen.js:67 (Diff revision 4) > var listener = debuggerServer.createListener(); > if (!listener) { > throw new Error(l10n.lookup("listenDisabledOutput")); > } > > + let webSocket = Services.prefs.getBoolPref("devtools.debugger.remote-websocket"); This way, the webSocket default value from the pref is never actually used. "args.protocol" always has some value, the default being "mozilla-rdp". Please move reading the pref into the code that determines the "protocol" default value and choose "mozilla-rdp" or "websocket" there. Like the "port" default value, which already does read the pref.

Comment on attachment 8786745 [details] Bug 1295080 - Add WebSocket option to GCLI 'listen' command https://reviewboard.mozilla.org/r/75660/#review76218 The code now works as expected. There are still few formatting issues that trigger ESlint warnings and errors though. Do you have eslint installed? If not, you should install it using the instructions here: https://wiki.mozilla.org/DevTools/CodingStandards and run it on the changed files. ::: devtools/shared/gcli/commands/listen.js:36 (Diff revision 5) > debuggerServer.allowChromeProcess = !l10n.hiddenByChromePref(); > return debuggerServer; > }); > > exports.items = [ > - { > + { Remove the extra space added here. ::: devtools/shared/gcli/commands/listen.js:55 (Diff revision 5) > - } > + }, > + { > + name: "protocol", > + get defaultValue() { > + let webSocket = Services.prefs.getBoolPref("devtools.debugger.remote-websocket"); > + if(webSocket === true) ESlint complains about a few formatting issues here: - there should be space between "if" and "(" - the if/else blocks should be always surrounded by curly braces {} - some lines are longer than the maximum limit of 90 characters.

Another formal issue - please edit the commit message so that it describes accurately what got fixed in the bug. Right now, it describes only the incremental changes made in the latest iteration of the patch.

Comment on attachment 8786745 [details] Bug 1295080 - Add WebSocket option to GCLI 'listen' command https://reviewboard.mozilla.org/r/75660/#review76300 Perfect! Thanks for working on this.

Pushed by jsnajdr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3a565916530e Add WebSocket option to GCLI 'listen' command r=jsnajdr

I've updated https://developer.mozilla.org/en-US/docs/Tools/GCLI#Commands for this.