Support nullable types in mojom.

Support nullable types in mojom.

This CL updates the lexer, parser and also the internal representation of mojom modules. The changes to the generators will be in a separate CL.

This CL doesn't change data_tests.py because it has been broken for quite a while. We could fix it in a separate CL if we think that it is still useful.

BUG=324170
TEST=those updated test files.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287997

[yzshen1, 2014-07-31 20:49:47]

Hi, Darin.

Would you please take a look? Thanks!

[darin (slow to review), 2014-08-03 05:57:58]

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:220: return (kind ==
STRING or kind == HANDLE or kind == DCPIPE or
what does it mean for a handle-type to be nullable? perhaps this falls out of
wanting to have nullable or non-nullable interfaces?

i'm also a bit curious what "interface&?" precisely means. i guess it means that
it is optional for the caller to not request a remote object (i.e., to not
supply a message pipe handle to be hooked up to a remote object). then, i
suppose "interface?&" might also be valid? that might mean it is OK for the
request to result in a null remote object? hmm... i wonder if that latter case
is useful.

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:224: class
Nullable(Kind):
I'm kind of surprised to see Nullable as a subclass of Kind. Isn't it more of an
attribute of a Kind (or of the Kinds that can be nullable)?

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/parse/translate.py (right):

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/parse/translate.py:42: if
decorated_kind[0] not in ('s', 'h', 'a', 'r', 'x'):
nit: might be nice to get this tuple a name, like nullable_kinds.

[yzshen1, 2014-08-03 08:27:46]

Thanks Darin!

(I haven't uploaded a new patch. I will do that after some questions are
answered.)

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:220: return (kind ==
STRING or kind == HANDLE or kind == DCPIPE or
On 2014/08/03 05:57:58, darin wrote:
> what does it mean for a handle-type to be nullable? perhaps this falls out of
> wanting to have nullable or non-nullable interfaces?
I think for a handle type, nullable means it could be an invalid handle.

At the mojo message level, an interface is a handle. I think it is not very
different from other kinds of handles. The fact that interface is wrapped as
"pointer" is a bindings-level feature and may be subject to change. For example,
in the future, we may create a wrapper DataConsumerPtr for
handle<data_consumer_pipe>, and data_consumer_ptr.is_null() <=> the underlying
pipe handle is invalid.
> 
> i'm also a bit curious what "interface&?" precisely means. i guess it means
that
> it is optional for the caller to not request a remote object (i.e., to not
> supply a message pipe handle to be hooked up to a remote object).
Exactly, that is what I have in mind.
It is similar to passing a NULL pointer to an output parameter.

> then, i
> suppose "interface?&" might also be valid? that might mean it is OK for the
> request to result in a null remote object? hmm... i wonder if that latter case
> is useful.
I think it may make sense to not support interface?&. Currently, we don't have a
way to distinguish "resulting in a NULL remote object" from "resulting in a
valid remote object but the connection is closed quickly after that".  We could
add support for sending back different error codes when the connection is
closed. But it doesn't seem very useful to me.

Another reason is that I wish "nullable" could have a very simple and consistent
meaning at the mojo message level: the pointer field may be NULL or the handle
field may be invalid. "interface?&" is different, the corresponding handle field
should always be valid! (If it is invalid, that is interface&?)

What do you think?

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:224: class
Nullable(Kind):
On 2014/08/03 05:57:58, darin wrote:
> I'm kind of surprised to see Nullable as a subclass of Kind. Isn't it more of
an
> attribute of a Kind (or of the Kinds that can be nullable)?

I agree that it should be redesigned. The biggest issue is that it is awkward to
use:
kind1 = Struct('some_struct')
kind2 = Nullable(kind1)
|kind1| and |kind2| should both be considered as struct kinds, but we cannot
call |kind2.name/fields/etc.|!

I think alternative (1) is: both |kind1| and |kind2| are Struct instances, the
only difference that the |is_nullable| member of |kind2| is set to True. The
drawback is that a lot of members are duplicated in |kind1| and |kind2|.

Alternative (2): 
kind1 = Struct('some_struct')
kind2 = NullableStruct(kind1)
# NullableStruct defines properties pointing to corresponding members in Struct,
so that |kind2.name/fields/etc.| work as expected.
This doesn't have the drawback of alternative (1), but seems ugly. :/

What do you think?

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/parse/translate.py (right):

https://codereview.chromium.org/437643002/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/parse/translate.py:42: if
decorated_kind[0] not in ('s', 'h', 'a', 'r', 'x'):
On 2014/08/03 05:57:58, darin wrote:
> nit: might be nice to get this tuple a name, like nullable_kinds.

Sounds good.

[yzshen1, 2014-08-04 22:49:39]

Hi, Darin.

New patch has been uploaded. Please take a look.

Thanks!

[yzshen1, 2014-08-05 05:11:43]

https://codereview.chromium.org/437643002/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/437643002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:280:
nullable_kind.__dict__  = ref_kind.__dict__.copy()
Hmm... Thinking more about this solution and it seems pretty hacky. :/

|ref_kind| is subject to change after calling MakeNullableKind(), e.g., the
|fields| attribute is filled. A shallow copy happens to work for this case
because |nullable_kind| and |ref_kind| points to the same list . However, if we
change the code to assign a new list to |fields|, |nullable_kind| and |ref_kind|
will be out of sync.

Maybe the previous solution makes more sense (having Nullable as a wrapper class
which points to a Struct/Array/etc. kind). Hmm...

[yzshen1, 2014-08-06 21:39:27]

https://codereview.chromium.org/437643002/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/437643002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:280:
nullable_kind.__dict__  = ref_kind.__dict__.copy()
On 2014/08/05 05:11:43, yzshen1 wrote:
> Hmm... Thinking more about this solution and it seems pretty hacky. :/
> 
> |ref_kind| is subject to change after calling MakeNullableKind(), e.g., the
> |fields| attribute is filled. A shallow copy happens to work for this case
> because |nullable_kind| and |ref_kind| points to the same list . However, if
we
> change the code to assign a new list to |fields|, |nullable_kind| and
|ref_kind|
> will be out of sync.
> 
> Maybe the previous solution makes more sense (having Nullable as a wrapper
class
> which points to a Struct/Array/etc. kind). Hmm...

I think I have addressed this issue in the latest patch. Please take a look.

[darin (slow to review), 2014-08-06 22:48:26]

LGTM

https://codereview.chromium.org/437643002/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/437643002/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:97: NLBL_STRING      
= ReferenceKind('?s', True)
I know we have been abbreviating labels here, but maybe it is getting out of
control. "NLBL" is pretty darn obscure. Maybe spelling things out would be
better.

[yzshen1, 2014-08-06 22:55:42]

https://codereview.chromium.org/437643002/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/437643002/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:97: NLBL_STRING      
= ReferenceKind('?s', True)
Done. Thanks!

[yzshen1, 2014-08-07 00:23:59]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:25:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/437643002/100001

[commit-bot: I haz the power, 2014-08-07 07:47:00]

Change committed as 287997