Opened 10 months ago

Closed 9 months ago

#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/
https://codereview.adblockplus.org/29372820/

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.)

Change History (10)

comment:1 Changed 10 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 4795 - Use modern JavaScript syntax

comment:3 Changed 10 months ago by kzar

  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-next

comment:4 Changed 10 months ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:5 Changed 10 months ago 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 10 months ago 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 10 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:8 Changed 9 months ago by kzar

  • Description modified (diff)

comment:9 Changed 9 months ago by abpbot

A commit referencing this issue has landed:
Issue 4795 - Avoid shorthand method syntax where prototype matters

comment:10 Changed 9 months ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.