Closed
Bug 850086
Opened 12 years ago
Closed 12 years ago
Recognize source URLs inside multiline comments
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
Details
Attachments
(1 file, 2 obsolete files)
5.52 KB,
patch
|
till
:
review+
till
:
checkin+
|
Details | Diff | Splinter Review |
To avoid a crashing bug in IE, several JavaScript transpilers (i.e. Coffeescript) wrap single line comments containing a source mapping URL inside a multiline comment to avoid a crashing bug in IE. We need to be able to handle this case.
Assignee | ||
Comment 1•12 years ago
|
||
Hey Jason, could you take a look at this? Btw, I owe you a review. I promise to look at it tomorrow.
Attachment #723749 -
Flags: review?(jorendorff)
Comment 2•12 years ago
|
||
Comment on attachment 723749 [details] [diff] [review] Proposed patch Review of attachment 723749 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/debug/Script-sourceMapURL.js @@ +37,2 @@ > assertEq(getSourceMapURL(), 'file:///var/quux.js.map'); > Additionally, we should have a test for /*\n@sourceMappingURL=file:///baz/bar/foo.js.map\n*/ Because all the transpilers generate the comments with newlines before and after the comment pragma.
Comment 3•12 years ago
|
||
Comment on attachment 723749 [details] [diff] [review] Proposed patch Review of attachment 723749 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/TokenStream.cpp @@ +764,2 @@ > { > /* Match comments of the form "//@ sourceMappingURL=<url>" */ I think you should probably give an example of the multiline form (forms, assuming Nick's form should be supported too) here as well -- I had to look at the test to figure out what the actual hackaround comment syntax was. I'd move the IE-hackaround comment's stuff up here as well -- explain at the same place examples are given. @@ +768,3 @@ > int32_t c; > > + if (peekChars(18, peeked) && CharsMatch(peeked, " sourceMappingURL=")) { Hmm, we probably should have some sort of helper to combine these. And throw some template niceness at it to get rid of that truly awful 18/19 here! Gag.
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 723749 [details] [diff] [review] Proposed patch I'd like to get a quick review for this, but I understand that Jason is currently unavailable, so I'm reassigning the review request to Waldo.
Attachment #723749 -
Flags: review?(jorendorff) → review?(jwalden+bmo)
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > Comment on attachment 723749 [details] [diff] [review] > Proposed patch > > Review of attachment 723749 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/frontend/TokenStream.cpp > @@ +764,2 @@ > > { > > /* Match comments of the form "//@ sourceMappingURL=<url>" */ > > I think you should probably give an example of the multiline form (forms, > assuming Nick's form should be supported too) here as well -- I had to look > at the test to figure out what the actual hackaround comment syntax was. > I'd move the IE-hackaround comment's stuff up here as well -- explain at the > same place examples are given. Agreed > > @@ +768,3 @@ > > int32_t c; > > > > + if (peekChars(18, peeked) && CharsMatch(peeked, " sourceMappingURL=")) { > > Hmm, we probably should have some sort of helper to combine these. And > throw some template niceness at it to get rid of that truly awful 18/19 > here! Gag. How about creating a helper function that takes the string length as a template parameter? (I don't think we can do anything as smart as getting the length of a string literal in a template argument, can we?)
Comment 6•12 years ago
|
||
Comment on attachment 723749 [details] [diff] [review] Proposed patch Review of attachment 723749 [details] [diff] [review]: ----------------------------------------------------------------- r=me with Nick's and Waldo's first comment addressed. I think introducing helpers and templification are out of scope for this bug, but maybe you could file a new one for them? Nice optimization with extracting the "@" detection, btw.
Attachment #723749 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #724037 -
Attachment is obsolete: true
Attachment #724037 -
Flags: checkin?(tschneidereit)
Attachment #724040 -
Flags: checkin?(tschneidereit)
Comment 9•12 years ago
|
||
Comment on attachment 724040 [details] [diff] [review] Patch to land https://hg.mozilla.org/integration/mozilla-inbound/rev/a073d9c49425
Attachment #724040 -
Flags: review+
Attachment #724040 -
Flags: checkin?(tschneidereit)
Attachment #724040 -
Flags: checkin+
Updated•12 years ago
|
Attachment #723749 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a073d9c49425
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•