media: Use native BufferingState in media mojo interfaces

media: Use native BufferingState in media mojo interfaces

BUG=611224
TEST=mojo_media_unittests still pass

Committed: https://crrev.com/6350fd61235fcee7ceb8fc610bc2a6b302c5d951
Cr-Commit-Position: refs/heads/master@{#424759}

[xhwang, 2016-09-29 20:24:59]

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

[xhwang, 2016-09-29 20:24:59]

NOT ready for full review, but I have a question for now on this first patch
set.

yzshen: FYI since you are OOO.

rockot: 

I am trying to replace mojo media type converters with typemaps. I just updated
two files to use "[Native] enum.." and have not added typemap files, but this is
actually compiling and tests are passing.

In renderer.mojom.h, we are using ::media::mojom::BufferingState, which is
defined as 

using BufferingState = mojo::NativeEnum;

in media_types.mojom-shared.h. It seems NativeEnum is just int32_t. I guess it's
just converting the enums to integer for serialization (see
UnmappedNativeEnumSerializerImpl).

My question is, should we prevent this from working? What if someone misuse this
by accident? Is there any legit use case for unmapped native enum?

[xhwang, 2016-09-29 22:12:27]

Description was changed from

==========
media: Use native media enums in media mojo interfaces

BUG=611224
TEST=mojo_media_unittests still pass
==========

to

==========
media: Use native BufferingState in media mojo interfaces

BUG=611224
TEST=mojo_media_unittests still pass
==========

[Ken Rockot(use gerrit already), 2016-09-29 22:13:49]

On 2016/09/29 at 20:24:59, xhwang wrote:
> NOT ready for full review, but I have a question for now on this first patch
set.
> 
> yzshen: FYI since you are OOO.
> 
> rockot: 
> 
> I am trying to replace mojo media type converters with typemaps. I just
updated two files to use "[Native] enum.." and have not added typemap files, but
this is actually compiling and tests are passing.
> 
> In renderer.mojom.h, we are using ::media::mojom::BufferingState, which is
defined as 
> 
> using BufferingState = mojo::NativeEnum;
> 
> in media_types.mojom-shared.h. It seems NativeEnum is just int32_t. I guess
it's just converting the enums to integer for serialization (see
UnmappedNativeEnumSerializerImpl).
> 
> My question is, should we prevent this from working? What if someone misuse
this by accident? Is there any legit use case for unmapped native enum?

Maybe we should prevent it, but it seems hard to do. It's useful to have these
map to int, and ints are trivial to serialize. We'd have to make native enums a
special type if we wanted to specialize different compile-time behavior e.g.
detecting the presence of a typemap.

I don't think it will be a common enough problem to be worth any amount of
effort, since use of native enums should be pretty rare.

In doing IPC conversions I've generally just moved enum definitions into mojom
or duplicated them and used explicit EnumTraits to map. The latter is more work,
but I don't think it's *much* more work, and it's more flexible than [Native].
Using [Native] restricts all dependent interfaces to being usable only in in
non-Blink Chromium C++ code, which is pretty limiting.

[xhwang, 2016-09-29 22:17:44]

This CL will serve as an example. If it looks good. I'll mass converting more
enum types.

rockot: This CL is now ready for review. PTAL. Also please take a look at my
previous question.

[xhwang, 2016-09-29 22:38:43]

On 2016/09/29 22:13:49, Ken Rockot wrote:
> On 2016/09/29 at 20:24:59, xhwang wrote:
> > NOT ready for full review, but I have a question for now on this first patch
> set.
> > 
> > yzshen: FYI since you are OOO.
> > 
> > rockot: 
> > 
> > I am trying to replace mojo media type converters with typemaps. I just
> updated two files to use "[Native] enum.." and have not added typemap files,
but
> this is actually compiling and tests are passing.
> > 
> > In renderer.mojom.h, we are using ::media::mojom::BufferingState, which is
> defined as 
> > 
> > using BufferingState = mojo::NativeEnum;
> > 
> > in media_types.mojom-shared.h. It seems NativeEnum is just int32_t. I guess
> it's just converting the enums to integer for serialization (see
> UnmappedNativeEnumSerializerImpl).
> > 
> > My question is, should we prevent this from working? What if someone misuse
> this by accident? Is there any legit use case for unmapped native enum?
> 
> Maybe we should prevent it, but it seems hard to do. It's useful to have these
> map to int, and ints are trivial to serialize. We'd have to make native enums
a
> special type if we wanted to specialize different compile-time behavior e.g.
> detecting the presence of a typemap.
> 
> I don't think it will be a common enough problem to be worth any amount of
> effort, since use of native enums should be pretty rare.

Agreed. Just to make sure we are aware of the potential (small) risk.
 
> In doing IPC conversions I've generally just moved enum definitions into mojom
> or duplicated them and used explicit EnumTraits to map. The latter is more
work,
> but I don't think it's *much* more work, and it's more flexible than [Native].
> Using [Native] restricts all dependent interfaces to being usable only in in
> non-Blink Chromium C++ code, which is pretty limiting.

Yeah, I am experimenting to see which solution is better. For EnumTraits, I just
felt there are too much boilerplate code to add. Also, if there's a typo in
those code it'll be hard to catch, unless we have tests. But the existing tests
also look pretty scary.

This makes me feel we should have a script to generate the EnumTraits. Or maybe
we should be able to define a enum in the mojom file, which is exactly the same
as a native definition, then auto-generate the EnumTraits at compile time, as
part of the bindings generation. All these actually looks more similar to using
IPC::ParamTraits. That's why I am using IPC::ParamTraits for enums now (in this
CL)..

[Ken Rockot(use gerrit already), 2016-09-30 17:22:12]

On 2016/09/29 at 22:38:43, xhwang wrote:
> On 2016/09/29 22:13:49, Ken Rockot wrote:
> > On 2016/09/29 at 20:24:59, xhwang wrote:
> > > NOT ready for full review, but I have a question for now on this first
patch
> > set.
> > > 
> > > yzshen: FYI since you are OOO.
> > > 
> > > rockot: 
> > > 
> > > I am trying to replace mojo media type converters with typemaps. I just
> > updated two files to use "[Native] enum.." and have not added typemap files,
but
> > this is actually compiling and tests are passing.
> > > 
> > > In renderer.mojom.h, we are using ::media::mojom::BufferingState, which is
> > defined as 
> > > 
> > > using BufferingState = mojo::NativeEnum;
> > > 
> > > in media_types.mojom-shared.h. It seems NativeEnum is just int32_t. I
guess
> > it's just converting the enums to integer for serialization (see
> > UnmappedNativeEnumSerializerImpl).
> > > 
> > > My question is, should we prevent this from working? What if someone
misuse
> > this by accident? Is there any legit use case for unmapped native enum?
> > 
> > Maybe we should prevent it, but it seems hard to do. It's useful to have
these
> > map to int, and ints are trivial to serialize. We'd have to make native
enums a
> > special type if we wanted to specialize different compile-time behavior e.g.
> > detecting the presence of a typemap.
> > 
> > I don't think it will be a common enough problem to be worth any amount of
> > effort, since use of native enums should be pretty rare.
> 
> Agreed. Just to make sure we are aware of the potential (small) risk.
>  
> > In doing IPC conversions I've generally just moved enum definitions into
mojom
> > or duplicated them and used explicit EnumTraits to map. The latter is more
work,
> > but I don't think it's *much* more work, and it's more flexible than
[Native].
> > Using [Native] restricts all dependent interfaces to being usable only in in
> > non-Blink Chromium C++ code, which is pretty limiting.
> 
> Yeah, I am experimenting to see which solution is better. For EnumTraits, I
just felt there are too much boilerplate code to add. Also, if there's a typo in
those code it'll be hard to catch, unless we have tests. But the existing tests
also look pretty scary.
> 
> This makes me feel we should have a script to generate the EnumTraits. Or
maybe we should be able to define a enum in the mojom file, which is exactly the
same as a native definition, then auto-generate the EnumTraits at compile time,
as part of the bindings generation. All these actually looks more similar to
using IPC::ParamTraits. That's why I am using IPC::ParamTraits for enums now (in
this CL)..

In this case, really I think you should just move the enum definition into
mojom. You can define a type alias and constants to satisfy existing references
if you don't want to update them all at once, though I don't think there are
even enough references to these values to justify that extra step.

In general, if an enum is not already being used for legacy IPC, I see it as a
regression to introduce legacy IPC traits for it just so you can use the native
typemapping feature. This feature is only meant to support incremental porting
of existing IPCs.

[xhwang, 2016-09-30 23:36:26]

On 2016/09/30 17:22:12, Ken Rockot wrote:
> On 2016/09/29 at 22:38:43, xhwang wrote:
> > On 2016/09/29 22:13:49, Ken Rockot wrote:
> > > On 2016/09/29 at 20:24:59, xhwang wrote:
> > > > NOT ready for full review, but I have a question for now on this first
> patch
> > > set.
> > > > 
> > > > yzshen: FYI since you are OOO.
> > > > 
> > > > rockot: 
> > > > 
> > > > I am trying to replace mojo media type converters with typemaps. I just
> > > updated two files to use "[Native] enum.." and have not added typemap
files,
> but
> > > this is actually compiling and tests are passing.
> > > > 
> > > > In renderer.mojom.h, we are using ::media::mojom::BufferingState, which
is
> > > defined as 
> > > > 
> > > > using BufferingState = mojo::NativeEnum;
> > > > 
> > > > in media_types.mojom-shared.h. It seems NativeEnum is just int32_t. I
> guess
> > > it's just converting the enums to integer for serialization (see
> > > UnmappedNativeEnumSerializerImpl).
> > > > 
> > > > My question is, should we prevent this from working? What if someone
> misuse
> > > this by accident? Is there any legit use case for unmapped native enum?
> > > 
> > > Maybe we should prevent it, but it seems hard to do. It's useful to have
> these
> > > map to int, and ints are trivial to serialize. We'd have to make native
> enums a
> > > special type if we wanted to specialize different compile-time behavior
e.g.
> > > detecting the presence of a typemap.
> > > 
> > > I don't think it will be a common enough problem to be worth any amount of
> > > effort, since use of native enums should be pretty rare.
> > 
> > Agreed. Just to make sure we are aware of the potential (small) risk.
> >  
> > > In doing IPC conversions I've generally just moved enum definitions into
> mojom
> > > or duplicated them and used explicit EnumTraits to map. The latter is more
> work,
> > > but I don't think it's *much* more work, and it's more flexible than
> [Native].
> > > Using [Native] restricts all dependent interfaces to being usable only in
in
> > > non-Blink Chromium C++ code, which is pretty limiting.
> > 
> > Yeah, I am experimenting to see which solution is better. For EnumTraits, I
> just felt there are too much boilerplate code to add. Also, if there's a typo
in
> those code it'll be hard to catch, unless we have tests. But the existing
tests
> also look pretty scary.
> > 
> > This makes me feel we should have a script to generate the EnumTraits. Or
> maybe we should be able to define a enum in the mojom file, which is exactly
the
> same as a native definition, then auto-generate the EnumTraits at compile
time,
> as part of the bindings generation. All these actually looks more similar to
> using IPC::ParamTraits. That's why I am using IPC::ParamTraits for enums now
(in
> this CL)..
> 
> In this case, really I think you should just move the enum definition into
> mojom. You can define a type alias and constants to satisfy existing
references
> if you don't want to update them all at once, though I don't think there are
> even enough references to these values to justify that extra step.

We decided a while back that the "media" target shouldn't depend on "mojo". I
see the trend that "mojo" is being used everywhere, but historically we did have
cases where "media" is used for a single-processed application where it doesn't
need to know anything about ipc/mojo. Given that we are trying to modularize
components so they can be reused more easily/broadly, I am still hesitant to
make this move.
 
> In general, if an enum is not already being used for legacy IPC, I see it as a
> regression to introduce legacy IPC traits for it just so you can use the
native
> typemapping feature. This feature is only meant to support incremental porting
> of existing IPCs.

I agree adding new IPC traits is bad. What I was proposing is that in case we do
need EnumTraits (e.g. for 3rd party libs where they can't use mojom enums
either), we have some automatic process to generate those EnumTraits (similar to
what we have for IPC ParamStraits) instead of having to add them manually.

[xhwang, 2016-10-11 21:37:36]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org

[xhwang, 2016-10-11 21:37:37]

rockot: Given the discussion on
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/y-c0l8nKcn0,
could you please take a look again on this CL?

jam: FYI

[Ken Rockot(use gerrit already), 2016-10-11 22:01:06]

lgtm

[xhwang, 2016-10-11 23:10:17]

rebase only

[xhwang, 2016-10-11 23:15:20]

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

[commit-bot: I haz the power, 2016-10-11 23:16:07]

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

[xhwang, 2016-10-11 23:16:16]

xhwang@chromium.org changed reviewers:
+ dcheng@chromium.org
- jam@chromium.org, yzshen@chromium.org

[xhwang, 2016-10-11 23:16:16]

move jam and yzshen1 to CC

dcheng: Please OWNERS review mojom and type converter changes.

[dcheng, 2016-10-11 23:22:04]

mojo lgtm

[commit-bot: I haz the power, 2016-10-11 23:44:43]

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

[commit-bot: I haz the power, 2016-10-11 23:44:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2016-10-11 23:49:59]

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

[commit-bot: I haz the power, 2016-10-11 23:50:46]

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-10-12 01:57:56]

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

[commit-bot: I haz the power, 2016-10-12 01:57:57]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-10-12 16:21:02]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-10-12 16:21:02]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2386443002/#ps80001
(title: "media: Use native media enums in media mojo interfaces")

[commit-bot: I haz the power, 2016-10-12 16:21:18]

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-10-12 16:27:10]

Description was changed from

==========
media: Use native BufferingState in media mojo interfaces

BUG=611224
TEST=mojo_media_unittests still pass
==========

to

==========
media: Use native BufferingState in media mojo interfaces

BUG=611224
TEST=mojo_media_unittests still pass
==========

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-12 16:29:00]

Description was changed from

==========
media: Use native BufferingState in media mojo interfaces

BUG=611224
TEST=mojo_media_unittests still pass
==========

to

==========
media: Use native BufferingState in media mojo interfaces

BUG=611224
TEST=mojo_media_unittests still pass

Committed: https://crrev.com/6350fd61235fcee7ceb8fc610bc2a6b302c5d951
Cr-Commit-Position: refs/heads/master@{#424759}
==========

[commit-bot: I haz the power, 2016-10-12 16:29:01]

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