New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 624146 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 624136



Sign in to add a comment

Mojo C++ bindings: support mapping T and T? differently

Project Member Reported by yzshen@chromium.org, Jun 28 2016

Issue description

We should make using base::Optional<T> as nullable type very convenient, which should be a common case.
 
Two options that I can think up:

Option 1: add a new property to the type_mappings entries:
type_mappings = [
  "mojom.Foo = CustomFoo[nullable=same_type]",
  "mojom.Bar = CustomBar[nullable=optional_wrapper]",
  "mojom.Qux = CustomQux"
]

- By default a custom mapped type doesn't allow nullable.
- If |nullable=same_type| is specified, StructTraits::SetToNull() and IsNull() will be used to deal with null state.
- If |nullable=optional_wrapper| is specified, base::Optional<T> is used for nullable.


Option 2: use a separate entry for nullable:
type_mappings = [
  "mojom.Foo = CustomFoo",
  "mojom.Foo? = CustomFoo",
  "mojom.Bar = CustomBar",
  "mojom.Bar? = base::Optional<CustomBar>"
]

Pros (comparing with #1):
- It is more flexible, users can use whatever type they want for nullable.

Cons:
- It is more verbose.
- We want to treat base::Optional<> specially -- users don't have to write StructTraits for base::Optional<T> as long as there is a StructTriats<T> defined. #2 is a little less clear that it is treated specially.


I personally prefer #1 to #2. WDYT?

Given a second thought, maybe it makes more sense to change the default of #1 to "optional_wrapper" and get rid of "none"?


I like #1, and nullable=optional_wrapper as the default but do you mean add "none" rather than get rid of "none"?
Thanks, Ken.

At the beginning I was thinking that "none" could be used to enforce that a type T is never marked as "nullable" in mojom files. Thinking about it some more, however, I feel that maybe it is not that useful. And it could be easily added later if needed.

WDYT? Thanks!
SGTM
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/289d8c4438650661530f17fbd47c8b678a98a228

commit 289d8c4438650661530f17fbd47c8b678a98a228
Author: yzshen <yzshen@chromium.org>
Date: Wed Jul 27 18:34:41 2016

Mojo C++ bindings: support mapping T and T? differently.

By default, when you specify typemap entry "T=CustomT", T (non-nullable) will be mapped to CustomT; while T? will be mapped to base::Optional<CustomT>. You could use attribute "nullable_is_same_type" if you want to map T/T? to the same type: "T=CustomT[nullable_is_same_type]". In this case, the corresponding StructTraits<> should implement IsNull()/SetToNull() methods.

BUG= 624146 

Review-Url: https://codereview.chromium.org/2179273002
Cr-Commit-Position: refs/heads/master@{#408196}

[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/components/arc/test/fake_app_instance.cc
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/components/arc/test/fake_app_instance.h
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/device/bluetooth/public/interfaces/bluetooth_uuid.typemap
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/cpp/bindings/tests/struct_traits_unittest.cc
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/cpp/bindings/tests/struct_with_traits.typemap
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/cpp/bindings/tests/struct_with_traits_impl.cc
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/cpp/bindings/tests/struct_with_traits_impl.h
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.h
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/interfaces/bindings/tests/struct_with_traits.mojom
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/tools/bindings/generate_type_mappings.py
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/mojo/public/tools/bindings/generators/mojom_cpp_generator.py
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/services/ui/public/cpp/lib/window_tree_client.cc
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/services/ui/public/cpp/tests/test_window_tree.cc
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/services/ui/public/cpp/tests/test_window_tree.h
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/services/ui/ws/scheduled_animation_group.cc
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/services/ui/ws/window_tree.cc
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/services/ui/ws/window_tree.h
[modify] https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228/skia/public/interfaces/skbitmap.typemap

Status: Fixed (was: Untriaged)

Sign in to add a comment