Reland: Web MIDI: split build rules for media/midi

Reland: Web MIDI: split build rules for media/midi

Changes from the first coommit.
- Define right dependencies to xdisplaycheck
- Fix build rules for midi_unittests_apk to run on Android
- Rebase error fixes (build rule deopendency adjustments)

Original commit is:
https://crrev.com/31624cad1a3b5cf26950b864837ba44b3aa38331

Original description follows:

Split MIDI related stuffs from media as a component in build files.
Also, MIDI related unit tests are split into midi_unittests.

BUG=475869
TEST=git cl try
TEST=ninja midi_unittests_apk && ./build/android/test_runner.py gtest midi_unittests

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

[Takashi Toyoshima, 2015-04-20 15:50:48]

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

[Takashi Toyoshima, 2015-04-20 15:52:58]

Dale, can you start reviewing this?
This will need other owners' reviews, but I'd start it from media/ owners since
major changes happen in media.
(maybe the next will be testing)

[DaleCurtis, 2015-04-20 16:23:37]

Looks great, thanks for doing this! I wonder if we want a top level midi/ folder
now :)

Otherwise, just a few questions.

https://codereview.chromium.org/1065743003/diff/300001/media/midi/BUILD.gn
File media/midi/BUILD.gn (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/BUILD.gn#ne...
media/midi/BUILD.gn:41: defines = [ "MEDIA_IMPLEMENTATION" ]
I think this one is fine to keep, we usually stick to <toplevel_dir>_EXPORT
naming style.

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi.gyp
File media/midi/midi.gyp (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi.gyp#ne...
media/midi/midi.gyp:76: '../../crypto/crypto.gyp:crypto',
What's this dep for?

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi_manage...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:77: int device_id =
static_cast<int>(devices_.size());
Is this code reachable / overflowable by a render process? I.e., could a
renderer get > INT_MAX devices inserted?

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_de...
File media/midi/usb_midi_device_factory_android.cc (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_de...
media/midi/usb_midi_device_factory_android.cc:33:
midi::Java_UsbMidiDeviceFactoryAndroid_close(
Do you want to point everything inside midi dir in midi namespace?

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_ou...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream.cc:101: uint8 code_index =
static_cast<uint8>(message_size) + 0x4;
Same question for all these casts, if any of these come from the renderer or can
be influenced by the render it's worrying.

[Takashi Toyoshima, 2015-04-21 06:50:46]

I'd keep it inside media for now.

https://codereview.chromium.org/1065743003/diff/300001/media/midi/BUILD.gn
File media/midi/BUILD.gn (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/BUILD.gn#ne...
media/midi/BUILD.gn:41: defines = [ "MEDIA_IMPLEMENTATION" ]
On 2015/04/20 16:23:37, DaleCurtis wrote:
> I think this one is fine to keep, we usually stick to <toplevel_dir>_EXPORT
> naming style.

Done.

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi.gyp
File media/midi/midi.gyp (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi.gyp#ne...
media/midi/midi.gyp:76: '../../crypto/crypto.gyp:crypto',
It's for crypto/sha2.h from midi_manager_alsa.cc.

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi_manage...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:77: int device_id =
static_cast<int>(devices_.size());
Practically, it never happen. I think 127 is the maximum device number per one
USB host. Even without USB spec restriction, INT_MAX is big enough to count
physical connected devices and other code path assumes the number of device is
countable by 32-bit integer. E.g., uint32 port_index.

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_de...
File media/midi/usb_midi_device_factory_android.cc (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_de...
media/midi/usb_midi_device_factory_android.cc:33:
midi::Java_UsbMidiDeviceFactoryAndroid_close(
I'm planning to move everything under media/midi into media::midi namespace
after this CL. At that time, I'll remove these midi:: namespace prefixes.

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_ou...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/1065743003/diff/300001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream.cc:101: uint8 code_index =
static_cast<uint8>(message_size) + 0x4;
Incoming |data| is parsed inside this while loop, and each message is picked up
from |data|. Each message is less than 3-bytes here by the USB MIDI spec
restriction.

We had one such bug before that malicious renderer can make the browser crash by
specifying out of range index via IPC on Android. Fortunately, clusterfuzz found
it before shipping. For now, we had range checks in IPC receiver, midi_host.cc.
Also, onflight data size is managed by a flow control between renderers and the
browser.

[Takashi Toyoshima, 2015-04-21 06:53:45]

toyoshim@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Takashi Toyoshima, 2015-04-21 06:59:09]

+phajdan.jr
Since there are not major concerns in media/, can you start reviewing
testing/buildbot/ that is secondary part of this CL.
Actually, I didn't have much understanding on what
"can_use_on_swarming_builders" is, and applied the same rules with the original
media_unittests against midi_unittests.

[Takashi Toyoshima, 2015-04-21 08:46:04]

Dale: FYI, the next CL to change the namespace is
https://codereview.chromium.org/1086073004/

[Paweł Hajdan Jr., 2015-04-21 15:50:08]

testing/buildbot LGTM

[DaleCurtis, 2015-04-21 15:58:30]

Thanks for the details! lgtm

[Takashi Toyoshima, 2015-04-21 18:41:05]

toyoshim@chromium.org changed reviewers:
+ dpranke@google.com

[Takashi Toyoshima, 2015-04-21 18:41:29]

toyoshim@chromium.org changed reviewers:
+ dpranke@chromium.org
- dpranke@google.com

[Takashi Toyoshima, 2015-04-21 18:43:05]

+dpranke for gn and gyp files. Can you take a look?

[Takashi Toyoshima, 2015-04-21 18:43:38]

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

[Takashi Toyoshima, 2015-04-21 18:43:56]

+boliu for android_webview

[Takashi Toyoshima, 2015-04-21 18:44:26]

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

[Takashi Toyoshima, 2015-04-21 18:44:49]

+damienv for chromecast/

[Takashi Toyoshima, 2015-04-21 18:45:25]

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

[Takashi Toyoshima, 2015-04-21 18:45:45]

+jam for content/

[Takashi Toyoshima, 2015-04-21 18:46:20]

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

[Takashi Toyoshima, 2015-04-21 18:46:43]

+thestig for tools/valgrind/

[jam, 2015-04-21 19:10:24]

On 2015/04/21 18:45:45, Takashi Toyoshima (chromium) wrote:
> +jam for content/

content lgtm

[boliu, 2015-04-21 19:43:26]

android_webview rs lgtm

[damienv1, 2015-04-21 20:11:54]

damienv@chromium.org changed reviewers:
+ lcwu@chromium.org

[damienv1, 2015-04-21 20:12:13]

damienv@chromium.org changed reviewers:
+ gunsch@chromium.org

[Dirk Pranke, 2015-04-21 20:31:27]

lgtm.

https://codereview.chromium.org/1065743003/diff/320001/media/midi/BUILD.gn
File media/midi/BUILD.gn (right):

https://codereview.chromium.org/1065743003/diff/320001/media/midi/BUILD.gn#ne...
media/midi/BUILD.gn:16: # NOT for exporting.
You can use ' visibility = [ ":*" ]' to make this effectively private to this
file, if you like.

[Lei Zhang, 2015-04-21 21:10:18]

On 2015/04/21 18:46:43, Takashi Toyoshima (chromium) wrote:
> +thestig for tools/valgrind/

lgtm

You may want to submit a master.chrome.memory.fyi change so the memory.fyi bots
run these tests too.

[Takashi Toyoshima, 2015-04-22 05:37:18]

Lei: Does master.chrome.memory.fyi exist in an internal repository? I could not
find it both in internal and public. So more information would be helpful.

Now I still didn't get l-g-t-m from chromecast/ owners, but it's a simple gyp
change. Other reviews may cover it?

https://codereview.chromium.org/1065743003/diff/320001/media/midi/BUILD.gn
File media/midi/BUILD.gn (right):

https://codereview.chromium.org/1065743003/diff/320001/media/midi/BUILD.gn#ne...
media/midi/BUILD.gn:16: # NOT for exporting.
On 2015/04/21 20:31:27, Dirk Pranke wrote:
> You can use ' visibility = [ ":*" ]' to make this effectively private to this
> file, if you like.

Done.

[Lei Zhang, 2015-04-22 05:40:47]

On 2015/04/22 05:37:18, Takashi Toyoshima (chromium) wrote:
> Lei: Does master.chrome.memory.fyi exist in an internal repository? I could
not
> find it both in internal and public. So more information would be helpful.

Oh, I meant master.chromium.memory.fyi.

[Takashi Toyoshima, 2015-04-22 06:56:30]

Thanks. I find it, and will send out a CL separately.

[Takashi Toyoshima, 2015-04-22 08:13:02]

CC: yhirano to avoid merge conflict that may be happening from java source files
replacement.

[Takashi Toyoshima, 2015-04-22 08:13:13]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-04-22 08:13:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, thestig@chromium.org, jam@chromium.org,
boliu@chromium.org, dalecurtis@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1065743003/#ps340001
(title: "use visibility")

[commit-bot: I haz the power, 2015-04-22 08:13:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1065743003/340001

[commit-bot: I haz the power, 2015-04-22 08:50:30]

Committed patchset #19 (id:340001)

[commit-bot: I haz the power, 2015-04-22 08:51:37]

Patchset 19 (id:??) landed as
https://crrev.com/31624cad1a3b5cf26950b864837ba44b3aa38331
Cr-Commit-Position: refs/heads/master@{#326247}

[aboxhall, 2015-04-22 18:22:42]

A revert of this CL (patchset #19 id:340001) has been created in
https://codereview.chromium.org/1085843003/ by aboxhall@chromium.org.

The reason for reverting is: Very sorry to have to revert this after so much
work, but midi_unittests has been consistently broken on Android Tests bot e.g.
https://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28db....

[Takashi Toyoshima, 2015-04-30 11:34:18]

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

[Takashi Toyoshima, 2015-04-30 11:37:42]

The problem was fixed (TEST=ninja midi_unittests_apk &&
./build/android/test_runner.py gtest midi_unittests).

Add yhirano@ for the second review on differences between the previous CL (Patch
Set 20) and the revised CL (Patch Set 21).

[yhirano, 2015-04-30 11:40:13]

lgtm

[Takashi Toyoshima, 2015-04-30 14:45:25]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-04-30 14:45:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, thestig@chromium.org, jam@chromium.org,
boliu@chromium.org, dalecurtis@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1065743003/#ps380001
(title: "fixed patch set")

[commit-bot: I haz the power, 2015-04-30 14:46:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1065743003/380001

[commit-bot: I haz the power, 2015-04-30 14:52:24]

Committed patchset #21 (id:380001)

[commit-bot: I haz the power, 2015-04-30 14:53:06]

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