Closed Bug 1026945 Opened 6 years ago Closed 4 years ago

ADB addon helper should start scanning for devices only when WebIDE starts

Categories

(DevTools :: WebIDE, defect)

x86_64
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: paul, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

There is no need to do that when Firefox starts.
Where's a good place to hook this in?  I need something similar for WiFi, but I think for that case I may only want to scan when the runtime popup is opened...?  Perhaps ADB vs. WiFi will take different approaches, just something to think about.
(In reply to J. Ryan Stinnett [:jryans] from comment #1)
> Where's a good place to hook this in?

When we start the App Manager?
Or even only when the panel shows up.

We should start ADB when we open the panel, and stop it when closed. I believe it would solve bug 1031229.
This is quite annoying, because I always have to mentally filter out the ADB stuff from the terminal logs when debugging. Whoever fixes this gets a free beverage of their choosing from me in the next work week!
(In reply to Panos Astithas [:past] from comment #4)
> This is quite annoying, because I always have to mentally filter out the ADB
> stuff from the terminal logs when debugging. Whoever fixes this gets a free
> beverage of their choosing from me in the next work week!

Paul silenced some of the logs recently... but it seems some are still around.  Filed bug 1060148 to clean those up.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Summary: ADB addon helper should start scanning for devices only when the appmanager starts → ADB addon helper should start scanning for devices only when WebIDE starts
Duplicate of this bug: 1149794
Blocks: 1114380
Attached file GitHub PR
Attachment #8612498 - Flags: review?(poirot.alex)
Comment on attachment 8612498 [details] [review]
GitHub PR

Miss a file, but that looks good.
Attachment #8612498 - Flags: review?(poirot.alex)
Comment on attachment 8612498 [details] [review]
GitHub PR

Issues from PR addressed.
Attachment #8612498 - Flags: review?(poirot.alex)
Comment on attachment 8612498 [details] [review]
GitHub PR

Looks good, just see my comment about fastboot.
Attachment #8612498 - Flags: review?(poirot.alex) → review+
Hmm, but I bet the fastboot add-on also wants ADB devices too, to detect your device and reboot it into fastboot mode if needed.

:gerard-majax, any thoughts here?  We'd like to avoid starting the ADB server, etc. until we know it's needed.  For WebIDE, we have a flag we can check to see if WebIDE has been used.  But, that makes things harder for your own installer add-on.  Any ideas on a good way to delay starting ADB / fastboot that also works for your use case?
Flags: needinfo?(lissyx+mozillians)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Hmm, but I bet the fastboot add-on also wants ADB devices too, to detect
> your device and reboot it into fastboot mode if needed.
> 
> :gerard-majax, any thoughts here?  We'd like to avoid starting the ADB
> server, etc. until we know it's needed.  For WebIDE, we have a flag we can
> check to see if WebIDE has been used.  But, that makes things harder for
> your own installer add-on.  Any ideas on a good way to delay starting ADB /
> fastboot that also works for your use case?

I don't understand anything: how WebIDE will detect ADB devices in this case?
I'm relying on Devices to play with ADB and Fastboot. For fastboot we do polling and the addon initiates it.
Flags: needinfo?(lissyx+mozillians)
Okay, I think I will rework this as :gerard-majax suggested, so that WebIDE signals to the add-on that's it's time to start, instead of the add-on depending on WebIDE.

This way, it's more flexible for other consumers to start / stop for ADB / fastboot.
Comment on attachment 8612498 [details] [review]
GitHub PR

Okay, I am now waiting for events like the fastboot case.

This means we'll have to require Firefox 41+ for the new add-on version, since only those Firefoxes have WebIDE which knows the add-on is lazy.
Attachment #8612498 - Flags: review+ → review?(poirot.alex)
Attachment #8623326 - Flags: review?(poirot.alex) → review+
Comment on attachment 8612498 [details] [review]
GitHub PR

See my comment about keeping FF41- compatibility.
I hope that makes sense and we can prevent breaking it.
Attachment #8612498 - Flags: review?(poirot.alex) → review+
Keywords: leave-open
Comment on attachment 8612498 [details] [review]
GitHub PR

Alex, could you sanity check my changes for 41 and earlier?  (The lazy loading support landed as part of Firefox 42.)
Attachment #8612498 - Flags: review+ → review?(poirot.alex)
Comment on attachment 8612498 [details] [review]
GitHub PR

We should still support FF38. I think that's fine to keep running adb there, instead of breaking support on the current release.
Attachment #8612498 - Flags: review?(poirot.alex) → review+
Updated per comments in PR.  I have tested that ADB works on: 38, 40, and 42, which covers all ways of starting ADB.
Blocks: 1179771
Add-on changes merged:

https://github.com/mozilla/adbhelper/commit/87786f6076c459602608f2c12a7e1be26f38d9d9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.