Web MIDI: use midi_service.mojom for media::midi::Result

Web MIDI: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing
enums Result via mojom to be shared with Blink and content.

BUG=582327
Committed: https://crrev.com/729663ff68ab25dcbcfd06266f2f6cb62e26b3e6
Cr-Commit-Position: refs/heads/master@{#425637}

[Takashi Toyoshima, 2016-10-07 11:07:34]

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

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

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-07 11:11:14]

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

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

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Takashi Toyoshima, 2016-10-07 18:02:27]

Description was changed from

==========
Web MIDI mojofication: introduce blink::mojom::WebMidiResult

As a first step for the mojofication, start from introducing enums and
structures by mojom to be shared with Blink and content.

BUG=582327
==========

to

==========
Web MIDI mojofication: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing enums by mojom to
be shared with Blink and content.

BUG=582327
==========

[Takashi Toyoshima, 2016-10-07 18:04:48]

Description was changed from

==========
Web MIDI mojofication: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing enums by mojom to
be shared with Blink and content.

BUG=582327
==========

to

==========
Web MID: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing enums by
mojom to be shared with Blink and content.

BUG=582327
==========

[Takashi Toyoshima, 2016-10-07 18:05:15]

Description was changed from

==========
Web MID: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing enums by
mojom to be shared with Blink and content.

BUG=582327
==========

to

==========
Web MID: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing
enums Result via mojom to be shared with Blink and content.

BUG=582327
==========

[Takashi Toyoshima, 2016-10-07 18:56:17]

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

[commit-bot: I haz the power, 2016-10-07 18:56:50]

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-07 20:38:34]

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

[commit-bot: I haz the power, 2016-10-07 20:38:34]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-10-11 06:43:12]

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

[commit-bot: I haz the power, 2016-10-11 06:43:25]

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

[Takashi Toyoshima, 2016-10-11 06:44:03]

toyoshim@chromium.org changed reviewers:
+ reillyg@chromium.org

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

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

[commit-bot: I haz the power, 2016-10-11 07:27:24]

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

[Takashi Toyoshima, 2016-10-11 08:46:18]

Reilly, can you take a look at this first step of Web MIDI mojofication?

I'm planning to make it in following steps,
 - Use Mojo for enum Result
 - Use Mojo for enum MidiPortState
 - Use Mojo for struct MidiPortInfo
 - Replace Web MIDI IPC with Mojo that depends on these Mojo enums and struct

I'm starting from a simple immigration so that I can ensure I'm placing things
in right places, not breaking layering rules.

media/midi is a component in terms of component build. And content use midi to
instantiate MidiManager.

[Reilly Grant (use Gerrit), 2016-10-11 17:57:57]

This generally looks good.

https://codereview.chromium.org/2404443002/diff/140001/media/midi/midi_servic...
File media/midi/midi_service.mojom (right):

https://codereview.chromium.org/2404443002/diff/140001/media/midi/midi_servic...
media/midi/midi_service.mojom:5: module media.midi;
Convention is to put mojo definitions into a namespace called "mojom" which is a
rare exception to the convention that we don't generally nest namespaces. So
this module should probably be either "midi.mojom", or "media.mojom" with types
renamed to start with "Midi".

https://codereview.chromium.org/2404443002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/DEPS (right):

https://codereview.chromium.org/2404443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/DEPS:15:
"+media/midi/midi_service.mojom-shared.h",
The shared header shouldn't be included directly. Use midi_service.mojom-blink.h
instead.

[Takashi Toyoshima, 2016-10-12 05:01:44]

https://codereview.chromium.org/2404443002/diff/140001/media/midi/midi_servic...
File media/midi/midi_service.mojom (right):

https://codereview.chromium.org/2404443002/diff/140001/media/midi/midi_servic...
media/midi/midi_service.mojom:5: module media.midi;
Thanks. So, probably midi.mojom looks better rather than media because midi is
historically placed in media, but it doesn't depend on media, and now we can
build it as a separate component. I thought eventually we would use top midi
namespace instead of media.midi in C++ world. So, I'd switch it now.

https://codereview.chromium.org/2404443002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/DEPS (right):

https://codereview.chromium.org/2404443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/DEPS:15:
"+media/midi/midi_service.mojom-shared.h",
WebMIDIAccessorClient.h includes this file, and it provides a client interface
that renderer_webmidiaccessor_impl implements in content.

So, if I use -blink.h here, that results in including -blink.h indirectly from
content. That results in compile error for failing including files from wtf/.
What's a right approach here?

[Reilly Grant (use Gerrit), 2016-10-12 17:48:06]

https://codereview.chromium.org/2404443002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/DEPS (right):

https://codereview.chromium.org/2404443002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/DEPS:15:
"+media/midi/midi_service.mojom-shared.h",
On 2016/10/12 at 05:01:44, toyoshim wrote:
> WebMIDIAccessorClient.h includes this file, and it provides a client interface
that renderer_webmidiaccessor_impl implements in content.
> 
> So, if I use -blink.h here, that results in including -blink.h indirectly from
content. That results in compile error for failing including files from wtf/.
What's a right approach here?

I checked with rockot@ and he says that including the mojom-shared.h file is the
correct thing to do here. Sorry for the confusion.

[Takashi Toyoshima, 2016-10-13 07:39:28]

toyoshim@chromium.org changed reviewers:
+ jochen@chromium.org, tsepez@chromium.org, yhirano@chromium.org

[Takashi Toyoshima, 2016-10-13 07:44:42]

Thank you, Reilly.

Since this direction looks working, I started adding other owners to reviewers
list.

reillyg@ still for Mojofication
jochen@ for test_runner, and content
tsepez@ for IPC, midi_message.h
yhirano@ for midi

Can you take a look?

[Tom Sepez, 2016-10-13 16:41:42]

lgtm

[Reilly Grant (use Gerrit), 2016-10-13 21:34:36]

lgtm

https://codereview.chromium.org/2404443002/diff/180001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2404443002/diff/180001/components/test_runner...
components/test_runner/test_runner.cc:1354: result ? midi::mojom::Result::OK
It's completely reasonable to add "using midi::mojom::Result" to the top of a
C++ file like this, though "Result" may be too generic to do that without
conflicts.

[Reilly Grant (use Gerrit), 2016-10-13 21:35:12]

Description was changed from

==========
Web MID: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing
enums Result via mojom to be shared with Blink and content.

BUG=582327
==========

to

==========
Web MIDI: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing
enums Result via mojom to be shared with Blink and content.

BUG=582327
==========

[Takashi Toyoshima, 2016-10-14 08:45:28]

https://codereview.chromium.org/2404443002/diff/180001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2404443002/diff/180001/components/test_runner...
components/test_runner/test_runner.cc:1354: result ? midi::mojom::Result::OK
Yeah, probably, I want not to use "using midi::mojom::Result" in common files
like test_runner, and use it in midi specific files, like midi_host.cc,
midi_message_filter.cc, and so on.

[jochen (gone - plz use gerrit), 2016-10-14 09:17:41]

lgtm

[yhirano, 2016-10-17 05:23:27]

lgtm

https://codereview.chromium.org/2404443002/diff/220001/content/browser/media/...
File content/browser/media/midi_host.h (right):

https://codereview.chromium.org/2404443002/diff/220001/content/browser/media/...
content/browser/media/midi_host.h:23: #include "media/midi/midi_port_info.h"
+media/midi/midi_service.mojom.h

https://codereview.chromium.org/2404443002/diff/220001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/2404443002/diff/220001/media/midi/midi_manage...
media/midi/midi_manager.cc:20: 
No blank line is needed here.

[Takashi Toyoshima, 2016-10-17 06:38:57]

https://codereview.chromium.org/2404443002/diff/220001/content/browser/media/...
File content/browser/media/midi_host.h (right):

https://codereview.chromium.org/2404443002/diff/220001/content/browser/media/...
content/browser/media/midi_host.h:23: #include "media/midi/midi_port_info.h"
On 2016/10/17 05:23:27, yhirano wrote:
> +media/midi/midi_service.mojom.h

Done.

https://codereview.chromium.org/2404443002/diff/220001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/2404443002/diff/220001/media/midi/midi_manage...
media/midi/midi_manager.cc:20: 
On 2016/10/17 05:23:27, yhirano wrote:
> No blank line is needed here.

Done.

https://codereview.chromium.org/2404443002/diff/240001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2404443002/diff/240001/content/browser/BUILD....
content/browser/BUILD.gn:83: "//media/midi:mojo_cpp_sources",
Adding the right include file results in this line being requested correctly by
gn --check

[Takashi Toyoshima, 2016-10-17 06:55:28]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-10-17 06:55:29]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
reillyg@chromium.org, jochen@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2404443002/#ps240001
(title: "yhirano@ review")

[commit-bot: I haz the power, 2016-10-17 06:55:45]

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-17 08:54:38]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-10-17 08:57:20]

Description was changed from

==========
Web MIDI: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing
enums Result via mojom to be shared with Blink and content.

BUG=582327
==========

to

==========
Web MIDI: use midi_service.mojom for media::midi::Result

As a first step for the mojofication, start from introducing
enums Result via mojom to be shared with Blink and content.

BUG=582327

Committed: https://crrev.com/729663ff68ab25dcbcfd06266f2f6cb62e26b3e6
Cr-Commit-Position: refs/heads/master@{#425637}
==========

[commit-bot: I haz the power, 2016-10-17 08:57:21]

Patchset 13 (id:??) landed as
https://crrev.com/729663ff68ab25dcbcfd06266f2f6cb62e26b3e6
Cr-Commit-Position: refs/heads/master@{#425637}

[Takashi Toyoshima, 2016-10-18 19:12:23]

Patchset #14 (id:260001) has been deleted