[Video Capture Service] Move Mojo structs and enums to media/capture/mojo

Move Mojo structs and enums to media/capture/mojo.

There are several structs and enums whose native C++ representation lives in media/capture, but whose correpsonding Mojo types and typemaps live in either content/common or services/video_capture.

This CL moves the Mojo types and typemaps to media/capture/mojo, so that Mojo types and corresponding native types live together.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing

Committed: https://crrev.com/b689c2a7b4937062619f435327188612a0040784
Cr-Commit-Position: refs/heads/master@{#428186}

[chfremer, 2016-10-11 23:34:03]

Description was changed from

==========
Move Mojo structs/enums/typemaps to media

BUG=
==========

to

==========
Move Mojo structs and enums from video_capture.mojom and content.mojom to
media.mojom.

Also move corresponding type mappings. 
This makes sense because the corresponding C++ types live in src/media.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]
CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-10-11 23:34:38]

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

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

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

[chfremer, 2016-10-11 23:35:45]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org, mcasas@chromium.org

[chfremer, 2016-10-11 23:35:46]

mcasas@: PTAL
emircan@: Optional Review

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

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

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

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chfremer, 2016-10-12 15:54:23]

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

[commit-bot: I haz the power, 2016-10-12 15:54:32]

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 17:02:12]

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

[commit-bot: I haz the power, 2016-10-12 17:02:13]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mcasas, 2016-10-12 17:18:26]

lgtm

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.mojom:102: // Kept in sync with
media::VideoPixelFormat via static_asserts.
nit: Totally unrelated to your CL, but would you
mind adding here a 
// TODO(...) use enum traits instead, https://crbug.com/655205 
?
(It's a new bug I just made, and marked GoodFirstBug).

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.cc (right):

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.cc:113: // TODO(mcasas): Use
EnumTraits for VideoPixelFormat, https://crbug.com/651897
nit: I was lazy and reused a bug number, would you mind
changing this to https://crbug.com/655205  ?
(A new bug I just made).

[chfremer, 2016-10-12 17:26:22]

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

[commit-bot: I haz the power, 2016-10-12 17:26:56]

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

[chfremer, 2016-10-12 17:39:20]

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.mojom:102: // Kept in sync with
media::VideoPixelFormat via static_asserts.
On 2016/10/12 17:18:26, mcasas wrote:
> nit: Totally unrelated to your CL, but would you
> mind adding here a 
> // TODO(...) use enum traits instead, https://crbug.com/655205 
> ?
> (It's a new bug I just made, and marked GoodFirstBug).

Done.

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.cc (right):

https://codereview.chromium.org/2405423002/diff/20001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.cc:113: // TODO(mcasas): Use
EnumTraits for VideoPixelFormat, https://crbug.com/651897
On 2016/10/12 17:18:26, mcasas wrote:
> nit: I was lazy and reused a bug number, would you mind
> changing this to https://crbug.com/655205  ?
> (A new bug I just made).

Done.

[chfremer, 2016-10-12 18:04:08]

chfremer@chromium.org changed reviewers:
+ avi@chromium.org, rockot@chromium.org, tsepez@chromium.org,
xhwang@chromium.org

[chfremer, 2016-10-12 18:04:08]

xhwang@ PTAL media/mojo/interfaces/*

tsepez@ PTAL security stuff (*.mojom, *_struct_traits.*)

avi@ please RS content/common/typemaps.gni and
content/common/video_capture.typemap

rockot@ please RS chromium_bindings_configuration.gni

[Avi (use Gerrit), 2016-10-12 18:08:07]

lgtm

stampity stamp

[xhwang, 2016-10-12 18:22:37]

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.typemap (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.typemap:25: "//media/capture",
drive-by comment: media/mojo shouldn't depend on media/capture. Does it make
sense to move capture related mojo interfaces to somewhere like
media/capture/interfaces?

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.cc (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.cc:50:
media::ResolutionChangePolicy* output) {
If you only use these in C++, you can probably use [Native]. See
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/y-c0l8nKcn0

But it's up to you since you already have these conversion functions.

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.h (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.h:60: return
static_cast<media::mojom::VideoPixelStorage>(format.pixel_storage);
You have static_cast here, do you have static_assert or test anywhere to make
sure the two enums are synced?

[Ken Rockot(use gerrit already), 2016-10-12 18:31:19]

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.typemap (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.typemap:25: "//media/capture",
On 2016/10/12 at 18:22:37, xhwang wrote:
> drive-by comment: media/mojo shouldn't depend on media/capture.

Does that mean the types being typemapped from media/capture need to move to an
acceptable public target, or does that mean that these types shouldn't be
typemapped?

> Does it make sense to move capture related mojo interfaces to somewhere like
media/capture/interfaces?

It was a conscious decision to place these new interfaces alongside the video
capture service.

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.h (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.h:60: return
static_cast<media::mojom::VideoPixelStorage>(format.pixel_storage);
On 2016/10/12 at 18:22:37, xhwang wrote:
> You have static_cast here, do you have static_assert or test anywhere to make
sure the two enums are synced?

Actually, I'd prefer that you didn't do static casts at all here. You should
either write out a switch to translate individual values, or introduce legacy
IPC traits and declare the mojom type as [Native]  (as done here:
https://codereview.chromium.org/2386443002)

I'm going to be adding a more convenient way to typemap a native enum to a mojom
declaration (one that doesn't require classic IPC traits) but it's not ready
yet.

static casts very easily mask errors. Writing out the translation for each value
guarantees that many types of errors are caught at compile time if one of the
enums changes without the other.

[xhwang, 2016-10-12 18:47:26]

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.typemap (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.typemap:25: "//media/capture",
On 2016/10/12 18:31:18, Ken Rockot wrote:
> On 2016/10/12 at 18:22:37, xhwang wrote:
> > drive-by comment: media/mojo shouldn't depend on media/capture.
> 
> Does that mean the types being typemapped from media/capture need to move to
an
> acceptable public target, or does that mean that these types shouldn't be
> typemapped?

I actually don't see any types in media/capture are being typemapped. Am I
missing something?

That being said, it seems to me "video_capture_types.h" and
"video_capturer_source.h" should be moved from media/base to media/capture. Then
we should have the video capture mojom types defined in media/capture as well.

> > Does it make sense to move capture related mojo interfaces to somewhere like
> media/capture/interfaces?
> 
> It was a conscious decision to place these new interfaces alongside the video
> capture service.

Sorry, I meant these video capture mojom enum types, not the actual capture mojo
interfaces.

[xhwang, 2016-10-12 18:54:43]

BTW, the CL title is ambiguous, maybe we should at least mention "video capture"
in the title.

[commit-bot: I haz the power, 2016-10-12 19:25:51]

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

[commit-bot: I haz the power, 2016-10-12 19:25:53]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-10-12 20:07:42]

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.typemap (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.typemap:25: "//media/capture",
On 2016/10/12 18:47:25, xhwang wrote:
> On 2016/10/12 18:31:18, Ken Rockot wrote:
> > On 2016/10/12 at 18:22:37, xhwang wrote:
> > > drive-by comment: media/mojo shouldn't depend on media/capture.
> > 
> > Does that mean the types being typemapped from media/capture need to move to
> an
> > acceptable public target, or does that mean that these types shouldn't be
> > typemapped?
> 
> I actually don't see any types in media/capture are being typemapped. Am I
> missing something?
> 
> That being said, it seems to me "video_capture_types.h" and
> "video_capturer_source.h" should be moved from media/base to media/capture.
Then
> we should have the video capture mojom types defined in media/capture as well.
> 
> > > Does it make sense to move capture related mojo interfaces to somewhere
like
> > media/capture/interfaces?
> > 
> > It was a conscious decision to place these new interfaces alongside the
video
> > capture service.
> 
> Sorry, I meant these video capture mojom enum types, not the actual capture
mojo
> interfaces.

I feel there is value in having the Mojo types and corresponding typemapped C++
structs/enums live in the same place. It also seems reasonable to keep the
capture-specific things separate from the other things in media.

How about I create a new CL that does the following: 
* Move "video_capture_types.h" and "video_capturer_source.h" to media/capture
* Create media/capture/mojo and move the capture-specific Mojo types and
typemaps into there.

The reason why I would like to move the Mojo equivalents of things like
VideoCaptureFormat and VideoCaptureParams out of services/video_capture is that
the Mojo API exposed by services/video_capture is different from the C++ API
exposed by the things in media/capture/video. For example, the service
guarantees that frames are delivered in I420, and therefore does not want to
expose a parameter for VideoPixelFormat.

[xhwang, 2016-10-12 20:16:12]

On 2016/10/12 20:07:42, chfremer wrote:
>
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
> File media/mojo/interfaces/media_types.typemap (right):
> 
>
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
> media/mojo/interfaces/media_types.typemap:25: "//media/capture",
> On 2016/10/12 18:47:25, xhwang wrote:
> > On 2016/10/12 18:31:18, Ken Rockot wrote:
> > > On 2016/10/12 at 18:22:37, xhwang wrote:
> > > > drive-by comment: media/mojo shouldn't depend on media/capture.
> > > 
> > > Does that mean the types being typemapped from media/capture need to move
to
> > an
> > > acceptable public target, or does that mean that these types shouldn't be
> > > typemapped?
> > 
> > I actually don't see any types in media/capture are being typemapped. Am I
> > missing something?
> > 
> > That being said, it seems to me "video_capture_types.h" and
> > "video_capturer_source.h" should be moved from media/base to media/capture.
> Then
> > we should have the video capture mojom types defined in media/capture as
well.
> > 
> > > > Does it make sense to move capture related mojo interfaces to somewhere
> like
> > > media/capture/interfaces?
> > > 
> > > It was a conscious decision to place these new interfaces alongside the
> video
> > > capture service.
> > 
> > Sorry, I meant these video capture mojom enum types, not the actual capture
> mojo
> > interfaces.
> 
> I feel there is value in having the Mojo types and corresponding typemapped
C++
> structs/enums live in the same place. It also seems reasonable to keep the
> capture-specific things separate from the other things in media.
> 
> How about I create a new CL that does the following: 
> * Move "video_capture_types.h" and "video_capturer_source.h" to media/capture
> * Create media/capture/mojo and move the capture-specific Mojo types and
> typemaps into there.
> 
> The reason why I would like to move the Mojo equivalents of things like
> VideoCaptureFormat and VideoCaptureParams out of services/video_capture is
that
> the Mojo API exposed by services/video_capture is different from the C++ API
> exposed by the things in media/capture/video. For example, the service
> guarantees that frames are delivered in I420, and therefore does not want to
> expose a parameter for VideoPixelFormat.

The plan sgtm. Thanks!

[Tom Sepez, 2016-10-12 20:32:08]

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.cc (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.cc:114: out->pixel_format =
static_cast<media::VideoPixelFormat>(data.pixel_format());
need to flag out-of-range values here.

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.h (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.h:60: return
static_cast<media::mojom::VideoPixelStorage>(format.pixel_storage);
What Ken said.  Typically, I'd expect to see a switch here.

[chfremer, 2016-10-21 15:32:50]

I have created a CL for moving files, which I would like to land before
continuing the work on this one.

https://codereview.chromium.org/2415703002/

I will continue here as soon as it has landed.

[chfremer, 2016-10-24 23:04:54]

Patchset #5 (id:80001) has been deleted

[chfremer, 2016-10-24 23:51:25]

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

[commit-bot: I haz the power, 2016-10-24 23:52:17]

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

[chfremer, 2016-10-24 23:57:13]

Description was changed from

==========
Move Mojo structs and enums from video_capture.mojom and content.mojom to
media.mojom.

Also move corresponding type mappings. 
This makes sense because the corresponding C++ types live in src/media.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]
CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
Move Mojo structs and enums to media/capture/mojo.

There are several structs and enums whose native C++ representation lives in
media/capture, but whose correpsonding Mojo types and typemaps live in either
content/common or services/video_capture. 

This CL moves the Mojo types and typemaps to media/capture/mojo, so that Mojo
types and corresponding native types live together.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]
CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[commit-bot: I haz the power, 2016-10-24 23:58:54]

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

[commit-bot: I haz the power, 2016-10-24 23:58:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[chfremer, 2016-10-26 16:57:11]

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

[commit-bot: I haz the power, 2016-10-26 16:57:49]

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-26 17:22:31]

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

[commit-bot: I haz the power, 2016-10-26 17:22:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chfremer, 2016-10-26 18:31:16]

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

[chfremer, 2016-10-26 18:31:34]

Patchset #7 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-10-26 18:32:18]

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-26 19:04:55]

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

[commit-bot: I haz the power, 2016-10-26 19:04:57]

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_...)

[chfremer, 2016-10-26 21:05:17]

Patchset #7 (id:160001) has been deleted

[chfremer, 2016-10-26 21:05:26]

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

[commit-bot: I haz the power, 2016-10-26 21:06:16]

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

[chfremer, 2016-10-26 23:07:13]

rockot@: PTAL
xhwang@: PTAL
tsepez@: PTAL

The CL [1] has landed and I am continuing this CL now. 
I have updated this CL based on the moved types in [1].
Also addressed the comments about replacing the static_cast<> with EnumTraits<>.

[1] https://codereview.chromium.org/2415703002/

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.typemap (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.typemap:25: "//media/capture",
On 2016/10/12 20:07:42, chfremer wrote:
> On 2016/10/12 18:47:25, xhwang wrote:
> > On 2016/10/12 18:31:18, Ken Rockot wrote:
> > > On 2016/10/12 at 18:22:37, xhwang wrote:
> > > > drive-by comment: media/mojo shouldn't depend on media/capture.
> > > 
> > > Does that mean the types being typemapped from media/capture need to move
to
> > an
> > > acceptable public target, or does that mean that these types shouldn't be
> > > typemapped?
> > 
> > I actually don't see any types in media/capture are being typemapped. Am I
> > missing something?
> > 
> > That being said, it seems to me "video_capture_types.h" and
> > "video_capturer_source.h" should be moved from media/base to media/capture.
> Then
> > we should have the video capture mojom types defined in media/capture as
well.
> > 
> > > > Does it make sense to move capture related mojo interfaces to somewhere
> like
> > > media/capture/interfaces?
> > > 
> > > It was a conscious decision to place these new interfaces alongside the
> video
> > > capture service.
> > 
> > Sorry, I meant these video capture mojom enum types, not the actual capture
> mojo
> > interfaces.
> 
> I feel there is value in having the Mojo types and corresponding typemapped
C++
> structs/enums live in the same place. It also seems reasonable to keep the
> capture-specific things separate from the other things in media.
> 
> How about I create a new CL that does the following: 
> * Move "video_capture_types.h" and "video_capturer_source.h" to media/capture
> * Create media/capture/mojo and move the capture-specific Mojo types and
> typemaps into there.
> 
> The reason why I would like to move the Mojo equivalents of things like
> VideoCaptureFormat and VideoCaptureParams out of services/video_capture is
that
> the Mojo API exposed by services/video_capture is different from the C++ API
> exposed by the things in media/capture/video. For example, the service
> guarantees that frames are delivered in I420, and therefore does not want to
> expose a parameter for VideoPixelFormat.

Done.

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types_typemap_traits.h (right):

https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types_typemap_traits.h:60: return
static_cast<media::mojom::VideoPixelStorage>(format.pixel_storage);
On 2016/10/12 20:32:08, Tom Sepez wrote:
> What Ken said.  Typically, I'd expect to see a switch here.

Done.

[commit-bot: I haz the power, 2016-10-26 23:49:21]

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

[commit-bot: I haz the power, 2016-10-26 23:49:22]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-10-27 04:57:09]

https://codereview.chromium.org/2405423002/diff/180001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2405423002/diff/180001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:123: "//media/capture:capture",
//media/capture is the same

https://codereview.chromium.org/2405423002/diff/180001/content/public/rendere...
File content/public/renderer/BUILD.gn (right):

https://codereview.chromium.org/2405423002/diff/180001/content/public/rendere...
content/public/renderer/BUILD.gn:75: "//media/capture:capture",
ditto

https://codereview.chromium.org/2405423002/diff/180001/content/shell/BUILD.gn
File content/shell/BUILD.gn (right):

https://codereview.chromium.org/2405423002/diff/180001/content/shell/BUILD.gn...
content/shell/BUILD.gn:243: "//media/capture:capture",
ditto

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:31: enum VideoPixelFormat {
OOC, why are you making this change?

[chfremer, 2016-10-27 17:15:44]

https://codereview.chromium.org/2405423002/diff/180001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2405423002/diff/180001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:123: "//media/capture:capture",
On 2016/10/27 04:57:08, xhwang wrote:
> //media/capture is the same

Done.

https://codereview.chromium.org/2405423002/diff/180001/content/public/rendere...
File content/public/renderer/BUILD.gn (right):

https://codereview.chromium.org/2405423002/diff/180001/content/public/rendere...
content/public/renderer/BUILD.gn:75: "//media/capture:capture",
On 2016/10/27 04:57:09, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/2405423002/diff/180001/content/shell/BUILD.gn
File content/shell/BUILD.gn (right):

https://codereview.chromium.org/2405423002/diff/180001/content/shell/BUILD.gn...
content/shell/BUILD.gn:243: "//media/capture:capture",
On 2016/10/27 04:57:09, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:31: enum VideoPixelFormat {
On 2016/10/27 04:57:09, xhwang wrote:
> OOC, why are you making this change?

My goal was to be able to use EnumTraits<> for typemapping in order to avoid the
static_cast<> that was used in the StructTraits<VideoCaptureFormat> here [1]. 

With the [Native] enum, I was not able to make things work. It seems to do this
conversion, Mojo requires us to provide a
IPC::ParamTraits<media::VideoPixelFormat>::Read(). We did not seem to have this,
and since IPC::ParamTraits is considered "legacy", I decided it would be better
to replace the native enum with a Mojo enum instead of implementing the missing
IPC::ParamTraits<>.

[1]
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...

[xhwang, 2016-10-27 17:22:37]

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:31: enum VideoPixelFormat {
On 2016/10/27 17:15:44, chfremer wrote:
> On 2016/10/27 04:57:09, xhwang wrote:
> > OOC, why are you making this change?
> 
> My goal was to be able to use EnumTraits<> for typemapping in order to avoid
the
> static_cast<> that was used in the StructTraits<VideoCaptureFormat> here [1]. 
> 
> With the [Native] enum, I was not able to make things work. It seems to do
this
> conversion, Mojo requires us to provide a
> IPC::ParamTraits<media::VideoPixelFormat>::Read(). We did not seem to have
this,
> and since IPC::ParamTraits is considered "legacy", I decided it would be
better
> to replace the native enum with a Mojo enum instead of implementing the
missing
> IPC::ParamTraits<>.
> 
> [1]
>
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...

IPC::ParamTraits is the current recommended/allowed way for enums, see:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/y-c0l8nKcn0

The manually typed EnumTraits is error prone and we don't have test covering it.
(You can add a test, but it'll be more boilerplate code).

The existing [Native] VideoPixelFormat should work for you. I think you are
missing some dependencies here. Could you please try again and post the exact
error here?

[Ken Rockot(use gerrit already), 2016-10-27 17:35:09]

On 2016/10/27 at 17:22:37, xhwang wrote:
>
https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
> File media/mojo/interfaces/media_types.mojom (right):
> 
>
https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
> media/mojo/interfaces/media_types.mojom:31: enum VideoPixelFormat {
> On 2016/10/27 17:15:44, chfremer wrote:
> > On 2016/10/27 04:57:09, xhwang wrote:
> > > OOC, why are you making this change?
> > 
> > My goal was to be able to use EnumTraits<> for typemapping in order to avoid
the
> > static_cast<> that was used in the StructTraits<VideoCaptureFormat> here
[1]. 
> > 
> > With the [Native] enum, I was not able to make things work. It seems to do
this
> > conversion, Mojo requires us to provide a
> > IPC::ParamTraits<media::VideoPixelFormat>::Read(). We did not seem to have
this,
> > and since IPC::ParamTraits is considered "legacy", I decided it would be
better
> > to replace the native enum with a Mojo enum instead of implementing the
missing
> > IPC::ParamTraits<>.
> > 
> > [1]
> >
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
> 
> IPC::ParamTraits is the current recommended/allowed way for enums, see:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/y-c0l8nKcn0

Sorry, while I completely agree that in this case you should keep using [Native]
because of media's unique constraints as a library, I really want to combat what
I think is some misinformation here.

This is not at all the "recommended" way to do enums. It is simply allowed when
there's sufficient justification. media has sufficient justification because you
want to avoid your library types having a dependency on mojom, and IIUC this is
because there are external parties which depend on //media types.

> 
> The manually typed EnumTraits is error prone 

There is no evidence to support this claim. We've had no problems with them that
I'm aware of. EnumTraits correctness is utterly trivial to verify in code
review.

> and we don't have test covering it. (You can add a test, but it'll be more
boilerplate code).

Test coverage for EnumTraits seems like extreme overkill.

> 
> The existing [Native] VideoPixelFormat should work for you. I think you are
missing some dependencies here. Could you please try again and post the exact
error here?

[chfremer, 2016-10-27 17:38:42]

On 2016/10/27 17:22:37, xhwang wrote:
>
https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
> File media/mojo/interfaces/media_types.mojom (right):
> 
>
https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
> media/mojo/interfaces/media_types.mojom:31: enum VideoPixelFormat {
> On 2016/10/27 17:15:44, chfremer wrote:
> > On 2016/10/27 04:57:09, xhwang wrote:
> > > OOC, why are you making this change?
> > 
> > My goal was to be able to use EnumTraits<> for typemapping in order to avoid
> the
> > static_cast<> that was used in the StructTraits<VideoCaptureFormat> here
[1]. 
> > 
> > With the [Native] enum, I was not able to make things work. It seems to do
> this
> > conversion, Mojo requires us to provide a
> > IPC::ParamTraits<media::VideoPixelFormat>::Read(). We did not seem to have
> this,
> > and since IPC::ParamTraits is considered "legacy", I decided it would be
> better
> > to replace the native enum with a Mojo enum instead of implementing the
> missing
> > IPC::ParamTraits<>.
> > 
> > [1]
> >
>
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
> 
> IPC::ParamTraits is the current recommended/allowed way for enums, see:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/y-c0l8nKcn0
>
> The manually typed EnumTraits is error prone and we don't have test covering
it.
> (You can add a test, but it'll be more boilerplate code).

Interesting discussion. I agree that manually typing the boiler plate for
EnumTraits<> seems unnecessary and can cause bugs due to typos.
I am all for auto-generating it. I don't see a good way of unit-testing a manual
implementation though, because the test code would be just a second "attempt" at
a correct implementation. Auto-generating is the only solution I see.
 
> The existing [Native] VideoPixelFormat should work for you. I think you are
> missing some dependencies here. Could you please try again and post the exact
> error here?

Here is the compiler error I am seeing when trying to use the native enum:

In file included from
../../media/capture/mojo/video_capture_types_typemap_traits.cc:5:
In file included from
../../media/capture/mojo/video_capture_types_typemap_traits.h:9:
In file included from gen/media/capture/mojo/video_capture_types.mojom.h:22:
In file included from ../../mojo/public/cpp/bindings/lib/serialization.h:17:
../../mojo/public/cpp/bindings/lib/native_enum_serialization.h:53:20: error: no
member named 'Read' in 'IPC::ParamTraits<media::VideoPixelFormat>'
    return Traits::Read(&pickle_view, &iter, output);
           ~~~~~~~~^
../../mojo/public/cpp/bindings/lib/serialization_forward.h:70:48: note: in
instantiation of member function
'mojo::internal::NativeEnumSerializerImpl<media::VideoPixelFormat>::Deserialize'
requested here
  return Serializer<MojomType, InputUserType>::Deserialize(
                                               ^
gen/media/capture/mojo/video_capture_types.mojom-shared.h:222:28: note: in
instantiation of function template specialization
'mojo::internal::Deserialize<mojo::NativeEnum, int &, media::VideoPixelFormat,
nullptr>' requested here
    return mojo::internal::Deserialize<::media::mojom::VideoPixelFormat>(
                           ^
../../media/capture/mojo/video_capture_types_typemap_traits.cc:110:13: note: in
instantiation of function template specialization
'media::mojom::VideoCaptureFormatDataView::ReadPixelFormat<media::VideoPixelFormat>'
requested here
  if (!data.ReadPixelFormat(&out->pixel_format))
            ^
1 error generated.

[xhwang, 2016-10-27 17:43:09]

On 2016/10/27 17:35:09, Ken Rockot wrote:
> On 2016/10/27 at 17:22:37, xhwang wrote:
> >
>
https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
> > File media/mojo/interfaces/media_types.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
> > media/mojo/interfaces/media_types.mojom:31: enum VideoPixelFormat {
> > On 2016/10/27 17:15:44, chfremer wrote:
> > > On 2016/10/27 04:57:09, xhwang wrote:
> > > > OOC, why are you making this change?
> > > 
> > > My goal was to be able to use EnumTraits<> for typemapping in order to
avoid
> the
> > > static_cast<> that was used in the StructTraits<VideoCaptureFormat> here
> [1]. 
> > > 
> > > With the [Native] enum, I was not able to make things work. It seems to do
> this
> > > conversion, Mojo requires us to provide a
> > > IPC::ParamTraits<media::VideoPixelFormat>::Read(). We did not seem to have
> this,
> > > and since IPC::ParamTraits is considered "legacy", I decided it would be
> better
> > > to replace the native enum with a Mojo enum instead of implementing the
> missing
> > > IPC::ParamTraits<>.
> > > 
> > > [1]
> > >
>
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
> > 
> > IPC::ParamTraits is the current recommended/allowed way for enums, see:
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/y-c0l8nKcn0
> 
> Sorry, while I completely agree that in this case you should keep using
[Native]
> because of media's unique constraints as a library, I really want to combat
what
> I think is some misinformation here.
> 
> This is not at all the "recommended" way to do enums. It is simply allowed
when
> there's sufficient justification. media has sufficient justification because
you
> want to avoid your library types having a dependency on mojom, and IIUC this
is
> because there are external parties which depend on //media types.
> 
> > 
> > The manually typed EnumTraits is error prone 
> 
> There is no evidence to support this claim. We've had no problems with them
that
> I'm aware of. EnumTraits correctness is utterly trivial to verify in code
> review.
> 
> > and we don't have test covering it. (You can add a test, but it'll be more
> boilerplate code).
> 
> Test coverage for EnumTraits seems like extreme overkill.
> 
> > 
> > The existing [Native] VideoPixelFormat should work for you. I think you are
> missing some dependencies here. Could you please try again and post the exact
> error here?

Thanks for the clarification and sorry for my misinformation. When I typed
"recommended" I felt something wrong already, but I didn't fix it, my bad.

For error-prone and tests, I definitely saw cases where people simply
copy/paste/replace to write simple enum conversion functions, and only be able
to find it in production when something weird happened.

[chfremer, 2016-10-27 20:29:34]

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

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

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

[chfremer, 2016-10-27 20:32:18]

xhwang@: PTAL

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2405423002/diff/180001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:31: enum VideoPixelFormat {
On 2016/10/27 17:22:37, xhwang wrote:
> On 2016/10/27 17:15:44, chfremer wrote:
> > On 2016/10/27 04:57:09, xhwang wrote:
> > > OOC, why are you making this change?
> > 
> > My goal was to be able to use EnumTraits<> for typemapping in order to avoid
> the
> > static_cast<> that was used in the StructTraits<VideoCaptureFormat> here
[1]. 
> > 
> > With the [Native] enum, I was not able to make things work. It seems to do
> this
> > conversion, Mojo requires us to provide a
> > IPC::ParamTraits<media::VideoPixelFormat>::Read(). We did not seem to have
> this,
> > and since IPC::ParamTraits is considered "legacy", I decided it would be
> better
> > to replace the native enum with a Mojo enum instead of implementing the
> missing
> > IPC::ParamTraits<>.
> > 
> > [1]
> >
>
https://codereview.chromium.org/2405423002/diff/60001/media/mojo/interfaces/m...
> 
> IPC::ParamTraits is the current recommended/allowed way for enums, see:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/y-c0l8nKcn0
> 
> The manually typed EnumTraits is error prone and we don't have test covering
it.
> (You can add a test, but it'll be more boilerplate code).
> 
> The existing [Native] VideoPixelFormat should work for you. I think you are
> missing some dependencies here. Could you please try again and post the exact
> error here?

I figured out the issue I had. I forgot to add #include
"media/base/ipc/media_param_traits_macros.h" to get the needed ParamTraits<>
implementation.

Reverted back to [Native]. Thanks for pointing this out.

[xhwang, 2016-10-27 20:40:34]

lgtm

[chfremer, 2016-10-27 20:45:21]

tsepez@: PTAL *traits.*, *.mojom
rockot@: Please RS chromium_bindings_configuration.gni

[Tom Sepez, 2016-10-27 20:54:25]

lgtm

[commit-bot: I haz the power, 2016-10-27 22:15:57]

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

[commit-bot: I haz the power, 2016-10-27 22:15:59]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-27 22:47:20]

rs lgtm

[chfremer, 2016-10-27 22:52:35]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-10-27 22:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2405423002/#ps220001
(title: "Revert Mojo enum VideoPixelFormat to native")

[commit-bot: I haz the power, 2016-10-27 22:53:17]

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-27 23:05:15]

Description was changed from

==========
Move Mojo structs and enums to media/capture/mojo.

There are several structs and enums whose native C++ representation lives in
media/capture, but whose correpsonding Mojo types and typemaps live in either
content/common or services/video_capture. 

This CL moves the Mojo types and typemaps to media/capture/mojo, so that Mojo
types and corresponding native types live together.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]
CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
Move Mojo structs and enums to media/capture/mojo.

There are several structs and enums whose native C++ representation lives in
media/capture, but whose correpsonding Mojo types and typemaps live in either
content/common or services/video_capture. 

This CL moves the Mojo types and typemaps to media/capture/mojo, so that Mojo
types and corresponding native types live together.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]
CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[commit-bot: I haz the power, 2016-10-27 23:05:17]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2016-10-27 23:07:42]

Description was changed from

==========
Move Mojo structs and enums to media/capture/mojo.

There are several structs and enums whose native C++ representation lives in
media/capture, but whose correpsonding Mojo types and typemaps live in either
content/common or services/video_capture. 

This CL moves the Mojo types and typemaps to media/capture/mojo, so that Mojo
types and corresponding native types live together.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]
CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
Move Mojo structs and enums to media/capture/mojo.

There are several structs and enums whose native C++ representation lives in
media/capture, but whose correpsonding Mojo types and typemaps live in either
content/common or services/video_capture.

This CL moves the Mojo types and typemaps to media/capture/mojo, so that Mojo
types and corresponding native types live together.

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]
CL1.7.2

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

Committed: https://crrev.com/b689c2a7b4937062619f435327188612a0040784
Cr-Commit-Position: refs/heads/master@{#428186}
==========

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

Patchset 9 (id:??) landed as
https://crrev.com/b689c2a7b4937062619f435327188612a0040784
Cr-Commit-Position: refs/heads/master@{#428186}