Tom S [:evilpie] Assignee
	Description • 7 years ago

So far I am not aware of any breakage caused by disabling this on Nightly. (bug 1319512). We should just disable this unconditionally and worst case we enable this again before release. The high telemetry usage we measured probably just comes from a fingerprinting script.

	Tom S [:evilpie] Assignee
	Comment 1 • 7 years ago

With this patch we have basically zero test coverage, which is of course quite unfortunate. Adding a runtime option for this in the parser seems like quite a bit of work. I think we used to do this for something, but I can't find the code at the moment.

	Jan de Mooij [:jandem]
	Comment 2 • 6 years ago

Comment on attachment 8938838 [details] [diff] [review]
Disable expression closures everywhere

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

What do you think about adding an expressionClosuresEnabled() shell function so we still have test coverage when compiling with JS_HAS_EXPR_CLOSURES?

No fuzz/test coverage makes me a bit uncomfortable, but it's probably ok for now since this is almost certainly dead.

	Tom S [:evilpie] Assignee
	Comment 3 • 6 years ago

We will do this in Firefox 60 now.

	Tom S [:evilpie] Assignee
	Comment 6 • 6 years ago

Comment on attachment 8945880 [details] [diff] [review]
Disable expression closures everywhere, but allow dynamic enabling them for tests

I removed the macro, because it just makes this code just more complicated to write. I doubt anyone needs it.

	Jan de Mooij [:jandem]
	Comment 7 • 6 years ago

Comment on attachment 8945880 [details] [diff] [review]
Disable expression closures everywhere, but allow dynamic enabling them for tests

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

Great, thanks!

	Pulsebot
	Comment 8 • 6 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/166a780e234f
Disable expression closures everywhere, but allow dynamic enabling them for tests. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/245e242e19af
Fix tests to explicitly enable expression closures. r=jandem

	Cosmin Sabou [:CosminS]
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/166a780e234f
https://hg.mozilla.org/mozilla-central/rev/245e242e19af

	Kohei Yoshino [:kohei]
	Comment 10 • 6 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/expression-closure-support-has-been-removed/

	Florian Scholz (Open Web Docs)
	Comment 11 • 6 years ago

https://github.com/mdn/browser-compat-data/pull/1138
https://developer.mozilla.org/en-US/Firefox/Releases/60#Removals_from_the_web_platform
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Expression_closures