Pause/resume button flickers when debugger first loads

RESOLVED FIXED in Firefox 32

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

Trunk
Firefox 32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8432481 [details]
debugger-flicker.gif

There is a weird glitch with the UI of the pause button the first time the debugger panel is opened.  It toggles between the pause and resume states quickly before landing on the pause (see gif).
(Assignee)

Comment 1

4 years ago
Also, the stepping arrows become active at the same time
(Assignee)

Comment 2

4 years ago
Some thoughts from #devtools:

> Has anyone else noticed the flicker between pause and resume states when the debugger first loads?

> The execution is paused while the debugger frontend attaches.

> A number of things need to be stable while the debugger server creates actors for things, sets breakpoints, etc.

> Also we need to get the list of sources once and then rely on onNewScript callbacks, but we need to make sure that no more scripts will appear until we're ready to handle them

> Would it be possible/practical to special case that initial pause and not have the button state change for it?  Like, not have the buttons listening for these events until after debugger is attached?

> yeah, might be.
(Assignee)

Comment 3

4 years ago
Also discussed initializing the debugger UI to the paused state to prevent the view from transitioning from running->paused->running and instead just going to paused->running.
How about just preventing changes to the button until after the initial resume?
How about just not changing the state of the buttons when the packet type is interrupt.

jlongster, fitzgen and I discussed this some time back on irc [0]. This will also remove the flicker in other cases like setting a breakpoint, etc.


[0] http://irclog.gr/#show/irc.mozilla.org/devtools/320538
(Assignee)

Comment 6

4 years ago
(In reply to Girish Sharma [:Optimizer] from comment #5)
> How about just not changing the state of the buttons when the packet type is
> interrupt.
> 
> jlongster, fitzgen and I discussed this some time back on irc [0]. This will
> also remove the flicker in other cases like setting a breakpoint, etc.
> 
> 
> [0] http://irclog.gr/#show/irc.mozilla.org/devtools/320538

Is the initial pause considered an interrupt or do we need to handle these two things separately?
The interrupt packets are not related to the initial attachment flickering.
(Assignee)

Comment 8

4 years ago
(In reply to Girish Sharma [:Optimizer] from comment #5)
> How about just not changing the state of the buttons when the packet type is
> interrupt.
> 
> jlongster, fitzgen and I discussed this some time back on irc [0]. This will
> also remove the flicker in other cases like setting a breakpoint, etc.
> 
> 
> [0] http://irclog.gr/#show/irc.mozilla.org/devtools/320538

There is some code indicating this already: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#477, but the flicker still happens (that function seems to still be called with "paused" and "resumed" events when setting a breakpoint).
We could check for pausePacket.why.type == "attached" and not go into the paused state, which I believe we already do for "interrupted" as mentioned above.

https://wiki.mozilla.org/Remote_Debugging_Protocol#Attaching_To_a_Thread
(Assignee)

Comment 10

4 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> We could check for pausePacket.why.type == "attached" and not go into the
> paused state, which I believe we already do for "interrupted" as mentioned
> above.
> 
> https://wiki.mozilla.org/Remote_Debugging_Protocol#Attaching_To_a_Thread

OK so in _update there is a comment indicating it is ignoring 'interrupted' events, but there will never be a case where aEvent === "interrupted".  When a breakpoint for instance, aPacket.why.type is "interrupted" but aEvent is "paused".

If I change the code to look for why.type == "interrupted" in the code, then the flicker goes away.  BUT it also stops updating the button when pressing the 'pause' button in the UI (this also comes through as event == "paused" why.type == "interrupted").

I'm still trying to figure out when why.type === "attached" (I don't see that come through when opening the debugger.  There is a call directly to this._update() in handleTabNavigation which seems to cause the flicker on page load, but it may also be needed for cleaning if it was paused on navigation.  I'll do a bit more investigation.
(Assignee)

Comment 11

4 years ago
Created attachment 8435249 [details] [diff] [review]
debugger-flicker-load.patch

This fixes the flicker on the initial page load.  Basically, on connect it is calling handleTabNavigation() which only calls _update().  We don't *want* the UI to be updated during connect.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fa1370e0addf.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8435249 - Flags: review?(nfitzgerald)
Attachment #8435249 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 12

4 years ago
Created attachment 8435273 [details] [diff] [review]
debugger-flicker-breakpoint.patch

Was thinking something like this for preventing the flicker on setting breakpoints, which still allowing interrupts for the resume button
Attachment #8435273 - Flags: feedback?(nfitzgerald)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8435273 [details] [diff] [review]
debugger-flicker-breakpoint.patch

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

::: browser/devtools/debugger/debugger-controller.js
@@ +488,1 @@
>      if (gTarget && (aEvent == "paused" || aEvent == "resumed")) {

One specific question is: do we want to lie to the gTarget and not emit the event in this case like it was (trying) to do before?  Or should we stick with the approach in this patch in which it just doesn't update the toolbar buttons.
Comment on attachment 8435273 [details] [diff] [review]
debugger-flicker-breakpoint.patch

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

::: browser/devtools/debugger/debugger-controller.js
@@ +473,5 @@
>     * Update the UI after a thread state change.
>     */
> +  _update: function(aEvent, aPacket) {
> +    // Ignore "interrupted" events, to avoid UI flicker. These are generated
> +    // by the slow script dialog and internal events  such as setting

Nit: "events            such"

@@ +478,5 @@
> +    // breakpoints. Pressing the resume button does need to be shown, though.
> +    let ignoreToolbarUpdate = aEvent == "paused" &&
> +                              aPacket.why.type == "interrupted" &&
> +                              !this.interruptedByResumeButton;
> +    this.interruptedByResumeButton = false;

Nit: Can you also add `this.interruptedByResumeButton = false;` in the constructor, so we don't fragment the shape tree?

@@ +488,1 @@
>      if (gTarget && (aEvent == "paused" || aEvent == "resumed")) {

I don't think we want to emit the event to the target because that would flash the debugger panel's tab green and bring focus to the devtools window. However, there might be tests that rely on these events, in which case I think we can push it off to a follow up.
Attachment #8435273 - Flags: feedback?(nfitzgerald) → feedback+
(Assignee)

Comment 15

4 years ago
Created attachment 8435384 [details] [diff] [review]
debugger-flicker-load-r=fitzgen.patch

Updated commit message
Attachment #8435249 - Attachment is obsolete: true
Attachment #8435384 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 1021365
(Assignee)

Updated

4 years ago
Attachment #8435273 - Attachment is obsolete: true

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e110c9dfcba0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
That last comment should have been bug 1018955.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.