1) Start stepping from some breakpoint 2) Hover a variable name to get a small "tooltip"-style popup showing the value 3) Press [F11] Bug: the highlight showing what line one is on does not change. Stepping does actually happen but the source view doesn't update correctly. There are two possible implementations: 1) Close tooltip, keep stepping (simpler) 2) Keep tooltip open, update its contents if the contents of that variable changes (more powerful and probably harder to get right)

(Testing with dev tools in yesterday's nightly, BTW)

I don't know if it is related, or how long the problem has been there, but on today's nightly I can very easily reproduce a problem on MacOS where drop-downs like the awesome bar suggestions or bugzilla flag drop-downs hang in the wrong monitor. In my case I'm using a MB Air and one external monitor.

I posted this on the mailing list but had no response so far: I'd like to kill bug 967853 so I've been browsing code a bit. So the bug is that the "tooltip" you see when pointing at something when stopped at a breakpoint doesn't go away if you then step onwards. Because I combine mouse and keyboard usage of the debugger a lot, this annoys me all the time. I think the closeOnEvents: [{...}] part of this code: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#1881 is responsible for closing the popup. I think if that event handling could be hooked up with thread-resumed events, like for example this code: this.target.on("thread-resumed", this.updateDebuggerPausedWarning); from http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#169 does, we might have a fix. But I don't know exactly how to hook those two bits together. Could someone help?

See here for how closeOnEvents is used: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js#L154 I think adding an array element like this should do it: { emitter: this.target, event: "thread-resumed" }

Past: thank you, but trying to add that causes this error in the browser console when I try to open the debugger: DebuggerPanel.prototype.open threw an exception: TypeError: invalid 'in' operand emitter Stack: Tooltip@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/Tooltip.js:221:1 VariableBubbleView.prototype.initialize@chrome://browser/content/devtools/debugger-panes.js:1881:21 DebuggerView.initialize@chrome://browser/content/devtools/debugger-view.js:61:5 DebuggerController.startupDebugger<@chrome://browser/content/devtools/debugger-controller.js:163:11 <snipping the rest of the stack> Patrick, David: I needinfo you because according to hg blame you authored or edited code in these two sections. Can you help me connect these dots, somehow?

(I assume that all I really need to learn is what the "emitter" property should be set to..)

That's because |this.target| is undefined where |closeOnEvents| is called. Try |DebuggerController._toolbox.target| instead.

That worked :)

Comment on attachment 8551630 [details] [diff] [review] b967853.patch Review of attachment 8551630 [details] [diff] [review]: ----------------------------------------------------------------- Looks good (apart from a minor formatting comment bellow). I think we need a new test for this though, to avoid any future regressions. There are *many* tests in /browser/devtools/debugger/test/, some of which deal with the variable tooltip and others with checking that stepping works properly, so it should be relatively easy to create a new test that does both :) I'm going to NeedInfo? Victor so he can point you to a couple of tests for you to take a look at. ::: browser/devtools/debugger/debugger-panes.js @@ +1887,5 @@ > event: "scroll", > useCapture: true > + }, { > + emitter: DebuggerController._toolbox.target, > + event: "thread-resumed" The indentation is off on these 2 lines, please make sure they are formatted like the ones above.

Victor, as said in comment 12, can you give Hallvord a few references to debugger tests that deal with variable tooltip and stepping through code.

ws fix

browser_dbg_variables-view-popup-* tests deal with the variable tooltip. Stepping through code is nicely exemplified in browser_dbg_pause-resume.js.

I'm still trying to make this test work. I don't understand how to make it wait until the "step into" command is done. This version (lots of copy'n'paste code) times out: http://pastebin.mozilla.org/8308904 (partial, omitted variable declarations and such) but if I remove the promise stuff and .then() everything fails even though it should pass.

The test is further simplified from that pastebin, but still doesn't work. Copied from IRC: 2:46 PM <hallvors> So, the problem is: I don't know the best way to express "wait until the debugger has done whatever it needs to do when you click 'Step' button, then complete the test". See http://pastebin.mozilla.org/8362484 line 26 for the current version, but I've tried a few 2:46 PM <hallvors> like 2:46 PM <hallvors> return promise.all([ 2:46 PM <hallvors> waitForDebuggerEvents(panel, gDebugger.EVENTS.FETCHED_SCOPES) 2:46 PM <hallvors> ]); 2:46 PM <hallvors> also timed out for reasons I couldn't quite understand because in the log it looked as if that event was sent Victor, any input welcome :)

Depends on what you want to wait for. If you simply want to wait for the line to change, use `waitForCaretUpdated` or `ensureCaretAt`. In this particular case, I think you want to wait for the caret and the variables view (in the pane, not in the popup) to update, so `waitForCaretAndScopes` is appropriate. See head.js for more info.

Comment on attachment 8559534 [details] [diff] [review] b967853.patch - now with test Review of attachment 8559534 [details] [diff] [review]: ----------------------------------------------------------------- I think Victor should be reviewing this new test.

Comment on attachment 8559534 [details] [diff] [review] b967853.patch - now with test Review of attachment 8559534 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments below addressed. ::: browser/devtools/debugger/test/browser_dbg_variables-view-popup-17.js @@ +22,5 @@ > + let bubble = gDebugger.DebuggerView.VariableBubble; > + let tooltip = bubble._tooltip.panel; > + > + // Always expand all items between pauses except 'window' variables. > + gVariables.commitHierarchyIgnoredItems = Object.create(null, { window: { value: true } }); Is this really needed? It looks like this test is a clone of browser_dbg_variables-view-reexpand-01.js Since we're not testing nodes expanding again in this test, this should be removed. @@ +30,5 @@ > + .then(() => ensureThreadClientState(gPanel, "resumed")) > + .then(pauseDebuggee) > + .then(() => openVarPopup(gPanel, { line: 20, ch: 17 })) > + .then(() => is(tooltip.querySelectorAll(".devtools-tooltip-simple-text").length, 1, > + "The popup should be open with a simple text entry")) Since the .then() is not returning a promise here, why have it as a separate method? @@ +79,5 @@ > + ok(!bubble._markedText, > + "The marked text in the editor was removed."); > + > + is(tooltip.querySelectorAll(".devtools-tooltip-simple-text").length, 0, > + "On stepping, the variable popup should close and its elements should no longer be in the DOM."); This is redundant, since we're checking for bubble._tooltip.isEmpty anyway a few lines above.

I'm not that familiar with Mercurial..and it's 4:30 AM.. so apologies for only managing to create another patch file that will fix the review comments if applied on top of the first one..

tried the first patch and it didn't apply cleanly: patching file browser/devtools/debugger/test/browser.ini Hunk #1 FAILED at 539 1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/debugger/test/browser.ini.rej could you take a look? thanks!

Please try this one - rebased and unified into a single patch.

Backed out for browser_dbg_variables-view-popup-10.js failures. Please verify that this is green on Try before requesting checkin next time. https://hg.mozilla.org/integration/fx-team/rev/58eed354d2a9 https://treeherder.mozilla.org/logviewer.html#?job_id=1954176&repo=fx-team

For the record, those tests failed because my patch unearthed a new bug in the "show eval code in debugger" fix which wasn't covered by the existing tests but now got caught by browser_dbg_variables-view-popup-10.js. When bug 1132443 gets fixed, that test should start passing even with my patch included.

How about trying it now? That bug should be fixed

Some code was moved, had to change the patch a bit, but all my code should be exactly the same so I don't think it needs more reviewing.. New try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5adc158cdac

(That try push apparently failed building.. no idea why. it builds fine locally, and a minor JS file change doesn't seem like something that would break a build..)

So, what do I do now?

The log says it all: 09:18:11 INFO - The error occurred while processing the following file or one of the files it includes: 09:18:11 INFO - /builds/slave/try-l64-0000000000000000000000/build/src/browser/devtools/debugger/moz.build 09:18:11 INFO - The error occurred when validating the result of the execution. The reported error is: 09:18:11 INFO - Test manifest (/builds/slave/try-l64-0000000000000000000000/build/src/browser/devtools/debugger/test/browser.ini) lists test that does not exist: browser_dbg_variables-view-popup-17.js You apparently forgot to add the new test to the updated patch :) Just push the fixed patch to try and set the checkin-needed flag once the result is green.

*sigh* - I missed the "log" button in the UI.. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba4ecb8dd7cd

@jlongster: perhaps you missed this comment? https://bugzilla.mozilla.org/show_bug.cgi?id=1132443#c6 What my fix here is aiming to do is to make sure the variable tooltip closed when we step in the debugger. I have tried to do so by making it listen to the thread-resumed event: emitter: DebuggerController._toolbox.target, event: "thread-resumed" Now, this doesn't work as expected - apparently because stuff the debugger does internally also makes the thread-resumed event fire. I thought it would only fire when a page script resumes. Using the RDP inspector, setting a variable watch and clicking another variable to show a popup when stopped at a breakpoint gives me this traffic when the variable is clicked: Sent clientEvaluate Received (object data) Received resumed Received newSource Received newSource Received paused I guess that "Received resumed" causes the thread-resumed event in the client code? Net result is that my fix hides the popup randomly because thread-resumed doesn't work as I expected. (In contrast, if I type something into the console, it does not use clientEvaluate. Traffic goes: Sent evaluateJSAsync Received (unnamed message with resultID) Received newSource Received evaluationResult ) Does the server really have to send "resumed" and "newSource" back to the client after a clientEvaluate message?

I'm not familiar enough with how watches and the tooltips work to say off-hand. I don't think the server should send arbitrary resumed events, and what might be happening is the frontend is calling `interrupt` and resuming itself to perform certain tasks. I can take a look at this in more depth but it's going to have to be later this week.

A resumed event comes in when the frontend invokes `clientEvaluate`: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L1261 The console uses a different evaluate command (in the console). I can't really comment on if it should do that or not, because it was like that before I started working on it. I think that is confusing, and we can work on making these events easier to comprehend. I don't know much about the tooltip API either, but it looks like you need to check to see if it resumed from a pause event that was actually a breakpoint. I can help work on this patch later this week if you need. I don't know off-hand how to do that with that "closeOnEvents" list, which is pretty restrictive.

(In reply to James Long (:jlongster) from comment #37) > The console uses a different evaluate command (in the > console). I can't really comment on if it should do that or not, because it > was like that before I started working on it. This was because we didn't want to start the debugger server in the past, since it was slow. Therefore, a faster eval method for the console was added to circumvent that. Not sure if it's still the case though.

(In reply to Victor Porof [:vporof][:vp] from comment #38) > (In reply to James Long (:jlongster) from comment #37) > > The console uses a different evaluate command (in the > > console). I can't really comment on if it should do that or not, because it > > was like that before I started working on it. > > This was because we didn't want to start the debugger server in the past, > since it was slow. Therefore, a faster eval method for the console was added > to circumvent that. > > Not sure if it's still the case though. Ah that's some good history, thanks! We don't need to go and change how all that works right now. I'm fine just getting a fix in for this bug and when we have time maybe we can take a look at these interactions. You might need to manually close the tooltip in the `_onResumed` event? I'm not sure yet.

Could watch and variable popup functionality use evaluateJSAsync instead of clientEvaluate? Should I report a separate bug saying the backend should not send resumed and paused events when running clientEvaluate scripts? (We might of course find some functionality that relies on these events if we stop sending them.. :-/ but they seem more likely to cause confusion)

It feels wrong to use evaluateJSAsync just because you want different events. We should look at if we really need to send those resume events. Looks like the only other time evaluate() is called (which calls clientEvaluate) is for handling conditional breakpoints, but those are handled server-side now. They are handled client-side when connecting to an old server, but I wonder if it's been long enough that we can remove that now. We may need to send the resume event because we send another paused event after we're done evaluating. That paused event contains the result of our eval call. The frontend might expect resumed->paused events to come in order, which is why we need that. I need to catch up on some other bugs before I look at this more deeply (need to land a few by the end of this week), but I'll come back to this if it hasn't been solved.

(In reply to Hallvord R. M. Steen [:hallvors] from comment #40) > Could watch and variable popup functionality use evaluateJSAsync instead of > clientEvaluate? > I don't know. I don't remember having `evaluateJSAsync` when the tooltip was written, and I don't know enough about `evaluateJSAsync`. I believe it is "correct" to send paused and resumed events when evaluating, because that's what's actually happening. The debugger needs to pause in order to eval something. Whether or not we want to expose this to consumers is a different discussion, but masking complexity might be a good idea in this case.

(In reply to Victor Porof [:vporof][:vp] from comment #42) > I believe it is "correct" to send paused and resumed events when evaluating, > because that's what's actually happening. The debugger needs to pause in > order to eval something. (Just for clarity: the problem here sort of is the opposite - we *are* paused, but when the clientEvaluate command is sent the backend seems to respond with a "resumed" event followed by another "paused". I guess whether considering that "correct" depends on what you believe the semantics of the "resumed" event to be: to let you know that the thread you're pausing/stepping in resumed, or to let you know that the backend engine "resumed" to do whatever work it might be asked to do by some other element in the system. I'm pretty sure that whoever wrote the code that picks up this event and dispatches it as a "thread-resumed" event inside the debugger client expected the former semantics..)

By definition, evaluating code in the debuggee will have to resume execution and then pause it again, once the result is available. That's what both the debugger and console do. The console however (both the new evaluateJSAsync and the old evaluateJS API) doesn't bother to inform the client of that fact, because it doesn't keep state that would be affected by it (or at least that's what I believe my thinking was when I reviewed this). The debugger however is probably already showing the current values of every variable in scope and every global, too. Evaluating an expression could have side effects that the user would expect to see in the variables view, so simply sending the resume and pause events that are actually happening let's the client do the right thing. I don't know if the debugger frontend has changed drastically recently and I've only spent 10 minutes looking at this, but I hope this helps.

(In reply to Panos Astithas [:past] from comment #44) > Evaluating an expression could have > side effects that the user would expect to see in the variables view, so > simply sending the resume and pause events that are actually happening let's > the client do the right thing. Frankly, if watching a variable or hovering something has side-effects, that would be a really severe bug.. The expressions we're talking about are evaluated because the debugger client/frontend needs information about watched or hovered variables. It's sort of an artifact of the implementation that this is done by evaluating some script at all - is the protocol missing an "inspect object" feature or something? (On the other hand, if I evaluate something that *has* side effects in the console, those effects are not show anywhere. For example, go to a site, set a breakpoint in code - top frame in call stack, this is global object. Expand "this" and add a variable watch for "foo" (should show a "not defined" error). Now open the console below the debugger and type "var foo=1". Neither the properties panel nor the watch list updates..) So - I still respectfully disagree that what currently happens is "the right thing". Actually it seems like a double bug now - the resumed event fires when no variables should need updating but doesn't fire when they might.. ;) However, if the resume event I'm arguing against is considered a feature and not a bug - how would you suggest fixing *this* issue? Since I can't rely on thread-resumed - how am I supposed to detect that the user starts stepping through code from the code that shows/hides the popup?

Imagine hovering over myDb.connHandler, which is implemented as a getter property. In order for the debugger to show you the value, it has to make the page execute the getter and generate a result. That getter would see if the DB connection is already established and return the handler, but if it isn't, it could conceivably try to connect and then return the new handler. At that point a whole bunch of stuff has happened to the page, (probably) unbeknownst to the user. You can't discount that as an implementation artifact, as there is no other way to actually compute the return value. This is what the resume & paused events are trying to help with: give the debugger the opportunity to update the value of, say a myDB.connectionCount, or whatever. As you rightly point out however, the console doesn't do this unfortunately and it is very visible now with the split console (not saying that it couldn't have been experienced before the split console existed, I just didn't think of it when reviewing the console evaluation patch). This is indeed a bug to fix: evaluating an expression should also send resume/pause events so that the debugger, if open, can update properly. Unfortunately I don't understand what is preventing the resumed event from working for this case, but I believe James is going to look into it this week and he knows what changed in the debugger frontend lately. If not, I'm happy to dive into it.

(In reply to Panos Astithas [:past] from comment #46) > Imagine hovering over myDb.connHandler, which is implemented as a getter > property. In order for the debugger to show you the value, it has to make > the page execute the getter and generate a result. Which is the most irritating design-choice/flaw in the debugger. /rant

(In reply to Russ from comment #47) > (In reply to Panos Astithas [:past] from comment #46) > > Imagine hovering over myDb.connHandler, which is implemented as a getter > > property. In order for the debugger to show you the value, it has to make > > the page execute the getter and generate a result. > > Which is the most irritating design-choice/flaw in the debugger. > > /rant What else could it do? The only thing I can think of is to just not support getters. I'm open to that but I'm just thinking off-hand right now and we'd need to think more seriously about this.

It's not about getters, James... (In reply to James Long (:jlongster) from comment #48) > (In reply to Russ from comment #47) > > (In reply to Panos Astithas [:past] from comment #46) > > > Imagine hovering over myDb.connHandler, which is implemented as a getter > > > property. In order for the debugger to show you the value, it has to make > > > the page execute the getter and generate a result. > > > > Which is the most irritating design-choice/flaw in the debugger. > > > > /rant > > What else could it do? The only thing I can think of is to just not support > getters. I'm open to that but I'm just thinking off-hand right now and we'd > need to think more seriously about this. Seriously, (and I hope I'm correctly understanding the context here)... 1 - when I'm viewing code in the code pane, 90+% of the time I am NOT thinking about (really REALLY dont care about) content of memory (vars etc). So the whole premise/need to popup *anything* over the logic I'm trying to read/interpret bugs the hell out of me. I'm ALWAYS having to take avoiding action with my mouse because thinks they know better than me, what my intentions are or might be. I've ranted about this before and apologies for doing it again but really, if it's just someone's pet "wouldn't it be cool" thing, please, gimme a switch so I can turn it off (because it's anything but cool). 2 - Now contrast #1 with the watch/vars pane. When viewing this pane I am 100% interested in content of vars - yet, there are NO POPUPS! Even a willful click (a CLEAR statement of INTENT) does not guarantee any object will remain expanded to view its content. On a whim, objects collapse and need to be reclicked over and over (as the debugger is stepped). /rant Aside: From the console, clicking an object/dump, brings up (what I'm calling) the object dump pane (does it have a name?). It never updates (AFAICT) which makes it static and useless 99% of the time. Was it meant to be a snapshot? If so, why can't it be refreshed? (without up-arrow/return) Thanks for reading.

(In reply to Russ from comment #49) > > I've ranted about this before and apologies for doing it again but really, > if it's just someone's pet "wouldn't it be cool" thing, please, gimme a > switch so I can turn it off (because it's anything but cool). > We had many user requests for this, and many IDEs have this feature. What I agree we should do though is make the popup more transient, or at least make its behavior unsurprising for the user. > 2 - Now contrast #1 with the watch/vars pane. When viewing this pane I am > 100% interested in content of vars - yet, there are NO POPUPS! Even a > willful click (a CLEAR statement of INTENT) does not guarantee any object > will remain expanded to view its content. On a whim, objects collapse and > need to be reclicked over and over (as the debugger is stepped). > This only happens for `window` and `this`, because we had some performance problems with displaying many properties at once. Doing that continuously while stepping would make debugging a bit slower, but recent work in other bugs will probably allow us to remove this limitation.

(In reply to Victor Porof [:vporof][:vp] from comment #50) > (In reply to Russ from comment #49) > > 2 - Now contrast #1 with the watch/vars pane. When viewing this pane I am > > 100% interested in content of vars - yet, there are NO POPUPS! Even a > > willful click (a CLEAR statement of INTENT) does not guarantee any object > > will remain expanded to view its content. On a whim, objects collapse and > > need to be reclicked over and over (as the debugger is stepped). > > > > This only happens for `window` and `this`, because we had some performance > problems with displaying many properties at once. Doing that continuously > while stepping would make debugging a bit slower, but recent work in other > bugs will probably allow us to remove this limitation. See Bug 1023386 for some ongoing work for this

(In reply to Panos Astithas [:past] from comment #46) > Imagine hovering over myDb.connHandler, which is implemented as a getter > property. In order for the debugger to show you the value, it has to make > the page execute the getter and generate a result. Getters are an unfortunate corner case for this debugger feature. Now, if the bug I'm looking into *has* to do something with the db connection failing and the page not handling it properly, making my random mouse movements in the debugger change the state of the connection turns this bug into a real "heisenbug", and making sense of what's happening becomes nearly impossible. So I think it's a serious bug if actions that seem side-effect free inside the debugger end up running getters and possibly change the state(s) of the debuggee. > You can't discount that as an implementation > artifact, as there is no other way to actually compute the return value. Well, I can argue that to keep what's going on in the page predictable, the debugger should give up showing any value if a getter is involved. It would be awesome if it could show a reference to the getter method, which would let you inspect the code that will run when the getter is invoked. > As you rightly point out however, the console doesn't do this unfortunately > and it is very visible now with the split console (not saying that it > couldn't have been experienced before the split console existed, I just > didn't think of it when reviewing the console evaluation patch). This is > indeed a bug to fix: evaluating an expression should also send resume/pause > events so that the debugger, if open, can update properly. Is this known, or should I report it? > Unfortunately I don't understand what is preventing the resumed event from > working for this case It fires too often ;)

Sounds like we need to figure out a better handling of getter properties - if I'm not mistaken this is what bug 830822 and bug 820878 are about although at first glance they seem to say the problem is the other way around.. Or maybe find another way to do it.. If the resumed event can't be used to say "the user used Step over/Step into/Resume", what should I listen to? The key events for [F11], [F10] and [F8]? It would work but is perhaps a bit ugly..

hide variable tooltip when stepping in devtools debugger, bug 967853. r?pbrosset

I'm trying out a new approach: just let any keydown events on document close the popup. But I'm having some problems with building Fx right now so this isn't actually tested, and the new test needs some adjustments..

https://reviewboard.mozilla.org/r/21371/#review19257 Test needs adapting to the new approach.

Comment on attachment 8670443 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r?pbrosset Sorry I haven't been involved enough in this bug for a wholte to be able to review this correctly. I think James, Victor or Panos are better reviewers.

Comment on attachment 8670443 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r?pbrosset hide variable tooltip when stepping in devtools debugger, bug 967853. r?pbrosset

So - I'm still working on being able to build Fx locally again, but I pushed a new attempt to the try server and it's green! Including the new test! So this patch should be good :) @pbrosset: sorry, I tried to push another review request with r?pbrosset since I hadn't seen your comment. Please ignore. I'm trying to figure out how to make ReviewBoard forget about that one or switch reviewers..

hide variable tooltip when stepping in devtools debugger, bug 967853. r?jlong

Comment on attachment 8670924 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r=jlongster https://reviewboard.mozilla.org/r/21483/#review19463 I'll defer to James on this one.

Comment on attachment 8670924 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r=jlongster https://reviewboard.mozilla.org/r/21483/#review19547 ::: devtools/client/debugger/test/mochitest/browser_dbg_variables-view-popup-17.js:33 (Diff revision 1) > + executeSoon(() => EventUtils.synthesizeKey('VK_F11', {})); I'd rather not hardcode the key here. Earlier on IRC we talked about this so if you could synthesize a mouse press instead, that would be great ::: devtools/client/debugger/views/variable-bubble-view.js:43 (Diff revision 1) > + event: "keydown" Why are we only doing this on keydown? I never step using keys, I press the stepping buttons. Wouldn't this not work with those?

(In reply to James Long (:jlongster) from comment #65) > Comment on attachment 8670924 [details] > MozReview Request: hide variable tooltip when stepping in devtools debugger, > bug 967853. r?jlong > > https://reviewboard.mozilla.org/r/21483/#review19547 > > ::: > devtools/client/debugger/test/mochitest/browser_dbg_variables-view-popup-17. > js:33 > (Diff revision 1) > > + executeSoon(() => EventUtils.synthesizeKey('VK_F11', {})); > > I'd rather not hardcode the key here. Earlier on IRC we talked about this Pardon the confusion, the test we discussed on IRC was a different, somewhat related one. This test needs to use key events. > > + event: "keydown" > > Why are we only doing this on keydown? I never step using keys, I press the > stepping buttons. Wouldn't this not work with those? This fix is explicitly scoped to solve the problem that occurs if you have a variable inspection "bubble" open and use shortcut keys to step. The problem is that the screen won't update to show the code you're stepping through while the variable popup is open. This makes the debugger look very broken, and sometimes makes one miss important sections of the code, meaning you have to restart the whole process to hit a certain breakpoint again.. especially annoying if the popup opens when you don't even intend it to just because you've happened to click and place the mouse somewhere :-/ My suggested fix here is to make the popup close on keydown events so that stepping will work as expected. I tried fixing this in a better way listening for "resume" events to make it also work when clicking the buttons (see discussion earlier in this bug) but it was not possible because the inspection popup itself runs some JS to get the value(s) and this caused thread-resumed events I could not distinguish from the resumed events that occur when you actually step. If there is away to do that it would be better, but also somewhat more invasive and risky. And clicking somewhere else (like on the "Step into" button) *already* closes the inspection popup, so actually the keydown fix is all we need.

(In reply to Hallvord R. M. Steen [:hallvors] from comment #66) > (In reply to James Long (:jlongster) from comment #65) > > Comment on attachment 8670924 [details] > > MozReview Request: hide variable tooltip when stepping in devtools debugger, > > bug 967853. r?jlong > > > > https://reviewboard.mozilla.org/r/21483/#review19547 > > > > ::: > > devtools/client/debugger/test/mochitest/browser_dbg_variables-view-popup-17. > > js:33 > > (Diff revision 1) > > > + executeSoon(() => EventUtils.synthesizeKey('VK_F11', {})); > > > > I'd rather not hardcode the key here. Earlier on IRC we talked about this > > Pardon the confusion, the test we discussed on IRC was a different, somewhat > related one. This test needs to use key events. > I see. > > I tried fixing this in a better way listening for "resume" events to make it > also work when clicking the buttons (see discussion earlier in this bug) but > it was not possible because the inspection popup itself runs some JS to get > the value(s) and this caused thread-resumed events I could not distinguish > from the resumed events that occur when you actually step. If there is away > to do that it would be better, but also somewhat more invasive and risky. > And clicking somewhere else (like on the "Step into" button) *already* > closes the inspection popup, so actually the keydown fix is all we need. Ok, so the buttons already close it as well. That's fine then, as long as the behavior is the same between the buttons and keypresses.

https://reviewboard.mozilla.org/r/21483/#review19603 ::: devtools/client/debugger/test/mochitest/browser_dbg_variables-view-popup-17.js:12 (Diff revision 1) > +let gBreakpoints, gSources, gVariables; Can you move these declarations into the `test` functions, and then remove the `registerCleanupFunction` usage? Scoping these inside `test` makes them automatically cleaned up.

Comment on attachment 8670924 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r=jlongster hide variable tooltip when stepping in devtools debugger, bug 967853. r?jlongster

(Aside: I guess fixed-in-fx-team in whiteboard is wrong?)

(In reply to Hallvord R. M. Steen [:hallvors] from comment #70) > (Aside: I guess fixed-in-fx-team in whiteboard is wrong?) Yeah, that's not used anymore and it looks like in this case it never got removed when it got backed out

Comment on attachment 8670924 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r=jlongster https://reviewboard.mozilla.org/r/21483/#review19915

Next step here - setting checkin-needed keyword? Sorry, I'm never quite sure if this workflow..

Sorry for the delay, the work week and everything always pushed stuff back. Has this run on try first? I can't tell if the previous run is up-to-date or not.

If you get a green try run, then yes all you need to do is add the `checkin-needed` keyword and the sheriffs will land it for you.

There's one odd orange thingy here, but I think it's not relevant - rest is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05e6804b2bb6

For future reference, I recommend running both opt and debug on all platforms and 32/64-bit. It's surprising how often there's a failure just on a specific config. That said this is a very minor change so it should be fine to check in.

Backed this out: https://hg.mozilla.org/integration/fx-team/rev/6ef6dbd7ce5f https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=6ef6dbd7ce5f Reasons: M(dt2) failure on Linux opt, pgo and asan: https://treeherder.mozilla.org/logviewer.html#?job_id=6278687&repo=fx-team 05:39:43 INFO - 469 INFO TEST-START | devtools/client/debugger/test/mochitest/browser_dbg_variables-view-popup-17.js 05:39:44 INFO - Frame script loaded. 05:39:44 INFO - JavaScript warning: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-redux.js, line 409: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create 05:39:45 INFO - ************************* 05:39:45 INFO - A coding exception was thrown and uncaught in a Task. 05:39:45 INFO - Full message: TypeError: gBreakpoints is undefined 05:39:45 INFO - Full stack: test/</testPopupHiding<@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_variables-view-popup-17.js:24:7 05:39:45 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:40:40 05:39:45 INFO - then@resource://devtools/shared/deprecated-sync-thenables.js:20:43 05:39:45 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:72:11 05:39:46 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:40:11 05:39:46 INFO - then@resource://devtools/shared/deprecated-sync-thenables.js:20:43 05:39:46 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:72:11 05:39:46 INFO - onEvent@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/head.js:360:7 05:39:46 INFO - EventEmitter_emit@resource://devtools/shared/event-emitter.js:147:11 05:39:46 INFO - DebuggerView._renderSourceText/<@chrome://devtools/content/debugger/debugger-view.js:518:7 05:39:46 INFO - setTimeout handler*DebuggerView._renderSourceText@chrome://devtools/content/debugger/debugger-view.js:517:1 05:39:46 INFO - DebuggerView.renderSourceText@chrome://devtools/content/debugger/debugger-view.js:447:1 05:39:46 INFO - onReducerEvents/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/debugger/content/utils.js:58:7 05:39:46 INFO - makeStateBroadcaster/<.subscribeToStore/</</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:98:17 05:39:46 INFO - makeStateBroadcaster/<.subscribeToStore/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:97:15 05:39:46 INFO - makeStateBroadcaster/<.subscribeToStore/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:90:11 05:39:46 INFO - createStore/dispatch/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:212:15 05:39:46 INFO - dispatch@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:211:6 05:39:46 INFO - waitUntilService/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/wait-service.js:60:20 05:39:46 INFO - promiseMiddleware/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/promise.js:16:1 05:39:46 INFO - thunk/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/thunk.js:16:9 05:39:46 INFO - task/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/task.js:31:12 05:39:46 INFO - dispatch@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:384:19 05:39:46 INFO - promiseMiddleware/</</</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/promise.js:35:1 05:39:46 INFO - makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14 05:39:46 INFO - ************************* 05:39:47 INFO - TEST-INFO | started process screentopng 05:39:48 INFO - TEST-INFO | screentopng: exit 0 05:39:48 INFO - 470 INFO checking window state 05:39:48 INFO - 471 INFO Initializing a debugger panel. 05:39:48 INFO - 472 INFO Adding tab: http://example.com/browser/devtools/client/debugger/test/mochitest/doc_with-frame.html 05:39:48 INFO - 473 INFO Loading frame script with url chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/code_frame-script.js. 05:39:48 INFO - 474 INFO Tab added and finished loading: http://example.com/browser/devtools/client/debugger/test/mochitest/doc_with-frame.html 05:39:48 INFO - 475 INFO Debugee tab added successfully: http://example.com/browser/devtools/client/debugger/test/mochitest/doc_with-frame.html 05:39:48 INFO - 476 INFO Console message: [JavaScript Warning: "mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-redux.js" line: 409}] M(dt8) on Windows if not run in e10s https://treeherder.mozilla.org/logviewer.html#?job_id=6281117&repo=fx-team 06:31:25 INFO - A coding exception was thrown and uncaught in a Task. 06:31:25 INFO - Full message: TypeError: gBreakpoints is undefined 06:31:25 INFO - Full stack: test/</testPopupHiding<@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_variables-view-popup-17.js:24:7 06:31:25 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:40:40 06:31:25 INFO - then@resource://devtools/shared/deprecated-sync-thenables.js:20:43 06:31:25 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:72:11 06:31:25 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:40:11 06:31:25 INFO - then@resource://devtools/shared/deprecated-sync-thenables.js:20:43 06:31:25 INFO - resolve@resource://devtools/shared/deprecated-sync-thenables.js:72:11 06:31:25 INFO - onEvent@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/head.js:360:7 06:31:25 INFO - EventEmitter_emit@resource://devtools/shared/event-emitter.js:147:11 06:31:25 INFO - DebuggerView._renderSourceText/<@chrome://devtools/content/debugger/debugger-view.js:518:7 06:31:25 INFO - setTimeout handler*DebuggerView._renderSourceText@chrome://devtools/content/debugger/debugger-view.js:517:1 06:31:25 INFO - DebuggerView.renderSourceText@chrome://devtools/content/debugger/debugger-view.js:447:1 06:31:25 INFO - onReducerEvents/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/debugger/content/utils.js:58:7 06:31:25 INFO - makeStateBroadcaster/<.subscribeToStore/</</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:98:17 06:31:25 INFO - makeStateBroadcaster/<.subscribeToStore/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:97:15 06:31:25 INFO - makeStateBroadcaster/<.subscribeToStore/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:90:11 06:31:25 INFO - createStore/dispatch/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:212:15 06:31:25 INFO - dispatch@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:211:6 06:31:25 INFO - waitUntilService/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/wait-service.js:60:20 06:31:25 INFO - promiseMiddleware/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/promise.js:16:1 06:31:25 INFO - thunk/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/thunk.js:16:9 06:31:25 INFO - task/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/task.js:31:12 06:31:25 INFO - dispatch@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:384:19 06:31:25 INFO - promiseMiddleware/</</</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/promise.js:35:1

And for OSX, it's dt7: https://treeherder.mozilla.org/logviewer.html#?job_id=6285528&repo=fx-team

(In reply to James Long (:jlongster) from comment #77) > For future reference, I recommend running both opt and debug on all > platforms and 32/64-bit. It's surprising how often there's a failure just on > a specific config. Seems so.. :-/ Is there a newbie-friendly "good try usage" document somewhere?

I don't know exactly what you're looking for but there's a few wiki pages, like this one: https://wiki.mozilla.org/ReleaseEngineering/TryChooser But really, I think in this case you didn't rebase on the latest fx-team before your try push, or your try push was quite old so you needed to do a new push so it was tested with the latest codebase. I think my refactor landed between this, which is what ended up failing. You'll need to tweak the test. I feel really bad that this bug is taking so long to land. That's my failure; I should have helped you push it through much faster. If there's anything I can do please ni? me; I would rebase your patch and try running the test again locally.

Well, I also lost sight of this bug for a while.. I find that both that Wiki page and the trychooser syntax builder assume you know a lot about what test suites are relevant and what platforms/variants are important to run on for a given change. For somebody like me starting from a rather superficial understanding of a) the code base, b) the test suites and c) the worrying number of build configurations, this boils down to making some guesses. And what I've learnt so far is that try-and-fail is a really stupid idea here - not only is it time consuming, but you end up wasting other people's time having poor code checked in to the tree and reverted if you fail to guess the right options for try. :-/ I think this may be the most woodoo-like part of my learning curve trying to make actual coding contributions.. Anyway, will try to rebase and re-do things now.

When in doubt, just go to http://trychooser.pub.build.mozilla.org/ and select "both" under build types (to get both debug and opt), select all the linux/mac/windows platforms, and then just select "mochitests(all)". You'll be good if you always use that try syntax.

Here's the syntax I use: try: -b do -p linux,linux64,macosx64,win32 -u xpcshell,mochitests -t none It's also documented here (but in the context of a git workflow): https://developer.mozilla.org/en-US/docs/Tools/Contributing#Pushing_to_try

Comment on attachment 8670924 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r=jlongster Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21483/diff/2-3/

Review commit: https://reviewboard.mozilla.org/r/28815/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28815/

Per this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e98364e7ddd0&selectedJob=14798230 Linux debug has some orange stuff, I don't know if that's relevant. Everything else is green..

Oh, I didn't realize this needed a review. The flag is properly set to my username. Looking now.

I can't figured out the Review Board UI for the life of me. I don't see any place to actually r+ these patches, probably because it's not assigned to me. I'll just r+ them here those, this looks good. You'll probably want to rebase a new patch, and at least run the tests again locally. You don't need a new try run. Then we can mark this as checkin-needed.

Review commit: https://reviewboard.mozilla.org/r/30711/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30711/

Review commit: https://reviewboard.mozilla.org/r/30713/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30713/

Review commit: https://reviewboard.mozilla.org/r/30715/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30715/

Comment on attachment 8707455 [details] MozReview Request: Bug 1154847, Add DAMP test for stepping in the debugger, r?bgrins Think these requests were auto-generated by reviewboard on accident and were intended for bug 1154874

Oops. No idea how that happened :-/ The right bug number is in the commit message and all.. Sorry.

Comment on attachment 8670924 [details] MozReview Request: hide variable tooltip when stepping in devtools debugger, bug 967853. r=jlongster Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21483/diff/3-4/

Seems I managed to clean up that review request again - the patches there are based on yesterday's pull from moz-central. Just to be sure I've asked for a new try build but I think this is ready for checkin.. James already gave it a +1 above :)

The review Mercurial server has been down since yesterday, I'll just attach an updated patch here.

Please note: Check in the attached b967853-final.patch, not the version that was last pushed to review.