Closed Bug 799151 Opened 8 years ago Closed 7 years ago

Display a prompt to allow remote debugging connections in Firefox OS

Categories

(DevTools :: Debugger, defect, P2)

All
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

RESOLVED FIXED
Firefox 21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: past, Assigned: fabrice)

References

Details

(Keywords: late-l10n, Whiteboard: [b2g])

Attachments

(2 files, 2 obsolete files)

The prompt to allow or deny remote connections that landed in bug 758696 did not work for B2G, since the required platform API was missing, and we decided to implement it later. Vivien had posted a rough sketch of how this should be implemented in bug 758696 comment 8.
Need someone to implement this in B2G. GPS or maybe another Gaia dev?
Priority: -- → P2
blocking-basecamp: --- → ?
QA Contact: fabrice
Assignee: nobody → fabrice
blocking-basecamp: ? → +
QA Contact: fabrice
Attached patch gecko patch (obsolete) — Splinter Review
This adds support for sending a debugger prompt request to gaia, and wait for the answer. The callback in Debugger.init() is a synchronous method, so we have to run the event loop there.
Attachment #699098 - Flags: review?(21)
Attached patch gaia patchSplinter Review
Gaia prompt, note that I had to add a new string.
Attachment #699099 - Flags: review?(21)
Keywords: late-l10n
Comment on attachment 699098 [details] [diff] [review]
gecko patch

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

::: b2g/chrome/content/shell.js
@@ +865,5 @@
> +    if (!DebuggerServer.initialized) {
> +      // Allow remote connections.
> +      DebuggerServer.init(this.prompt.bind(this));
> +      DebuggerServer.addBrowserActors();
> +      //DebuggerServer.addActors('chrome://browser/content/dbg-browser-actors.js');

How come this is commented? These are the b2g-specific overrides necessary to get the debugger to work in Firefox OS.
Attachment #699098 - Flags: review?(21)
Attached patch gecko patch v2 (obsolete) — Splinter Review
Using #ifndefs in dbg-server.js didn't work (this prevented the debugger to start at all, and I tried many combinations).

With this patch, a successful connection to the devices doesn't offer any tabs or process in firefox desktop UI.
Attachment #699098 - Attachment is obsolete: true
Attachment #699270 - Flags: review?(past)
Comment on attachment 699270 [details] [diff] [review]
gecko patch v2

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

Hmm, weird. I wonder what was the error output.
This change will also work for what this patch is about, but will break the debugger server extensibility that you need for the app installation functionality that we discussed. If that's not a goal for now, I'm fine with it.
Attachment #699270 - Flags: review?(past) → review+
Ha no, that's not fine then. Can you investigate how to disable what we don't need? Meanwhile I'll make progress on our app installation actor.
Sure, I'll take a look tomorrow.
Comment on attachment 699099 [details] [diff] [review]
gaia patch

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

r+ with nit.

::: apps/system/js/remote_debugger.js
@@ +14,5 @@
> +      if (e.detail.type !== 'remote-debugger-prompt') {
> +        return;
> +      }
> +
> +      console.log('Got remote-debugger-prompt event');

Remove this extra console.log
Attachment #699099 - Flags: review?(21) → review+
Attached patch gecko patch v3Splinter Review
This does the trick. Now the only available actor in desktop-b2g is the profiler actor.
Attachment #699270 - Attachment is obsolete: true
Attachment #699647 - Flags: review?(21)
Comment on attachment 699647 [details] [diff] [review]
gecko patch v3

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

I will hold the review until you confirmed that the process can be closed in the described conditions.

::: b2g/chrome/content/shell.js
@@ +849,3 @@
>  
> +    while(!this._promptDone) {
> +      Services.tm.currentThread.processNextEvent(true);

Can you make sure this won't prevent the b2g process to be closed if there is a prompt opened and the user long press the home button in order to display the system menu and press restart?

@@ +855,5 @@
> +  },
> +
> +  handleEvent: function debugger_handleEvent(detail) {
> +    RemoteDebugger._promptAnswer = detail.value;
> +    RemoteDebugger._promptDone = true;

RemoteDebugger -> this
Attachment #699647 - Flags: review?(21)
Comment on attachment 699647 [details] [diff] [review]
gecko patch v3

Fabrice proves me that it works, so r+ with the small nit.
Attachment #699647 - Flags: review+
Marking FIXED as this is landed on inbound and b2g18, and for gaia as well.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Note to future readers: the patch in this bug has also disabled the web console and JS debugging in B2G.
Target Milestone: --- → Firefox 21
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.