If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jlongster, Assigned: jlongster)

Tracking

unspecified
mozilla35
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

3 years ago
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

3 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

3 years ago
Nevermind, talked to fitzgen on IRC and he said we'd just commit everything on inbound.
(Assignee)

Comment 3

3 years ago
Created attachment 8476390 [details] [diff] [review]
1056409.patch
(Assignee)

Updated

3 years ago
Assignee: nobody → jlong
(Assignee)

Updated

3 years ago
Attachment #8476390 - Flags: review?(jorendorff)
(Assignee)

Comment 4

3 years ago
@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
(Assignee)

Updated

3 years ago
Blocks: 905700
(Assignee)

Comment 7

3 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

3 years ago
There was a test I hadn't fixed. https://tbpl.mozilla.org/?tree=Try&rev=08a2ff446664
(Assignee)

Comment 9

3 years ago
Created attachment 8496000 [details] [diff] [review]
1056409.patch

Same patch with correct headers
Attachment #8476390 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
try from comment 8 is green
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d4bd3746ec
Keywords: checkin-needed
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

3 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

3 years ago
Accidentally didn't include a fix in my last patch... https://tbpl.mozilla.org/?tree=Try&rev=b46ffb713179
(Assignee)

Comment 15

3 years ago
Created attachment 8497631 [details] [diff] [review]
1056409.patch
Attachment #8496000 - Attachment is obsolete: true
(Assignee)

Comment 16

3 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)

Updated

3 years ago
Blocks: 1074745
(Assignee)

Comment 17

3 years ago
Created attachment 8498154 [details]
warning-output.png

I tested that the warning is still generated for web code, here's proof.
(Assignee)

Comment 18

3 years ago
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)
(Assignee)

Comment 20

3 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.
Proper hg style would be nice.
Flags: needinfo?(ryanvm)

Comment 22

3 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

3 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 24

3 years ago
Created attachment 8498890 [details] [diff] [review]
1056409.patch

Rebased patch
Attachment #8497631 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Since I had to rebase, new try: https://tbpl.mozilla.org/?tree=Try&rev=830820584f28
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 27

3 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

3 years ago
Created attachment 8499646 [details] [diff] [review]
1056409.patch
Attachment #8498890 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8500552 [details] [diff] [review]
1056409.patch

Just to make doubly sure that this is the latest patch
Attachment #8499646 - Attachment is obsolete: true
(Assignee)

Comment 30

3 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

3 years ago
Created attachment 8500585 [details] [diff] [review]
1056409.patch

Argh, didn't generate a properly formatted patch before.
Attachment #8500552 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc2eaf50c04
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6dc2eaf50c04
Status: NEW → RESOLVED
Last Resolved: 3 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.