Closed Bug 912898 Opened 8 years ago Closed 8 years ago

B2G: Don't kill adb (or lock the screen) if a debugger client is connected

Categories

(DevTools Graveyard :: WebIDE, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: paul, Assigned: fabrice)

References

Details

(Whiteboard: [needs-coverage])

Attachments

(1 file, 1 obsolete file)

Soon, we will be able to connect our tools to Firefox OS via adb.

Firefox OS kills adb on lock.

Can we not kill adb if a client is connected?
Blocks: appmgr_v1
So what we need to achieve this is to have the ability to query if a debug session is in progress. This only needs to be queryable from chrome.

I'm not familiar with the remote debugging code that runs on the server. Does anyone know where that can be found?
I said server and meant phone (although I'm guessing that they're the same)
The server keeps a map of current connections in DebuggerServer._connections, so you can count the active connections with Object.keys(DebuggerServer._connections).length.
Attached patch patch v1 (obsolete) — Splinter Review
With this patch we don't shutdown adb anymore when a debugging session is ongoing. It's probably good enough for most cases, but I would be slightly happier is there is a way to be notified by the DebuggerServer when connections are opened and closed. Panos, is that possible?
Assignee: nobody → fabrice
Attachment #800401 - Flags: feedback?(past)
Comment on attachment 800401 [details] [diff] [review]
patch v1

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

We should probably also disable the timer if a debug session is active. Something along the lines of:

if (enableAdb && !isDebugging)
  startTimer
else
  stopTimer
Comment on attachment 800401 [details] [diff] [review]
patch v1

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

We do have a connection notification mechanism in the client side, but not in the server side:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/connection-manager.js#58

However we do send a DOM event when the server is shutting down:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#36

Either of those approaches would work. Do we need this in this bug or as a followup?
Attachment #800401 - Flags: feedback?(past) → feedback+
Attached patch patch v2Splinter Review
Panos, I tried the window event, but it's firing before the DebuggerServer._connections list is updated. Instead I added a way to get notified of connections being opened or closed. Works fine for me, but let me know if you prefer that to be implemented differently.
Attachment #800401 - Attachment is obsolete: true
Attachment #801052 - Flags: review?(past)
Comment on attachment 801052 [details] [diff] [review]
patch v2

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

I would have preferred using EventEmitter to cope with multiple callbaccks, but I see that we haven't moved it to toolkit/ yet, so this should suffice for now.

::: toolkit/devtools/server/main.js
@@ +185,5 @@
> +
> +  _fireConnectionChange: function() {
> +    if (this.onConnectionChange &&
> +        typeof this.onConnectionChange === "function") {
> +      this.onConnectionChange(this);

Passing the whole DebuggerServer sounds too much. Why not pass a state parameter to _fireConnectionChange (e.g."opened", "closed") and let that propagate to the callback?
Attachment #801052 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/0b719ae93f4e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Dave, thanks a lot for you reactivity on this issue.
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.