Issue 6526 - Use the maybe version of Compile() and Run()

Issue 6526 - Use the maybe version of Compile() and Run()

[hub, 2018-06-19 17:17:04]

https://codereview.adblockplus.org/29810586/diff/29810587/src/JsError.h
File src/JsError.h (right):

https://codereview.adblockplus.org/29810586/diff/29810587/src/JsError.h#newco...
src/JsError.h:38: class JsValueError : public std::runtime_error
On second thought, maybe I shoudln't make a new error type.

[hub, 2018-06-19 17:40:29]

updated patch

[sergei, 2018-06-20 15:09:56]

https://codereview.adblockplus.org/29810586/diff/29810600/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29810600/src/JsEngine.cpp#ne...
src/JsEngine.cpp:210: return JsValue(shared_from_this(),
result.ToLocalChecked());
I think it's very important to keep the exception if there were some. It helped
me several times, so I would propose to not remove the call
`CheckTryCatch(GetIsolate(), tryCatch);`.

What do you think about something like

auto script = CheckedToLocal(tryCatch, CompileScript(GetIsolate(), source,
filename);
auto result = CheckedToLocal(tryCatch,
script->Run(GetIsolate()->GetCurrentContext()));
return JsValue(shared_from_this(), CheckedToLocal(tryCatch, result));

where CheckedToLocal is perhaps a template function, we can elaborate here
later, BTW
template<typename T>
v8::Local<T> CheckedToLocal(tryCatch, value, filename, line)
{
  CheckTryCatch(GetIsolate(), tryCatch);
  if (value.IsEmpty()) // here could even use V8_UNLIKELY
    throw AdblockPlus::JsError("Empty value at ", filename, line);
  return value.ToLocalChecked();
}
(the macro with __FILE__, __LINE__ is omitted).

[hub, 2018-06-20 16:09:21]

updated patch

https://codereview.adblockplus.org/29810586/diff/29810600/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29810600/src/JsEngine.cpp#ne...
src/JsEngine.cpp:210: return JsValue(shared_from_this(),
result.ToLocalChecked());
On 2018/06/20 15:09:55, sergei wrote:
> I think it's very important to keep the exception if there were some. It
helped
> me several times, so I would propose to not remove the call
> `CheckTryCatch(GetIsolate(), tryCatch);`.
> 
> What do you think about something like
> 
> auto script = CheckedToLocal(tryCatch, CompileScript(GetIsolate(), source,
> filename);
> auto result = CheckedToLocal(tryCatch,
> script->Run(GetIsolate()->GetCurrentContext()));
> return JsValue(shared_from_this(), CheckedToLocal(tryCatch, result));
> 
> where CheckedToLocal is perhaps a template function, we can elaborate here
> later, BTW
> template<typename T>
> v8::Local<T> CheckedToLocal(tryCatch, value, filename, line)
> {
>   CheckTryCatch(GetIsolate(), tryCatch);
>   if (value.IsEmpty()) // here could even use V8_UNLIKELY
>     throw AdblockPlus::JsError("Empty value at ", filename, line);
>   return value.ToLocalChecked();
> }
> (the macro with __FILE__, __LINE__ is omitted).

Done.

[sergei, 2018-06-20 16:33:00]

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp#ne...
src/JsEngine.cpp:214: v8::MaybeLocal<v8::Script> script = CompileScript(isolate,
source,
what do you think about calling CHECKED_TO_LOCAL and don't use local variables
of v8::MaybeLocal type?

[hub, 2018-06-20 16:41:48]

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp#ne...
src/JsEngine.cpp:214: v8::MaybeLocal<v8::Script> script = CompileScript(isolate,
source,
On 2018/06/20 16:32:59, sergei wrote:
> what do you think about calling CHECKED_TO_LOCAL and don't use local variables
> of v8::MaybeLocal type?

I can't. CheckedToLocal() 2nd argument is expected to be an l-value.

[hub, 2018-06-20 18:08:09]

Moved CheckedToLocal() to Utils because we'll need then elsewhere too. Better do
it now.

[sergei, 2018-06-20 19:01:50]

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp#ne...
src/JsEngine.cpp:214: v8::MaybeLocal<v8::Script> script = CompileScript(isolate,
source,
On 2018/06/20 16:41:47, hub wrote:
> On 2018/06/20 16:32:59, sergei wrote:
> > what do you think about calling CHECKED_TO_LOCAL and don't use local
variables
> > of v8::MaybeLocal type?
> 
> I can't. CheckedToLocal() 2nd argument is expected to be an l-value.
Can the argument be r-value, v8::MaybeLocal<T>&& value?

[hub, 2018-06-21 01:13:43]

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp#ne...
src/JsEngine.cpp:214: v8::MaybeLocal<v8::Script> script = CompileScript(isolate,
source,
On 2018/06/20 19:01:50, sergei wrote:
> On 2018/06/20 16:41:47, hub wrote:
> > On 2018/06/20 16:32:59, sergei wrote:
> > > what do you think about calling CHECKED_TO_LOCAL and don't use local
> variables
> > > of v8::MaybeLocal type?
> > 
> > I can't. CheckedToLocal() 2nd argument is expected to be an l-value.
> Can the argument be r-value, v8::MaybeLocal<T>&& value?

Yes.
But I don't find the result very readable. And I don't think it change anything.
Here is what it looks like:


  const JsContext context(*this);
  auto isolate = GetIsolate();
  const v8::TryCatch tryCatch(isolate);
  return JsValue(
    shared_from_this(),
    CHECKED_TO_LOCAL(
      isolate,
      CHECKED_TO_LOCAL(
        isolate,
        CompileScript(isolate, source, filename),
        tryCatch
      )->Run(isolate->GetCurrentContext()),
      tryCatch));

[sergei, 2018-06-21 08:45:46]

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp#ne...
src/JsEngine.cpp:214: v8::MaybeLocal<v8::Script> script = CompileScript(isolate,
source,
On 2018/06/21 01:13:43, hub wrote:
> On 2018/06/20 19:01:50, sergei wrote:
> > On 2018/06/20 16:41:47, hub wrote:
> > > On 2018/06/20 16:32:59, sergei wrote:
> > > > what do you think about calling CHECKED_TO_LOCAL and don't use local
> > variables
> > > > of v8::MaybeLocal type?
> > > 
> > > I can't. CheckedToLocal() 2nd argument is expected to be an l-value.
> > Can the argument be r-value, v8::MaybeLocal<T>&& value?
> 
> Yes.
> But I don't find the result very readable. And I don't think it change
anything.
> Here is what it looks like:
> 
> 
>   const JsContext context(*this);
>   auto isolate = GetIsolate();
>   const v8::TryCatch tryCatch(isolate);
>   return JsValue(
>     shared_from_this(),
>     CHECKED_TO_LOCAL(
>       isolate,
>       CHECKED_TO_LOCAL(
>         isolate,
>         CompileScript(isolate, source, filename),
>         tryCatch
>       )->Run(isolate->GetCurrentContext()),
>       tryCatch));

If one has to use the variable more than one time then there will be several
calls of CHECKED_TO_LOCAL for the same variable. This is also readable.

auto script = CHECKED_TO_LOCAL(isolate, CompileScript(isolate, source,
filename), tryCatch);
auto result = CHECKED_TO_LOCAL(isolate,
script->Run(isolate->GetCurrentContext()), tryCatch);
return JsValue(shared_from_this(), result);

[hub, 2018-06-21 13:02:17]

update patch

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29810586/diff/29811601/src/JsEngine.cpp#ne...
src/JsEngine.cpp:214: v8::MaybeLocal<v8::Script> script = CompileScript(isolate,
source,
On 2018/06/21 08:45:45, sergei wrote:
> On 2018/06/21 01:13:43, hub wrote:
> > On 2018/06/20 19:01:50, sergei wrote:
> > > On 2018/06/20 16:41:47, hub wrote:
> > > > On 2018/06/20 16:32:59, sergei wrote:
> > > > > what do you think about calling CHECKED_TO_LOCAL and don't use local
> > > variables
> > > > > of v8::MaybeLocal type?
> > > > 
> > > > I can't. CheckedToLocal() 2nd argument is expected to be an l-value.
> > > Can the argument be r-value, v8::MaybeLocal<T>&& value?
> > 
> > Yes.
> > But I don't find the result very readable. And I don't think it change
> anything.
> > Here is what it looks like:
> > 
> > 
> >   const JsContext context(*this);
> >   auto isolate = GetIsolate();
> >   const v8::TryCatch tryCatch(isolate);
> >   return JsValue(
> >     shared_from_this(),
> >     CHECKED_TO_LOCAL(
> >       isolate,
> >       CHECKED_TO_LOCAL(
> >         isolate,
> >         CompileScript(isolate, source, filename),
> >         tryCatch
> >       )->Run(isolate->GetCurrentContext()),
> >       tryCatch));
> 
> If one has to use the variable more than one time then there will be several
> calls of CHECKED_TO_LOCAL for the same variable. This is also readable.
> 
> auto script = CHECKED_TO_LOCAL(isolate, CompileScript(isolate, source,
> filename), tryCatch);
> auto result = CHECKED_TO_LOCAL(isolate,
> script->Run(isolate->GetCurrentContext()), tryCatch);
> return JsValue(shared_from_this(), result);

Done.

[sergei, 2018-06-21 13:33:55]

LGTM