957164 - Integrate libadb.js inside Firefox

Status: RESOLVED WONTFIX

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

WIP: https://github.com/mozilla/libadb.js

Comment 1

[Paul Rouget [:paul]]

This is the way we want to use libadb from inside firefox:

- libadb doesn't need to run all the time
- we will introduce a new panel called "runtime"
- when the panel is opened, we want to list the plugged devices
- when the panel is closed, we probably can kill libadb
- when a plugged device is clicked, we want to execute adb forward

So basically, we need a JS module that exposes methods like:
- adb.start()
- adb.scan() <= returns list of device
- adb.forward(…)
- adb.stop()
- "device list updated" event. We would then run adb.scan() again

start and stop would be called when we open and close the runtime panel.

Ideally, libadb would run in its own process.

We don't want to use the addon helper.

Comment 2

[Brendan Dahl [:bdahl]]

The api for the add-on is basically there already, but with slightly different names. To give us a bit of flexibility in the future, I would like to return promises from all of those functions so we can get rid of some of the blocking code.

For the prototype, do you want it integrated with firefox or as an add-on?

Comment 3

[Paul Rouget [:paul]]

(In reply to Brendan Dahl [:bdahl] from comment #2)
> The api for the add-on is basically there already, but with slightly
> different names. To give us a bit of flexibility in the future, I would like
> to return promises from all of those functions so we can get rid of some of
> the blocking code.

Ok!

> For the prototype, do you want it integrated with firefox or as an add-on?

With Firefox directly.

Comment 4

[Paul Rouget [:paul]]

Any update on this?

Comment 5

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

Attachment: Support full refresh in case server dies

Here's a small libadb change that listens for a "refresh" event to restart scanning if needed.

Comment 6

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

Attachment: Add device refresh button to App Manager. r=paul

Comment 7

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

Comment on attachment 8405036 [details] [diff] [review]
Add device refresh button to App Manager. r=paul

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

Paul, we don't need to land this change (although we could!), but this is how I tested the rescan feature locally.

In UI v2, you would just call Devices.refresh() when you open the list.

Note: If you want to test this feature, I recommend waiting for the libadb change from here and bug 994980 first.

Comment 8

[Paul Rouget [:paul]]

I want to make sure I understand: with this patch, if we call Devices.refresh() when the popup shows up in the new appmgr, we're sure that adb will restart even if, in the meantime, another instance of adb has been used. Right?

Comment 9

[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 #8)
> I want to make sure I understand: with this patch, if we call
> Devices.refresh() when the popup shows up in the new appmgr, we're sure that
> adb will restart even if, in the meantime, another instance of adb has been
> used. Right?

Yes, exactly.  I tested many rounds of refreshing via my UI button (which is what your popup would call on open or something), then using ADB from the terminal, back and forth.

Though, there are two outstanding patches for libadb itself to enable this behavior, so we need those first: one here and one in bug 994980.

Comment 10

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #9)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #8)
> > I want to make sure I understand: with this patch, if we call
> > Devices.refresh() when the popup shows up in the new appmgr, we're sure that
> > adb will restart even if, in the meantime, another instance of adb has been
> > used. Right?
> 
> Yes, exactly.  I tested many rounds of refreshing via my UI button (which is
> what your popup would call on open or something), then using ADB from the
> terminal, back and forth.
> 
> Though, there are two outstanding patches for libadb itself to enable this
> behavior, so we need those first: one here and one in bug 994980.

What about the fact that the device gets disconnected when running "adb logcat" on the side?

Comment 11

[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)
> What about the fact that the device gets disconnected when running "adb
> logcat" on the side?

Well, the device does get disconnected if you use "adb logcat".  Similarly, if you refresh the device list while using "adb logcat", your logging session ends.

The original ADB Helper and libadb take two different approaches in general:

* ADB Helper wraps the "adb" command you already have installed so you don't need to type out port forwarding commands
  * It will coexist with other ADB things you might also try to do on the terminal
* libadb instead *contains* all of ADB (and so it works for people who do not yet have ADB)
  * As part of this assumption, it ends up being in complete control of your use of ADB
  * It does not (currently) expect you to try to run other "adb" commands from your terminal

So, libadb has two main advantages: 

* If you don't currently have ADB, you don't need to install it (though you'd still need drivers on Windows and udev rules on Linux)
* We can modify the full source of ADB to fix issues like the Mac replugging thing

It's not clear to me how important we believe this case really is.  I can't recall anyone that has showed up in #devtools with an App Manager question and did not have ADB installed at all.  Usually, they have it, but then have some related problem, like udev or whatever. 

At the same time, maybe there are people out there who were stumped by ADB or put off by the fact that they had to install it, so they just gave up.  My guess is we never hear from those people, so it's hard to know how many of them there are.

With more effort, we could actually combine the approaches that each of these addons takes today into one addon:

1. If you are already running an ADB server (because you ran some "adb" terminal command), we can try to connect to it and use that
  * This is like what happens today with ADB Helper
  * You can then freely mix terminal commands with using the AM
2. If you have the "adb" command installed but the server is not running, let's start it and use that
  * After that, this is now the same as case 1 above
3. If you are not running an ADB server and do not have the "adb" command, we start our own ADB server from the libadb code (so no installed adb is needed)
  * This server would be exclusive to the AM
  * It would still disconnect / be mean to you if you do use "adb" from the terminal, but case 1 & 2 should ensure that you don't have it

If we did this though, we don't benefit from any fixes we make to ADB source (like Mac replugging) in case 1 / 2 where someone has ADB.

Again, this is more work to do, assuming we think it is a good idea.

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> The original ADB Helper and libadb take two different approaches in general:
> 
> * ADB Helper wraps the "adb" command you already have installed so you don't
> need to type out port forwarding commands
>   * It will coexist with other ADB things you might also try to do on the
> terminal
> * libadb instead *contains* all of ADB (and so it works for people who do
> not yet have ADB)

ADB Helper ships ADB binary, so that it will also work for people that don't have ADB installed!

> * We can modify the full source of ADB to fix issues like the Mac replugging
> thing

ADB Helper may benefit from such fixes if we ship custom builds of adb executable with these patches.


> It's not clear to me how important we believe this case really is.  I can't
> recall anyone that has showed up in #devtools with an App Manager question
> and did not have ADB installed at all.  Usually, they have it, but then have
> some related problem, like udev or whatever. 

About that, I still haven't look at https://github.com/MozillaOnline/pc-sync-tool/
But they are providing an addon very close to adb helper, they may have custom adb builds...
Also they manage to ease the drivers issues, at least on windows by shipping drivers into the addon!!
(DO WANT!)

> With more effort, we could actually combine the approaches that each of
> these addons takes today into one addon:
> 
> 1. If you are already running an ADB server (because you ran some "adb"
> terminal command), we can try to connect to it and use that
>   * This is like what happens today with ADB Helper
>   * You can then freely mix terminal commands with using the AM
> 2. If you have the "adb" command installed but the server is not running,
> let's start it and use that
>   * After that, this is now the same as case 1 above
> 3. If you are not running an ADB server and do not have the "adb" command,
> we start our own ADB server from the libadb code (so no installed adb is
> needed)
>   * This server would be exclusive to the AM
>   * It would still disconnect / be mean to you if you do use "adb" from the
> terminal, but case 1 & 2 should ensure that you don't have it

I would simplify to:
1. is there a server up and running, yes ?
  > use it
2. no ?
  > start our own, with our fixes

Also, with the new design, we do not really need to watch for new devices, but just have the list of currently connected one at one precise time. It means that we won't have any more issue when our own server gets killed, we will just spawn a new one when the list is requested.

So about your gecko patch, instead of emitting a refresh event, we would instead expose a Devices.list() method that would return the list of connected devices.

Devices.list = function () {
  // ... Somehow call Adb.list() method of the addon.
  return Adb.list();
}

Adb.list = function () {
  // Is no server is up and running, starts our own
  if (!isRunning()) {
    startOurOwn();
  }
  // Then we have either our own server or the user one up and running,
  // we just query it:
  return this.listDevices();
}

(this pseudo code is much more adb-helper than libadb.js oriented)

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8405027 [details] [review]
Support full refresh in case server dies

It may help the current app manager behavior,
but I'm not sure that what we want for UIv2.

Comment 14

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

(In reply to Alexandre Poirot (:ochameau) from comment #12)
> (In reply to J. Ryan Stinnett [:jryans] from comment #11)
> > The original ADB Helper and libadb take two different approaches in general:
> > 
> > * ADB Helper wraps the "adb" command you already have installed so you don't
> > need to type out port forwarding commands
> >   * It will coexist with other ADB things you might also try to do on the
> > terminal
> > * libadb instead *contains* all of ADB (and so it works for people who do
> > not yet have ADB)
> 
> ADB Helper ships ADB binary, so that it will also work for people that don't
> have ADB installed!

Yes, of course you are right...  Not sure how I forgot this! :/

> > * We can modify the full source of ADB to fix issues like the Mac replugging
> > thing
> 
> ADB Helper may benefit from such fixes if we ship custom builds of adb
> executable with these patches.

That's another approach we can take, yes.  I am little wary of trying to discern
which changes are fixes vs. stuff need libadb to function...  Could take some
time to pull out just the "fixes" successfully.

> > It's not clear to me how important we believe this case really is.  I can't
> > recall anyone that has showed up in #devtools with an App Manager question
> > and did not have ADB installed at all.  Usually, they have it, but then have
> > some related problem, like udev or whatever. 
> 
> About that, I still haven't look at
> https://github.com/MozillaOnline/pc-sync-tool/
> But they are providing an addon very close to adb helper, they may have
> custom adb builds...
> Also they manage to ease the drivers issues, at least on windows by shipping
> drivers into the addon!!
> (DO WANT!)

It's pretty sad to me that Windows needs not only just drivers, but they are
specific to different phone OEMs...  There are similar tools from the Android
community too, like this one from XDA forums[1].  Even the udev rules are quite
silly.  Sigh.

> > With more effort, we could actually combine the approaches that each of
> > these addons takes today into one addon:
> > 
> > 1. If you are already running an ADB server (because you ran some "adb"
> > terminal command), we can try to connect to it and use that
> >   * This is like what happens today with ADB Helper
> >   * You can then freely mix terminal commands with using the AM
> > 2. If you have the "adb" command installed but the server is not running,
> > let's start it and use that
> >   * After that, this is now the same as case 1 above
> > 3. If you are not running an ADB server and do not have the "adb" command,
> > we start our own ADB server from the libadb code (so no installed adb is
> > needed)
> >   * This server would be exclusive to the AM
> >   * It would still disconnect / be mean to you if you do use "adb" from the
> > terminal, but case 1 & 2 should ensure that you don't have it
> 
> I would simplify to:
> 1. is there a server up and running, yes ?
>   > use it
> 2. no ?
>   > start our own, with our fixes

Well, I was just thinking that if you have the ADB command installed (somewhere
outside of our addons), that means you are more likely to run to your terminal
and attempt to use it.  So, if we start up our own server, it would interfere
with that.

Something else we could look into that I haven't really explored yet (but might
be worth testing) is: currently the ADB command kills the server if there is a
version mismatch, but the functionality that we need is quite basic.  We could
try to be more tolerant and just use any ADB server.  Though, that would be an
ADB client change, so it would need to be in libadb, or possibly the binary
version if we end up pulling up our changes to that one.

> Also, with the new design, we do not really need to watch for new devices,
> but just have the list of currently connected one at one precise time. It
> means that we won't have any more issue when our own server gets killed, we
> will just spawn a new one when the list is requested.

Yes, you are right...  Paul asked me to prototype what he would need, so this
was assembled somewhat quickly. :)

> So about your gecko patch, instead of emitting a refresh event, we would
> instead expose a Devices.list() method that would return the list of
> connected devices.

Well, it might be good to keep tracking devices when it's possible to do so. 
That way, we'd keep the device list closer to reality in the background.  It can
take some time for the ADB server to start up, check devices, etc.  In the
"ideal" / best path case, the server is already running and was tracking
devices, so there is nothing to do when you show the devices menu, as it already
has all the info it needs.

I still think I would use an event because the indirection allows the UIv2 to
request an updated list, and then the addon "replies" by registering /
unregistering devices with the Devices module.  We could still show the menu
immediately, and if some devices take a few seconds to be recognized, then they
appear in the menu after you've opened it, which seems okay.  I don't see a need
to require a list() method that returns a full set of devices directly.

[1]: http://forum.xda-developers.com/showthread.php?t=2091922

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Filter on 86b7095e-2bd0-499e-a704-d00f2524aeef / PAUL STOP SETTING QA CONTACT TO THE DEVTOOLS COMPONENT'S WATCHERS EMAIL FOR BUGS YOU FILE :)

Comment 16

[Paul Rouget [:paul]]

With all the things that happened recently (mostly: the fact that we're looking at wifi debugging, that we auto-install the ADB addon, and that it sounds possible to just include the ADB binaries in Firefox Desktop), what do we want to do about this bug?

Comment 17

[Paul Rouget [:paul]]

Comment on attachment 8405036 [details] [diff] [review]
Add device refresh button to App Manager. r=paul

see previous comment.

Comment 18

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

I guess let's close this for now.  We seem sufficiently happy with the ADB Helper mode for the moment (plus active work on WiFi too).

We can always take another look at this later if ADB Helper doesn't seem good enough for the USB device path.