Closed Bug 1338582 Opened 4 years ago Closed 4 years ago

implement debug server actors to draw paused overlay with controls

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: clarkbw, Assigned: pbro)

References

()

Details

Attachments

(1 file, 1 obsolete file)

We need a new debug server Actor / API for toggling an overlay that indicates to the user / developer that the current page is paused in the debugger and gives the user some controls.

Here's the design from helen:
https://cloud.githubusercontent.com/assets/2134/22383567/2ffd2ce6-e47f-11e6-984b-d3f5a0ac2e53.png

Coming from this original doc:
https://projects.invisionapp.com/share/AY8OEG3C6#/screens/117937554_Debugger

The main components are an Overlay + Toolbar which are shown whenever the debugger is paused.

The overlay is always present except when a user chooses the Inspector we should hide the overlay but persist the toolbar. I think helen's design had this overlay only showing when on the debugger tab but I don't think other tabs actually need access to the page and would be better served as a notification that the page is paused because the toolbar could be missed.  If someone were using the inspector and the page paused it might be nice to show the overlay and animate it away in the same way it would dissolve when you tab into the inspector.

The toolbar is always present on the page when the page is paused to help people control the paused state of the page.

Here are the components of the controls toolbar:

+---------------------------------------------------+
|   Message  | Continue | Step(Over|In|Out) | Close |
+---------------------------------------------------+

Message: is a display message that can be passed by the debugger client with a default message of ("paused in debugger").  This allows the debugger to be more specific by saying "Paused on Exception"; meaning the debugger will likely update this message a user steps or as new pause events happen.  Clicking on this message should open the debugger panel.

Continue + Step Controls are the standard debugger commands that should be passed directly to the debug server.

Close: This button should be considered the HALP! option.  I believe this likely means "Continue to the end" as opposed to the regular continue button would could just get you back into another paused state.

The debugger.html is tracking the client side work required to call this API here: https://github.com/devtools-html/debugger.html/issues/1743
Assignee: nobody → pbrosset
Priority: -- → P2
Patrick, I assigned this to you.  Do you think you have time to take this on?
Flags: needinfo?(pbrosset)
Sorry Bryan, I had not realized this was assigned to me. I'll create a WIP patch that shows how to get this done. It's really not that complex at all. The complex part will be having buttons in the toolbar that actually control the debugger. But this can be done in a separate patch. I'll upload one just for the toolbar with message and overlay first.
Flags: needinfo?(pbrosset)
I wonder if the inconsistency between the RDM toolbar and the paused state toolbar as seen in the designs was voluntary or simply exists because they were made at different times. I'm going to go for the RDM toolbar's design instead. This can be changed very easily, but I feel like the consistency would be beneficial here.
Attached patch paused-debugger-overlay.patch (obsolete) — Splinter Review
Here's the patch I was talking about.

So, this is only the actor-part. Nothing in the debugger actually uses it yet.

Here's what the code for using it would have to look like:

// You need a toolbox object for this to work.
let toolbox = ....;

// Instantiate a new highlighter, do this only once for the debugger.
toolbox.highlighterUtils.getHighlighterByType("PausedDebuggerOverlay").then(h => {
  // Store the instance somewhere to reuse it later.
  this.pausedStateOverlay = h;

  // Display the overlay, with a message.
  this.pausedStateOverlay.show(toolbox.walker.rootNode, {
    reason: "paused in debugger"
  }).then(...);

  // Just display the toolbar, with a message (not the overlay).
  this.pausedStateOverlay.show(toolbox.walker.rootNode, {
    reason: "paused in debugger",
    onlyToolbar: true
  }).then(...);

  // Hide the highlighter.
  this.pausedStateOverlay.hide().then(...);
});

I need to explain one weird trick: toolbox.walker.rootNode.

Highlighters were originally only made for the inspector. The first highlighter that ever existed was the Box-Model highlighter. It shows the box-model regions around a given node.
So, very simply, it took that node as the first argument of the show method.

Since then, a lot of different highlighters have been done, not all of them actually requiring a DOM node as context. But still, all of them belonging to the inspector somehow, so we kept that requirement of having to pass a DOM node as the first argument.
This requirement is important here because all highlighters share the same show method signature: show(node, options).

Changing this might be hard for backward-compatibility reasons. So I'd suggest not doing it in this bug.

The new highlighter in this patch doesn't need a DOM node to be passed, it only needs any node so that the show method doesn't complain, that's all.
So we can pass any node, and one that is easy to retrieve is toolbox.walker.rootNode.
This is always defined, even if you've only opened the debugger, and not the inspector, because the moment you call toolbox.highlighterUtils.getHighlighterByType, then walker is actually loaded.

So I guess we can live with this here.

Now I don't know how to get access to the toolbox from within debugger.html, but plugging this in should be rather simple.
Some ideas about how to implement the step/resume buttons:

We could simply pass a new option to the show method:

overlay.show(toolbox.walker.rootNode, {
  threadActor: toolbox.threadClient._actor
});

Then, in the show method implementation, in the highlighter, we could retrieve the thread actor like so:

let threadActor = this.env._tabActor.conn.getActor(options.threadActor);

Now, using this actor, we could resume: threadActor.onResume(); or do other things.

What I don't know yet is: will the debugger front-end just know that we stepped over/in/out or resumed? We're basically using the threadActor on the server, so not initiating the action from the front-end. So the front-end will need to know that this happened.

There is also a problem: when the script execution is paused, all events are blocked. This is how the debugger works. But that also means that clicking on a resume button that is inside the page (because that's how highlighters work, they are injected in the page) won't work!
This might be solvable with bug 1177346.
(In reply to Patrick Brosset <:pbro> from comment #3)
> I wonder if the inconsistency between the RDM toolbar and the paused state
> toolbar as seen in the designs was voluntary or simply exists because they
> were made at different times. I'm going to go for the RDM toolbar's design
> instead. This can be changed very easily, but I feel like the consistency
> would be beneficial here.

Helen referenced her debugger toolbar design when making the RDM toolbar, so I believe she made them similar intentionally.
Attachment #8853415 - Attachment is obsolete: true
Jason, I flagged you for review so it would give you a chance to see how highlighters are made in DevTools. It's a very simple patch too, but if you'd rather have someone else give the final R+ on this, feel free to forward to :gl or :zer0.

If you want to test it locally, do this:
- get the patch, build Firefox locally
- open Firefox, open DevTools (any panel)
- open scratchpad, and paste the following code in it:

/* -sp-context:browser */
var t = [...gDevTools._toolboxes][0][1];
t.highlighterUtils.getHighlighterByType("PausedDebuggerOverlay").then(h => {
  return h.show(t.walker.rootNode, {
    reason: "paused in debugger"
  });
}).catch(e => console.error(e));

- execute the code

You should see the overlay appear on top of the content page where you opened DevTools. It should have a message centered at the top.

I'm just suggesting to use scratchpad to test this, because there is nothing inside DevTools that uses this highlighter yet!
Comment on attachment 8853964 [details]
Bug 1338582 - New devtools highlighter for signaling paused state;

https://reviewboard.mozilla.org/r/125978/#review128682

Thanks, I reviewed this with :gl. It's nice to see the overlay come together.
Attachment #8853964 - Flags: review?(jlaster) → review+
It is not important for this version, but I prefer Helen's invision "top" bar designs over the consistency of an RDM styled overlay. 

https://github.com/devtools-html/debugger.html/issues/1743
Thanks Jason and Gabriel.
Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df12723df82dcee58ec1a546d626f2ef57f0afb&group_state=expanded
I'm not sure my chrome test got run however, I might need to change my try syntax.
New, more complete, try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c7a083098e24ffe190fa74e2175037d6a79ec51&group_state=expanded
Although it's not finished yet, the new test I added in this patch already passed on at least one platform.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ddfcf35be96
New devtools highlighter for signaling paused state; r=jlast
https://hg.mozilla.org/mozilla-central/rev/0ddfcf35be96
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1353564
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.