Closed
Bug 1056409
Opened 10 years ago
Closed 10 years ago
[jsdbg2] move the sourceMapURL property from D.Script to D.Source
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jlong, Assigned: jlong)
References
Details
Attachments
(1 file, 7 obsolete files)
21.66 KB,
patch
|
Details | Diff | Splinter Review |
The sourceMapURL is appropriate for Debugger.Source, not Debugger.Script, where it doesn't even exist right now. Should be a very small patch.
Assignee | ||
Comment 1•10 years ago
|
||
Question: when I commit this, this will break our devtools debugger tests. But I can't commit the changes to the debugger until this is committed. And I can't commit both at once because this goes on inbound, and debugger is on fx-team. How do we coordinate these kinds of cross-channel breaking changes?
Assignee | ||
Comment 2•10 years ago
|
||
Nevermind, talked to fitzgen on IRC and he said we'd just commit everything on inbound.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jlong
Assignee | ||
Updated•10 years ago
|
Attachment #8476390 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•10 years ago
|
||
@jorendorff any chance you could review this?
Comment 5•10 years ago
|
||
Comment on attachment 8476390 [details] [diff] [review] 1056409.patch Review of attachment 8476390 [details] [diff] [review]: ----------------------------------------------------------------- Update the documentation too.
Attachment #8476390 -
Flags: review?(jorendorff) → review+
Comment 6•10 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #5) > Comment on attachment 8476390 [details] [diff] [review] > 1056409.patch > > Review of attachment 8476390 [details] [diff] [review]: > ----------------------------------------------------------------- > > Update the documentation too. Which can be found in js/src/doc/Debugger
Assignee | ||
Updated•10 years ago
|
Blocks: dbg-source
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=02f423a26d3d Where are those other commits coming from? I haven't actually landed something in a little while, and I forget how that happens.
Assignee | ||
Comment 8•10 years ago
|
||
There was a test I hadn't fixed. https://tbpl.mozilla.org/?tree=Try&rev=08a2ff446664
Assignee | ||
Comment 9•10 years ago
|
||
Same patch with correct headers
Attachment #8476390 -
Attachment is obsolete: true
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/627e848b2bf3 for mochitest-oth failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=49004067&tree=Mozilla-Inbound
Flags: needinfo?(jlong)
Assignee | ||
Comment 13•10 years ago
|
||
Sorry, didn't realize other tests relied on this. Just grepped the whole codebase and am running all the mochitests here: https://tbpl.mozilla.org/?tree=Try&rev=82b956e7a956. Should be good this time if it's green.
Flags: needinfo?(jlong)
Assignee | ||
Comment 14•10 years ago
|
||
Accidentally didn't include a fix in my last patch... https://tbpl.mozilla.org/?tree=Try&rev=b46ffb713179
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8496000 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
The test failures in comment 14 are timeouts unrelated to this patch. OSX 10.6 opt is timing out a lot but I looked at the bug and it's not related to sourcemaps at all, it's some media thing.
Keywords: checkin-needed
Assignee | ||
Comment 17•10 years ago
|
||
I tested that the warning is still generated for web code, here's proof.
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8498154 [details]
warning-output.png
This was for the wrong bug.
Attachment #8498154 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
had to backout after checkin since the commit message was little strange, showed date/time instead of what was supposed but i guess ryan knows how to handle that checkin :)
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #19) > had to backout after checkin since the commit message was little strange, > showed date/time instead of what was supposed but i guess ryan knows how to > handle that checkin :) Weird, as far as I know I made it the same way I've made patches before. If I did something wrong I'd love to know and I'll change my workflow.
Comment 22•10 years ago
|
||
(In reply to James Long (:jlongster) from comment #20) > Weird, as far as I know I made it the same way I've made patches before. If > I did something wrong I'd love to know and I'll change my workflow. If you find me at the London office today, I can look over your patches and see what's going on.
Assignee | ||
Comment 23•10 years ago
|
||
Thanks jimb. I know what happened; I generated a diff straight from a git branch and it included the commit info there. Going to get a script from fitzgen that will always generate a proper patch.
Assignee | ||
Comment 25•10 years ago
|
||
Since I had to rebase, new try: https://tbpl.mozilla.org/?tree=Try&rev=830820584f28
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 26•10 years ago
|
||
(In reply to James Long (:jlongster) from comment #25) > Since I had to rebase, new try: > https://tbpl.mozilla.org/?tree=Try&rev=830820584f28 We're just ignoring those hazard analysis failures?
Keywords: checkin-needed
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #26) > (In reply to James Long (:jlongster) from comment #25) > > Since I had to rebase, new try: > > https://tbpl.mozilla.org/?tree=Try&rev=830820584f28 > > We're just ignoring those hazard analysis failures? I've been seeing several build timeouts recently so that failure slipped from me :( Sorry, thought it was a timeout. Turns out it was an assert that was stripped out in opts builds. From now on I'm going to test both opt/debug builds because it's too common to only find errors in one of them. https://tbpl.mozilla.org/?tree=Try&rev=a9e2a92a59e7
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8498890 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
Just to make doubly sure that this is the latest patch
Attachment #8499646 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
I push it to try again: https://tbpl.mozilla.org/?tree=Try&rev=36a70b546e4b I don't think any of those oranges are my fault, but it's hard to tell. All of them seem unrelated and and have existing bugs. Regardless I fired off reruns of those that are orange, but I still don't think they are related to this.
Keywords: checkin-needed
Assignee | ||
Comment 31•10 years ago
|
||
Argh, didn't generate a properly formatted patch before.
Attachment #8500552 -
Attachment is obsolete: true
Comment 32•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc2eaf50c04
Keywords: checkin-needed
Comment 33•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6dc2eaf50c04
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•