[Web MIDI] Use Android MIDI API.

[Web MIDI] Use Android MIDI API.

This CL introduces WebMIDI implementation using Android native MIDI
API. The implementation is enabled only when the device has a newer sdk
version and the experimental flag "use-android-midi-api" is enabled.

BUG=486584
Committed: https://crrev.com/e8b2e7dee39f272747554f0b26451d15b786e06d
Cr-Commit-Position: refs/heads/master@{#349419}

[yhirano, 2015-08-18 13:52:04]

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

[yhirano, 2015-08-18 13:53:03]

Patchset #2 (id:20001) has been deleted

[yhirano, 2015-08-18 13:53:13]

Patchset #2 (id:40001) has been deleted

[yhirano, 2015-08-18 13:53:19]

Patchset #2 (id:60001) has been deleted

[yhirano, 2015-08-18 13:53:24]

Patchset #1 (id:1) has been deleted

[yhirano, 2015-08-18 14:11:54]

yhirano@chromium.org changed reviewers:
+ philburk@google.com

[philburk, 2015-08-19 01:40:16]

Looks like it is headed in the right direction.
I am concerned about opening input ports even if not used, and about ignoring
timestamps.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
This will open all the input ports on the device in exclusive access mode. An
input port can have only one writer. So this will prevent other apps from
sharing the device. Can you open input ports only as needed?

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
openInputPort() can fail and return null if the port is already in use by
another app.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
File media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:23: * A Java
class implementing media::midi::MidiDeviceAndroid functionality.
maybe c/MidiDeviceAndroid/MidiManagerAndroid/

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:36: * The
device information instances which are begin initialized.
"are begin"
typo?

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:95:
openDevice(info);
This automatically opens the input ports on every device. Thus running Chrome on
Android will prevent any other app from using Android MIDI.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_input_p...
File media/midi/midi_input_port_android.cc (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_input_p...
media/midi/midi_input_port_android.cc:31: // We don't use |timestamp| because
MidiManager wants base::TimeTicks.
Ignoring timestamp can cause rendering errors. Some messages may get rendered
out of order.
Can you convert the timestamp, which is MONOTONIC nanoseconds, into
base::TimeTicks?

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_output_...
File media/midi/midi_output_port_android.cc (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_output_...
media/midi/midi_output_port_android.cc:19: const uint8* head = data.size() ?
&data[0] : nullptr;
If there is no data maybe skip the send call.

[yhirano, 2015-08-19 06:24:42]

Patchset #2 (id:100001) has been deleted

[yhirano, 2015-08-19 06:32:03]

Thanks for the review!

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
On 2015/08/19 01:40:15, philburk wrote:
> openInputPort() can fail and return null if the port is already in use by
> another app.

Done.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
On 2015/08/19 01:40:15, philburk wrote:
> This will open all the input ports on the device in exclusive access mode. An
> input port can have only one writer. So this will prevent other apps from
> sharing the device. Can you open input ports only as needed?

Hm, WebMIDI spec has explicit open / close, but we haven't implemented it yet.
I added open() method for ports and made it and close() callable from the C++
layer.
I call open() from media::midi::MidiManagerAndroid as a workaround and added
TODO. Is that OK?

Please note that even with the current implementation midi ports are not opened
until a web page uses WebMIDI functions.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
File media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:23: * A Java
class implementing media::midi::MidiDeviceAndroid functionality.
On 2015/08/19 01:40:15, philburk wrote:
> maybe c/MidiDeviceAndroid/MidiManagerAndroid/

Done.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:36: * The
device information instances which are begin initialized.
On 2015/08/19 01:40:15, philburk wrote:
> "are begin"
> typo?

Done.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:95:
openDevice(info);
On 2015/08/19 01:40:15, philburk wrote:
> This automatically opens the input ports on every device. Thus running Chrome
on
> Android will prevent any other app from using Android MIDI.

(partially) addressed.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_input_p...
File media/midi/midi_input_port_android.cc (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_input_p...
media/midi/midi_input_port_android.cc:31: // We don't use |timestamp| because
MidiManager wants base::TimeTicks.
On 2015/08/19 01:40:15, philburk wrote:
> Ignoring timestamp can cause rendering errors. Some messages may get rendered
> out of order.
> Can you convert the timestamp, which is MONOTONIC nanoseconds, into
> base::TimeTicks?

Done.

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_output_...
File media/midi/midi_output_port_android.cc (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/midi_output_...
media/midi/midi_output_port_android.cc:19: const uint8* head = data.size() ?
&data[0] : nullptr;
On 2015/08/19 01:40:15, philburk wrote:
> If there is no data maybe skip the send call.

Done.

[philburk, 2015-08-19 20:22:27]

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
> I call open() from media::midi::MidiManagerAndroid as a workaround and added
> TODO. Is that OK?

Maybe. I am not sure how your code works. I just know the Android input ports
should not be opened if they are not used. You could even dynamically open it on
the first write and then close it when the page is left.

A WebMidi app might want to write MIDI to an Android MIDI processing app that
then writes to an Android synth. But Chrome may open the synth ports and block
the processing app. That would be bad.

> Please note that even with the current implementation midi ports are not
opened
> until a web page uses WebMIDI functions.

When are they closed?

https://codereview.chromium.org/1177973003/diff/140001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/140001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:78: if
(mPort == null) {
Potential race condition if mPort is set null by another thread before
mPort.close().
This might happen if closing the browser and killing off multiple tabs.

[yhirano, 2015-08-20 07:38:29]

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
On 2015/08/19 20:22:26, philburk wrote:
> > I call open() from media::midi::MidiManagerAndroid as a workaround and added
> > TODO. Is that OK?
> 
> Maybe. I am not sure how your code works. I just know the Android input ports
> should not be opened if they are not used. You could even dynamically open it
on
> the first write and then close it when the page is left.
>

With this CL MidiInputPort is opened when the first send signal arrives from the
renderer process.

To "close it when the page is left" is a bit difficult and I would like to
implement it in a separate CL.
 
> A WebMidi app might want to write MIDI to an Android MIDI processing app that
> then writes to an Android synth. But Chrome may open the synth ports and block
> the processing app. That would be bad.
> 
> > Please note that even with the current implementation midi ports are not
> opened
> > until a web page uses WebMIDI functions.
> 
> When are they closed?

Never. It should be fixed, though.

https://codereview.chromium.org/1177973003/diff/140001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/140001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:78: if
(mPort == null) {
On 2015/08/19 20:22:26, philburk wrote:
> Potential race condition if mPort is set null by another thread before
> mPort.close().
> This might happen if closing the browser and killing off multiple tabs.

All IPC messages arrive on the io thread and I make all Java callbacks run on
the thread using android.os.Handle.

I've just noticed that I didn't set the correct handle for MidiSender.connect.
Can you tell me which thread is used to run the callback?

[yhirano, 2015-09-04 13:35:54]

Phil, can you take a look again?

For the first point, I would like to land this change before fixing open /
close. It should not be a big problem because this change is under an
experimental flag.

Thanks,

On 2015/08/20 07:38:29, yhirano wrote:
>
https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
> File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java
(right):
> 
>
https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
> media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
> mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
> On 2015/08/19 20:22:26, philburk wrote:
> > > I call open() from media::midi::MidiManagerAndroid as a workaround and
added
> > > TODO. Is that OK?
> > 
> > Maybe. I am not sure how your code works. I just know the Android input
ports
> > should not be opened if they are not used. You could even dynamically open
it
> on
> > the first write and then close it when the page is left.
> >
> 
> With this CL MidiInputPort is opened when the first send signal arrives from
the
> renderer process.
> 
> To "close it when the page is left" is a bit difficult and I would like to
> implement it in a separate CL.
>  
> > A WebMidi app might want to write MIDI to an Android MIDI processing app
that
> > then writes to an Android synth. But Chrome may open the synth ports and
block
> > the processing app. That would be bad.
> > 
> > > Please note that even with the current implementation midi ports are not
> > opened
> > > until a web page uses WebMIDI functions.
> > 
> > When are they closed?
> 
> Never. It should be fixed, though.
> 
>
https://codereview.chromium.org/1177973003/diff/140001/media/midi/java/src/or...
> File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
> (right):
> 
>
https://codereview.chromium.org/1177973003/diff/140001/media/midi/java/src/or...
> media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:78: if
> (mPort == null) {
> On 2015/08/19 20:22:26, philburk wrote:
> > Potential race condition if mPort is set null by another thread before
> > mPort.close().
> > This might happen if closing the browser and killing off multiple tabs.
> 
> All IPC messages arrive on the io thread and I make all Java callbacks run on
> the thread using android.os.Handle.
> 
> I've just noticed that I didn't set the correct handle for MidiSender.connect.
> Can you tell me which thread is used to run the callback?

[Takashi Toyoshima, 2015-09-07 16:42:20]

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/80001/media/midi/java/src/org...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:46:
mOutputPorts[i] = new MidiOutputPortAndroid(device.openInputPort(i));
Web MIDI introduced open() and close() methods recently in order to make it easy
to implement backend for Windows. Android will need it too.
But these open() and close() methods are not implemented yet, and will be
implemented in weeks. So I think it's ok to leave this TODO now. I think open()
and close() will be implemented before this new implementation goes out of the
experimental flag.

Since MidiManager and related classes are single instance, and shared from
multiple tabs running on isolated processes, we need to manage reference count
to realize open and close. That's why we can not easily implement open and close
in android specific implementation side. But, once all existing tabs are closed,
Chrome browser process exists and releases all devices now.

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

https://codereview.chromium.org/1177973003/diff/220001/media/midi/BUILD.gn#ne...
media/midi/BUILD.gn:99: "midi_device_android.cc",
optional: since now we have many files for Android, it would be nice to have
media/midi/android sub directory and move these Android specific source files
and java directory under the android.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:42: // Note
we use "input" and "output" in the WebMIDI manner.
s/WebMIDI/Web MIDI/

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiInputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiInputPortAndroid.java:16: //
Note "InputPort" is named in the WebMIDI manner. It corresponds to
MidiOutputPort class in the
s/WebMIDI/Web MIDI/

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:90: },
mHandler);
This is a comment for the open/close implementation that will be introduced in
another CL in the future.

By the way, cannot we set a handler to know that the device is released from
another application?
When MIDIPort#open() fails, Web MIDI marks the port status to "pending" and is
expected to perform "open" operation automatically when the device gets to be
available again.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:18: //
Note "OutputPort" is named in the WebMIDI manner. It corresponds to
MidiInputPort class in the
s/WebMIDI/Web MIDI/

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:69: // We
can do nothing here. Just ignore the error.
It's nice to log the error.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_device...
File media/midi/midi_device_android.h (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_device...
media/midi/midi_device_android.h:37: jobject raw_device() const { return
raw_device_.obj(); }
Maybe "IsEqualTo(const jobject raw_device) const" looks better for now?

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_input_...
File media/midi/midi_input_port_android.cc (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_input_...
media/midi/midi_input_port_android.cc:48: // nanoseconds,
Maybe "// nanoseconds. Both are monotonic."

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

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_manage...
media/midi/midi_manager_android.cc:125: // We implicitely open input ports here,
because there are no signal
s/implicitely/implicitly/

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_manage...
File media/midi/midi_manager_android.h (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_manage...
media/midi/midi_manager_android.h:27: // MidiManagerAndroid is a midi manager
subclass for Android M or newer. For
a MidiManager subclass?

[yhirano, 2015-09-10 11:43:50]

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

https://codereview.chromium.org/1177973003/diff/220001/media/midi/BUILD.gn#ne...
media/midi/BUILD.gn:99: "midi_device_android.cc",
On 2015/09/07 16:42:19, Takashi Toyoshima (ooo-Sep.30) wrote:
> optional: since now we have many files for Android, it would be nice to have
> media/midi/android sub directory and move these Android specific source files
> and java directory under the android.

I'd put TODO now and think about the directory structure in a separate CL.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:42: // Note
we use "input" and "output" in the WebMIDI manner.
On 2015/09/07 16:42:19, Takashi Toyoshima (ooo-Sep.30) wrote:
> s/WebMIDI/Web MIDI/

Done.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiInputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiInputPortAndroid.java:16: //
Note "InputPort" is named in the WebMIDI manner. It corresponds to
MidiOutputPort class in the
On 2015/09/07 16:42:19, Takashi Toyoshima (ooo-Sep.30) wrote:
> s/WebMIDI/Web MIDI/

Done.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:90: },
mHandler);
On 2015/09/07 16:42:19, Takashi Toyoshima (ooo-Sep.30) wrote:
> This is a comment for the open/close implementation that will be introduced in
> another CL in the future.
> 
> By the way, cannot we set a handler to know that the device is released from
> another application?
> When MIDIPort#open() fails, Web MIDI marks the port status to "pending" and is
> expected to perform "open" operation automatically when the device gets to be
> available again.

I don't know such an API. I planned to reject the promise in such a case. Phil,
is there such an API?

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:18: //
Note "OutputPort" is named in the WebMIDI manner. It corresponds to
MidiInputPort class in the
On 2015/09/07 16:42:20, Takashi Toyoshima (ooo-Sep.30) wrote:
> s/WebMIDI/Web MIDI/

Done.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:69: // We
can do nothing here. Just ignore the error.
On 2015/09/07 16:42:19, Takashi Toyoshima (ooo-Sep.30) wrote:
> It's nice to log the error.

Done.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_device...
File media/midi/midi_device_android.h (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_device...
media/midi/midi_device_android.h:37: jobject raw_device() const { return
raw_device_.obj(); }
On 2015/09/07 16:42:20, Takashi Toyoshima (ooo-Sep.30) wrote:
> Maybe "IsEqualTo(const jobject raw_device) const" looks better for now?

Done.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_input_...
File media/midi/midi_input_port_android.cc (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_input_...
media/midi/midi_input_port_android.cc:48: // nanoseconds,
On 2015/09/07 16:42:20, Takashi Toyoshima (ooo-Sep.30) wrote:
> Maybe "// nanoseconds. Both are monotonic."

Done.

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

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_manage...
media/midi/midi_manager_android.cc:125: // We implicitely open input ports here,
because there are no signal
On 2015/09/07 16:42:20, Takashi Toyoshima (ooo-Sep.30) wrote:
> s/implicitely/implicitly/

Thanks, done.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_manage...
File media/midi/midi_manager_android.h (right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/midi_manage...
media/midi/midi_manager_android.h:27: // MidiManagerAndroid is a midi manager
subclass for Android M or newer. For
On 2015/09/07 16:42:20, Takashi Toyoshima (ooo-Sep.30) wrote:
> a MidiManager subclass?

Done.

[Takashi Toyoshima, 2015-09-14 11:44:28]

lgtm for midi per s/WebMIDI/Web MIDI/ on CL description.

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/220001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiManagerAndroid.java:90: },
mHandler);
ok, let's discuss this offline later.

[yhirano, 2015-09-15 04:13:43]

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

[yhirano, 2015-09-15 04:13:44]

+qinmin@ for media/midi/java/ files.

[qinmin, 2015-09-15 16:54:08]

lgtm % nits

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:21: private
MidiDevice mDevice;
nit: final, and same for variables that are passed in from ctor in other files

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:49: *
Opens this port.
s/Opens/Open/

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:62: *
Sends the data to the underlying output port.
s/Sends/Send/

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:78: *
Closes the port.
s/Closes/Close/

[yhirano, 2015-09-16 05:44:34]

Patchset #10 (id:280001) has been deleted

[yhirano, 2015-09-16 05:47:15]

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java (right):

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiDeviceAndroid.java:21: private
MidiDevice mDevice;
On 2015/09/15 16:54:08, qinmin wrote:
> nit: final, and same for variables that are passed in from ctor in other files

Done.

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:49: *
Opens this port.
On 2015/09/15 16:54:08, qinmin wrote:
> s/Opens/Open/

https://source.android.com/source/code-style.html#use-standard-java-annotations
says 

"
Every class and nontrivial public method you write must contain a Javadoc
comment with at least one sentence describing what the class or method does.
This sentence should start with a 3rd person descriptive verb.
"

Aren't we using the rule?

[qinmin, 2015-09-16 17:01:41]

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java
(right):

https://codereview.chromium.org/1177973003/diff/260001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/MidiOutputPortAndroid.java:49: *
Opens this port.
On 2015/09/16 05:47:15, yhirano wrote:
> On 2015/09/15 16:54:08, qinmin wrote:
> > s/Opens/Open/
> 
>
https://source.android.com/source/code-style.html#use-standard-java-annotations
> says 
> 
> "
> Every class and nontrivial public method you write must contain a Javadoc
> comment with at least one sentence describing what the class or method does.
> This sentence should start with a 3rd person descriptive verb.
> "
> 
> Aren't we using the rule?

ok, never mind then. Somehow i remember we don't add "s" to verbs in our other
java files, but that might be an outdated

[yhirano, 2015-09-17 12:00:37]

Thanks, all reviewers.

I will land this CL. We are planning to implement port-wise open / close by M48.

[yhirano, 2015-09-17 12:00:45]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-09-17 12:00:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
toyoshim@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1177973003/#ps300001
(title: " ")

[commit-bot: I haz the power, 2015-09-17 12:00:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177973003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1177973003/300001

[commit-bot: I haz the power, 2015-09-17 12:44:39]

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

[commit-bot: I haz the power, 2015-09-17 12:44:40]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2015-09-17 13:33:31]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-09-17 13:33:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177973003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1177973003/300001

[commit-bot: I haz the power, 2015-09-17 14:41:50]

Committed patchset #10 (id:300001)

[commit-bot: I haz the power, 2015-09-17 14:43:00]

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