IDL: Support using nullable on any method return type

IDL: Support using nullable on any method return type

Previously, nullable string and wrapper types was supported, since those
types have a native/natural null representation in C++.  Other types,
like primitives (long, boolean) or complex types like sequence<>, could
not be nullable.

With this CL, nullable return types other than string or wrapper types
are handled using Nullable<T>, like so:

  Nullable<T> result = impl->method(...);
  if (result.isNull())
    v8SetReturnValueNull(info);
  else
    v8SetReturnValue...(info, ... result.get() ...);

This CL does not change code generation since currently no method return
such a nullable type.

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

[Jens Widell, 2014-07-10 14:31:13]

PTAL

A counter-proposal to [TreatReturnedEmptyArrayAs]; thrown together largely to
understand what I was actually proposing.

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:772: sequence_is_nullable =
node.GetProperty('NULLABLE') or False
This fixes sequence<T>?; we previously used the NULLABLE property on a node that
never had it.

We still can't do sequence<T?>, but that type variant seems less useful to have
in an API. There's a FIXME in idl_types.py about restructuring type
representation to support sequence-of, array-of and nullable as independent
modifiers.

[haraken, 2014-07-10 14:43:57]

The approach looks good.

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:123: is_nullable = idl_type.is_nullable
and not idl_type.is_nullable_simple

It's a bit confusing that "is_nullable" and "is_nullable_simple" are mutually
exclusive. Shall we rename to something better to make it clear that they are
mutually exclusive? (I couldn't come up with a better name though. It would be
something like is_xxx_nullable and is_yyy_nullable.)

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:727: # Nullable type where the corresponding
C++ type supports a null value.

Let's add a comment about what C++ type supports it how.

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

https://codereview.chromium.org/385603002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:255: {% elif method.is_use_result_local
and not method.union_arguments %}

Can we fix the code generator so that we always use the local variable (probably
before this CL)? It's a bit shame that we have to introduce use_result_local.

[Jens Widell, 2014-07-10 14:56:44]

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:123: is_nullable = idl_type.is_nullable
and not idl_type.is_nullable_simple
On 2014/07/10 14:43:57, haraken wrote:
> 
> It's a bit confusing that "is_nullable" and "is_nullable_simple" are mutually
> exclusive. Shall we rename to something better to make it clear that they are
> mutually exclusive? (I couldn't come up with a better name though. It would be
> something like is_xxx_nullable and is_yyy_nullable.)

Yes, I'm also beginning to feel that they don't quite work in their current
form.

They aren't mutually exclusive, exactly; the situation is rather that "nullable
simply" mostly brings us back to the non-nullable case. (Maybe you meant that
it's confusing that they *aren't* mutually exclusive?)

I'm also not 100 % thrilled about using 'is_nullable' for the derived value
here. Here "nullable" sort of stands for "Nullable<>", IOW, it's "the type of
the result local is Nullable<>" rather than "the returned IDL type is nullable".

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:727: # Nullable type where the corresponding
C++ type supports a null value.
On 2014/07/10 14:43:57, haraken wrote:
> 
> Let's add a comment about what C++ type supports it how.

Done.

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

https://codereview.chromium.org/385603002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:255: {% elif method.is_use_result_local
and not method.union_arguments %}
On 2014/07/10 14:43:57, haraken wrote:
> 
> Can we fix the code generator so that we always use the local variable
(probably
> before this CL)? It's a bit shame that we have to introduce use_result_local.

I had that for a while. The generated code ends up uglier, but mostly that's of
no significance of course.

I think the worry is really only whether introducing an unnecessary local of
type RefPtr<> will cost in performance due to increased ref-count churn.

[Jens Widell, 2014-07-10 14:57:57]

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:123: is_nullable = idl_type.is_nullable
and not idl_type.is_nullable_simple
On 2014/07/10 14:56:44, Jens Lindström wrote:
> (Maybe you meant that
> it's confusing that they *aren't* mutually exclusive?)

Or maybe I'm just confused. :-)

Either way, I understand what you mean.

[Jens Widell, 2014-07-10 15:03:44]

Naming of things...

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:123: is_nullable = idl_type.is_nullable
and not idl_type.is_nullable_simple
On 2014/07/10 14:43:57, haraken wrote:
> something like is_xxx_nullable and is_yyy_nullable

How about is_implicit_nullable (for "simple") and is_explicit_nullable for the
other case (Nullable<> here and "bool& isNull" for attributes)?

I also like is_native_nullable/is_natively_nullable for the "simple" case, but
can't come up with anything that matches that for the other case.

[haraken, 2014-07-10 15:11:44]

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:123: is_nullable = idl_type.is_nullable
and not idl_type.is_nullable_simple
On 2014/07/10 15:03:43, Jens Lindström wrote:
> On 2014/07/10 14:43:57, haraken wrote:
> > something like is_xxx_nullable and is_yyy_nullable
> 
> How about is_implicit_nullable (for "simple") and is_explicit_nullable for the
> other case (Nullable<> here and "bool& isNull" for attributes)?
> 
> I also like is_native_nullable/is_natively_nullable for the "simple" case, but
> can't come up with anything that matches that for the other case.

is_implicit_nullable and is_explicit_nullable sound good to me.

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

https://codereview.chromium.org/385603002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:255: {% elif method.is_use_result_local
and not method.union_arguments %}
On 2014/07/10 14:56:44, Jens Lindström wrote:
> On 2014/07/10 14:43:57, haraken wrote:
> > 
> > Can we fix the code generator so that we always use the local variable
> (probably
> > before this CL)? It's a bit shame that we have to introduce
use_result_local.
> 
> I had that for a while. The generated code ends up uglier, but mostly that's
of
> no significance of course.
> 
> I think the worry is really only whether introducing an unnecessary local of
> type RefPtr<> will cost in performance due to increased ref-count churn.

I think we should investigate. Since you're taking off to a vacation, I'm fine
with landing this CL first, but we definitely need to clean up the local
variable issue in the future.

[Jens Widell, 2014-07-10 15:16:37]

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/385603002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:123: is_nullable = idl_type.is_nullable
and not idl_type.is_nullable_simple
On 2014/07/10 15:11:44, haraken wrote:
> On 2014/07/10 15:03:43, Jens Lindström wrote:
> > On 2014/07/10 14:43:57, haraken wrote:
> > > something like is_xxx_nullable and is_yyy_nullable
> > 
> > How about is_implicit_nullable (for "simple") and is_explicit_nullable for
the
> > other case (Nullable<> here and "bool& isNull" for attributes)?
> > 
> > I also like is_native_nullable/is_natively_nullable for the "simple" case,
but
> > can't come up with anything that matches that for the other case.
> 
> is_implicit_nullable and is_explicit_nullable sound good to me.

Are you fine with fixing that in a follow-up?

[haraken, 2014-07-10 15:21:37]

LGTM

> > > How about is_implicit_nullable (for "simple") and is_explicit_nullable for
> the
> > > other case (Nullable<> here and "bool& isNull" for attributes)?
> > > 
> > > I also like is_native_nullable/is_natively_nullable for the "simple" case,
> but
> > > can't come up with anything that matches that for the other case.
> > 
> > is_implicit_nullable and is_explicit_nullable sound good to me.
> 
> Are you fine with fixing that in a follow-up?

OK.

- "is_nullable" means whether the IDL type is nullable or not.
- "is_explicit_nullable" means that we should use Nullable<T> in C++.
- "is_implicit_nullable" means that we should use C++ default values to
represent null values.

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

https://codereview.chromium.org/385603002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:70: def use_result_local(method):

use_result_local => use_local_result

https://codereview.chromium.org/385603002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:169: 'is_use_result_local':
use_result_local(method),

is_use_result_local => use_local_result

[Jens Widell, 2014-07-10 15:28:54]

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

https://codereview.chromium.org/385603002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:255: {% elif method.is_use_result_local
and not method.union_arguments %}
On 2014/07/10 15:11:44, haraken wrote:
> I think we should investigate.

Very quick investigation (I just hacked my use_local_result() to always return
True):

gen/blink/bindings/modules/v8/V8EntrySync.cpp:242:57: error: conversion from
'WebCore::EntrySync*' to 'WTF::RawPtr<WebCore::DirectoryEntrySync>' is ambiguous

That's not exactly a reason not to do it, but it means it might not be 100 %
straight-forward.

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

https://codereview.chromium.org/385603002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:70: def use_result_local(method):
On 2014/07/10 15:21:37, haraken wrote:
> 
> use_result_local => use_local_result

Done.

https://codereview.chromium.org/385603002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:169: 'is_use_result_local':
use_result_local(method),
On 2014/07/10 15:21:37, haraken wrote:
> 
> is_use_result_local => use_local_result

Done.

[Jens Widell, 2014-07-10 15:29:08]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-10 15:29:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/385603002/40001

[commit-bot: I haz the power, 2014-07-10 16:42:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16262)

[commit-bot: I haz the power, 2014-07-10 17:49:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:49:51]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[haraken, 2014-07-10 23:23:19]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-10 23:24:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/385603002/40001

[commit-bot: I haz the power, 2014-07-10 23:25:01]

Change committed as 177887