bindings: Add a macro for V8 Maybe<T> APIs.

bindings: Add a macro for V8 Maybe<T> APIs.

These APIs require !IsNothing() check before getting values. It's worth adding a macro which does the check.

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

[bashi, 2015-03-11 02:16:36]

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

[bashi, 2015-03-11 02:16:37]

I think it's worth adding a macro for v8::Maybe<T>. WDYT?

[bashi, 2015-03-11 02:17:49]

Forgot to put a link to Maybe<T>.
https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...

[haraken, 2015-03-11 02:43:21]

https://codereview.chromium.org/995203002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/995203002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8BindingMacros.h:90: if (handle.IsEmpty() ||
!v8Maybe(handle->methodCall, outVariable)) { \

Do we need the handle.IsEmpty() check? It could be a responsibility of the
caller side to make sure that the handle is not empty.

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
File Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp
(right):

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp:83:
V8_MAYBE(value, result, BooleanValue(isolate->GetCurrentContext()), return
false);

I guess this should be 'return true'.

It seems that this handleEvent wants to return false only when it succeeded in
the callback without causing any error and the result of the callback is true.
If that is the case, we want to return true when an empty handle is returned
from the callback. (Note: The only case where exceptionCatcher.HasCaught() is
false but an empty handle is returned is the case worker termination is
requested.)

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
File Source/bindings/modules/v8/custom/V8DeviceMotionEventCustom.cpp (right):

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
Source/bindings/modules/v8/custom/V8DeviceMotionEventCustom.cpp:52: V8_MAYBE(x,
xValue, NumberValue(context), return nullptr);

I might want to have a better name for the macro though.

V8_CALL or V8_CALL_WITH_EMPTY_CHECK?

[bashi, 2015-03-11 03:46:27]

Thank you for review!

https://codereview.chromium.org/995203002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/995203002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8BindingMacros.h:90: if (handle.IsEmpty() ||
!v8Maybe(handle->methodCall, outVariable)) { \
On 2015/03/11 02:43:21, haraken wrote:
> 
> Do we need the handle.IsEmpty() check? It could be a responsibility of the
> caller side to make sure that the handle is not empty.

I think IsEmpty() check reduces many hand-written checks. I'd prefer having this
in this macro.

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
File Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp
(right):

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp:83:
V8_MAYBE(value, result, BooleanValue(isolate->GetCurrentContext()), return
false);
On 2015/03/11 02:43:21, haraken wrote:
> 
> I guess this should be 'return true'.
> 
> It seems that this handleEvent wants to return false only when it succeeded in
> the callback without causing any error and the result of the callback is true.
> If that is the case, we want to return true when an empty handle is returned
> from the callback. (Note: The only case where exceptionCatcher.HasCaught() is
> false but an empty handle is returned is the case worker termination is
> requested.)

Ah, right. Done.

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
File Source/bindings/modules/v8/custom/V8DeviceMotionEventCustom.cpp (right):

https://codereview.chromium.org/995203002/diff/1/Source/bindings/modules/v8/c...
Source/bindings/modules/v8/custom/V8DeviceMotionEventCustom.cpp:52: V8_MAYBE(x,
xValue, NumberValue(context), return nullptr);
On 2015/03/11 02:43:21, haraken wrote:
> 
> I might want to have a better name for the macro though.
> 
> V8_CALL or V8_CALL_WITH_EMPTY_CHECK?

Changed to V8_CALL(). (This sounds a bit too generic, but I'd want to keep the
name as short as possible)

[haraken, 2015-03-11 03:54:13]

LGTM on my side. Jens?

[Jens Widell, 2015-03-11 07:18:22]

LGTM

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

https://codereview.chromium.org/995203002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:90: if (handle.IsEmpty() ||
!getValueFromMaybe(handle->methodCall, outVariable)) { \
Wrap the whole macro body in a do{}while()? Defined like this,

  if (<condition>)
    V8_CALL(...);
  else
    <fallback>;

does not do what one expects.

[bashi, 2015-03-11 07:33:43]

Thanks!

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

https://codereview.chromium.org/995203002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:90: if (handle.IsEmpty() ||
!getValueFromMaybe(handle->methodCall, outVariable)) { \
On 2015/03/11 07:18:22, Jens Widell wrote:
> Wrap the whole macro body in a do{}while()? Defined like this,
> 
>   if (<condition>)
>     V8_CALL(...);
>   else
>     <fallback>;
> 
> does not do what one expects.

Done.

[bashi, 2015-03-11 07:36:07]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-03-11 07:36:08]

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

[commit-bot: I haz the power, 2015-03-11 07:36:19]

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

[Jens Widell, 2015-03-11 07:39:34]

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:92: do {                              
                                        \
This inner do{}while() is OTOH strictly speaking redundant. The
'failureExpression' parameter is already expanded inside {}.

[bashi, 2015-03-11 07:44:41]

The CQ bit was unchecked by bashi@chromium.org

[bashi, 2015-03-11 07:51:18]

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:92: do {                              
                                        \
On 2015/03/11 07:39:34, Jens Widell wrote:
> This inner do{}while() is OTOH strictly speaking redundant. The
> 'failureExpression' parameter is already expanded inside {}.

Oh, right. Done.

[Yuki, 2015-03-11 07:55:22]

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:81: bool inline
getValueFromMaybe(v8::Maybe<T> maybe, T& outVariable)
nit: s/bool inline/inline bool/

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:89: #define V8_CALL(outVariable,
handle, methodCall, failureExpression)                    \
nit: You cannot use 'break' in failureExpression.

It would be better to comment that users of this macro shouldn't use 'break' or
'continue' in failureExpression, or define this macro as:

if (...) {
  failureExpression;
} else

# The following semicolon ends the else clause.

[Yuki, 2015-03-11 07:56:36]

lgtm

[bashi, 2015-03-11 10:22:35]

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:81: bool inline
getValueFromMaybe(v8::Maybe<T> maybe, T& outVariable)
On 2015/03/11 07:55:22, Yuki wrote:
> nit: s/bool inline/inline bool/

Done.

https://codereview.chromium.org/995203002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:89: #define V8_CALL(outVariable,
handle, methodCall, failureExpression)                    \
On 2015/03/11 07:55:22, Yuki wrote:
> nit: You cannot use 'break' in failureExpression.
> 
> It would be better to comment that users of this macro shouldn't use 'break'
or
> 'continue' in failureExpression, or define this macro as:
> 
> if (...) {
>   failureExpression;
> } else
> 
> # The following semicolon ends the else clause.

Thanks, I took "if (...) { failureExpression; } else" approach.

[bashi, 2015-03-11 10:22:44]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-03-11 10:22:44]

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

[commit-bot: I haz the power, 2015-03-11 10:22:57]

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

[commit-bot: I haz the power, 2015-03-11 13:25:22]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191704