975591 - Discover remote devices with Dev Tools server

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

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

With a service discovery mechanism (similar to Bonjour / ZeroConf), we can detect devices that have an enabled Dev Tools server on a local network.

Comment 1

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

Attachment: Part 1: Add device discovery service

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.

Comment 2

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

Attachment: Part 2: Show DevTools devices in WebIDE

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.

Comment 3

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

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

Comment 4

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

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.

Comment 5

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

Looking better in this try build: https://tbpl.mozilla.org/?tree=Try&rev=57ae5e4ee2b3

Comment 6

[Paul Rouget [:paul]]

(I'm looking at this, it will take me some time)

Comment 7

[Paul Rouget [:paul]]

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

Comment 8

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

Attachment: Test discovery in the simulator

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

Comment 9

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

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 10

[Paul Rouget [:paul]]

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.

Comment 11

[Paul Rouget [:paul]]

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

Comment 12

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

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

Comment 13

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

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.

Comment 14

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

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

Comment 15

[Paul Rouget [:paul]]

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

Comment 16

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

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

Comment 17

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

Attachment: Part 1: Add device discovery service (v2, paul: r+)

Adds a logging pref for the discovery service.

Comment 18

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

Attachment: Part 2: Show DevTools devices in WebIDE (v2, paul: r+)

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/57f870a3e9c1
https://hg.mozilla.org/integration/fx-team/rev/82a75a2df1c2

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57f870a3e9c1
https://hg.mozilla.org/mozilla-central/rev/82a75a2df1c2