Opened 2 years ago

Last modified 2 years ago

#6526 new change

Stop using of deprecated V8 API. — at Version 9

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

Description (last modified by oleksandr)


We are using pretty a lot of deprecated V8 API, and in order to simplify our life, avoid unnecessary delays in such issues like #6063, and be able to experiment with new techniques like WebAssembly, we should update our code base to use up-to-date API.

What to change

The list of API

  • [done] v8::TryCatch should accept v8::Isolate
  • [done] v8::Isolate::IdleNotification is renamed to v8::Isolate::IdleNotitifcationDeadline
  • look into v8::Script::Compile, related #6448
  • [done] use v8::Local instead of v8::Handle
  • [question is below] use v8 functions which return maybe values, e.g.
    • v8::Value::IntegerValue
    • v8::FunctionTemplate::GetFunction
    • v8::String::NewFromUtf8
    • v8::Value::BooleanValue
    • v8::Object::Get(uint32_t)
    • v8::Object::GetOwnPropertyNames
    • v8::Function::Call
    • v8::Message::GetLineNumber
    • etc
  • those maybe functions accept v8::Context, therefore we need to carefully check that we pass the correct context. Perhaps in the callbacks we should rather use v8::Isolate::GetCurrentContext instead of obtaining it from JsEngine. Additionally pay attention to JsContext which works with the context too.
  • to be continued ...

Here we need to decide regarding the behavior if there is no value in maybe value. The options are basically either to use an artificial default value, crash or propagate such resembling monadic approach to our API.

Change History (9)

comment:1 Changed 2 years ago by sergz

  • Review URL(s) modified (diff)

comment:2 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6526 - update usage of V8 API

comment:3 Changed 2 years ago by sergz

  • Cc rjeschke hfiguiere added
  • Description modified (diff)
  • Review URL(s) modified (diff)

I would like to get your opinion especially regarding the dealing with the maybevalues in our code.

In order to get compiler warnings and errors one can add

  'defines': [

to 'target_defaults' in common.gypi and compile our code with prebuilt V8.

comment:4 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6526 - replace deprecated v8::Handle by v8::Local

comment:5 Changed 2 years ago by sergz

  • Description modified (diff)

comment:6 Changed 2 years ago by sergz

  • Owner set to sergz

comment:7 Changed 2 years ago by hfiguiere

maybes are a good idea. As a Rust user I'm a big fan of Option<> (std::optional<> in C++ 17). While this often complicate code a bit, it definitely improve the code safety. It is like checking for nullptr.

I'm not sure if requiring C++17 for this is a good idea, but on the other hand we could have our own version of std::optional<> to implement this pattern.

comment:8 Changed 2 years ago by oleksandr

I also think propagating to our API is probably best in the long term. Having default values could cause unintended consequences and crashing just doesn't seem right to me.

comment:9 Changed 2 years ago by oleksandr

  • Description modified (diff)
Note: See TracTickets for help on using tickets.