Closed
Bug 1131756
Opened 9 years ago
Closed 9 years ago
Debugger doesn't break in react hot reload demo
Categories
(DevTools :: Debugger, defect, P1)
Tracking
(firefox40 fixed, firefox41 fixed, relnote-firefox -)
RESOLVED
FIXED
Firefox 41
People
(Reporter: canuckistani, Assigned: jlong)
References
(Blocks 1 open bug)
Details
(Whiteboard: [polish-backlog])
Attachments
(3 files, 3 obsolete files)
1.00 KB,
patch
|
Details | Diff | Splinter Review | |
14.26 KB,
patch
|
Details | Diff | Splinter Review | |
13.45 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
In the react hotreload demo the user is told to un-comment a line in the editor that should cause the debugger to pause: // Feel like debugging something? // Open DevTools and uncomment next line: console.log(e); debugger; // Now type something into the input. // Nice, huh? Remove this line so we can move on. see http://gaearon.github.io/react-hot-loader/
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jlong
Updated•9 years ago
|
Blocks: dbg-breakpoint
Assignee | ||
Updated•9 years ago
|
Whiteboard: [devedition-40]
Assignee | ||
Comment 1•9 years ago
|
||
It does break in nightly, but the editor doesn't show the right source and switching through the stack frames is messed up. This is because we hide eval sources that don't have a URL. I'm going to see how easy it is to hide them, but make one appear if the editor needs it.
Assignee | ||
Comment 2•9 years ago
|
||
Needs tests, so not quite finished yet, but it works: https://twitter.com/jlongster/status/593155967317102592
Reporter | ||
Updated•9 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•9 years ago
|
||
This patch makes it so that if we break inside a script that is not listed in the sources pane (happens when a `debugger` statement is inside an anonymous eval), we add it to the source pane so we can interact with it. Doing so makes `debugger` statements work properly; the eval script is shown. This reminds me that we need to implement functionality for jumping to the introduction site at some point
Attachment #8598927 -
Attachment is obsolete: true
Attachment #8599462 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bace10a8512
Assignee | ||
Comment 5•9 years ago
|
||
oops bad test, fixed https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebe423de474d
Comment 6•9 years ago
|
||
Comment on attachment 8599462 [details] [diff] [review] 1131756.patch Review of attachment 8599462 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good to me. ::: browser/devtools/debugger/debugger-controller.js @@ +689,5 @@ > for (let frame of this.activeThread.cachedFrames) { > let { depth, source, where: { line } } = frame; > > let isBlackBoxed = source ? this.activeThread.source(source).isBlackBoxed : false; > + DebuggerView.StackFrames.addFrame(frame, line, depth, isBlackBoxed); This is much cleaner. I like it. ::: browser/devtools/debugger/views/sources-view.js @@ +121,5 @@ > this._copyUrlMenuItem.removeEventListener("command", this._onCopyUrlCommand, false); > this._newTabMenuItem.removeEventListener("command", this._onNewTabCommand, false); > }, > > + empty: function() { You don't seem to be calling this method explicitly anywhere. Is it somehow called implicitly by WidgetMethods?
Attachment #8599462 -
Flags: review?(ejpbruel) → review+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #6)\ > > ::: browser/devtools/debugger/views/sources-view.js > @@ +121,5 @@ > > this._copyUrlMenuItem.removeEventListener("command", this._onCopyUrlCommand, false); > > this._newTabMenuItem.removeEventListener("command", this._onNewTabCommand, false); > > }, > > > > + empty: function() { > > You don't seem to be calling this method explicitly anywhere. Is it somehow > called implicitly by WidgetMethods? It is called explicitly in various places, like in `handleTabNavigation` in debugger-view.js. It is called whenever we want to clear out the sources.
Assignee | ||
Comment 8•9 years ago
|
||
new patch to fix a test failure
Attachment #8599462 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1681803bd065
Assignee | ||
Comment 10•9 years ago
|
||
There are some strange failures in there but I don't think any of them are related to this: * everything has the "menu.getItemByValue(...) is undefined" messages, even green runs so that must be something failing in a promise that we need to fix (but unrelated, coming from netmonitor) * on linux debug, tests are timing out in various ways * on windows 7 opt, tilt tests are failing
Keywords: checkin-needed
Comment 11•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #10) > There are some strange failures in there but I don't think any of them are > related to this: > > * everything has the "menu.getItemByValue(...) is undefined" messages, even > green runs so that must be something failing in a promise that we need to > fix (but unrelated, coming from netmonitor) Filed Bug 1162961 for that
Assignee | ||
Comment 12•9 years ago
|
||
We need to make sure to land this today for devedition-40.
Comment 13•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #12) > We need to make sure to land this today for devedition-40. We can always request uplift - we just need to make sure it's in by 40.1.
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #13) > (In reply to James Long (:jlongster) from comment #12) > > We need to make sure to land this today for devedition-40. > > We can always request uplift - we just need to make sure it's in by 40.1. This adds new l10n strings though, and as far as I'm aware those are a lot harder (impossible?) to uplift. Worst case it could be unlocalized I guess, but still.
Comment 15•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #14) > (In reply to Brian Grinstead [:bgrins] from comment #13) > > (In reply to James Long (:jlongster) from comment #12) > > > We need to make sure to land this today for devedition-40. > > > > We can always request uplift - we just need to make sure it's in by 40.1. > > This adds new l10n strings though, and as far as I'm aware those are a lot > harder (impossible?) to uplift. Worst case it could be unlocalized I guess, > but still. Oh yeah, we would need to get strings in today. Ryan, is it going to be possible to land this before the merge? Trees are closed, and I'm not sure when it needs to be in m-c to make the release.
Flags: needinfo?(ryanvm)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 16•9 years ago
|
||
If you want to land just the strings on m-c, a=me (with a DONTBUILD in the commit message to avoid useless builds/tests). Otherwise, sorry, this is the risk you take by not getting landed by the Friday before the uplift.
Flags: needinfo?(ryanvm)
Comment 17•9 years ago
|
||
James, can you break out the strings so we can push them to m-c? You'll need to keep evalGroupLabel in the file and add anonymousSourcesLabel alongside it since the strings are going to have to ship separate from the patch. Then in the new version of the full patch you can remove evalGroupLabel
Flags: needinfo?(jlong)
Assignee | ||
Comment 18•9 years ago
|
||
Alright, here are just the strings to be landed before uplift. Sorry about that.
Flags: needinfo?(jlong)
Comment 19•9 years ago
|
||
Pushed to m-c: https://hg.mozilla.org/mozilla-central/rev/480b91b38c80
Comment 20•9 years ago
|
||
The other patch will need a rebase now for the string files
Keywords: checkin-needed → leave-open
Assignee | ||
Comment 22•9 years ago
|
||
Pushing a new try because the last one had weird unrelated failures and I'm hoping I can get a green run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39f60dd0a4aa
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 25•9 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: NA [User impact if declined]: debugger doesn't break correctly in eval scripts [Describe test coverage new/current, TreeHerder]: on m-c [Risks and why]: not much risk, only in the debugger devtool [String/UUID change made/needed]:
Attachment #8606367 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Assignee | ||
Comment 26•9 years ago
|
||
Can we get the uplift approved soon? This needs to sit on aurora for a little bit before June 2
Flags: needinfo?(lhenry)
Assignee | ||
Comment 27•9 years ago
|
||
sorry for the spam, I think this is the right contact
Flags: needinfo?(lhenry) → needinfo?(lmandel)
Comment 28•9 years ago
|
||
Comment on attachment 8606367 [details] [diff] [review] 1131756-aurora.patch Approved for uplift to aurora; this has been stable on m-c for a few days. Looks like it should improve dev tools, so it will be good to have this on aurora.
Attachment #8606367 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 29•9 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: [Suggested wording]: [Links (documentation, blog post, etc)]:
relnote-firefox:
--- → ?
Assignee | ||
Comment 30•9 years ago
|
||
Oops, I didn't notice that it put text in this field. Release Note Request (optional, but appreciated) [Why is this notable]: Developers debugging unnamed eval scripts will notice that they can put `debugger` statements in them now and we will probably debug them [Suggested wording]: Eval scripts without a `sourceURL` pragma can break on debugger statements [Links (documentation, blog post, etc)]:
Comment 31•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/940a60bb7865
Flags: in-testsuite+
Comment 32•9 years ago
|
||
I'm not sure this is good for release notes but maybe good for the MDN page over here: https://developer.mozilla.org/en-US/Firefox/Releases/39
Updated•9 years ago
|
Updated•9 years ago
|
Flags: needinfo?(lmandel)
Reporter | ||
Updated•9 years ago
|
Whiteboard: [devedition-40] → [polish-backlog]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•