Issue 5552 - use strict mode for all JS in libadblocklus

[anton, 2017-08-22 07:37:05]

https://codereview.adblockplus.org/29523555/diff/29523556/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29523555/diff/29523556/src/JsEngine.cpp#ne...
src/JsEngine.cpp:79: std::string cmd = "--use_strict";
https://issues.adblockplus.org/ticket/5552#comment:2

[anton, 2017-08-22 08:22:33]

https://codereview.adblockplus.org/29523555/diff/29523563/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29523555/diff/29523563/src/JsEngine.cpp#ne...
src/JsEngine.cpp:54: v8::V8::Initialize();
in v8_initiliazer.cc in chromium `v8::V8::Initialize();` is invoked after flags
set:
  v8::V8::InitializePlatform(vp);

  ...
  if (IsolateHolder::kStrictMode == mode) {
    static const char use_strict[] = "--use_strict";
    v8::V8::SetFlagsFromString(use_strict, sizeof(use_strict) - 1);
  }
  ...
  v8::V8::Initialize();

did/can you make sure the order you've used is not important here?

[sergei, 2017-08-22 10:52:19]

https://codereview.adblockplus.org/29523555/diff/29523563/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29523555/diff/29523563/src/JsEngine.cpp#ne...
src/JsEngine.cpp:54: v8::V8::Initialize();
On 2017/08/22 08:22:33, anton wrote:
> in v8_initiliazer.cc in chromium `v8::V8::Initialize();` is invoked after
flags
> set:
>   v8::V8::InitializePlatform(vp);
> 
>   ...
>   if (IsolateHolder::kStrictMode == mode) {
>     static const char use_strict[] = "--use_strict";
>     v8::V8::SetFlagsFromString(use_strict, sizeof(use_strict) - 1);
>   }
>   ...
>   v8::V8::Initialize();
> 
> did/can you make sure the order you've used is not important here?

Good catch! Actually this particular flag is used only when a compilation of a
script is called, namely for us it's when JsEngine::Evaluate is called, so the
current order is fine. Actually I have taken the order from
https://github.com/v8/v8/blob/master/src/d8.cc. However, in both d8.cc (call of
Shell::SetOptions) and in the code of v8 I see that some flags should be set
before v8::V8::Initialize();. So, to simplify the catching of potential bugs
when we decide to add new flags I have moved setting of the flags even before
creation of a platform, as it's done in d8.cc.

[anton, 2017-08-22 10:55:02]

On 2017/08/22 10:52:19, sergei wrote:
> https://codereview.adblockplus.org/29523555/diff/29523563/src/JsEngine.cpp
> File src/JsEngine.cpp (right):
> 
>
https://codereview.adblockplus.org/29523555/diff/29523563/src/JsEngine.cpp#ne...
> src/JsEngine.cpp:54: v8::V8::Initialize();
> On 2017/08/22 08:22:33, anton wrote:
> > in v8_initiliazer.cc in chromium `v8::V8::Initialize();` is invoked after
> flags
> > set:
> >   v8::V8::InitializePlatform(vp);
> > 
> >   ...
> >   if (IsolateHolder::kStrictMode == mode) {
> >     static const char use_strict[] = "--use_strict";
> >     v8::V8::SetFlagsFromString(use_strict, sizeof(use_strict) - 1);
> >   }
> >   ...
> >   v8::V8::Initialize();
> > 
> > did/can you make sure the order you've used is not important here?
> 
> Good catch! Actually this particular flag is used only when a compilation of a
> script is called, namely for us it's when JsEngine::Evaluate is called, so the
> current order is fine. Actually I have taken the order from
> https://github.com/v8/v8/blob/master/src/d8.cc. However, in both d8.cc (call
of
> Shell::SetOptions) and in the code of v8 I see that some flags should be set
> before v8::V8::Initialize();. So, to simplify the catching of potential bugs
> when we decide to add new flags I have moved setting of the flags even before
> creation of a platform, as it's done in d8.cc.

LGTM

[hub, 2017-08-22 13:04:38]

LGTM