bindings: Add toBoolean() to V8Binding.h

bindings: Add toBoolean() to V8Binding.h

We should use Maybe version of BooleanValue(), and it requires some
additional handlings. Specifically, on failures we should rethrow
a exception to ExceptionState so that call sites can detect failures.
toBoolean() does the job.

BUG=462402
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193528

[bashi, 2015-04-10 05:37:29]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[bashi, 2015-04-10 05:37:30]

PTAL?

[vivekg, 2015-04-10 05:43:46]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-04-10 05:43:47]

lgtm

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:393: if (value->IsBoolean())
Shall we split this into fast (inline toBoolean) and slower (non-inline
toBooleanSlow) path as is the case with other conversions?

[vivekg, 2015-04-10 05:47:36]

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:393: if (value->IsBoolean())
On 2015/04/10 at 05:43:47, vivekg_ wrote:
> Shall we split this into fast (inline toBoolean) and slower (non-inline
toBooleanSlow) path as is the case with other conversions?

Or may be wrap it using 

inline bool toBoolean(v8::Isolate* isolate, v8::Local<v8::Value> value,
ExceptionState& exceptionState)
{
    if (LIKELY(value->IsBoolean()))
        return value.As<v8::Boolean>()->Value();

    ...
}

[haraken, 2015-04-10 05:48:01]

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:393: if (value->IsBoolean())
On 2015/04/10 05:43:47, vivekg_ wrote:
> Shall we split this into fast (inline toBoolean) and slower (non-inline
> toBooleanSlow) path as is the case with other conversions?

Yeah, it's good to minimize the inlined part. We can move the TryCatch part into
.cpp.

[Yuki, 2015-04-10 06:17:29]

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:48: bool booleanMember =
toBoolean(isolate, booleanMemberValue, exceptionState);
This code looks to me a lesser version of Maybe.  The caller can ignore a
possible exception.  People may forget to check if an exception is thrown or
not.

Why don't you use the following style that you used for other part?

  bool booleanMember;
  if (!v8CallToBoolean(isolate, booleanMemberValue, booleanMember,
exceptionState))
      return;

where v8CallToBoolean is annotated with "warn unused return value".

I think that the above style is closer to the mind of V8's Maybe and your
design.

[bashi, 2015-04-10 06:39:54]

Thank you for reviews.

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1051753003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:393: if (value->IsBoolean())
On 2015/04/10 05:48:01, haraken wrote:
> On 2015/04/10 05:43:47, vivekg_ wrote:
> > Shall we split this into fast (inline toBoolean) and slower (non-inline
> > toBooleanSlow) path as is the case with other conversions?
> 
> Yeah, it's good to minimize the inlined part. We can move the TryCatch part
into
> .cpp.

Done.

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:48: bool booleanMember =
toBoolean(isolate, booleanMemberValue, exceptionState);
On 2015/04/10 06:17:29, Yuki wrote:
> This code looks to me a lesser version of Maybe.  The caller can ignore a
> possible exception.  People may forget to check if an exception is thrown or
> not.
> 
> Why don't you use the following style that you used for other part?
> 
>   bool booleanMember;
>   if (!v8CallToBoolean(isolate, booleanMemberValue, booleanMember,
> exceptionState))
>       return;
> 
> where v8CallToBoolean is annotated with "warn unused return value".
> 
> I think that the above style is closer to the mind of V8's Maybe and your
> design.

It will require substantial changes in the code generator.
- We use v8_value_to_cpp_value() here and v8_value_to_cpp_value() isn't allowed
to return statements (need to return an expression).
- If we want to allow v8_value_to_cpp_value() returns statements, we need to
modify all places where we use v8_value_to_cpp_value(), which requires many
changes.
- We don't have a v8Call() variant which takes exceptionState. If we want to add
it, it would look like:
  bool v8Call(..., ExceptionState& exceptionState) {
    v8::TryCatch block;
    if (!...) {
      exceptionState.rethrowV8Exception(block.Exception());
      return false;
    }
    ...
  }
  and we will have two TryCatch in the call path, which looks redundant. To
avoid having two TryCatch, we will need additional logic.
- We already use this patterns like toDouble().
- We might want to change the current design for handling exception (i.e. using
exceptionState to detect errors), but it will also require a bunch of changes
and would be a 
separate issue.

Given that, I choose this design.

[haraken, 2015-04-10 06:45:07]

LGTM

[Yuki, 2015-04-10 07:01:43]

lgtm

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:48: bool booleanMember =
toBoolean(isolate, booleanMemberValue, exceptionState);
On 2015/04/10 06:39:54, bashi1 wrote:
> On 2015/04/10 06:17:29, Yuki wrote:
> > This code looks to me a lesser version of Maybe.  The caller can ignore a
> > possible exception.  People may forget to check if an exception is thrown or
> > not.
> > 
> > Why don't you use the following style that you used for other part?
> > 
> >   bool booleanMember;
> >   if (!v8CallToBoolean(isolate, booleanMemberValue, booleanMember,
> > exceptionState))
> >       return;
> > 
> > where v8CallToBoolean is annotated with "warn unused return value".
> > 
> > I think that the above style is closer to the mind of V8's Maybe and your
> > design.
> 
> It will require substantial changes in the code generator.
> - We use v8_value_to_cpp_value() here and v8_value_to_cpp_value() isn't
allowed
> to return statements (need to return an expression).
> - If we want to allow v8_value_to_cpp_value() returns statements, we need to
> modify all places where we use v8_value_to_cpp_value(), which requires many
> changes.
> - We don't have a v8Call() variant which takes exceptionState. If we want to
add
> it, it would look like:
>   bool v8Call(..., ExceptionState& exceptionState) {
>     v8::TryCatch block;
>     if (!...) {
>       exceptionState.rethrowV8Exception(block.Exception());
>       return false;
>     }
>     ...
>   }
>   and we will have two TryCatch in the call path, which looks redundant. To
> avoid having two TryCatch, we will need additional logic.
> - We already use this patterns like toDouble().
> - We might want to change the current design for handling exception (i.e.
using
> exceptionState to detect errors), but it will also require a bunch of changes
> and would be a 
> separate issue.
> 
> Given that, I choose this design.

Thanks for the explanation.  Reinvention of exception handling costs us.

[bashi, 2015-04-10 07:08:48]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-04-10 07:08:48]

The patchset sent to the CQ was uploaded after l-g-t-m from vivekg@chromium.org
Link to the patchset: https://codereview.chromium.org/1051753003/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-04-10 07:08:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1051753003/20001

[vivekg, 2015-04-10 07:17:30]

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:391: bool toBooleanSlow(v8::Isolate*,
v8::Local<v8::Value>, ExceptionState&);
Should we export this symbol using CORE_EXPORT?

[bashi, 2015-04-10 07:22:27]

The CQ bit was unchecked by bashi@chromium.org

[bashi, 2015-04-10 07:25:20]

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1051753003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:391: bool toBooleanSlow(v8::Isolate*,
v8::Local<v8::Value>, ExceptionState&);
On 2015/04/10 07:17:30, vivekg_ wrote:
> Should we export this symbol using CORE_EXPORT? 

Done.

[bashi, 2015-04-10 07:25:24]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-04-10 07:25:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, vivekg@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1051753003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-04-10 07:25:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1051753003/40001

[commit-bot: I haz the power, 2015-04-10 09:19:56]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193528