Closed Bug 1000407 Opened 6 years ago Closed 6 years ago

Dev Tools debugging over Wi-Fi setting


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

Gonk (Firefox OS)
Not set


(b2g-v2.0 fixed)

2.0 S1 (9may)
Tracking Status
b2g-v2.0 --- fixed


(Reporter: jryans, Assigned: jryans)




(1 file)

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).
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.
Attachment #8411268 - Flags: review?(21)
Comment on attachment 8411268 [details] [review]
Add hidden WiFi debugging setting

I'm fine if Jan review this.
Attachment #8411268 - Flags: review?(21) → review?(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").
Attachment #8411268 - Flags: review?(janx) → feedback+
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 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.
Attachment #8411268 - Flags: review?(janx)
Comment on attachment 8411268 [details] [review]
Add hidden WiFi debugging setting

Looks good to me, r+ with one nit!
Attachment #8411268 - Flags: review?(janx) → review+
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.
Flags: needinfo?(janx)
Flags: needinfo?(janx)
I thought there might be a checkin flag, but I guess not. :)
Flags: needinfo?(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 :)
Flags: needinfo?(janx)
Keywords: checkin-needed
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
You need to log in before you can comment on or make changes to this bug.