Closed Bug 1056409 Opened 5 years ago Closed 5 years ago

[jsdbg2] move the sourceMapURL property from D.Script to D.Source

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jlong, Assigned: jlong)

References

Details

Attachments

(1 file, 7 obsolete files)

The sourceMapURL is appropriate for Debugger.Source, not Debugger.Script, where it doesn't even exist right now. Should be a very small patch.
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?
Nevermind, talked to fitzgen on IRC and he said we'd just commit everything on inbound.
Attached patch 1056409.patch (obsolete) — Splinter Review
Assignee: nobody → jlong
Attachment #8476390 - Flags: review?(jorendorff)
@jorendorff any chance you could review this?
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+
(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
Blocks: dbg-source
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.
There was a test I hadn't fixed. https://tbpl.mozilla.org/?tree=Try&rev=08a2ff446664
Attached patch 1056409.patch (obsolete) — Splinter Review
Same patch with correct headers
Attachment #8476390 - Attachment is obsolete: true
try from comment 8 is green
Keywords: checkin-needed
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)
Accidentally didn't include a fix in my last patch... https://tbpl.mozilla.org/?tree=Try&rev=b46ffb713179
Attached patch 1056409.patch (obsolete) — Splinter Review
Attachment #8496000 - Attachment is obsolete: true
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
Blocks: 1074745
Attached image warning-output.png (obsolete) —
I tested that the warning is still generated for web code, here's proof.
Comment on attachment 8498154 [details]
warning-output.png

This was for the wrong bug.
Attachment #8498154 - Attachment is obsolete: true
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)
(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.
Proper hg style would be nice.
Flags: needinfo?(ryanvm)
(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.
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.
Attached patch 1056409.patch (obsolete) — Splinter Review
Rebased patch
Attachment #8497631 - Attachment is obsolete: true
Since I had to rebase, new try: https://tbpl.mozilla.org/?tree=Try&rev=830820584f28
Keywords: checkin-needed
Keywords: checkin-needed
(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
(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
Attached patch 1056409.patch (obsolete) — Splinter Review
Attachment #8498890 - Attachment is obsolete: true
Attached patch 1056409.patch (obsolete) — Splinter Review
Just to make doubly sure that this is the latest patch
Attachment #8499646 - Attachment is obsolete: true
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
Attached patch 1056409.patchSplinter Review
Argh, didn't generate a properly formatted patch before.
Attachment #8500552 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6dc2eaf50c04
Status: NEW → RESOLVED
Closed: 5 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.