1000428 - Wire up WiFi debugging setting

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

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

Comment 1

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

Renaming since there may not be a pref.

Comment 2

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

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

Comment 3

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

Attachment: Part 1: Drive WiFi UI option visibility from pref

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.

Comment 4

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

Attachment: Part 2: Wire up WiFi enabled setting

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

Comment 5

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

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

Comment 6

[[:fabrice] Fabrice Desré]

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 7

[Jan Keromnes [:janx]]

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 8

[Panos Astithas (he/him) [:past] (please ni?)]

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?

Comment 9

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

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

Comment 10

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

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

Comment 11

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

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

Comment 12

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

Filed code cleanup as bug 1039493.

Comment 13

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

Attachment: Part 2: Wire up WiFi enabled setting (v2, past: r+, fabrice: r+)

Addressed review comments.

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Julien Wajsberg [:julienw]]

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.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Paul Rouget [:paul]]

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

Comment 19

[Jan Keromnes [:janx]]

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

Comment 20

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

Attachment: Part 2: Wire up WiFi enabled setting (v3, past: r+, fabrice: r+)

* 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

Comment 21

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

All tests passing, including Gaia JS integration tests.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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