Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation

Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation

WebMIDI spec draft: http://www.w3.org/TR/webmidi/ 

WebMIDI API guarantees that MIDIInput::onmessage is called back with a single MIDI message. To guarantee this, this CL introduces MIDIMessageQueue class, which allows you to
- maintain fragmented MIDI message.
- Skip any invalid data sequence.
- Reorder MIDI messages so that "System Real Time Message", which can be inserted at any point of the byte stream, can be placed at the boundary of complete MIDI messages.
- (Optional) Reconstruct complete MIDI messages from data stream that is compressed with "running status".

This CL also replaces existing System Exclusive message validation logic in MIDIHost::OnSendData with MIDIHost::IsValidWebMIDIData, which can detect SysEx message even when it is concatenated with non-SysEx messages.

With this change, renderer/blink can be much simpler and free from this kind of data validation.

BUG=303599, 317355
TEST=media_unittests --gtest_filter=MIDI*, content_unittests --gtest_filter=MIDI*

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

[yukawa, 2013-11-11 06:40:41]

Hi Toyoshima-san,

I'm wondering if we should format MIDI messages from external MIDI devices to
the browser in the browser side rather than blink side.  What do you think about
this approach?

Regarding Windows, MIDIManagerWin requires MIDIMessageQueue anyway.  So I'd like
to check in at least MIDIMessageQueue. Here is the current WIP patch.
https://codereview.chromium.org/29793006/

[Takashi Toyoshima, 2013-11-12 21:59:36]

Thank you for sending a great patch.
As I commented, MIDIHost is a better place to do this kind of operations. SysEx
check is already there, and it stores incoming data to vector<> for an IPC.
These should be integrated to MIDIMessageQueue, I think.

Also, handling running status in this class looks good approach. Operating
systems handle it differently. One platform may not pass running status, but the
other may. JavaScript API explicitly says that midi message event data should
not use running status. This layer looks the best layer to convert it.

https://codereview.chromium.org/68353002/diff/230001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/68353002/diff/230001/media/midi/midi_manager....
media/midi/midi_manager.cc:63: 
This message queue introduces two additional memory copies.
Can we move this queue implementation to MIDIHost?
In MIDIHost, we originally have a buffer for sending it as an IPC. We may want
to replace it with this queue implementation. It saves memory usage and copy
operation.

[yukawa, 2013-11-14 14:55:30]

PTAL?  Now MIDIMessageQueue is used inside MIDIHost as you suggested.

https://codereview.chromium.org/68353002/diff/230001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/68353002/diff/230001/media/midi/midi_manager....
media/midi/midi_manager.cc:63: 
On 2013/11/12 21:59:36, Takashi Toyoshima (chromium) wrote:
> This message queue introduces two additional memory copies.
> Can we move this queue implementation to MIDIHost?
> In MIDIHost, we originally have a buffer for sending it as an IPC. We may want
> to replace it with this queue implementation. It saves memory usage and copy
> operation.

Thank you for the suggestion. I moved MIDIMessageQueue into MIDIHost.

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

https://codereview.chromium.org/68353002/diff/430001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:156: if (message[0] ==
kSysExMessage) {
This condition is slightly different from the previous one, which rejected not
only 0xf0 but also [0xf1, 0xff].  Please let me know if we should keep the
previous condition.

[yukawa, 2013-11-15 05:46:35]

On 2013/11/14 14:55:30, yukawa wrote:
> PTAL?  Now MIDIMessageQueue is used inside MIDIHost as you suggested.
> 
>
https://codereview.chromium.org/68353002/diff/230001/media/midi/midi_manager.cc
> File media/midi/midi_manager.cc (right):
> 
>
https://codereview.chromium.org/68353002/diff/230001/media/midi/midi_manager....
> media/midi/midi_manager.cc:63: 
> On 2013/11/12 21:59:36, Takashi Toyoshima (chromium) wrote:
> > This message queue introduces two additional memory copies.
> > Can we move this queue implementation to MIDIHost?
> > In MIDIHost, we originally have a buffer for sending it as an IPC. We may
want
> > to replace it with this queue implementation. It saves memory usage and copy
> > operation.
> 
> Thank you for the suggestion. I moved MIDIMessageQueue into MIDIHost.
> 
>
https://codereview.chromium.org/68353002/diff/430001/content/browser/renderer...
> File content/browser/renderer_host/media/midi_host.cc (right):
> 
>
https://codereview.chromium.org/68353002/diff/430001/content/browser/renderer...
> content/browser/renderer_host/media/midi_host.cc:156: if (message[0] ==
> kSysExMessage) {
> This condition is slightly different from the previous one, which rejected not
> only 0xf0 but also [0xf1, 0xff].  Please let me know if we should keep the
> previous condition.

Updated the patch. Now MIDIHost::OnSendData also uses MIDIMessageQueue.

[Takashi Toyoshima, 2013-11-19 01:08:37]

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

https://codereview.chromium.org/68353002/diff/430001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:156: if (message[0] ==
kSysExMessage) {
I chatted this with Chris, and double checked that it's ok to pass non-sysex
0xf* messages here.
So your implementation is right.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:131:
CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);
Can we make it a member variable?
IIRC, this code constructs an instance on stack, whenever this method is called.
We never want to add an extra latency for non-sysex messages here.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:143: // Sanity check that we
won't send too large message.
Here, we concern the buffer size which may be allocated by MIDIManager.
So, in_flight_lock_ + message.size() > kMaxInFlightBytes is right check as
original code did.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:144: if (message.size() >
kMaxInFlightBytes)
This check looks redundant.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:159: message.size() +
sent_bytes_in_flight_ > kMaxInFlightBytes)
Same TODO here, and any reason to check twice?
I feel the latter check meets our expectation here, and former check looks
redundant.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:180:
CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);
ditto. I think we can share the same instance between sender side and receiver
side.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.h (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
Any reason to keep it having in not content/browser/renderer_host/media, but
media/ ?

[Takashi Toyoshima, 2013-11-19 01:45:30]

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.cc (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:18: return data < 128;
I'm slightly prefer (data & 0x80) == 0 or something which reminds me bit
operations.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:29: size_t GetMessageLength(uint8 data) {
We may want to report some UMA for undefined messages, e.g., 0xf4, 0xf5, 0xf9,
0xfd?

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:36: else if (0xe0 <= data && data <= 0xf0)
latter condition should be data <= 0xef ?

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:99: // Invalid data. Ignore |next|.
Can you add some comment here, like "MIDI protocol doesn't provide any error
correction mechanism in physical layers, and incoming messages can be corrupted,
and should be corrected here."

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:104: next_message_.push_back(next);
What happens if the status byte was set by allow_running_status handling at line
126.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:106: DCHECK(!next_message_.empty());
this DCHECK looks too trivial.... but it's ok to keep having :)

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:117: DCHECK(IsDataByte(next));
This should not be DCHECK because as a commented at line 99, broken data can be
received. We should just dispose corrupted messages and restart to parse from
the next message.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:126: next_message_.push_back(status_byte);
I think this code doesn't work.

[Takashi Toyoshima, 2013-11-19 02:12:22]

+scherkus for media owner review

[yukawa, 2013-11-19 09:41:41]

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:131:
CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);
On 2013/11/19 01:08:37, Takashi Toyoshima (chromium) wrote:
> Can we make it a member variable?
> IIRC, this code constructs an instance on stack, whenever this method is
called.
> We never want to add an extra latency for non-sysex messages here.

Hmm, I think the construction cost of |can_send_sys_ex| is regrettable. It never
does any costly tasks until get() is called (and get() will never be called for
non-sysex message).

I'm not sure the policy is really immutable. Is it OK not to check the result of
 CanSendMIDISysExMessage(renderer_process_id_) whenever MIDIHost::OnSendData is
called?  If it's OK, why we don't store the result of CanSendMIDISysExMessage at
the constructor of MIDIHost in the constructor of MIDIHost.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:143: // Sanity check that we
won't send too large message.
On 2013/11/19 01:08:37, Takashi Toyoshima (chromium) wrote:
> Here, we concern the buffer size which may be allocated by MIDIManager.
> So, in_flight_lock_ + message.size() > kMaxInFlightBytes is right check as
> original code did.

Done.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:144: if (message.size() >
kMaxInFlightBytes)
On 2013/11/19 01:08:37, Takashi Toyoshima (chromium) wrote:
> This check looks redundant.

I put this condition here as an early exit route because this condition does not
require |in_flight_lock_|.  Anyway, I'll adopt your suggestion.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:159: message.size() +
sent_bytes_in_flight_ > kMaxInFlightBytes)
On 2013/11/19 01:08:37, Takashi Toyoshima (chromium) wrote:
> Same TODO here, and any reason to check twice?
> I feel the latter check meets our expectation here, and former check looks
> redundant.

Done.
BTW, the redundancy came from the original code...

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:180:
CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);
On 2013/11/19 01:08:37, Takashi Toyoshima (chromium) wrote:
> ditto. I think we can share the same instance between sender side and receiver
> side.

See my previous comment.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.cc (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:18: return data < 128;
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> I'm slightly prefer (data & 0x80) == 0 or something which reminds me bit
> operations.

Done.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:29: size_t GetMessageLength(uint8 data) {
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> We may want to report some UMA for undefined messages, e.g., 0xf4, 0xf5, 0xf9,
> 0xfd?

Hmm, I cannot imagine how we can utilize such UMA.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:36: else if (0xe0 <= data && data <= 0xf0)
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> latter condition should be data <= 0xef ?

Nice catch. Fixed.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:99: // Invalid data. Ignore |next|.
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> Can you add some comment here, like "MIDI protocol doesn't provide any error
> correction mechanism in physical layers, and incoming messages can be
corrupted,
> and should be corrected here."

Done.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:104: next_message_.push_back(next);
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> What happens if the status byte was set by allow_running_status handling at
line
> 126.

Yeah, this is a trick part. In such case, |next_message_| is not empty at line
88. And |next_message_| should already be cleared if the condition at line 90 is
true.
Does this make sense?

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:106: DCHECK(!next_message_.empty());
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> this DCHECK looks too trivial.... but it's ok to keep having :)

Oops. Agreed.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:117: DCHECK(IsDataByte(next));
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> This should not be DCHECK because as a commented at line 99, broken data can
be
> received. We should just dispose corrupted messages and restart to parse from
> the next message.

See the trick condition at line 88-90.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:126: next_message_.push_back(status_byte);
On 2013/11/19 01:45:31, Takashi Toyoshima (chromium) wrote:
> I think this code doesn't work.

See the trick condition at line 88-90.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.h (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
On 2013/11/19 01:08:37, Takashi Toyoshima (chromium) wrote:
> Any reason to keep it having in not content/browser/renderer_host/media, but
> media/ ?

Because it was originally written for media/midi/midi_manager_win.cc. Probably
now we can move this file to content/browser/renderer_host/media if you prefer.

[Takashi Toyoshima, 2013-11-19 12:49:59]

lgtm with some comment requests.
Also please wait for scherkus's owner review.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:131:
CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);
I see. It's ok to keep this code as is.

FYI, ChildSecurityPolicy is set just before calling OnStartSession. Originally
the policy database should be immutable for a session. But actually it can be
modified if the same renderer is going to open another session. This is a
limitation of Chrome security model.

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:159: message.size() +
sent_bytes_in_flight_ > kMaxInFlightBytes)
lol

https://codereview.chromium.org/68353002/diff/710001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:180:
CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);
ok

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.cc (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:29: size_t GetMessageLength(uint8 data) {
We can catch up unknown usage, or spec update for undefined messages.
But, MIDI is mature enough. As you said, it may not worth.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:91: // Status byte appears. Clear the existing
incomplete data.
Ah, I see. This comment "Status byte appears" is a little vague. How about
explicitly saying "an invalid data sequence is found".

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.cc:104: next_message_.push_back(next);
I see. But this code has a state, and a little hard to understand. Some comments
will be helpful.

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.h (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
If you don't have a plan to use this file in media/midi, I feel
content/browser/renderer_host/media sounds better.

[yukawa, 2013-11-19 14:49:31]

Thank you for the review!

Now I'm a bit concerned about additional memory copy cost in
MIDIHost::OnSendData introduced by my patch set #5 and prior. So I've made
another patch set #6, where we introduce another validation function named
IsValidMIDIMessagesForWebMIDI that does not require memory copy and is designed
to be used MIDIHost::OnSendData only.

MIDIMessageQueue is not changed between patch set #5 and #6.

Which approach do you like better?

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.h (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
On 2013/11/19 12:49:59, Takashi Toyoshima (chromium) wrote:
> If you don't have a plan to use this file in media/midi, I feel
> content/browser/renderer_host/media sounds better.

Yeah, I'm planning to use it from media/midi/midi_manager_win.cc so please let
me keep this file here at the moment.

[Takashi Toyoshima, 2013-11-20 10:04:57]

still lgtm

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
File media/midi/midi_message_queue.h (right):

https://codereview.chromium.org/68353002/diff/710001/media/midi/midi_message_...
media/midi/midi_message_queue.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
sgtm

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

https://codereview.chromium.org/68353002/diff/1110001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:40: sysex_allowed_(false),
I like this new approach.

https://codereview.chromium.org/68353002/diff/1110001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:84: sysex_allowed_ =
ChildProcessSecurityPolicyImpl::GetInstance()->
FYI, midi_dispatcher_host.cc is the exact place to store the permission.

https://codereview.chromium.org/68353002/diff/1110001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:120: if (data.size() +
sent_bytes_in_flight_ > kMaxInFlightBytes)
Can you add TODO here?
We should handle internal buffer overflow later, and may want to throw some
error in JavaScript. We'll discuss it in W3C.

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

https://codereview.chromium.org/68353002/diff/1110001/media/midi/midi_message...
media/midi/midi_message_util.h:75: MEDIA_EXPORT bool
IsValidMIDIMessagesForWebMIDI(
I prefer IsValidWebMIDIMessage or IsCompleteMIDIMessage.

[yukawa, 2013-11-20 11:06:08]

Thanks!

scherkus@, could you take an owner review?

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

https://codereview.chromium.org/68353002/diff/1110001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:40: sysex_allowed_(false),
On 2013/11/20 10:04:58, Takashi Toyoshima (chromium) wrote:
> I like this new approach.

Thanks. And I renamed |sysex_allowed_| with |has_sys_ex_permission_| so that we
can share similar terminology between MIDIDispatcherHost and MIDIHost.

https://codereview.chromium.org/68353002/diff/1110001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:84: sysex_allowed_ =
ChildProcessSecurityPolicyImpl::GetInstance()->
On 2013/11/20 10:04:58, Takashi Toyoshima (chromium) wrote:
> FYI, midi_dispatcher_host.cc is the exact place to store the permission.

Thank you for the info!  Slightly revised the comment.

https://codereview.chromium.org/68353002/diff/1110001/content/browser/rendere...
content/browser/renderer_host/media/midi_host.cc:120: if (data.size() +
sent_bytes_in_flight_ > kMaxInFlightBytes)
On 2013/11/20 10:04:58, Takashi Toyoshima (chromium) wrote:
> Can you add TODO here?
> We should handle internal buffer overflow later, and may want to throw some
> error in JavaScript. We'll discuss it in W3C.

Done.

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

https://codereview.chromium.org/68353002/diff/1110001/media/midi/midi_message...
media/midi/midi_message_util.h:75: MEDIA_EXPORT bool
IsValidMIDIMessagesForWebMIDI(
On 2013/11/20 10:04:58, Takashi Toyoshima (chromium) wrote:
> I prefer IsValidWebMIDIMessage or IsCompleteMIDIMessage.

Yeah, agreed. I chose IsValidWebMIDIMessage.

[scherkus (not reviewing), 2013-11-20 20:15:59]

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
File media/midi/midi_message_util.cc (right):

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.cc:32: else if (0x80 <= data && data <= 0xbf)
nit: since you return every time I'd drop all the elses

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

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:17: // allows you to
s/to/to:/

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:29: //     if
(is_incomming_midi_data_available()) {
s/incomming/incoming/

here + rest of example code

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:45: explicit MIDIMessageQueue(bool
allow_running_status);
it looks like test code is the only code that passes in |false|

do we really need to keep allow_running_status=false code path around? can we
add it in a future CL when production code is using it?

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:75: MEDIA_EXPORT bool IsValidWebMIDIMessage(const
std::vector<uint8>& data);
if this returns false, am I still allowed to call MIDIMessageQueue::Add()?

I also don't know what the comment about running status refers to here since
that seems to be a concept of MIDIMessageQueue and not this function

in short ... I'm confused as to how this should be used. It's also something
that's named after a web api and hints that it should be inside
content/browser/renderer_host/media/midi_host.cc rather than in midi

(I'd also like for this file to be renamed to midi_message_queue.h)

[yukawa, 2013-11-21 01:19:39]

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
File media/midi/midi_message_util.cc (right):

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.cc:32: else if (0x80 <= data && data <= 0xbf)
On 2013/11/20 20:16:00, scherkus wrote:
> nit: since you return every time I'd drop all the elses

Done.

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

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:17: // allows you to
On 2013/11/20 20:16:00, scherkus wrote:
> s/to/to:/

Done.

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:29: //     if
(is_incomming_midi_data_available()) {
On 2013/11/20 20:16:00, scherkus wrote:
> s/incomming/incoming/
> 
> here + rest of example code

Done.

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:45: explicit MIDIMessageQueue(bool
allow_running_status);
On 2013/11/20 20:16:00, scherkus wrote:
> it looks like test code is the only code that passes in |false|
> 
> do we really need to keep allow_running_status=false code path around? can we
> add it in a future CL when production code is using it?

Yes. A subsequent CL depends on this functionality.
https://codereview.chromium.org/29793006/diff/650001/media/midi/midi_manager_...

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:75: MEDIA_EXPORT bool IsValidWebMIDIMessage(const
std::vector<uint8>& data);
On 2013/11/20 20:16:00, scherkus wrote:
> if this returns false, am I still allowed to call MIDIMessageQueue::Add()?
> 
> I also don't know what the comment about running status refers to here since
> that seems to be a concept of MIDIMessageQueue and not this function
> 
> in short ... I'm confused as to how this should be used. It's also something
> that's named after a web api and hints that it should be inside
> content/browser/renderer_host/media/midi_host.cc rather than in midi
> 
> (I'd also like for this file to be renamed to midi_message_queue.h)

MIDIMessageQueue is originally designed to format data that is retrieved from
native MIDI API rather than WebMIDI API.  Then I realized that we also need
something to validate data sent from WebMIDI API but MIDIMessageQueue is not so
efficient and useful to such data validation. This is why I added
IsValidWebMIDIMessage.

Regarding 'running status', it is a kind of abbreviation technique that is
widely recognized in MIDI world.  I revised the comment to be a bit more clear.

Regarding file name, I'm hesitating to put IsValidWebMIDIMessage in a file named
'midi_message_queue.h' because MIDIMessageQueue and IsValidWebMIDIMessage are
used for different purposes in practice.  Another option is extracting
MIDIMessageQueue into 'midi_message_queue.h' with keeping IsValidWebMIDIMessage
in 'midi_message_util.h'.  I also realized we need to expose
GetMIDIMessageLength for a subsequent CL
https://codereview.chromium.org/29793006/ so having 'midi_message_queue.h' as
well as 'midi_message_util.h' might be an option IMO.  What do you think?

[yukawa, 2013-11-25 14:06:51]

ping?

[scherkus (not reviewing), 2013-11-25 23:14:36]

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

https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
media/midi/midi_message_util.h:75: MEDIA_EXPORT bool IsValidWebMIDIMessage(const
std::vector<uint8>& data);
On 2013/11/21 01:19:40, yukawa wrote:
> On 2013/11/20 20:16:00, scherkus wrote:
> > if this returns false, am I still allowed to call MIDIMessageQueue::Add()?
> > 
> > I also don't know what the comment about running status refers to here since
> > that seems to be a concept of MIDIMessageQueue and not this function
> > 
> > in short ... I'm confused as to how this should be used. It's also something
> > that's named after a web api and hints that it should be inside
> > content/browser/renderer_host/media/midi_host.cc rather than in midi
> > 
> > (I'd also like for this file to be renamed to midi_message_queue.h)
> 
> MIDIMessageQueue is originally designed to format data that is retrieved from
> native MIDI API rather than WebMIDI API.  Then I realized that we also need
> something to validate data sent from WebMIDI API but MIDIMessageQueue is not
so
> efficient and useful to such data validation. This is why I added
> IsValidWebMIDIMessage.
> 
> Regarding 'running status', it is a kind of abbreviation technique that is
> widely recognized in MIDI world.  I revised the comment to be a bit more
clear.
> 
> Regarding file name, I'm hesitating to put IsValidWebMIDIMessage in a file
named
> 'midi_message_queue.h' because MIDIMessageQueue and IsValidWebMIDIMessage are
> used for different purposes in practice.  Another option is extracting
> MIDIMessageQueue into 'midi_message_queue.h' with keeping
IsValidWebMIDIMessage
> in 'midi_message_util.h'.  I also realized we need to expose
> GetMIDIMessageLength for a subsequent CL
> https://codereview.chromium.org/29793006/ so having 'midi_message_queue.h' as
> well as 'midi_message_util.h' might be an option IMO.  What do you think?

I would advocate for the following:
  1) Have MIDIMessageQueue placed in midi_message_queue.{cc,h}
  2) Introduce midi_util.{cc,h} for GetMIDIMessageLength()
  3) Move IsValidWebMIDIMessage() to
content/browser/renderer_host/media/midi_host.{cc,h} (why? well AFAIK WebMIDI is
a web platform and src/content concept and shouldn't "leak" into a lower level
library such as src/media, especially if no other code in src/media needs to
call it)

WDYT?

[yukawa, 2013-11-26 03:30:01]

On 2013/11/25 23:14:36, scherkus wrote:
>
https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
> File media/midi/midi_message_util.h (right):
> 
>
https://codereview.chromium.org/68353002/diff/1210001/media/midi/midi_message...
> media/midi/midi_message_util.h:75: MEDIA_EXPORT bool
IsValidWebMIDIMessage(const
> std::vector<uint8>& data);
> On 2013/11/21 01:19:40, yukawa wrote:
> > On 2013/11/20 20:16:00, scherkus wrote:
> > > if this returns false, am I still allowed to call MIDIMessageQueue::Add()?
> > > 
> > > I also don't know what the comment about running status refers to here
since
> > > that seems to be a concept of MIDIMessageQueue and not this function
> > > 
> > > in short ... I'm confused as to how this should be used. It's also
something
> > > that's named after a web api and hints that it should be inside
> > > content/browser/renderer_host/media/midi_host.cc rather than in midi
> > > 
> > > (I'd also like for this file to be renamed to midi_message_queue.h)
> > 
> > MIDIMessageQueue is originally designed to format data that is retrieved
from
> > native MIDI API rather than WebMIDI API.  Then I realized that we also need
> > something to validate data sent from WebMIDI API but MIDIMessageQueue is not
> so
> > efficient and useful to such data validation. This is why I added
> > IsValidWebMIDIMessage.
> > 
> > Regarding 'running status', it is a kind of abbreviation technique that is
> > widely recognized in MIDI world.  I revised the comment to be a bit more
> clear.
> > 
> > Regarding file name, I'm hesitating to put IsValidWebMIDIMessage in a file
> named
> > 'midi_message_queue.h' because MIDIMessageQueue and IsValidWebMIDIMessage
are
> > used for different purposes in practice.  Another option is extracting
> > MIDIMessageQueue into 'midi_message_queue.h' with keeping
> IsValidWebMIDIMessage
> > in 'midi_message_util.h'.  I also realized we need to expose
> > GetMIDIMessageLength for a subsequent CL
> > https://codereview.chromium.org/29793006/ so having 'midi_message_queue.h'
as
> > well as 'midi_message_util.h' might be an option IMO.  What do you think?
> 
> I would advocate for the following:
>   1) Have MIDIMessageQueue placed in midi_message_queue.{cc,h}
>   2) Introduce midi_util.{cc,h} for GetMIDIMessageLength()
>   3) Move IsValidWebMIDIMessage() to
> content/browser/renderer_host/media/midi_host.{cc,h} (why? well AFAIK WebMIDI
is
> a web platform and src/content concept and shouldn't "leak" into a lower level
> library such as src/media, especially if no other code in src/media needs to
> call it)
> 
> WDYT?

Yeah, that sounds pretty reasonable to me.  Revised the CL accordingly.  PTAL?
(I also renamed IsValidWebMIDIMessage to IsValidWebMIDIData since it is valid
 to contain multiple MIDI messages in |data|)

[scherkus (not reviewing), 2013-11-26 19:05:36]

lgtm w/ nits

thanks!

https://codereview.chromium.org/68353002/diff/1630001/media/midi/midi_message...
File media/midi/midi_message_queue.h (right):

https://codereview.chromium.org/68353002/diff/1630001/media/midi/midi_message...
media/midi/midi_message_queue.h:51: // this method returns.
nit: assuming this class is designed to be accessed on a single thread, you can
drop the "|data| must be valid until this method returns" bit

... otherwise it sounds as if data could somehow get changed by a different
thread while Add() is running (and why wouldn't this apply to the const-ref
std::vector version above?)

[yukawa, 2013-11-26 19:19:07]

https://codereview.chromium.org/68353002/diff/1630001/media/midi/midi_message...
File media/midi/midi_message_queue.h (right):

https://codereview.chromium.org/68353002/diff/1630001/media/midi/midi_message...
media/midi/midi_message_queue.h:51: // this method returns.
On 2013/11/26 19:05:36, scherkus wrote:
> nit: assuming this class is designed to be accessed on a single thread, you
can
> drop the "|data| must be valid until this method returns" bit
> 
> ... otherwise it sounds as if data could somehow get changed by a different
> thread while Add() is running (and why wouldn't this apply to the const-ref
> std::vector version above?)

Yes, it is definitely for single thread only. Remove the unnecessary bit to
unify the comment line to the previous one.

[yukawa, 2013-11-26 19:24:04]

+jochen for the owner's review of content/content_tests.gypi.

[jochen (gone - plz use gerrit), 2013-11-27 09:14:05]

lgtm

[commit-bot: I haz the power, 2013-11-27 10:07:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukawa@chromium.org/68353002/1650001

[commit-bot: I haz the power, 2013-11-27 12:09:38]

Change committed as 237558