1251419 - Remove non-standard JavaScript syntax |try ... catch (ex if ex instanceof ExceptionType) from comm-central

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=45df7a6f199c&selectedJob=16574

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: suite, v1

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: chat, v1

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: calendar, v1

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: mail, v1

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: mailnews, v1

Comment 6

[:aceman]

Comment on attachment 8723809 [details] [diff] [review]
mail, v1

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

Thanks, looks fine to me. Please add proper commit message (not "imported patch mail").

But can you please provide the reason for this (the new code does not look more readable)? If the core JS engine is removing support for this syntax, please link the bug.

Comment 7

[Philip Chee]

Comment on attachment 8723799 [details] [diff] [review]
suite, v1

(In reply to :aceman from comment #6)
> But can you please provide the reason for this (the new code does not look
> more readable)? If the core JS engine is removing support for this syntax,
> please link the bug.

I too would like to know. Cancelling review request until I hear back on the reasoning.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8723812 [details] [diff] [review]
mailnews, v1

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

There's a few places here using 

if(foo)
  return;
else
  return;

Please make it just 

if(foo)
  return;
return;

instead (no else clause for such cases)

Comment 9

[Magnus Melin [:mkmelin]]

Dunno if there's a m-c bug for it, but yes as this is a non-standard feature we should just get rid of it.

Comment 10

[:aceman]

I don't think that is a solid reason. Standards are only necessary on webpages and other formats of interchange between apps/users. In internal code every app uses whatever it wants. Even 'webextensions' are to be extended with non-standard features to suit FF's extensions' needs. And what is Servo in Rust?

Comment 11

[Magnus Melin [:mkmelin]]

Well, core is working to remove all non-standard js thingies so it would bite us at some point.

Comment 12

[:aceman]

Depends on what is called non-standard. Till now, Mozilla was the one adding things then pushing them into standards. So why would they go into the opposite direction now.

Comment 13

[Magnus Melin [:mkmelin]]

There's a difference between features and syntax. I think all the big players are pretty much committed to standardized ES6 (and beyond) syntax at this point.

Comment 14

[Philip Chee]

Comment on attachment 8723799 [details] [diff] [review]
suite, v1

This is useless churn. Please re-flag for review only if removal of this functionality is imminent.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

chat: https://hg.mozilla.org/comm-central/rev/f2acd8984cea

Comment 16

[Robert Kaiser]

(In reply to Philip Chee from comment #14)
> Comment on attachment 8723799 [details] [diff] [review]
> suite, v1
> 
> This is useless churn. Please re-flag for review only if removal of this
> functionality is imminent.

You mean, SeaMonkey will be dead soon anyhow so we don't need to make it more modern? Otherwise, if I was still involved, I'd be happy people are actually making us more standards-compliants and happy people still care about writing patches for suite/ and not leaving it to the waning SM team.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8723812 [details] [diff] [review]
mailnews, v1

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

r=mkmelin with the if-else return nits from comment 8

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8723808 [details] [diff] [review]
calendar, v1

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

I'm ok with this change, thank you very much for making the effort. I personally like the condensed syntax more and I really hope that ES6/7 allows us to do this in one line again. Maybe you can patch it back when they do :)

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I found yet another reason to use the standards compliant syntax, if you have any hopes of getting mach eslint running clean, it won't understand this non-standard syntax.

I've fixed the review comments and pushed the remaining patches. Philip, do you want to reconsider this for suite?

https://hg.mozilla.org/comm-central/rev/032de7a87b2a
https://hg.mozilla.org/comm-central/rev/ebdb04f579e8
https://hg.mozilla.org/comm-central/rev/5e1d4159745d

Comment 20

[Philip Chee]

> I found yet another reason to use the standards compliant syntax, if you
> have any hopes of getting mach eslint running clean, it won't understand
> this non-standard syntax.
eslint is extensible. You can write a rule to recognize multiple catch clauses ;)

> I've fixed the review comments and pushed the remaining patches. Philip, do
> you want to reconsider this for suite?
I like the compact form better. So, not unless this feature is imminently slated for removal from Spidermonkey.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

What needs checkin here?

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Eh, oops. Not sure why I added the keyword, everything in this bug is checked in now.