[bindings] Support is_explicit_nullable for method arguments.

[bindings] Support is_explicit_nullable for method arguments.

R=jl@opera.com

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

[vivekg, 2015-02-24 13:58:10]

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

[vivekg, 2015-02-24 13:58:11]

Hi Jens, 

Can you please take look? Its not ready yet but just wanted to know if I am
heading in the right direction.

The generated diffs are like this:

Document.idl => http://www.mergely.com/z5IWcpRV/
SQLTransaction.idl => http://www.mergely.com/Buh88mfp/

But the Document.idl check looks redundant here.

Please suggest. 

Thank you,
Vivek

[Jens Widell, 2015-02-24 14:29:03]

To solve the Document.idl/NodeIterator case (redundant check, and actually
incorrect code in Oilpan builds) we should make sure
v8_types.cpp_type_has_null_value() returns true for it. I think
idl_type.is_callback_interface is the condition we need to add to that function
to fix it.

You also need to make sure 'cpp_type' is set to 'Nullable<T>' instead of 'T' if
idl_type.is_explicit_nullable is true (in v8_methods.argument_context(), that
is.)  For reference, see how 'cpp_type' is set in v8_methods.method_context().
In the generated code for executeSql(), the 'arguments' local should be declared
as

  Nullable<Vector<ScriptValue>> arguments;

https://codereview.chromium.org/954723002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/954723002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:175:
{{argument.v8_value_to_local_cpp_value}};
This may or may not be the only part that should be conditional. :-) (This macro
is very hard to read; we should probably refactor it.)

I thought about implementing the conditional wrapping in the
generate_arguments() where we already have similar wrapping for handling
optional arguments (with default values), but that's not really a good idea
since it would need to be duplicated then.

Other options are probably available, but I think "refactor this macro" is the
approach we ought to take before thinking much more about this (assuming this
addition turns out to be enough, at least for now.)

[Jens Widell, 2015-02-24 14:30:29]

https://codereview.chromium.org/954723002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/954723002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:175:
{{argument.v8_value_to_local_cpp_value}};
On 2015/02/24 14:29:03, Jens Widell wrote:
> This may or may not be the only part that should be conditional. :-) (This
macro
> is very hard to read; we should probably refactor it.)
> 
> I thought about implementing the conditional wrapping in the
> generate_arguments() where we already have similar wrapping for handling
> optional arguments (with default values), but that's not really a good idea
> since it would need to be duplicated then.
> 
> Other options are probably available, but I think "refactor this macro" is the
> approach we ought to take before thinking much more about this (assuming this
> addition turns out to be enough, at least for now.)

To clarify: I think this addition here is probably fine.

[vivekg, 2015-03-02 07:34:22]

jl@, sorry I was on a medical leave.

I have uploaded a new patch, PTAL. Thanks.

[Jens Widell, 2015-03-02 07:53:13]

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:218: else idl_type.cpp_type_args(
I think it would be better to move this call to idl_type.cpp_type_args() up,
storing the result in a variable ('this_cpp_type' is commonly used for that
purpose) and then use that variable in both branches here.

Also, the condition for using Nullable<> should also technically be
"idl_type.is_explicit_nullable and not argument.is_variadic".

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:183:
{{v8_value_to_local_cpp_value(argument) | indent }}
Nit: there should be no space between "indent" and "}}".

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestInterface.idl (right):

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestInterface.idl:109: void
methodWithExplicitNullableArgument(optional object[]? arguments);
I think this rather belongs in TestObject.idl, where we have numerous tests for
specific method argument types already.

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/V8TestObject.cpp (right):

https://codereview.chromium.org/954723002/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestObject.cpp:6218: Nullable<ScriptValue>
voidCallbackFunctionArg;
This is wrong; ScriptValue can store null, so shouldn't be "explicitly
nullable".

I guess idl_type.is_callback_function is the condition we need to add to
v8_types.cpp_type_has_null_value().

[vivekg, 2015-03-02 08:36:47]

Thank you Jens, I have uploaded a new patch with all the comments addressed.
PTAL.

[Jens Widell, 2015-03-02 08:42:17]

https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:215: if idl_type.is_explicit_nullable and
not argument.is_variadic:
This is not quite what I meant. I meant to do

  this_cpp_type = idl_type.cpp_type_args(...)

and then

  'cpp_type': (
    v8_types.cpp_template_type('Nullable', this_cpp_type)
    if <condition>
    else this_cpp_type),

[vivekg, 2015-03-02 10:45:20]

On 2015/03/02 at 08:42:17, jl wrote:
>
https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/...
> File Source/bindings/scripts/v8_methods.py (right):
> 
>
https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_methods.py:215: if idl_type.is_explicit_nullable
and not argument.is_variadic:
> This is not quite what I meant. I meant to do
> 
>   this_cpp_type = idl_type.cpp_type_args(...)
> 
> and then
> 
>   'cpp_type': (
>     v8_types.cpp_template_type('Nullable', this_cpp_type)
>     if <condition>
>     else this_cpp_type),

Done. PTAL, thank you.

[Jens Widell, 2015-03-02 11:04:35]

LGTM after one more fix.

https://codereview.chromium.org/954723002/diff/60001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/954723002/diff/60001/Source/bindings/template...
Source/bindings/templates/methods.cpp:182: if
(!info[{{argument.index}}]->IsNull()) {
The check here should be

  if (!isUndefinedOrNull(info[{{argument.index}}])) {

(I realize I suggested using IsNull() earlier; I was mistaken then. Web IDL
actually says to treat both ECMAScript null and undefined as "IDL null".)

[vivekg, 2015-03-02 11:08:07]

On 2015/03/02 at 11:04:35, jl wrote:
> LGTM after one more fix.
> 
>
https://codereview.chromium.org/954723002/diff/60001/Source/bindings/template...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/954723002/diff/60001/Source/bindings/template...
> Source/bindings/templates/methods.cpp:182: if
(!info[{{argument.index}}]->IsNull()) {
> The check here should be
> 
>   if (!isUndefinedOrNull(info[{{argument.index}}])) {
> 
> (I realize I suggested using IsNull() earlier; I was mistaken then. Web IDL
actually says to treat both ECMAScript null and undefined as "IDL null".)

Done. :)

[vivekg, 2015-03-02 11:50:10]

The CQ bit was checked by vivekg@chromium.org

[vivekg, 2015-03-02 11:50:11]

The patchset sent to the CQ was uploaded after l-g-t-m from jl@opera.com
Link to the patchset: https://codereview.chromium.org/954723002/#ps80001 (title:
"Patch for landing!")

[commit-bot: I haz the power, 2015-03-02 11:51:03]

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

[commit-bot: I haz the power, 2015-03-02 13:10:59]

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