Closed Bug 1000428 Opened 6 years ago Closed 5 years ago

Wire up WiFi debugging setting

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S1 (1aug)

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Summary: Map WiFi debugging setting to a pref → Wire up WiFi debugging setting
No longer depends on: 942756
Bug 942756 likely is not happening for a while, so I'll ignore it and keep progressing here.
Depends on: 1033079
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.
Attachment #8455521 - Flags: review?(janx)
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
Attachment #8455531 - Flags: review?(past)
Attachment #8455531 - Flags: review?(fabrice)
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.
Attachment #8455531 - Flags: review?(fabrice) → review+
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 :)
Attachment #8455521 - Flags: review?(janx) → review+
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?
Attachment #8455531 - Flags: review?(past) → review+
(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.
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
Filed code cleanup as bug 1039493.
Addressed review comments.

Try: https://tbpl.mozilla.org/?tree=Try&rev=58f11a7f61b3
Attachment #8455531 - Attachment is obsolete: true
Attachment #8456888 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06a1ebb05c63
https://hg.mozilla.org/mozilla-central/rev/5ed0d1e11971
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
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.
Keywords: checkin-needed
backed out as requested in comment #16 from mozilla-central and so with the next merge from the other trees
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
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
Flags: needinfo?(jryans)
* 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
Attachment #8456888 - Attachment is obsolete: true
Attachment #8459638 - Flags: review+
Flags: needinfo?(jryans)
All tests passing, including Gaia JS integration tests.
Keywords: checkin-needed
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
You need to log in before you can comment on or make changes to this bug.