Closed Bug 1258821 Opened 8 years ago Closed 8 years ago

Cannot open the expression panel of conditional breakpoint

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: magicp.jp, Assigned: jlast)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160322030417

Steps to reproduce:

1. Start Nightly
2. Go to any sites (e.g. about:home)
3. Open Debugger
4. Add two or more conditional breakpoint
5. Open the expression panel selecting conditional breakpoint detail
6. Select next conditional breakpoint detail remain opening the expression panel


Actual results:

Cannot open the expression panel of conditional breakpoint. (cannot keep open, because immediately close automatically)

Reproduce on Nightly, but other versions will have same issue. Because "Add conditional breakpoint" does not work correctly in other versions now.


Expected results:

The expression panel should open when selecting conditional breakpoint detail.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Debugger
OS: Unspecified → All
Hardware: Unspecified → All
This looks like a regression. Jlast, could this be fallout from bug 900763?

In any case, this should be fixed asap. Marking as p1.
Flags: needinfo?(jlaster)
Priority: -- → P1
Looking into it now ejbruel. It seems likely that bug 900763 caused it.
Flags: needinfo?(jlaster)
Actually ejbruel, magicp, it looks like the regression was masked by bug 1256704, which was breaking conditional breakpoints entirely. I reverted the patch from 900763 and the regression was still there. 

I'm going to continue to look into the bug and see if I can get to the bottom of it.
I believe the issue is the popup's opening/closing animation. Basically, we open the second popup immediately, and when the first popup is done animating away we close both of them. 

One way to see this happening is to set a breakpoint here: 
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/content/views/sources-view.js#L687
and evaluating _cbPanel.getAttribute('animation')

This patch fixes it https://gist.github.com/jasonLaster/2768b58da831460e7f57, but uses a hard coded value. I'm going to see if I can observe the animation attribute changing, which avoid the hard coded value.
Attached patch cpopup.1.patch (obsolete) — Splinter Review
Problem: the conditional breakpoint panel is shared between breakpoints. When you try to see a breakpoint expression and the panel is already open it breaks. (see attachment 1 [details] [diff] [review]). It breaks because the panel is animating out while attempting to show the new expression at the same time.

The fix is to wait on opening the panel until the panel has been hidden. 

Note, I'm using the `_cbPanel.hidden` property to check if the panel is open or closed. Previously, the hidden property did not have a default. Also, it was set to hidden immediately. This patch fixes those things so the property is reliable.
Attachment #8735440 - Flags: review?(jlong)
Comment on attachment 8735440 [details] [diff] [review]
cpopup.1.patch

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

Looks good! I only looked at the general idea and I assume that the flow works as it should.

::: devtools/client/debugger/content/views/sources-view.js
@@ +696,5 @@
> +      cbPanel.removeEventListener('popuphidden', openPopup, false);
> +    }
> +
> +    // Wait until the other cb panel is closed
> +    if (!this._cbPanel.hidden) {

You assigned `this._cbPanel` to a variable above so you could use that here also
Attachment #8735440 - Flags: review?(jlong) → review+
You should assign yourself to bugs that you work on :)
Assignee: nobody → jlaster
> You assigned `this._cbPanel` to a variable above so you could use that here also

Yep, that's correct `this._cbPanel.hidden` was not reliable the way it was setup before.
Keywords: checkin-needed
has problems to apply:

patching file devtools/client/debugger/content/views/sources-view.js
Hunk #1 FAILED at 59
Hunk #3 FAILED at 144
2 out of 5 hunks FAILED -- saving rejects to file devtools/client/debugger/content/views/sources-view.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh cpopup.1.patch
Flags: needinfo?(jlaster)
Keywords: checkin-needed
Attached patch cpopup.2.patchSplinter Review
Attachment #8733520 - Attachment is obsolete: true
Attachment #8735440 - Attachment is obsolete: true
Flags: needinfo?(jlaster)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/646cc46e3cfc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.