1000407 - Dev Tools debugging over Wi-Fi setting

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

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

I'll be prototyping WiFi debugging as part of bug 962308 and its child bugs.

I'm going to add a setting to enable this (which I'll map back to a pref later).

Comment 1

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

Attachment: Add hidden WiFi debugging setting

Adds an option to control WiFi debugging.

Right now, this does nothing. I've hidden it by default, and used another setting (with no UI control) to determine if it's visible. I'd like to keep this hidden until it actually has some effect.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8411268 [details] [review]
Add hidden WiFi debugging setting

I'm fine if Jan review this.

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8411268 [details] [review]
Add hidden WiFi debugging setting

I'm mostly ok with this change, but I have a question first, and I'm worried that our remote debugging options are getting very confusing.

In bug 1000428, you are planning to map the "devtools.remote.wifi.enabled" setting to a pref that will control if the wifi remote debugging code runs on the device or not. This code still being experimental, I guess you'll hide the whole feature behind another pref. Will you map the "devtools.remote.wifi.visible" setting to that pref? If so, please mention it in bug 1000428.

My second concern is that the relationship between the "Remote debugging" dropdown menu and the new "Wi-Fi debugging" checkbox is not clear. If I understand things correctly, your wifi debugging code doesn't care about the "debugger.remote-mode" pref, and wifi debugging can still happen if "debugger.remote-mode" is set to "disabled":

- If that is true, you did the right thing by using a separate checkbox instead of adding a level to the "Remote debugging" dropdown menu. However, the dropdown's name is not relevant anymore, please change it to "USB debugging".

- If that is not the case, and wifi debugging depends on the state of the "debugger.remote-mode" pref, please remove the checkbox and add a level to the "Remote debugging" dropdown (e.g. <option value=adb-devtools-wifi>USB/Wi-Fi debugging</option>, also renaming the "ADB and Devtools" option to "USB debugging").

Comment 4

[Jan Keromnes [:janx]]

Nit: In comment 3, I talk about "Wi-Fi debugging" and "USB debugging", but you can also call them "Debugging via Wi-Fi" and "Debugging via USB" as you do in your patch. The second option is longer, but I'm ok with it if the strings fit on the screen of a keon.

Comment 5

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

Comment on attachment 8411268 [details] [review]
Add hidden WiFi debugging setting

Alright, after much debate about names, I've changed "Remote Debugging" to "Debugging via USB", and the new WiFi one to "Dev Tools via WiFi".

There will likely be a pref that is read into the visible setting on startup.

The enabled setting will probably just run some code, but not sure exactly yet.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8411268 [details] [review]
Add hidden WiFi debugging setting

Looks good to me, r+ with one nit!

Comment 7

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

Okay, spelling corrected!  I changed "ADB and Devtools" to "ADB and DevTools" to match as well.

Please land for me, as I don't yet have push access.

Comment 8

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

I thought there might be a checkin flag, but I guess not. :)

Comment 9

[Jan Keromnes [:janx]]

Thanks a lot for addressing the nit!

Gaia's tree is currently closed (see #gaia's topic in irc), that might be why you didn't see a merge button. Let's have a sheriff land this once the tree is open again :)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/34eb3c1375e9825c8727f7b1d8c0617c917b7ae6