Opened 10 months ago

Closed 10 months ago

Last modified 5 months ago

#7303 closed change (fixed)

Deprecate the use of String.prototype.substr()

Reported by: hfiguiere Assignee: hfiguiere
Priority: P4 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/30011555

Description (last modified by hfiguiere)

Background

According to MDN:

Warning: Although String.prototype.substr(…) is not strictly deprecated (as in "removed from the Web standards"), it is considered a legacy function and should be avoided when possible. It is not part of the core JavaScript language and may be removed in the future. If at all possible, use the substring() method instead.

We should move to towards using String.prototype.substring() everywhere

What to change

Replace substr() with substring(). One caveat: the optional second argument in not the length but instead the index of the first character to exclude, so attention must be paid to that when changing.

Hint for tester

Changes are small and spread out in the code base. The unit tests did pass. Interesting errors with string truncated or at the wrong offset could be symptoms of errors in that changeset.

Change History (6)

comment:1 Changed 10 months ago by hfiguiere

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

comment:2 Changed 10 months ago by hfiguiere

  • Owner set to hfiguiere

comment:3 Changed 10 months ago by mjethani

  • Priority changed from Unknown to P4
  • Ready set

comment:4 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 7303 - Deprecate the use of String.substr()

comment:5 Changed 10 months ago by hfiguiere

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

Two locations have been left out of the patch deliberately as they are in hot path and substr() is apparently (a very little bit) faster on V8.

comment:6 Changed 5 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Does not look to have caused any regessions.

ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

Note: See TracTickets for help on using tickets.