Closed
Bug 952612
Opened 11 years ago
Closed 6 years ago
Preserve breakpoint when prettyprinting
Categories
(DevTools :: Debugger, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: codacodercodacoder, Unassigned)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [lang=js])
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018
Steps to reproduce:
1 - Set a break point in loaded source
2 - Click Prettify
Affected: FF26, 27, 28, 29
Actual results:
In FF27, 28, 29, the breakpoint moved.
In FF26 it was deleted.
Expected results:
Breakpoints should be (re)assigned to new renumbered line or assigned a "logical" line that will survive Prettify/Unprettify.
Updated•11 years ago
|
Component: Untriaged → Developer Tools: Debugger
Updated•11 years ago
|
Priority: -- → P3
Comment 1•11 years ago
|
||
Are there any plans to change the location of the breakpoint. We already have sourcemapped information, how about moving the breakpoint to the corresponding location ?
Flags: needinfo?(nfitzgerald)
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 2•11 years ago
|
||
(In reply to Girish Sharma [:Optimizer] from comment #1)
> Are there any plans to change the location of the breakpoint. We already
> have sourcemapped information, how about moving the breakpoint to the
> corresponding location ?
We would need to iterate over all breakpoints in the source being prettified[0] and run them through the new source map[1] so that the pretty print RDP response[2] includes a list of new locations for every breakpoint. Then the frontend would have to move the existing breakpoint icons to the new locations when it gets the pretty print response[3].
I don't have any plans on working on this in the near future since 100% of my time is dedicated to memory tooling at the moment.
Happy to mentor anyone who wants to work on this, though.
General devtools hacking info: https://wiki.mozilla.org/DevTools/Hacking
[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#178
[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#4670
[2] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2494
[3] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#433
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(nfitzgerald)
Whiteboard: [mentor=nfitzgerald@mozilla.com][lang=js]
Comment 3•11 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> (In reply to Girish Sharma [:Optimizer] from comment #1)
> > Are there any plans to change the location of the breakpoint. We already
> > have sourcemapped information, how about moving the breakpoint to the
> > corresponding location ?
>
> We would need to iterate over all breakpoints in the source being
> prettified[0] and run them through the new source map[1] so that the pretty
> print RDP response[2] includes a list of new locations for every breakpoint.
> Then the frontend would have to move the existing breakpoint icons to the
> new locations when it gets the pretty print response[3].
>
> I don't have any plans on working on this in the near future since 100% of
> my time is dedicated to memory tooling at the moment.
>
> Happy to mentor anyone who wants to work on this, though.
>
> General devtools hacking info: https://wiki.mozilla.org/DevTools/Hacking
>
> [0]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> script.js#178
> [1]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> script.js#4670
> [2]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> script.js#2494
> [3]
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/
> debugger-panes.js#433
@fitgen, I'd like to start working on this bug.
Updated•11 years ago
|
Assignee: nobody → hudson.deary
Comment 4•11 years ago
|
||
Thanks and good luck!
Comment 5•11 years ago
|
||
Feel free to ask any questions of me or anyone else on IRC, the mailing list, or in this bug!
Comment 6•11 years ago
|
||
fitzgen, so the goal here is to
1) Iterate through breakpoints being prettified
( scripts line #178: findBreakPoints(...) )
2) Iterate the breakpoints through the sourcemap, which returns a pretty-print response with the new locations in the javascript source.
( scripts.js line #4677 getOriginalLocation(...) )
3) Produce the correct line numbers from the pretty print response
( scripts.js line#2501 onPrettyPrint(...) )
4) Then iterate through the pretty print response the new locations of the breakpoints, and render them on the front end.
(debugger-pane.js line #435)
And (if I may interject) if prettify is clicked *again*, revert to the original "ugly" line number(s).
Comment 8•11 years ago
|
||
in doing that, all breakpoints (old & new) would have to be reiterated through the generated source map for it's correct line number in the minified file.
I'm not sure what you're saying...
There isn't necessarily a minified file. Prettify potentially changes the layout of any file in whatever state it was originally found, minified or not. Therefore, unprettify (when prettify is clicked a second time) should put the file back to its original "ugly" state.
The bug here is that Prettify is not taking into account previously set breakpoints. Likewise, unprettify. If Prettify cannot be used in conjunction with previously set breakpoints, offer a warning before blowing away my breakpoints. Either that, or change the way the button behaves - atm it behaves like a toggle - which doesn't work (with breakpoints).
okay?
Comment 10•11 years ago
|
||
Okay, I'm able to re-produce the bug. After I place breakpoints in the debugger, then press prettify, the breakpoints are ignored and not placed in their correct (or logical) place in the prettified source. Pressing prettify again (a second time), the breakpoints are ignored again and maintain their current positions both before/after prettify was pressed.
In the case that prettify is clicked a second time, you are correct that prettify is ignoring the breakpoints on that event as well. unless there is a way to logically find the correct breakpoint line numbers going back and forth from "pretty" to "ugly" source, a warning message should be displayed that the breakpoints will not work in a prettified source, then blowing them away.
Reporter | ||
Comment 11•11 years ago
|
||
Right... now we're on the same page. Ask Nick for his input - I don't know the source at all... he may have other ideas how it might best be handled.
Comment 12•11 years ago
|
||
Right now the devtools team is out on a meeting in Portland. they'll be on irc tomorrow, so I'll shout and get in touch with one of the guys.
Comment 13•11 years ago
|
||
You're both right. My proposed solution in comment 2 was to move the existing breakpoints to the corresponding location in the prettified source rather than displaying a warning and removing all the breakpoints. I think this is a better user experience than removing the breakpoints a user meticulously set.
Deary, comment 6 is the right track. On top of what you mentioned, note that the moved breakpoints should be listed in the "prettyPrint" and "unPrettyPrint" RDP responses.
Comment 14•11 years ago
|
||
fitz: I've set some logs on the prettified source after the source has been prettified, and when breakpoints are set (regardless of line number)the numbers do not reflect where they are being set on the prettified code in findBreakpoints(...)
Assignee | ||
Updated•10 years ago
|
Mentor: nfitzgerald
Whiteboard: [mentor=nfitzgerald@mozilla.com][lang=js] → [lang=js]
Comment 15•10 years ago
|
||
(In reply to Deary Hudson jr. from comment #14)
> fitz: I've set some logs on the prettified source after the source has been
> prettified, and when breakpoints are set (regardless of line number)the
> numbers do not reflect where they are being set on the prettified code in
> findBreakpoints(...)
Hey Deary, sorry I missed this comment :(
In the future, setting "need more information from" to me should help make sure I reply. Sorry, again.
Are you still interested in working on this bug? I'm not sure I follow exactly what you mean in comment 14.
Updated•10 years ago
|
Flags: needinfo?(hudson.deary)
Updated•10 years ago
|
Flags: needinfo?(hudson.deary)
Updated•10 years ago
|
Blocks: dbg-prettyprint
Updated•10 years ago
|
Assignee: hudson.deary → nobody
Comment 17•10 years ago
|
||
@Fitz, sorry I missed your comment. At this point, I am unable to finish working on this bug due to work related projects that are sucking up most of my time.
(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> (In reply to Deary Hudson jr. from comment #14)
> > fitz: I've set some logs on the prettified source after the source has been
> > prettified, and when breakpoints are set (regardless of line number)the
> > numbers do not reflect where they are being set on the prettified code in
> > findBreakpoints(...)
>
> Hey Deary, sorry I missed this comment :(
>
> In the future, setting "need more information from" to me should help make
> sure I reply. Sorry, again.
>
> Are you still interested in working on this bug? I'm not sure I follow
> exactly what you mean in comment 14.
Updated•10 years ago
|
Summary: Prettify in Debugger misplaces assigned breakpoints → Prettyprinting should preserve breakpoints
Updated•10 years ago
|
Summary: Prettyprinting should preserve breakpoints → Preserve breakpoint when prettyprinting
Comment 18•10 years ago
|
||
Deary would like to work on this again.
Assignee: nobody → hudson.deary
Status: NEW → ASSIGNED
Updated•10 years ago
|
Flags: needinfo?(nfitzgerald)
Comment 19•10 years ago
|
||
Hi Deary, how can I help you out?
Flags: needinfo?(nfitzgerald) → needinfo?(hudson.deary)
Updated•10 years ago
|
Assignee: hudson.deary → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(hudson.deary)
Updated•10 years ago
|
Blocks: dbg-breakpoint
Comment 20•10 years ago
|
||
Hey Nick, me again...
I did some quick glance on this bug, and found that it depends on bug1129784.
Prettifying source -> move breakpoints -> current BPs do not respect indent (having no generatedColumn value) -> error BP moving -> error prettified souce BP behaviours
Flags: needinfo?(nfitzgerald)
Comment 21•10 years ago
|
||
While bug1129784 is about column breakpoints, I filed a new bug for normal line breakpoints. bug1138471
Comment 22•10 years ago
|
||
Hey Zimon! Glad to see you back around these parts.
I think :ejpbruel is a better person to talk about this bug with, since he is currently refactoring the way the debugger does breakpoints.
The fact that this is currently blocked, means it might not be a good bug to take right now, however. If that is the case, then perhaps you could pick up bug 1135435 in the meantime?
Flags: needinfo?(nfitzgerald) → needinfo?(ejpbruel)
Comment 23•10 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #22)
> Hey Zimon! Glad to see you back around these parts.
>
> I think :ejpbruel is a better person to talk about this bug with, since he
> is currently refactoring the way the debugger does breakpoints.
>
> The fact that this is currently blocked, means it might not be a good bug to
> take right now, however. If that is the case, then perhaps you could pick up
> bug 1135435 in the meantime?
Ah, I'm waiting for bug983469 patch being landed...
Comment 24•10 years ago
|
||
(In reply to Zimon Dai [:zimonD] from comment #23)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #22)
> > Hey Zimon! Glad to see you back around these parts.
> >
> > I think :ejpbruel is a better person to talk about this bug with, since he
> > is currently refactoring the way the debugger does breakpoints.
> >
> > The fact that this is currently blocked, means it might not be a good bug to
> > take right now, however. If that is the case, then perhaps you could pick up
> > bug 1135435 in the meantime?
>
> Ah, I'm waiting for bug983469 patch being landed...
It should be soon, I wouldn't worry too much about it. You can work on top of that in your patch queue.
Comment 25•10 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #22)
> Hey Zimon! Glad to see you back around these parts.
>
> I think :ejpbruel is a better person to talk about this bug with, since he
> is currently refactoring the way the debugger does breakpoints.
>
> The fact that this is currently blocked, means it might not be a good bug to
> take right now, however. If that is the case, then perhaps you could pick up
> bug 1135435 in the meantime?
The way breakpoint sliding should work is this. When a breakpoint is set on a location that does not have any code, line breakpoints should slide to the next closest line, while column breakpoint should slide to the next closest column (possibly crossing lines).
Currently, both line and column breakpoints slide to the next closest line. In addition, line breakpoints in source mapped sources are treated as column breakpoints (because computing the generated location results in a generatedColumn), but still slide to the next closest line.
In other words, there are multiple things broken here, but I expect to fix most of them within the next few weeks. Fixing breakpoint sliding for non-source-mapped sources is in bug 1138975, and I have patches coming up for fixing breakpoint sliding for source-mapped sources. After that, I plan to spend a few weeks writing tests and cleaning things up. Hopefully I'll be able to get to 1129784 during that time as well.
Flags: needinfo?(ejpbruel)
Updated•9 years ago
|
Mentor: nfitzgerald
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 26•6 years ago
|
||
The new debugger shows breakpoints relative to their original position when pretty-printed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•