[WebMIDI] Introduce UsbMidi{Input, Output}Stream.

[WebMIDI] Introduce UsbMidi{Input, Output}Stream.

UsbMidiInputStream converts USB-MIDI messages to MIDI messages.
UsbMidiOutputStream converts MIDI MIDI messages to USB-MIDI messages.

BUG=303596
R=toyoshim@chromium.org

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

[yhirano, 2013-12-20 09:11:32]

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

[Takashi Toyoshima, 2013-12-24 04:19:31]

First rough review on Patch Set 3.

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_devi...
File media/midi/usb_midi_device.h (right):

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_devi...
media/midi/usb_midi_device.h:49: virtual void Send(uint8 endpoint_number, const
std::vector<uint8>& data) = 0;
uint8 and int are used for endpoint_number. Any reason to use different types?

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream.cc:6: 
string.h for memcpy ?

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream.cc:81: // packet[0:4] is accessible here.
It's nice to have some information about USB MIDI spec. E.g., section number of
the spec is useful information here. So, see also Universal Serial Bus Device
Class Definition for MIDI Devices Release 1.0 "4 USB-MIDI Event Packets" or
something like that.

If possible, please add spec reference here and there. It's useful to read these
source code and review this change.

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream_unittest.cc:20: typedef
::testing::MockFunction<void(int)> Checkpoint;  // NOLINT
Can you elaborate why we need to use NOLINT annotation here?
Just a question since I didn't use it in chromium and am not familiar.

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream_unittest.cc:96: scoped_ptr<UsbMidiInputStream>
stream_;
DISALLOW_... ?

[yhirano, 2013-12-24 05:57:01]

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_devi...
File media/midi/usb_midi_device.h (right):

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_devi...
media/midi/usb_midi_device.h:49: virtual void Send(uint8 endpoint_number, const
std::vector<uint8>& data) = 0;
On 2013/12/24 04:19:32, Takashi Toyoshima (chromium) wrote:
> uint8 and int are used for endpoint_number. Any reason to use different types?

I would use uint8 in UsbMidiJack and UsbMidiDescriptorParser because they are
low-level things and use int in other places.
Hence UsbMidiDevice should use int, thanks!

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream.cc:6: 
On 2013/12/24 04:19:32, Takashi Toyoshima (chromium) wrote:
> string.h for memcpy ?

Done.

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream.cc:81: // packet[0:4] is accessible here.
On 2013/12/24 04:19:32, Takashi Toyoshima (chromium) wrote:
> It's nice to have some information about USB MIDI spec. E.g., section number
of
> the spec is useful information here. So, see also Universal Serial Bus Device
> Class Definition for MIDI Devices Release 1.0 "4 USB-MIDI Event Packets" or
> something like that.
> 
> If possible, please add spec reference here and there. It's useful to read
these
> source code and review this change.

Added comments in headers.

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream_unittest.cc:20: typedef
::testing::MockFunction<void(int)> Checkpoint;  // NOLINT
On 2013/12/24 04:19:32, Takashi Toyoshima (chromium) wrote:
> Can you elaborate why we need to use NOLINT annotation here?
> Just a question since I didn't use it in chromium and am not familiar.

Without this, cpplint complains like this:
All parameters should be named in a function  [readability/function] [3]

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream_unittest.cc:96: scoped_ptr<UsbMidiInputStream>
stream_;
On 2013/12/24 04:19:32, Takashi Toyoshima (chromium) wrote:
> DISALLOW_... ?

Done.

[Takashi Toyoshima, 2013-12-24 06:47:18]

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/40001/media/midi/usb_midi_inpu...
media/midi/usb_midi_input_stream_unittest.cc:20: typedef
::testing::MockFunction<void(int)> Checkpoint;  // NOLINT
So maybe you can move this typedef inside UsbMidiInputStreamTest class
declaration.

[Takashi Toyoshima, 2013-12-24 07:39:41]

Another incomplete review on Patch Set 5 (output side review is not finished).

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:34: std::hash<int>()(key.cable_number) * 31;
Can you add some comments on these magic numbers, 17, 19, and 31?

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:55: double timestamp) {
The 32bit packet is defined as a USB transport layer's packet. I guess it isn't
fragmented. If my guess is true, DCHECK for pending should be DCHECK_EQ(0,
pending_size), and another logic can be simplified. Also we can remove memcpy
here.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:82: // packet[0:4] is accessible here.
[0:3]

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:83: uint8 cin = packet[0] & 0x0f;
code_index is better here since it is consistent with the following cable_number
(CN) and it is recommended to avoid using abbreviations in Chromium.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:16: const uint8 kSystemMessagePattern =
0xf0;
We already have similar definitions at
content/browser/renderer_host/media/midi_host.cc.
I think these constant values should be moved to midi_message_util.h and
midi_host.cc and usb_midi_output_strea,cc use it.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:21: // To prevent link errors caused by
DCHECK_*.
It seems to be a check between a constant value and a constant value.
COMPILE_ASSERT looks better and will work?

[yhirano, 2014-01-14 10:52:48]

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:34: std::hash<int>()(key.cable_number) * 31;
On 2013/12/24 07:39:41, Takashi Toyoshima (chromium) wrote:
> Can you add some comments on these magic numbers, 17, 19, and 31?

Done.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:55: double timestamp) {
On 2013/12/24 07:39:41, Takashi Toyoshima (chromium) wrote:
> The 32bit packet is defined as a USB transport layer's packet. I guess it
isn't
> fragmented. If my guess is true, DCHECK for pending should be DCHECK_EQ(0,
> pending_size), and another logic can be simplified. Also we can remove memcpy
> here.

Done.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:82: // packet[0:4] is accessible here.
On 2013/12/24 07:39:41, Takashi Toyoshima (chromium) wrote:
> [0:3]
I used the python notation, but it seems not clear.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:83: uint8 cin = packet[0] & 0x0f;
On 2013/12/24 07:39:41, Takashi Toyoshima (chromium) wrote:
> code_index is better here since it is consistent with the following
cable_number
> (CN) and it is recommended to avoid using abbreviations in Chromium. 

Done.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:16: const uint8 kSystemMessagePattern =
0xf0;
On 2013/12/24 07:39:41, Takashi Toyoshima (chromium) wrote:
> We already have similar definitions at
> content/browser/renderer_host/media/midi_host.cc.
> I think these constant values should be moved to midi_message_util.h and
> midi_host.cc and usb_midi_output_strea,cc use it.

Done.

https://codereview.chromium.org/107513012/diff/140001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:21: // To prevent link errors caused by
DCHECK_*.
On 2013/12/24 07:39:41, Takashi Toyoshima (chromium) wrote:
> It seems to be a check between a constant value and a constant value.
> COMPILE_ASSERT looks better and will work?
The DCHECK_LE on l59 can't be replaced, so I would leave them for consistency.

[Takashi Toyoshima, 2014-01-15 12:08:40]

https://codereview.chromium.org/107513012/diff/430001/content/browser/rendere...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/107513012/diff/430001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:49: using
media::kSystemExclusiveEndByte;
EOX or End of SysEx is an official name and I'm using EndOfSysEx in blink.

https://codereview.chromium.org/107513012/diff/430001/media/midi/midi_message...
File media/midi/midi_message_util.h (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/midi_message...
media/midi/midi_message_util.h:25: const uint8 kSystemMessagePattern = 0xf0;
Can you insert one line after the line 24, and how about renaming and adding
like this.

kMessageTypeMask = 0xf0;
kSystemMessageType = 0xf0;

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

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
Maybe 2014 is better now, but it's optional suggestion.
Ditto on other new files.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:33: return std::hash<void*>()(key.device) *
17 +
Please fix compile errors on using std::hash.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:77: // These CINs are reserved. Ignore them.
DVLOG(1) << something_informative ?

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:23: uint8 first_byte = Get(data, current);
Real-Time messages can be inserted in the midst of sysex sequence. E.g., 0xF0,
0xFE, 0x00, 0x01, 0x02, ..., 0xF7 is valid sequence.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:28: } else if ((first_byte & 0xf0) ==
kSystemMessagePattern) {
(first_byte & kMessageTypeMask) == kSystemMessageType

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:43: DVLOG(1) << "Unknown byte: " <<
static_cast<unsigned int>(first_byte);
You may want to add similar DVLOG() for other returning false cases of Push*().

[yhirano, 2014-01-15 13:25:41]

https://codereview.chromium.org/107513012/diff/430001/content/browser/rendere...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/107513012/diff/430001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:49: using
media::kSystemExclusiveEndByte;
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> EOX or End of SysEx is an official name and I'm using EndOfSysEx in blink.

Done.

https://codereview.chromium.org/107513012/diff/430001/media/midi/midi_message...
File media/midi/midi_message_util.h (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/midi_message...
media/midi/midi_message_util.h:25: const uint8 kSystemMessagePattern = 0xf0;
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> Can you insert one line after the line 24, and how about renaming and adding
> like this.
> 
> kMessageTypeMask = 0xf0;
> kSystemMessageType = 0xf0;

Done.

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

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> Maybe 2014 is better now, but it's optional suggestion.
> Ditto on other new files.

Done.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:33: return std::hash<void*>()(key.device) *
17 +
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> Please fix compile errors on using std::hash.

Done.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:77: // These CINs are reserved. Ignore them.
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> DVLOG(1) << something_informative ?

Done.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:23: uint8 first_byte = Get(data, current);
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> Real-Time messages can be inserted in the midst of sysex sequence. E.g., 0xF0,
> 0xFE, 0x00, 0x01, 0x02, ..., 0xF7 is valid sequence.
Thank you, I didn't know that.
Done in PushSysExMessage.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:28: } else if ((first_byte & 0xf0) ==
kSystemMessagePattern) {
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> (first_byte & kMessageTypeMask) == kSystemMessageType

Done.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:43: DVLOG(1) << "Unknown byte: " <<
static_cast<unsigned int>(first_byte);
On 2014/01/15 12:08:40, Takashi Toyoshima (chromium) wrote:
> You may want to add similar DVLOG() for other returning false cases of
Push*().
Push* function may return false, but it is not an error and hence an error
message is not needed.

[Takashi Toyoshima, 2014-01-16 03:43:35]

lgtm with 1 request.
defer to media owners.

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:23: uint8 first_byte = Get(data, current);
Why don't you handle it here, but in PushSysExMessage?

[yhirano, 2014-01-16 04:15:24]

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:23: uint8 first_byte = Get(data, current);
On 2014/01/16 03:43:36, Takashi Toyoshima (chromium) wrote:
> Why don't you handle it here, but in PushSysExMessage?

Is it possible without inlining PushSysExMessage here?
My understanding is a System RT message can be inserted at arbitrary point in
SysEx message, is that right?

[yhirano, 2014-01-16 04:18:51]

+scherkus@chromium.org for OWNER review

[Takashi Toyoshima, 2014-01-16 04:31:22]

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/430001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:23: uint8 first_byte = Get(data, current);
I thought one USB event packet contains only one non-sysex message, or a part of
sysex messages. So, RT messages can not be inside sysex messages of CIN=4,5,6,7.
But it's possible to receive a sequence of event packets like this; [0x40, 0xF0,
0x00, 0x00], [0x50, 0xFF, 0x00, 0x00], [0x05, 0xF7, 0x00, 0x00]. This is the
case I assumed in the last comment.

But, I rechecked the spec, and I'm a bit confused and have no confidence now. In
Table 4-2, there is an example of real-time message, but which uses CIN=3 with
following two bytes data which may be different type messages.

[scherkus (not reviewing), 2014-01-16 21:55:51]

https://codereview.chromium.org/107513012/diff/900001/content/browser/rendere...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/107513012/diff/900001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:122: (std::find(data.begin(),
data.end(), kSysExByte)
looks like you can drop the extra ()'s around std::find() and also move the "!=
data.end()" to this line

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

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:17: class MEDIA_EXPORT UsbMidiDeviceDelegate {
I don't see this class being used anywhere -- can we remove it?

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:26: class MEDIA_EXPORT UsbMidiDevice {
class needs docs

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:29: class Factory {
I don't see Factory being used anywhere

In general I advocate for not checking in code that isn't used, otherwise it can
tend to stick around unnoticed (not to mention it makes it harder to review
since there is no calling code)

If this CL can land w/o Factory being included I'd go ahead and remove it.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:89: delegate_->OnReceivedData(it->second,
&packet[1], packet_size, timestamp);
the .h says that the key in jack_dictionary_ points to an index inside of
jacks_, but it looks you never really use jacks_ aside from the constructor

there also isn't any code that uses the jacks() getter

can you explain how this is supposed to be used? I'm a bit confused :(

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

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:39: void OnReceivedData(UsbMidiDevice*
device,
nit: add blank line to separate functions and needs docs

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:44: const std::vector<UsbMidiJack>& jacks()
const { return jacks_; }
don't see this being called anywhere -- remove?

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:59: void OnReceivedData(size_t jack_index,
I'm not seeing this function in the .cc -- remove?

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream_unittest.cc:92: scoped_ptr<TestUsbMidiDevice>
device1_;
it doesn't look like any of these need to be scoped_ptr<>

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream_unittest.cc:123: EXPECT_CALL(*delegate_,
OnReceivedData(0, ToVector(expected3), 0));
These days I heavily discourage writing tests using gmock w/ InSequence. While
it seems convenient ... it makes the tests extremely fragile and extremely
difficult to debug when they fail.

For this style of test where you want to validate some output data given a set
of input, I find having human-readable test expectations more maintainable.

Example:

class TestDelegate : public UsbMidiInputStream::Delegate {
  std::string received_data() { return received_data_; }

  // Convert received data into a human readable string.
  void OnReceivedData(size_t jack_index, const uint8* data,
                      size_t size, double timestamp) OVERRIDE {
    received_data_ += base::StringPrintf("%zu { ", jack_index);
    for (size_t i = 0; i < size; ++i) {
      received_data_ += base::StringPrintf("0x%02x ", data[i]);
    }
    received_data_ += base::StringPrintf("} %f\n", timestamp);
  }

 private:
  std::string received_data_;
};

TEST_F(UsbMidiInputStreamTest, SystemCommonMessage) {
  uint8 data[] = {
    0x45, 0xf8, 0x00, 0x00,
    0x42, 0xf3, 0x22, 0x00,
    0x43, 0xf2, 0x33, 0x44,
  };

  stream_->OnReceivedData(device1_.get(), 7, data, arraysize(data), 0);

  EXPECT_EQ(
      "0 { 0xf8 } 0.000000\n"
      "0 { 0xf3 0x22 } 0.000000\n"
      "0 { 0xf2 0x33 0x44 } 0.000000\n",
      delegate_.received_data());
}


You capture the same amount of detail in your test with each line of text
corresponding to a call to OnReceivedData(), except now when the test fails it's
immediately obvious what changed (e.g., # of times OnReceivedData() was called,
which parameters changed, how the data is different).

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:17: DCHECK_EQ(3u, kPacketContentSize);
what is this DCHECK() protecting against?

UsbMidiOutputStream::kPacketContentSize is a constant -- do we really need a
run-time check to make sure the constant is still 3?

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.h (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.h:22: : jack_(jack), pending_size_(0),
is_sending_sysex_(false) {}
nit: I woudln't bother inlining the constructor

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.h:24: const UsbMidiJack& jack() const { return
jack_; }
I don't see this called anywhere -- remove?

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.h:25: void Send(const std::vector<uint8>&
data);
needs docs

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream_unittest.cc:40: scoped_ptr<MockUsbMidiDevice>
device_;
it doesn't look like these need to be scoped_ptr<>

[yhirano, 2014-01-20 09:12:18]

https://codereview.chromium.org/107513012/diff/900001/content/browser/rendere...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/107513012/diff/900001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:122: (std::find(data.begin(),
data.end(), kSysExByte)
On 2014/01/16 21:55:52, scherkus wrote:
> looks like you can drop the extra ()'s around std::find() and also move the
"!=
> data.end()" to this line

Done.

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

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:17: class MEDIA_EXPORT UsbMidiDeviceDelegate {
On 2014/01/16 21:55:52, scherkus wrote:
> I don't see this class being used anywhere -- can we remove it?

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:26: class MEDIA_EXPORT UsbMidiDevice {
On 2014/01/16 21:55:52, scherkus wrote:
> class needs docs

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_dev...
media/midi/usb_midi_device.h:29: class Factory {
On 2014/01/16 21:55:52, scherkus wrote:
> I don't see Factory being used anywhere
> 
> In general I advocate for not checking in code that isn't used, otherwise it
can
> tend to stick around unnoticed (not to mention it makes it harder to review
> since there is no calling code)
> 
> If this CL can land w/o Factory being included I'd go ahead and remove it.

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.cc:89: delegate_->OnReceivedData(it->second,
&packet[1], packet_size, timestamp);
On 2014/01/16 21:55:52, scherkus wrote:
> the .h says that the key in jack_dictionary_ points to an index inside of
> jacks_, but it looks you never really use jacks_ aside from the constructor
> 
> there also isn't any code that uses the jacks() getter
> 
> can you explain how this is supposed to be used? I'm a bit confused :(

deleted jacks_

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

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:39: void OnReceivedData(UsbMidiDevice*
device,
On 2014/01/16 21:55:52, scherkus wrote:
> nit: add blank line to separate functions and needs docs

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:44: const std::vector<UsbMidiJack>& jacks()
const { return jacks_; }
On 2014/01/16 21:55:52, scherkus wrote:
> don't see this being called anywhere -- remove?

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream.h:59: void OnReceivedData(size_t jack_index,
On 2014/01/16 21:55:52, scherkus wrote:
> I'm not seeing this function in the .cc -- remove?

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream_unittest.cc:92: scoped_ptr<TestUsbMidiDevice>
device1_;
On 2014/01/16 21:55:52, scherkus wrote:
> it doesn't look like any of these need to be scoped_ptr<>

Do you mean they should be objects rather than pointers?

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream_unittest.cc:123: EXPECT_CALL(*delegate_,
OnReceivedData(0, ToVector(expected3), 0));
On 2014/01/16 21:55:52, scherkus wrote:
> These days I heavily discourage writing tests using gmock w/ InSequence. While
> it seems convenient ... it makes the tests extremely fragile and extremely
> difficult to debug when they fail.
> 
> For this style of test where you want to validate some output data given a set
> of input, I find having human-readable test expectations more maintainable.
> 
> Example:
> 
> class TestDelegate : public UsbMidiInputStream::Delegate {
>   std::string received_data() { return received_data_; }
> 
>   // Convert received data into a human readable string.
>   void OnReceivedData(size_t jack_index, const uint8* data,
>                       size_t size, double timestamp) OVERRIDE {
>     received_data_ += base::StringPrintf("%zu { ", jack_index);
>     for (size_t i = 0; i < size; ++i) {
>       received_data_ += base::StringPrintf("0x%02x ", data[i]);
>     }
>     received_data_ += base::StringPrintf("} %f\n", timestamp);
>   }
> 
>  private:
>   std::string received_data_;
> };
> 
> TEST_F(UsbMidiInputStreamTest, SystemCommonMessage) {
>   uint8 data[] = {
>     0x45, 0xf8, 0x00, 0x00,
>     0x42, 0xf3, 0x22, 0x00,
>     0x43, 0xf2, 0x33, 0x44,
>   };
> 
>   stream_->OnReceivedData(device1_.get(), 7, data, arraysize(data), 0);
> 
>   EXPECT_EQ(
>       "0 { 0xf8 } 0.000000\n"
>       "0 { 0xf3 0x22 } 0.000000\n"
>       "0 { 0xf2 0x33 0x44 } 0.000000\n",
>       delegate_.received_data());
> }
> 
> 
> You capture the same amount of detail in your test with each line of text
> corresponding to a call to OnReceivedData(), except now when the test fails
it's
> immediately obvious what changed (e.g., # of times OnReceivedData() was
called,
> which parameters changed, how the data is different).

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.cc:17: DCHECK_EQ(3u, kPacketContentSize);
On 2014/01/16 21:55:52, scherkus wrote:
> what is this DCHECK() protecting against?
> 
> UsbMidiOutputStream::kPacketContentSize is a constant -- do we really need a
> run-time check to make sure the constant is still 3?

I added it for readability, but OK, I deleted.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream.h (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.h:22: : jack_(jack), pending_size_(0),
is_sending_sysex_(false) {}
On 2014/01/16 21:55:52, scherkus wrote:
> nit: I woudln't bother inlining the constructor

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.h:24: const UsbMidiJack& jack() const { return
jack_; }
On 2014/01/16 21:55:52, scherkus wrote:
> I don't see this called anywhere -- remove?

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream.h:25: void Send(const std::vector<uint8>&
data);
On 2014/01/16 21:55:52, scherkus wrote:
> needs docs

Done.

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
File media/midi/usb_midi_output_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_out...
media/midi/usb_midi_output_stream_unittest.cc:40: scoped_ptr<MockUsbMidiDevice>
device_;
On 2014/01/16 21:55:52, scherkus wrote:
> it doesn't look like these need to be scoped_ptr<>

Done.

[scherkus (not reviewing), 2014-01-21 21:25:25]

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/900001/media/midi/usb_midi_inp...
media/midi/usb_midi_input_stream_unittest.cc:92: scoped_ptr<TestUsbMidiDevice>
device1_;
On 2014/01/20 09:12:19, yhirano wrote:
> On 2014/01/16 21:55:52, scherkus wrote:
> > it doesn't look like any of these need to be scoped_ptr<>
> 
> Do you mean they should be objects rather than pointers?

yep!

https://codereview.chromium.org/107513012/diff/1020001/content/browser/render...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/107513012/diff/1020001/content/browser/render...
content/browser/renderer_host/media/midi_host.cc:122: std::find(data.begin(),
data.end(), kSysExByte)
nit: can you move the "!= data.end()" to this line?

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_in...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_in...
media/midi/usb_midi_input_stream_unittest.cc:13: #include
"testing/gmock/include/gmock/gmock.h"
I believe you can remove this include now

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_in...
media/midi/usb_midi_input_stream_unittest.cc:144: "0xe0 0xbb 0xcc \n",
delegate_.received_data());
nice tests :)

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_ou...
File media/midi/usb_midi_output_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream_unittest.cc:40: StrictMock<MockUsbMidiDevice>
device_;
think you can update this test to use string-based verification similar to how
you updated usb_midi_input_stream_unittest.cc ?

[yhirano, 2014-01-22 02:33:31]

https://codereview.chromium.org/107513012/diff/1020001/content/browser/render...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/107513012/diff/1020001/content/browser/render...
content/browser/renderer_host/media/midi_host.cc:122: std::find(data.begin(),
data.end(), kSysExByte)
On 2014/01/21 21:25:25, scherkus wrote:
> nit: can you move the "!= data.end()" to this line?

Done.

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_in...
File media/midi/usb_midi_input_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_in...
media/midi/usb_midi_input_stream_unittest.cc:13: #include
"testing/gmock/include/gmock/gmock.h"
On 2014/01/21 21:25:25, scherkus wrote:
> I believe you can remove this include now

Done.

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_ou...
File media/midi/usb_midi_output_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/1020001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream_unittest.cc:40: StrictMock<MockUsbMidiDevice>
device_;
On 2014/01/21 21:25:25, scherkus wrote:
> think you can update this test to use string-based verification similar to how
> you updated usb_midi_input_stream_unittest.cc ?

Done.

[scherkus (not reviewing), 2014-01-22 06:51:45]

lgtm w/ nits

thanks for de-gmock-ifying the tests!

https://codereview.chromium.org/107513012/diff/1150001/media/midi/usb_midi_ou...
File media/midi/usb_midi_output_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/1150001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream_unittest.cc:48: };
nit: these classes can still have DISALLOW_ macros in them

https://codereview.chromium.org/107513012/diff/1150001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream_unittest.cc:78: device_.Checkpoint(1);
an alternative to checkpoints is the check device_.log() at different points in
time

for example .. that device_.log() is still an empty string after a call to
Send()

[yhirano, 2014-01-22 08:58:23]

https://codereview.chromium.org/107513012/diff/1150001/media/midi/usb_midi_ou...
File media/midi/usb_midi_output_stream_unittest.cc (right):

https://codereview.chromium.org/107513012/diff/1150001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream_unittest.cc:48: };
On 2014/01/22 06:51:46, scherkus wrote:
> nit: these classes can still have DISALLOW_ macros in them

Done.

https://codereview.chromium.org/107513012/diff/1150001/media/midi/usb_midi_ou...
media/midi/usb_midi_output_stream_unittest.cc:78: device_.Checkpoint(1);
On 2014/01/22 06:51:46, scherkus wrote:
> an alternative to checkpoints is the check device_.log() at different points
in
> time
> 
> for example .. that device_.log() is still an empty string after a call to
> Send()
That's a nice idea. Done.

[commit-bot: I haz the power, 2014-01-22 08:59:06]

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

[commit-bot: I haz the power, 2014-01-22 10:39:44]

Change committed as 246277