Closed Bug 975591 Opened 10 years ago Closed 10 years ago

Discover remote devices with Dev Tools server

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

With a service discovery mechanism (similar to Bonjour / ZeroConf), we can detect devices that have an enabled Dev Tools server on a local network.
Adds a UDP multicast device discovery service.  The protocol is described in more detail in the patch.

This includes a unit test of the service with a mocked transport.
Attachment #8443219 - Flags: review?(paul)
This adds any discovered devices with the DevTools service to a new WiFi runtimes list.

Locally, I've built a modified simulator that enables discovery and shows up in the new list.
Attachment #8443220 - Flags: review?(paul)
Just saw your comment on bug 1026945 to scan when we enter WebIDE, so I will probably add that too.  At the moment, we only scan on opening the runtime list.
(I'm looking at this, it will take me some time)
(In reply to J. Ryan Stinnett [:jryans] from comment #2)
> Locally, I've built a modified simulator that enables discovery and shows up
> in the new list.

Can you share a temporary patch? Just for testing.
Attached patch Test discovery in the simulator (obsolete) — Splinter Review
Here's the patch you can add to the simulator.  However, you'll then need to build your own simulator XPI.  You can do that in a .mozconfig for b2g-desktop and adding: 

export FXOS_SIMULATOR=1
export GAIADIR=<path to gaia>

Then, "mach build && mach package" should produce a simulator XPI you can install.

Alternatively, my try build[1] has simulator XPIs in the build directories for the b2g desktop platforms.

Once you have a modified simulator:

1. Start in outside of WebIDE, either from the old App Manager or the command line
2. In WebIDE, click the runtime list, and under "WiFi devices" you should see an entry with the same name as your computer's host name (better name ideas welcome!)
3. Connect to that, and it should work as expected

I have only tested this on OS X at the moment.  I definitely intend to test Linux and Windows myself before landing, as there may easily be some strange issues here.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=57ae5e4ee2b3
Oh, I left out one more step.

In Firefox desktop (after applying the patches from this bug), you need to enable the pref "devtools.remote.wifi.enabled".  I am hiding WiFi work under this pref until there is a logical workflow to actually use it sensibly.

I've now tested OS X, Windows, and Linux.  All 3 are able to connect to a simulator with the patch above.

I also tried this patch queue on the Flame, and all 3 desktop OSes are able to connect to it over WiFi.

Known issues on device (will be addressed in future bugs):
* You'll need to set the "devtools.debugger.force-local" pref to false on the device
* If you change WiFi networks, you'll need to restart DevTools by toggling Debugging via USB from to ADB and then back to ADB and DevTools
Comment on attachment 8443219 [details] [diff] [review]
Part 1: Add device discovery service

Review of attachment 8443219 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/discovery/discovery.js
@@ +42,5 @@
> +// TODO Bug 1027456: May need to reserve these with IANA
> +const SCAN_PORT = 50624;
> +const UPDATE_PORT = 50625;
> +const ADDRESS = "224.0.0.200";
> +const REPLY_TIMEOUT = 5000;

Can these values be stored in a pref? Not required, just a suggestion.
Attachment #8443219 - Flags: review?(paul) → review+
Comment on attachment 8443220 [details] [diff] [review]
Part 2: Show DevTools devices in WebIDE

Review of attachment 8443220 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webide/content/webide.js
@@ +235,5 @@
>    updateRuntimeList: function() {
> +    if (!AppManager.isWiFiRuntimeEnabled) {
> +      let wifiHeader = document.querySelector("#runtime-header-wifi-devices");
> +      if (wifiHeader) {
> +        wifiHeader.parentNode.removeChild(wifiHeader);

wifiHeader.remove();

And what about hiding it? (setAttribute("hidden","true")).
And what if the pref is toggled later on?

::: modules/libpref/src/init/all.js
@@ +632,5 @@
>  // Used for devtools debugging
>  pref("devtools.dump.emit", false);
>  
> +// Disable devtools via WiFi
> +pref("devtools.remote.wifi.enabled", false);

This preference means "scan wifi to find devices". Make it clear it's not "make this device discoverable".
Attachment #8443220 - Flags: review?(paul) → review+
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #10)
> Comment on attachment 8443219 [details] [diff] [review]
> Part 1: Add device discovery service
> 
> Review of attachment 8443219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/discovery/discovery.js
> @@ +42,5 @@
> > +// TODO Bug 1027456: May need to reserve these with IANA
> > +const SCAN_PORT = 50624;
> > +const UPDATE_PORT = 50625;
> > +const ADDRESS = "224.0.0.200";
> > +const REPLY_TIMEOUT = 5000;
> 
> Can these values be stored in a pref? Not required, just a suggestion.

Could be, but it seems of limited value to me.  You need the address and ports to match on all devices to make any sense.  I think I'll leave as-is for now.
Paul, I noticed one UX issue with this.

On Windows, the first time a device scan is triggered, a prompt to allow Firefox firewall access appears.  You need to allow the prompt, since Firefox is listening on a new socket as part of the scan.  

At the moment, this dialog appears the first time you click on "Runtime List" (since that's what triggers a scan).  Is that okay?  Should we trigger at a different point in time?  I am worried Windows users would be confused by the dialog, especially if they are not trying to use the WiFi feature at all.
Flags: needinfo?(paul)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #11)
> Comment on attachment 8443220 [details] [diff] [review]
> Part 2: Show DevTools devices in WebIDE
> 
> Review of attachment 8443220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webide/content/webide.js
> @@ +235,5 @@
> >    updateRuntimeList: function() {
> > +    if (!AppManager.isWiFiRuntimeEnabled) {
> > +      let wifiHeader = document.querySelector("#runtime-header-wifi-devices");
> > +      if (wifiHeader) {
> > +        wifiHeader.parentNode.removeChild(wifiHeader);
> 
> wifiHeader.remove();
> 
> And what about hiding it? (setAttribute("hidden","true")).
> And what if the pref is toggled later on?

Okay, I'm using a pref observer now to track and untrack when the pref changes.  This function sets / unsets the hidden attribute as needed.

> ::: modules/libpref/src/init/all.js
> @@ +632,5 @@
> >  // Used for devtools debugging
> >  pref("devtools.dump.emit", false);
> >  
> > +// Disable devtools via WiFi
> > +pref("devtools.remote.wifi.enabled", false);
> 
> This preference means "scan wifi to find devices". Make it clear it's not
> "make this device discoverable".

Changed the pref to "devtools.remote.wifi.scan" and clarified the comment.
(In reply to J. Ryan Stinnett [:jryans] from comment #13)
> Paul, I noticed one UX issue with this.
> 
> On Windows, the first time a device scan is triggered, a prompt to allow
> Firefox firewall access appears.  You need to allow the prompt, since
> Firefox is listening on a new socket as part of the scan.  
> 
> At the moment, this dialog appears the first time you click on "Runtime
> List" (since that's what triggers a scan).  Is that okay?  Should we trigger
> at a different point in time?  I am worried Windows users would be confused
> by the dialog, especially if they are not trying to use the WiFi feature at
> all.

I see. This indeed can be annoying especially when you're not interested in wifi devices.

Here is an idea, and that can be implemented in a follow-up: when the runtime panel shows up, don't scan right away, add a button "Scan WiFi", on click:
1. prevent the panel to close if focus change
2. start scanning (windows popups shows up)
3. after a couple of seconds, or on close, reset focus behavior
4. show a throbber
5. show results

I can take care of that if needed.
Flags: needinfo?(paul)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #15)
> (In reply to J. Ryan Stinnett [:jryans] from comment #13)
> > Paul, I noticed one UX issue with this.
> > 
> > On Windows, the first time a device scan is triggered, a prompt to allow
> > Firefox firewall access appears.  You need to allow the prompt, since
> > Firefox is listening on a new socket as part of the scan.  
> > 
> > At the moment, this dialog appears the first time you click on "Runtime
> > List" (since that's what triggers a scan).  Is that okay?  Should we trigger
> > at a different point in time?  I am worried Windows users would be confused
> > by the dialog, especially if they are not trying to use the WiFi feature at
> > all.
> 
> I see. This indeed can be annoying especially when you're not interested in
> wifi devices.
> 
> Here is an idea, and that can be implemented in a follow-up: when the
> runtime panel shows up, don't scan right away, add a button "Scan WiFi", on
> click:
> 1. prevent the panel to close if focus change
> 2. start scanning (windows popups shows up)
> 3. after a couple of seconds, or on close, reset focus behavior
> 4. show a throbber
> 5. show results
> 
> I can take care of that if needed.

I've filed bug 1031045 about this, so we can discuss more there.

I don't feel that it's urgent, just something to do before defaulting on.  If you'd like to do it, that would be great, otherwise I can get to it eventually.
Adds a logging pref for the discovery service.
Attachment #8443219 - Attachment is obsolete: true
Attachment #8445265 - Attachment is obsolete: true
Attachment #8446870 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/57f870a3e9c1
https://hg.mozilla.org/mozilla-central/rev/82a75a2df1c2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.