Closed Bug 1251419 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set

Tracking

(firefox47 affected)

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: aryx, Assigned: aryx)

Details

Attachments

(4 files, 1 obsolete file)

Attached patch suite, v1 (obsolete) — Splinter Review
Attachment #8723799 - Flags: review?(philip.chee)
Attached patch mailnews, v1Splinter Review
Attachment #8723812 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8723809 - Flags: review?(acelists) → review+
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.
Attachment #8723799 - Flags: review?(philip.chee)
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)
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.
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?
Well, core is working to remove all non-standard js thingies so it would bite us at some point.
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.
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.
Attachment #8723800 - Flags: review?(aleth) → review+
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.
(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 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
Attachment #8723812 - Flags: review?(mkmelin+mozilla) → review+
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 :)
Attachment #8723808 - Flags: review?(philipp) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(philip.chee)
Keywords: checkin-needed
Resolution: --- → FIXED
> 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.
Flags: needinfo?(philip.chee)
What needs checkin here?
Flags: needinfo?(philipp)
Eh, oops. Not sure why I added the keyword, everything in this bug is checked in now.
Flags: needinfo?(philipp)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.