Eddy Bruel [:ejpbruel] Reporter
	Description • 11 years ago

Per the discussion on the source map mailing list

	Eddy Bruel [:ejpbruel] Reporter
	Comment 1 • 11 years ago

Peter van der Zee rightly pointed out that we shouldn't just drop support for the old pragma syntax, but emit a deprecation warning instead.

	Jason Orendorff [:jorendorff]
	Comment 2 • 11 years ago

Comment on attachment 747462 [details] [diff] [review]
Patch to be reviewed (v2)

Review of attachment 747462 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/TokenStream.cpp
@@ +804,5 @@
>       * To avoid a crashing bug in IE, several JavaScript transpilers
>       * wrap single line comments containing a source mapping URL
>       * inside a multiline comment to avoid a crashing bug in IE. To
>       * avoid potentially expensive lookahead and backtracking, we
> +     * only check for this case if we encounter an '#' character.

"a '#' character", not "an"

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 3 • 11 years ago

Spec has been updated, this can land.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 4 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc37773787ce

	Phil Ringnalda (:philor)
	Comment 5 • 11 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6058a619fc99 - devtools/server/tests/unit/test_dbgsocket.js would prefer you not do that without including it, https://tbpl.mozilla.org/php/getParsedLog.php?id=23664182&tree=Mozilla-Inbound

	Eddy Bruel [:ejpbruel] Reporter
	Comment 6 • 11 years ago

(In reply to Phil Ringnalda (:philor) from comment #5)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6058a619fc99 -
> devtools/server/tests/unit/test_dbgsocket.js would prefer you not do that
> without including it,
> https://tbpl.mozilla.org/php/getParsedLog.php?id=23664182&tree=Mozilla-
> Inbound

My apologies. I was not aware of that file.

	Phil Ringnalda (:philor)
	Comment 7 • 11 years ago

Heh, what really got mad about it, though, was mochitest-1 - between httpd.js and the js component loader each having to emit 50 or so deprecation warnings every time they turn around, the debug mochitest-1 logs overflowed buildbot's maximum size of 50MB, and got truncated - https://tbpl.mozilla.org/php/getParsedLog.php?id=23664098&tree=Mozilla-Inbound

	Eddy Bruel [:ejpbruel] Reporter
	Comment 8 • 11 years ago

(In reply to Phil Ringnalda (:philor) from comment #7)
> Heh, what really got mad about it, though, was mochitest-1 - between
> httpd.js and the js component loader each having to emit 50 or so
> deprecation warnings every time they turn around, the debug mochitest-1 logs
> overflowed buildbot's maximum size of 50MB, and got truncated -
> https://tbpl.mozilla.org/php/getParsedLog.php?id=23664098&tree=Mozilla-
> Inbound

That's kind of a problem. We really do want those deprecation warnings. If we have to change all the code in the tree that currently uses source map pragmas, that kind of defeats the point.

What's the best course of action here?

	Phil Ringnalda (:philor)
	Comment 9 • 11 years ago

http://mxr.mozilla.org/mozilla-central/source/config/Preprocessor.py#135 and the copy of it in js/config, and you're down to just fixing some devtools tests, pushing to try with -u all and looking at a log for every test type to make sure you haven't left any other glaring instances.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 10 • 11 years ago

Nick, I'm terribly sorry, but I don't have time to land this before I leave on PTO. Could you please push this into production during my absence? Looks like it only involves some changes in JS files, so you should be able to do this on your own. Philor described the steps you need to do in comment 9. Thanks!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 11 • 11 years ago

Try server is down, will add a try push when it's back up.

* s/an/a/ that Jason pointed out

* s/getAtSourceMappingURL/getSourceMappingURL/ because there is no longer an "@"

* Moved the reporting of the deprecation warning inside getSourceMappingURL so that it isn't fired off every time we find "//@" and instead only when we find "//@ sourceMappingURL=...". This fixes the billions of warnings being issued because of "//@line" comments

* s/@ sourceMappingURL/# sourceMappingURL/ in the devtools tests

	Phil Ringnalda (:philor)
	Comment 12 • 11 years ago

Comment on attachment 757711 [details] [diff] [review]
v3

Happy to look at the results of your try push if you wind up with failures you're not sure about, but you're not touching anything I should even pretend to have the right to review.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 13 • 11 years ago

Try push: https://tbpl.mozilla.org/?tree=Try&rev=931911388f27

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 14 • 11 years ago

Eddy changed the contents of one of the asm.js messages, and after reverting that change the tests are not complaining anymore.

Try push looking good.

https://tbpl.mozilla.org/?tree=Try&rev=a22e0035f1ee

	Jason Orendorff [:jorendorff]
	Comment 15 • 11 years ago

Comment on attachment 759159 [details] [diff] [review]
v4

Review of attachment 759159 [details] [diff] [review]:
-----------------------------------------------------------------

Use options("werror") and assertThrowsInstanceOf(() => eval(...), SyntaxError) to check for the deprecation warning.

::: js/src/frontend/TokenStream.cpp
@@ +845,4 @@
>       *
>       * To avoid a crashing bug in IE, several JavaScript transpilers
>       * wrap single line comments containing a source mapping URL
>       * inside a multiline comment to avoid a crashing bug in IE. To

(pre-existing nit) "To avoid a crashing bug in IE" appears too many times in this comment. Once is enough.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 16 • 11 years ago

Fixed comment mentioned above!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 17 • 11 years ago

Rebased on central so that it will apply easier for whoever lands this :)

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c85bb7761528

Please make sure you have hg configured per the directions below so that all the needed commit information is included in the patches you attach.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/c85bb7761528

	Florian Scholz (Open Web Docs)
	Comment 20 • 8 years ago

Docs:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Deprecated_source_map_pragma
https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Use_a_source_map
https://developer.mozilla.org/en-US/Firefox/Releases/24#Developer_Tools

fwiw, this warning is one of the top warnings emitted and seen in the Firefox console.