I'm adding a setting to control WiFi debugging in bug 1000407. I need to make it actually tell the Dev Tools code to do something. I'll wait for Paul's grand unification of the server in bug 942756 and then add something to the |DebuggerServerController|.

Renaming since there may not be a pref.

Bug 942756 likely is not happening for a while, so I'll ignore it and keep progressing here.

Jan, this maps the pref "devtools.remote.wifi.visible" to the setting of the same name. The setting makes the UI control to toggle WiFi debugging (which was added in bug 1000407) visible.

This links up the "devtools.remote.wifi.enabled" *setting* to toggle the WiFi DevTools server on and off. With both of these patches applied it is now possible to use an (insecure) version of WiFi debugging, after enabling various prefs and settings. See the wiki[1] for how the current version is used, if you wish to try it on your on device. None of this will be available by default until it's deemed secure. Panos, since you reviewed the multiple listeners, you seem like good reviewer for the DevTools API side of my usage of it here. Fabrice, asking for your review to make you aware of this change and also since it refactors various debugging bits of b2g chrome. [1]: https://wiki.mozilla.org/DevTools/WiFi_Debugging

Try: https://tbpl.mozilla.org/?tree=Try&rev=eacadc518e53

Comment on attachment 8455531 [details] [diff] [review] Part 2: Wire up WiFi enabled setting Review of attachment 8455531 [details] [diff] [review]: ----------------------------------------------------------------- lgtm but really needs an extra pair of eyes for the shell.js changes. I also wonder if you could not move all the debugger related code in its own file, but that doesn't need to be done here. ::: b2g/chrome/content/settings.js @@ +435,5 @@ > +// Track these separately here so we can determine the correct value for the > +// pref "devtools.debugger.remote-enabled", which is true when either mode of > +// using DevTools is enabled. > +let devtoolsUSB = false; > +let devtoolsWiFi = false; Not a big fan to have these as global... ::: b2g/chrome/content/shell.js @@ +930,2 @@ > try { > + dump("Starting USB debugger on " + portOrPath + "\n"); Please use debug() instead.

Comment on attachment 8455521 [details] [diff] [review] Part 1: Drive WiFi UI option visibility from pref Review of attachment 8455521 [details] [diff] [review]: ----------------------------------------------------------------- Exciting! I can't wait to use that feature :)

Comment on attachment 8455531 [details] [diff] [review] Part 2: Wire up WiFi enabled setting Review of attachment 8455531 [details] [diff] [review]: ----------------------------------------------------------------- I would have expected to find USBRemoteDebugger and WiFiRemoteDebugger extend RemoteDebugger, but I can't see anything wrong with this setup either. ::: b2g/chrome/content/shell.js @@ +888,5 @@ > + webappsActor: DebuggerServer.globalActorFactories.webappsActor, > + deviceActor: DebuggerServer.globalActorFactories.deviceActor, > + } : DebuggerServer.globalActorFactories > + }; > + let devtools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools; Is this still necessary now that you have added a lazy getter for the loader above?

(In reply to Fabrice Desré [:fabrice] from comment #6) > Comment on attachment 8455531 [details] [diff] [review] > Part 2: Wire up WiFi enabled setting > > Review of attachment 8455531 [details] [diff] [review]: > ----------------------------------------------------------------- > > lgtm but really needs an extra pair of eyes for the shell.js changes. I also > wonder if you could not move all the debugger related code in its own file, > but that doesn't need to be done here. I'll go ahead and move it now I think. Just the shell.js parts, or settings.js parts too? > ::: b2g/chrome/content/settings.js > @@ +435,5 @@ > > +// Track these separately here so we can determine the correct value for the > > +// pref "devtools.debugger.remote-enabled", which is true when either mode of > > +// using DevTools is enabled. > > +let devtoolsUSB = false; > > +let devtoolsWiFi = false; > > Not a big fan to have these as global... I'll wrap this section in an IIFE to avoid it.

(In reply to Panos Astithas [:past] from comment #8) > Comment on attachment 8455531 [details] [diff] [review] > Part 2: Wire up WiFi enabled setting > > Review of attachment 8455531 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would have expected to find USBRemoteDebugger and WiFiRemoteDebugger > extend RemoteDebugger, but I can't see anything wrong with this setup either. Yeah, I wondered if that would be surprising... I'll just make them extend for less surprises. > ::: b2g/chrome/content/shell.js > @@ +888,5 @@ > > + webappsActor: DebuggerServer.globalActorFactories.webappsActor, > > + deviceActor: DebuggerServer.globalActorFactories.deviceActor, > > + } : DebuggerServer.globalActorFactories > > + }; > > + let devtools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools; > > Is this still necessary now that you have added a lazy getter for the loader > above? Nope, I'll remove it, thanks!

Never mind, I'll do a follow up to split the files.

Filed code cleanup as bug 1039493.

Addressed review comments. Try: https://tbpl.mozilla.org/?tree=Try&rev=58f11a7f61b3

https://hg.mozilla.org/integration/b2g-inbound/rev/06a1ebb05c63 https://hg.mozilla.org/integration/b2g-inbound/rev/5ed0d1e11971

https://hg.mozilla.org/mozilla-central/rev/06a1ebb05c63 https://hg.mozilla.org/mozilla-central/rev/5ed0d1e11971

Can we please backout this? This is very likely producing issues in Gaia Marionette JS tests. See [1] for example. [1] https://tbpl.mozilla.org/php/getParsedLog.php?id=44062640&tree=Mozilla-Central&full=1 I guess we'll need to change the related Gaia tests but in the mean time we need a green build. Cc Etienne and Kevin that know the related failing tests.

backed out as requested in comment #16 from mozilla-central and so with the next merge from the other trees

There are still references to RemoveDebugger.handleEvent and RemoveDebugger.start in shell.js and settings.js.

You forgot two calls to RemoteDebugger, which after your patches becomes a class. This broke the prompt confirmation: http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#708 and the Developer HUD: http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools.js#61

* Revert RemoteDebugger back to singleton * Update DevTools HUD to call new |initServer| method * Prompt works again, since we're back to a singleton This should resolve the issues from v2. I have verified locally that the HUD and prompt work correctly, and I'll check the Try output from the hidden integration tests as well. Try: https://tbpl.mozilla.org/?tree=Try&rev=61b72452644f

All tests passing, including Gaia JS integration tests.

https://hg.mozilla.org/integration/b2g-inbound/rev/dc931ef2aacf https://hg.mozilla.org/integration/b2g-inbound/rev/7986463161f2

https://hg.mozilla.org/mozilla-central/rev/dc931ef2aacf https://hg.mozilla.org/mozilla-central/rev/7986463161f2