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

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
Committed: https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228
Cr-Commit-Position: refs/heads/master@{#408196}

[yzshen1, 2016-07-26 03:06:01]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 03:06:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-07-26 03:14:05]

yzshen@chromium.org changed reviewers:
+ reed@chromium.org, rockot@chromium.org

[yzshen1, 2016-07-26 03:14:05]

Hi, Ken and Mike.

Would you please take a look? Thanks!

Mike: OWNER for skia
Ken: everything

[yzshen1, 2016-07-26 03:42:23]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 03:42:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 03:53:34]

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

[commit-bot: I haz the power, 2016-07-26 03:53:35]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[reed1, 2016-07-26 12:54:46]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-07-26 12:54:47]

skia lgtm

[Ken Rockot(use gerrit already), 2016-07-26 15:24:19]

change fundamentally looks good, though it seems like you need to update several
more bits of code. all the errors look trivial (i.e. copying from a
base::Optional<T> value rather than a T value) so lgtm

[yzshen1, 2016-07-26 19:54:56]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 19:55:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-07-26 20:43:50]

yzshen@chromium.org changed reviewers:
+ dcheng@chromium.org, lhchavez@chromium.org, sky@chromium.org

[yzshen1, 2016-07-26 20:43:50]

+more owners:

Luis: please look at components/arc/ changes.

Scott: please look at the changes to animations.mojom and window_tree.mojom. I
changed two nullable fields to non-nullable. No one has tried to send null
values because the custom types don't support null state at all (there are no
IsNull/SetToNull defined in the corresponding StructTraits).

Daniel: security review for mojom files.

Thanks!

[commit-bot: I haz the power, 2016-07-26 21:21:02]

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

[commit-bot: I haz the power, 2016-07-26 21:21:03]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-26 21:26:16]

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/animations.mojom (right):

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/animations.mojom:25: gfx.mojom.Transform
transform;
This structure is like a union, so we want to support '?'. Is there a reason we
can't allow it?

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:107: SetHitTestMask(uint32
window_id, gfx.mojom.Rect mask);
We want to allow null here too.

[yzshen1, 2016-07-26 21:32:36]

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/animations.mojom (right):

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/animations.mojom:25: gfx.mojom.Transform
transform;
On 2016/07/26 21:26:16, sky wrote:
> This structure is like a union, so we want to support '?'. Is there a reason
we
> can't allow it?

Sure we can do that.
I made this change because the code never took advantage of nullable:  prior to
this CL the field was mapped to gfx::Transform and according to how the
StructTraits is written, it's impossible to encode gfx::Transform as null.

With this change, we will map gfx.mojom.Transform? to
base::Optional<gfx::Transform>. If you think that is useful, I can keep it as
nullable and modify the code accordingly.

WDYT?

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2179273002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:107: SetHitTestMask(uint32
window_id, gfx.mojom.Rect mask);
On 2016/07/26 21:26:16, sky wrote:
> We want to allow null here too.

Please see my comment for the other place.

[dcheng, 2016-07-27 08:07:29]

mojom LGTM once sky's concerns are addressed.

I have no strong feeling about whether these should be nullable or not: I
personally think we should probably be using nullable less, rather than more,
but that's a stylistic preference.

[yzshen1, 2016-07-27 16:12:06]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 16:12:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-07-27 16:13:08]

On 2016/07/27 08:07:29, dcheng wrote:
> mojom LGTM once sky's concerns are addressed.
> 
> I have no strong feeling about whether these should be nullable or not: I
> personally think we should probably be using nullable less, rather than more,
> but that's a stylistic preference.

Hi, Scott.

New patch has been uploaded. Please take another look. Thanks!

[Luis Héctor Chávez, 2016-07-27 16:30:12]

components/arc lgtm

[commit-bot: I haz the power, 2016-07-27 17:21:20]

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

[commit-bot: I haz the power, 2016-07-27 17:21:21]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-27 18:22:56]

Thanks! LGTM

[yzshen1, 2016-07-27 18:29:34]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-07-27 18:29:35]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
reed@google.com, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2179273002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-07-27 18:29:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 18:34:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-27 18:37:20]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228
Cr-Commit-Position: refs/heads/master@{#408196}
==========

[commit-bot: I haz the power, 2016-07-27 18:37:22]

Patchset 5 (id:??) landed as
https://crrev.com/289d8c4438650661530f17fbd47c8b678a98a228
Cr-Commit-Position: refs/heads/master@{#408196}