Last Comment Bug 870361 - Change the symbol for source map pragmas from @ to #
: Change the symbol for source map pragmas from @ to #
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-09 07:21 PDT by Eddy Bruel [:ejpbruel]
Modified: 2016-05-23 04:54 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to be reviewed (5.19 KB, patch)
2013-05-09 07:21 PDT, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Review
Patch to be reviewed (v2) (9.55 KB, patch)
2013-05-09 08:53 PDT, Eddy Bruel [:ejpbruel]
jorendorff: review+
Details | Diff | Review
v3 (18.39 KB, patch)
2013-06-03 16:51 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
v4 (17.58 KB, patch)
2013-06-06 08:37 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
jorendorff: review+
Details | Diff | Review
v4.1 (17.81 KB, patch)
2013-06-07 15:31 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
v4.2 (17.81 KB, patch)
2013-06-07 15:42 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review

Description Eddy Bruel [:ejpbruel] 2013-05-09 07:21:34 PDT
Created attachment 747415 [details] [diff] [review]
Patch to be reviewed

Per the discussion on the source map mailing list
Comment 1 Eddy Bruel [:ejpbruel] 2013-05-09 08:53:00 PDT
Created attachment 747462 [details] [diff] [review]
Patch to be reviewed (v2)

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.
Comment 2 Jason Orendorff [:jorendorff] 2013-05-15 22:46:54 PDT
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"
Comment 3 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-05-16 09:31:27 PDT
Spec has been updated, this can land.
Comment 5 Phil Ringnalda (:philor) 2013-05-31 22:23:54 PDT
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
Comment 6 Eddy Bruel [:ejpbruel] 2013-05-31 22:28:58 PDT
(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.
Comment 7 Phil Ringnalda (:philor) 2013-05-31 22:34:52 PDT
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
Comment 8 Eddy Bruel [:ejpbruel] 2013-05-31 22:56:42 PDT
(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?
Comment 9 Phil Ringnalda (:philor) 2013-05-31 23:16:07 PDT
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.
Comment 10 Eddy Bruel [:ejpbruel] 2013-06-01 16:37:07 PDT
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!
Comment 11 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-06-03 16:51:27 PDT
Created attachment 757711 [details] [diff] [review]
v3

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
Comment 12 Phil Ringnalda (:philor) 2013-06-03 17:51:44 PDT
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.
Comment 13 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-06-03 17:56:31 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=931911388f27
Comment 14 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-06-06 08:37:39 PDT
Created attachment 759159 [details] [diff] [review]
v4

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
Comment 15 Jason Orendorff [:jorendorff] 2013-06-07 15:18:16 PDT
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.
Comment 16 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-06-07 15:31:24 PDT
Created attachment 759986 [details] [diff] [review]
v4.1

Fixed comment mentioned above!
Comment 17 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-06-07 15:42:43 PDT
Created attachment 759989 [details] [diff] [review]
v4.2

Rebased on central so that it will apply easier for whoever lands this :)
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-06-10 05:50:55 PDT
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
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-06-10 12:53:16 PDT
https://hg.mozilla.org/mozilla-central/rev/c85bb7761528

Note You need to log in before you can comment on or make changes to this bug.