Opened 2 years ago

Closed 2 years ago

#5552 closed change (fixed)

Use strict mode for all JS in libadblocklus

Reported by: sergz Assignee: sergz
Priority: P3 Milestone:
Module: Libadblockplus Keywords:
Cc: asmirnov, hfiguiere Blocked By:
Blocking: #5509 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29523555/

Description

Background

In order to accomplish #5509 as well as allow to use some other V8 (e.g. chromium's V8 runs JS in strict mode) we should enable strict mode for all our JS.

What to change

  • enable strict mode for V8, e.g. by using command line args
  • adjust JS code breaking the work

Change History (5)

comment:1 Changed 2 years ago by sergz

  • Review URL(s) modified (diff)

comment:2 follow-up: Changed 2 years ago by asmirnov

Can we have any cases where strict mode is not preferred? I mean should we still allow to use nonstrict mode by adding ScopedV8Isolate ctor argument bool strict = true?

comment:3 in reply to: ↑ 2 Changed 2 years ago by sergz

Replying to asmirnov:

Can we have any cases where strict mode is not preferred? I mean should we still allow to use nonstrict mode by adding ScopedV8Isolate ctor argument bool strict = true?

I doubt about it because as far as I know we require strict mode for all our JS code in all other projects, therefore, I think, there should not be an exception for libadblockplus. In addition if we add some flag we will have to support it, e.g. at least ensure that the everything works when this flag is set to false. I would say let's not add it until there is a real demand in it.

Generally speaking it can indeed break some third-party code based on libadblockplus like it happened with some our tests. However it seems not difficult to make relevant changes and forcing of strict mode can actually give more benefits because JS engine can perform better optimization for performance and memory usage.

comment:4 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5552 - use strict mode for all JS in libadblocklus

comment:5 Changed 2 years ago by sergz

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