Closed
Bug 1253902
Opened 8 years ago
Closed 8 years ago
[DevTools][Debugger] Cannot add conditional breakpoint
Categories
(DevTools :: Debugger, defect, P1)
DevTools
Debugger
Tracking
(firefox45 affected, firefox46 affected, firefox47 affected, firefox48 fixed, firefox-esr45 affected)
RESOLVED
FIXED
Firefox 48
People
(Reporter: magicp.jp, Assigned: jlong)
References
Details
Attachments
(3 files, 1 obsolete file)
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160305030241 Steps to reproduce: 1. Start Nightly (reproduce in Firefox 45 or later version) 2. Go to any sites (e.g. about:home) 3. Open DevTools > Debugger 4. Select a source item 5. Add conditional breakpoint using right-click context menu on line number Actual results: Non-conditional breakpoint is added. If right-click on current line, conditional breakpoint is added on wrong line. And also cannot added two or more conditional breakpoints. Expected results: "Add Conditional Breakpoint" works correctly, and can add two or more conditional breakpoints.
Regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=15522bc2931a52ec435f22e233f747264b90f647&tochange=0e47cb06470145725732f0f362c46048969126d8
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr45:
--- → affected
Component: Untriaged → Developer Tools: Debugger
Depends on: 1200798
Flags: needinfo?(jlong)
OS: Unspecified → All
Hardware: Unspecified → All
Version: 45 Branch → unspecified
Assignee | ||
Comment 2•8 years ago
|
||
I can't reproduce anything except the main symptom which is that right-click > add conditional breakpoint does not open the popup to enter a condition expression. The way conditional breakpoints work is it's considered a conditional breakpoint if the breakpoint has a conditional expression. So I just need to figure out why the popup isn't coming up (if you don't enter an expression in the popup, it's considered a normal breakpoint). No idea why you can't add more than 1 or things like that.
Flags: needinfo?(jlong)
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33acdb5d80a1
Hi James, I have uploaded two videos. If you have any questions, please let me know.
Assignee | ||
Comment 7•8 years ago
|
||
Thanks! Yeah, the only problem is that the popup doesn't open. This patch should fix that. I'm not sure why it opens that one time; it must be some interaction with the cursor. The current patch should make it always open.
Comment 8•8 years ago
|
||
Basic functionality that should just work, and James already has a patch for it, so marking this as P1.
Priority: -- → P1
Assignee | ||
Comment 9•8 years ago
|
||
The tests were failing. I fixed them, the problem was that their weren't properly waiting for requests to be resolved. The behavior before didn't expose this because state happened to be what we were checking. I did have to make one semantic change: if you add a breakpoint with an empty condition, disable and re-enable the breakpoint, it stays as a conditional breakpoint. The code is otherwise super confusing to try and "remove" the empty condition, and I deemed it not worth it. It makes more sense to me anyway... why would the breakpoint type change? If you really want it to change just remove it and add a new breakpoint.
Attachment #8727943 -
Attachment is obsolete: true
Attachment #8730882 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62eaf32f4185
Comment 11•8 years ago
|
||
Comment on attachment 8730882 [details] [diff] [review] 1253902.patch Review of attachment 8730882 [details] [diff] [review]: ----------------------------------------------------------------- This is one of those subtle edge cases where code that seems to be correct at first glance is actually wrong (due to the falsyness of ""). I think it would be good practice if we started adding comments when we fix these edge cases, to explain what the problem was, and how we fixed it. Patch looks good to me otherwise, so I'll leave it up to you how much commenting you think is prudent. ::: devtools/client/debugger/content/reducers/breakpoints.js @@ +11,5 @@ > const initialState = Immutable({ > breakpoints: {} > }); > > +function stringOr(...args) { This name is not particularly descriptive of what this function does. It's not a big deal, since it's a small function, but I still think a one line comment about what this function does would be helpful. @@ +12,5 @@ > breakpoints: {} > }); > > +function stringOr(...args) { > + for(var arg of args) { Nit: space between for and ( @@ +13,5 @@ > }); > > +function stringOr(...args) { > + for(var arg of args) { > + if(typeof arg === "string") { Nit: space between if and ( @@ +32,5 @@ > > state = setIn(state, ['breakpoints', id], bp.merge({ > disabled: false, > loading: true, > + condition: stringOr(action.condition, bp.condition) If I were new to this code and not familiar with the subtle problem that stringOr solves here, I might be tempted to change it back. Perhaps a comment here to explain the issue would be useful? ::: devtools/client/debugger/test/mochitest/browser_dbg_server-conditional-bp-02.js @@ +114,5 @@ > gDebugger.document.querySelectorAll(".dbg-breakpoint")[aIndex], > gDebugger); > } > > + function waitForConditionUpdate() { If you're using this in multiple tests it should probably be moved to head.js.
Attachment #8730882 -
Flags: review?(ejpbruel) → review+
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/90b4e3e5d0f4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jlong
Comment 14•8 years ago
|
||
I have reproduced this bug with Nightly 47.0a1 (2016-03-05) on Windows 7, 64 Bit! The bug's fix has been verified in latest Nightly, Developer Edition, and Beta. Build ID 20160714030208 User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID 20160708004052 User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID 20160714050942 User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•