[WebMIDI] Introduce MidiManagerUsb

[WebMIDI] Introduce MidiManagerUsb

Introduce MidiManagerUsb, a MIDIManager subclass for USB-MIDI.
Android port will use this class.

BUG=303596
R=toyoshim@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247665

[yhirano, 2013-12-24 03:37:13]

This CL depends on https://codereview.chromium.org/107513012

[Takashi Toyoshima, 2013-12-24 04:33:23]

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_usb.cc:21: void Noop(bool result) {
Placeholder for something?

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_usb.h:24: class MIDIManagerUsb : public MIDIManager,
Any reason to implement this as a MIDIManager subclass?
How will you use this class from MIDIManagerAndroid?

[yhirano, 2013-12-24 04:42:50]

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_usb.cc:21: void Noop(bool result) {
On 2013/12/24 04:33:23, Takashi Toyoshima (chromium) wrote:
> Placeholder for something?

Added a comment.

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/107163008/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_usb.h:24: class MIDIManagerUsb : public MIDIManager,
On 2013/12/24 04:33:23, Takashi Toyoshima (chromium) wrote:
> Any reason to implement this as a MIDIManager subclass?
> How will you use this class from MIDIManagerAndroid?

We have two device dependent classes, UsbMidiDevice and UsbMidiDevice::Factory.
The next (and hopefully last) step is implement them for Android.
MIDIManager::Create will return a MIDIManagerUsb with
UsbMidiDeviceFactoryAndroid.

[Takashi Toyoshima, 2014-01-15 12:25:48]

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb.cc:22: // This function should go away when
MIDIManager::Initialize() becomes
TODO(yhirano): This function ...

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb.cc:56: // TODO(yhirano): Call
AccumulateMIDIBytesSent.
Is this difficult to solve? If the function is not called, following messages
will be disposed after the first 10MB since  MIDIHost think previous messages
has not been sent.

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
optional: 2014 now

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
File media/midi/midi_manager_usb_unittest.cc (right):

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:185: 
remove one empty line?

[yhirano, 2014-01-23 04:19:10]

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb.cc:22: // This function should go away when
MIDIManager::Initialize() becomes
On 2014/01/15 12:25:49, Takashi Toyoshima (chromium) wrote:
> TODO(yhirano): This function ...

Done.

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb.cc:56: // TODO(yhirano): Call
AccumulateMIDIBytesSent.
On 2014/01/15 12:25:49, Takashi Toyoshima (chromium) wrote:
> Is this difficult to solve? If the function is not called, following messages
> will be disposed after the first 10MB since  MIDIHost think previous messages
> has not been sent.

Done.

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/01/15 12:25:49, Takashi Toyoshima (chromium) wrote:
> optional: 2014 now

Done.

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
File media/midi/midi_manager_usb_unittest.cc (right):

https://codereview.chromium.org/107163008/diff/260001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:185: 
On 2014/01/15 12:25:49, Takashi Toyoshima (chromium) wrote:
> remove one empty line?

Done.

[Takashi Toyoshima, 2014-01-27 03:49:03]

lgtm, defer to media owners.

[yhirano, 2014-01-27 04:22:57]

+scherkus for OWNER review

[scherkus (not reviewing), 2014-01-27 19:23:49]

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb.cc:59: 
nit: remove extra blank line

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb.h:25: class MIDIManagerUsb : public MIDIManager,
nit: I'd like to see you folks agree on a naming convention

for example ... you have MIDI vs. Midi, not to mention Usb ... but no USB yet ;)

looking at the rest of the code it looks like you prefer Midi

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb.h:65: static void OnRequestPermissionDone(bool
result);
I can't find this function -- remove?

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
File media/midi/midi_manager_usb_unittest.cc (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:33: };
DISALLOW etc...

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:62: };
DISALLOW etc...

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:87: };
DISALLOW etc...

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:99: };
DISALLOW etc...

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:111: ADD_FAILURE();
I'd print out the contents of the log:


ADD_FAILURE() << log_string;

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:122: is_initialized_ = true;
nit: I think a better name for these two would be "initialize_callback_run_" and
"initialize_result_"

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:133: };
DISALLOW etc...

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_dev...
File media/midi/usb_midi_device.h (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:19: // Delegate class for USbMidiDevice.
typo: USbMidiDevice -> UsbMidiDevice

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.h (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:62: std::vector<JackUniqueKey>
RegisteredJackKeys() const;
instead of having a comment saying this is for testing, add "ForTesting" to the
end of this method name

src/PRESUBMIT.py will do a presubmit check to make sure this code is only
executed in test files

[yhirano, 2014-01-28 04:31:05]

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb.cc:59: 
On 2014/01/27 19:23:49, scherkus wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb.h:25: class MIDIManagerUsb : public MIDIManager,
On 2014/01/27 19:23:49, scherkus wrote:
> nit: I'd like to see you folks agree on a naming convention
> 
> for example ... you have MIDI vs. Midi, not to mention Usb ... but no USB yet
;)
> 
> looking at the rest of the code it looks like you prefer Midi

Discussed with toyoshim@.
We are going to change MIDI to Midi in future CLs.
In this CL, I renamed things I introduced such as MidiManagerUsb.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb.h:65: static void OnRequestPermissionDone(bool
result);
On 2014/01/27 19:23:49, scherkus wrote:
> I can't find this function -- remove?

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
File media/midi/midi_manager_usb_unittest.cc (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:33: };
On 2014/01/27 19:23:49, scherkus wrote:
> DISALLOW etc...

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:62: };
On 2014/01/27 19:23:49, scherkus wrote:
> DISALLOW etc...

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:87: };
On 2014/01/27 19:23:49, scherkus wrote:
> DISALLOW etc...

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:99: };
On 2014/01/27 19:23:49, scherkus wrote:
> DISALLOW etc...

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:111: ADD_FAILURE();
On 2014/01/27 19:23:49, scherkus wrote:
> I'd print out the contents of the log:
> 
> 
> ADD_FAILURE() << log_string;

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:122: is_initialized_ = true;
On 2014/01/27 19:23:49, scherkus wrote:
> nit: I think a better name for these two would be "initialize_callback_run_"
and
> "initialize_result_"

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:133: };
On 2014/01/27 19:23:49, scherkus wrote:
> DISALLOW etc...

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_dev...
File media/midi/usb_midi_device.h (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:19: // Delegate class for USbMidiDevice.
On 2014/01/27 19:23:49, scherkus wrote:
> typo: USbMidiDevice -> UsbMidiDevice

Done.

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.h (right):

https://codereview.chromium.org/107163008/diff/460001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:62: std::vector<JackUniqueKey>
RegisteredJackKeys() const;
On 2014/01/27 19:23:49, scherkus wrote:
> instead of having a comment saying this is for testing, add "ForTesting" to
the
> end of this method name
> 
> src/PRESUBMIT.py will do a presubmit check to make sure this code is only
> executed in test files

Done.

[scherkus (not reviewing), 2014-01-28 17:56:42]

lgtm w/ nits

https://codereview.chromium.org/107163008/diff/740001/media/midi/midi_manager...
File media/midi/midi_manager_usb_unittest.cc (right):

https://codereview.chromium.org/107163008/diff/740001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:116: : initialize_callback_run_(false),
initialize_result_(false) {
this should be indented by 4 more spaces

https://codereview.chromium.org/107163008/diff/740001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:124: ADD_FAILURE() << "There are some
log messages not consumed: ";
shouldn't you be printing out the logs?

std::string leftover_logs = logger_.TakeLog();
if (!leftover_logs.empty()) {
  ADD_FAILURE() << "Logs should be empty:" << leftover_logs;
}

[yhirano, 2014-01-29 01:20:52]

https://codereview.chromium.org/107163008/diff/740001/media/midi/midi_manager...
File media/midi/midi_manager_usb_unittest.cc (right):

https://codereview.chromium.org/107163008/diff/740001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:116: : initialize_callback_run_(false),
initialize_result_(false) {
On 2014/01/28 17:56:43, scherkus wrote:
> this should be indented by 4 more spaces

Done.

https://codereview.chromium.org/107163008/diff/740001/media/midi/midi_manager...
media/midi/midi_manager_usb_unittest.cc:124: ADD_FAILURE() << "There are some
log messages not consumed: ";
On 2014/01/28 17:56:43, scherkus wrote:
> shouldn't you be printing out the logs?
> 
> std::string leftover_logs = logger_.TakeLog();
> if (!leftover_logs.empty()) {
>   ADD_FAILURE() << "Logs should be empty:" << leftover_logs;
> }

Done.

[commit-bot: I haz the power, 2014-01-29 01:23:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/107163008/760001

[commit-bot: I haz the power, 2014-01-29 02:34:09]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-29 06:08:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/107163008/810001

[commit-bot: I haz the power, 2014-01-29 06:09:45]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-29 06:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/107163008/330010

[commit-bot: I haz the power, 2014-01-29 09:27:43]

Change committed as 247665