Opened on 01/13/2017 at 08:58:10 AM
Closed on 02/17/2017 at 01:57:16 PM
#4795 closed change (fixed)
Use modern JavaScript syntax where possible (let, arrow functions, for of)
| Reported by: | kzar | Assignee: | kzar |
|---|---|---|---|
| Priority: | P3 | Milestone: | Adblock-Plus-1.13-for-Chrome-Opera |
| Module: | Platform | Keywords: | |
| Cc: | sebastian | Blocked By: | #4722 |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | yes | Confidential: | no |
| Tester: | Unknown | Verified working: | no |
| Review URL(s): |
https://codereview.adblockplus.org/29371763/ |
||
Description (last modified by kzar)
Background
With #4722 we dropped support for older versions of Chrome and with #4551 Safari. We're now free to use some more recent JavaScript APIs and syntax.
What to change
Where possible replace:
- Anonymous functions with arrow functions.
- var variable definitions with let ones.
- for i = 0; i < whatever.length with for ... of whatever.
Hints for testers
Unfortunately since this issue meant changing neraly every JavaScript file in the repository we'll need to test all functionality as far as possible. The options page, ad blocking, sitekeys, first run page, uninstallation page, whitelisting, notifications, etc... The list goes on! Please make sure to test on Chrome 49 as well as a more recent version. (I recommend testing this together with #4823 and if possible #4864, #4871, #4878 since they are all similar large changes which require really thorough testing.)
Attachments (0)
Change History (10)
comment:1 Changed on 01/13/2017 at 12:14:19 PM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:2 Changed on 01/19/2017 at 04:08:51 AM by abpbot
comment:3 Changed on 01/19/2017 at 04:31:48 AM by kzar
- Description modified (diff)
- Milestone set to Adblock-Plus-for-Chrome-Opera-next
comment:4 Changed on 01/19/2017 at 11:01:43 AM by kzar
- Resolution set to fixed
- Status changed from reviewing to closed
comment:5 Changed on 01/23/2017 at 12:38:09 PM by kzar
- Resolution fixed deleted
- Status changed from closed to reopened
I've noticed a regression whereby when the subscriptions "Update" button is pushed in the Options page an exception is shown in the background console. It still appears to funciton properly however.
It seems to be that somehow XMLHttpRequest.prototype.channel is undefined when checked by adblockpluscore/lib/downloader.js, which causes the instaceof Ci.nsIHttpChannel check to throw. Investigating...
comment:6 Changed on 01/23/2017 at 01:05:45 PM by kzar
OK it's because methods defined with the shorthand syntax have a prototype of undefined which causes typeof tests to fail.
comment:7 Changed on 01/23/2017 at 01:44:17 PM by kzar
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:9 Changed on 02/17/2017 at 01:56:08 PM by abpbot
A commit referencing this issue has landed:
Issue 4795 - Avoid shorthand method syntax where prototype matters
comment:10 Changed on 02/17/2017 at 01:57:16 PM by kzar
- Resolution set to fixed
- Status changed from reviewing to closed
A commit referencing this issue has landed:
Issue 4795 - Use modern JavaScript syntax