Web MIDI API back-end (work-in-progress)

Implement Web MIDI API back-end

(this relands 16025005 with fix for perf regression)

This involves browser-side support and IPC for sending and receiving
MIDI messages.  Initially support for OSX is included.

BUG=163795
R=palmer@chromium.org, piman@chromium.org, scherkus@chromium.org

[scherkus (not reviewing), 2013-06-03 22:44:59]

Overall I'm liking the structure. It's straight-forward and easy to comprehend.
Nice!

Can you upload a CL w/ the the Blink-side changes (e.g., WebMIDIAccessor)? I'm
curious to see those bits as well.

Note: I intentionally didn't cover any style stuff, but let me know if you're
interested :)

Biggest questions / hurdles:
  1) Start up time of MIDI code as its executing on main UI thread during
browser launch
  2) Whether we can start / stop MIDI listening code only when approved clients
are listening (in other words, should we worry about having the MIDI procs
permanently registered during the lifetime of Chrome? see also #4)
  3) Whether device changes need to be handled
  4) Nailing down the security / approval model. Today it seems all renderer
processes will receive MIDI data whether they asked for it or not as well as
whether they were approved or not.

OOC have you thought about a Chrome eng review? This seems like a large enough
feature to warrant getting some extra advice from those folks.

https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:52:
printf("MIDIHost::OnRequestAccess()\n");
I'm assuming this will have to prompt UI at some point before it can start
forwarding data?

https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.h (right):

https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.h:34: virtual void
ReceiveMIDIData(
note: called on high-priority audio thread

https://codereview.chromium.org/16025005/diff/12001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/12001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:35: return g_filter;
AFAIK you can replace this global and
RendererWebMIDIAccessorImpl::midi_message_filter() with a call to
RenderThreadImpl::Get()->midi_message_filter()

I forget why we ended up using g_filter in AudioMessageFilter ... I'm assuming
it might be legacy due to a time when we had code that needed the global but
couldn't access RenderThreadImpl::Get(). Do you happen to remember?

https://codereview.chromium.org/16025005/diff/12001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:120: client_ = NULL;
//????????????????? multiple clients??
I didn't see anything in the spec that mentioned multiple clients:
http://webaudio.github.io/web-midi-api/#requestmidiaccess

... however if the same render process had two separate tabs that accessed MIDI
we'd end up with multiple clients, so I believe this is a case you need to
handle.

It also seems this class would need to maintain a list + fan out events to
clients that were granted access as it currently seems this class receives MIDI
data regardless of whether access was approved or not

https://codereview.chromium.org/16025005/diff/12001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.cc (right):

https://codereview.chromium.org/16025005/diff/12001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.cc:16: MIDIManagerMac::MIDIManagerMac() {
FYI this will be executed on the main UI thread when Chrome launches

do you have #s on what the impact of running this code is?

can we do it lazily e.g., the first time we successfully approve MIDI access?

https://codereview.chromium.org/16025005/diff/12001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/16025005/diff/12001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.h:28: static void ReadMidiDispatch(const
MIDIPacketList *pktlist,
may want to mention from apple docs that these procs are "called on a separate
high-priority thread owned by CoreMIDI."

[Chris Rogers, 2013-06-04 00:53:03]

Andrew, thanks for having such a quick look!  The Blink side changes are
partially landed in modules/webmidi (thanks to Takashi), but I have some further
iteration in this Blink CL:
https://codereview.chromium.org/16288002/

I have already posted the intention to implement to the Chrome eng mailing list,
and Brett had some initial comments and suggestions.  He felt that it was ok to
proceed.

I'll try to address your comments tomorrow.  In the meantime, I've added the
support to send the MIDI input/output port information from browser->renderer as
required by the Web MIDI spec:
http://webaudio.github.io/web-midi-api/

[Chris Rogers, 2013-06-05 01:20:24]

Andrew I've iterated this a fair bit since the last time you looked.  I hope to
answer *most* of your
questions below, and will continue iterating.

Would you mind having another look?  I'm hoping this is getting closer to
reviewable now.

On 2013/06/03 22:44:59, scherkus wrote:
> Overall I'm liking the structure. It's straight-forward and easy to
comprehend.
> Nice!
> 
> Can you upload a CL w/ the the Blink-side changes (e.g., WebMIDIAccessor)? I'm
> curious to see those bits as well.

https://codereview.chromium.org/16288002/

> 
> Note: I intentionally didn't cover any style stuff, but let me know if you're
> interested :)

Sure! I've made an attempt to get the style in shape since the last CL - maybe
not perfect, but hopefully better

> 
> Biggest questions / hurdles:
>   1) Start up time of MIDI code as its executing on main UI thread during
> browser launch

The latest CL avoids initializing the MIDI system during startup, but instead
lazily
initializes if MIDIHostMsg_RequestAccess succeeds.  So it will now happen later,
and only if the Web MIDI API is used.

>   2) Whether we can start / stop MIDI listening code only when approved
clients
> are listening (in other words, should we worry about having the MIDI procs
> permanently registered during the lifetime of Chrome? see also #4)

If necessary, we can add a system to uninitialize the callbacks once all clients
have "gone away".  For now, the callbacks only start if Web MIDI API is used at
least one time during the lifetime of Chrome.  We'll initially be handling this
feature as a run-time enable flag "Enable Web MIDI"

>   3) Whether device changes need to be handled

It *is* part of the Web MIDI spec, but for now is unimplemented.  I'll add a
TODO

>   4) Nailing down the security / approval model. Today it seems all renderer
> processes will receive MIDI data whether they asked for it or not as well as
> whether they were approved or not.

Please see reply to (1) -- the MIDI system is now lazily initialized

> 
> OOC have you thought about a Chrome eng review? This seems like a large enough
> feature to warrant getting some extra advice from those folks.
> 
>
https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
> File content/browser/renderer_host/media/midi_host.cc (right):
> 
>
https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
> content/browser/renderer_host/media/midi_host.cc:52:
> printf("MIDIHost::OnRequestAccess()\n");
> I'm assuming this will have to prompt UI at some point before it can start
> forwarding data?

We've given this some thought and believe it should be ok to approve access
without UI, provided that
System Exclusive messages are not involved.


> 
>
https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
> File content/browser/renderer_host/media/midi_host.h (right):
> 
>
https://codereview.chromium.org/16025005/diff/12001/content/browser/renderer_...
> content/browser/renderer_host/media/midi_host.h:34: virtual void
> ReceiveMIDIData(
> note: called on high-priority audio thread

It seems that IPC Send() function is smart enough to post this to the IO thread
automatically.  At least my tracing seems to indicate that.

> 
>
https://codereview.chromium.org/16025005/diff/12001/content/renderer/media/mi...
> File content/renderer/media/midi_message_filter.cc (right):
> 
>
https://codereview.chromium.org/16025005/diff/12001/content/renderer/media/mi...
> content/renderer/media/midi_message_filter.cc:35: return g_filter;
> AFAIK you can replace this global and
> RendererWebMIDIAccessorImpl::midi_message_filter() with a call to
> RenderThreadImpl::Get()->midi_message_filter()
> 
> I forget why we ended up using g_filter in AudioMessageFilter ... I'm assuming
> it might be legacy due to a time when we had code that needed the global but
> couldn't access RenderThreadImpl::Get(). Do you happen to remember?
> 
>
https://codereview.chromium.org/16025005/diff/12001/content/renderer/media/mi...
> content/renderer/media/midi_message_filter.cc:120: client_ = NULL;
> //????????????????? multiple clients??
> I didn't see anything in the spec that mentioned multiple clients:
> http://webaudio.github.io/web-midi-api/#requestmidiaccess
> 
> ... however if the same render process had two separate tabs that accessed
MIDI
> we'd end up with multiple clients, so I believe this is a case you need to
> handle.

Yes, exactly

> 
> It also seems this class would need to maintain a list + fan out events to
> clients that were granted access as it currently seems this class receives
MIDI
> data regardless of whether access was approved or not

A lot of this access will be controlled at the Blink layer, where an
EventHandler can only
be set on a port that was actually granted permission.  Not all of that checking
is yet in place
in the early WIP Blink code.

> 
>
https://codereview.chromium.org/16025005/diff/12001/media/midi/midi_manager_m...
> File media/midi/midi_manager_mac.cc (right):
> 
>
https://codereview.chromium.org/16025005/diff/12001/media/midi/midi_manager_m...
> media/midi/midi_manager_mac.cc:16: MIDIManagerMac::MIDIManagerMac() {
> FYI this will be executed on the main UI thread when Chrome launches
> 
> do you have #s on what the impact of running this code is?
> 
> can we do it lazily e.g., the first time we successfully approve MIDI access?

Latest CL is now lazy init.

[Chris Rogers, 2013-06-05 01:21:49]

+palmer

Hi Chris, I have enough of the IPC in place that it might make sense for you to
have a look when you have a spare moment.

[scherkus (not reviewing), 2013-06-11 02:27:24]

looking really good -- one q about yet-to-be-authorized clients in the renderer

I'll follow up reviewing style 'n stuff

https://codereview.chromium.org/16025005/diff/33001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/33001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:179:
(*i).first->receiveMIDIData(port, data.data(), data.size(), timestamp);
won't this send midi data to everyone in |clients_| even if they haven't
received a corresponding HandleAccessApproved() message?

[Chris Rogers, 2013-06-11 02:44:37]

On 2013/06/11 02:27:24, scherkus wrote:
> looking really good -- one q about yet-to-be-authorized clients in the
renderer
> 
> I'll follow up reviewing style 'n stuff
> 
>
https://codereview.chromium.org/16025005/diff/33001/content/renderer/media/mi...
> File content/renderer/media/midi_message_filter.cc (right):
> 
>
https://codereview.chromium.org/16025005/diff/33001/content/renderer/media/mi...
> content/renderer/media/midi_message_filter.cc:179:
> (*i).first->receiveMIDIData(port, data.data(), data.size(), timestamp);
> won't this send midi data to everyone in |clients_| even if they haven't
> received a corresponding HandleAccessApproved() message?

Yes, this is true.  However, the client itself will not yet have any ports added
to it, so it will just ignore the call.  I should probably add a comment about
that  For details, you can look at MIDIAccess::receiveMIDIData() in:
https://codereview.chromium.org/16288002/diff/15001/Source/modules/webmidi/MI...

Takashi will land the actual CL for that, but it gives you an idea...

[palmer, 2013-06-11 17:29:13]

Some initial thoughts. Let me know when you're getting closer to finishing it
and I'll take a close look.

https://codereview.chromium.org/16025005/diff/33001/content/common/media/medi...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/16025005/diff/33001/content/common/media/medi...
content/common/media/media_param_traits.cc:103: m->WriteInt(n);
I think you want WriteInt64, or cast the size_t down to a uint32_t (safely, of
course; using checked_numeric_cast or a similarly safe mechanism) and use
WriteInt32.

https://codereview.chromium.org/16025005/diff/33001/content/common/media/medi...
content/common/media/media_param_traits.cc:117: if (!m->ReadInt(iter, &n) || n <
0)
And then ReadFoo here, where Foo is whatever WriteFoo you used above (Int32 or
Int64).

[Chris Rogers, 2013-06-11 19:23:25]

Chris, thanks for looking.

https://codereview.chromium.org/16025005/diff/33001/content/common/media/medi...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/16025005/diff/33001/content/common/media/medi...
content/common/media/media_param_traits.cc:103: m->WriteInt(n);
On 2013/06/11 17:29:13, Chromium Palmer wrote:
> I think you want WriteInt64, or cast the size_t down to a uint32_t (safely, of
> course; using checked_numeric_cast or a similarly safe mechanism) and use
> WriteInt32.

Fixed, using int64

https://codereview.chromium.org/16025005/diff/33001/content/common/media/medi...
content/common/media/media_param_traits.cc:117: if (!m->ReadInt(iter, &n) || n <
0)
On 2013/06/11 17:29:13, Chromium Palmer wrote:
> And then ReadFoo here, where Foo is whatever WriteFoo you used above (Int32 or
> Int64).

Done.

[scherkus (not reviewing), 2013-06-12 01:28:47]

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:24: :
midi_manager_(midi_manager) {
indent two more spaces

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:49: 
nit: remove extra blank line

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:88: // Send to the renderer
nit: comments end w/ periods

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.h:21: class CONTENT_EXPORT
MIDIHost :
nit: ":" should be on next line

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.h:34: // MIDIManager::Client
nit: "MIDIManager::Client implementation."

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:613: 
nit: remove extra blank lines

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:615: 
nit: remove extra blank lines

https://codereview.chromium.org/16025005/diff/64001/content/common/media/medi...
File content/common/media/media_param_traits.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/common/media/medi...
content/common/media/media_param_traits.h:37: struct CONTENT_EXPORT
ParamTraits<media::MIDIPortInfoList> {
hrm... if MIDIPortInfoList was a typedef would you still need all this stuff?

also you may want to take a look at the IPC_STRUCT_TRAITS_XXX() set of macros --
I believe they help autogenerate this kind of code

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:168: ));
this looks a bit funky

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:107: base::MessageLoop*
client_message_loop_;
is it possible to use scoped_refptr<base::MessageLoopProxy> instead?

advantages include not having to worry about the pointer becoming invalid should
the loop go away

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/re...
File content/renderer/media/renderer_webmidiaccessor_impl.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/re...
content/renderer/media/renderer_webmidiaccessor_impl.h:19:
RendererWebMIDIAccessorImpl(WebKit::WebMIDIAccessor::Client* client);
explicit

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/re...
content/renderer/media/renderer_webmidiaccessor_impl.h:22: //
WebKit::WebMIDIAccessor
nit: ... implementation.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:63: 
remove blanks here

https://codereview.chromium.org/16025005/diff/64001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:398: 
remove extra blanks

https://codereview.chromium.org/16025005/diff/64001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:36: 
remove extra blanks

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

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.c...
media/midi/midi_manager.cc:38: };
remove extra  ;

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.h...
media/midi/midi_manager.h:25: class Client {
although it is prevalent elsewhere in media code, inner classes like this are
discouraged since they can't be forward declared

typical recommendation is to go with class MIDIManagerClient { ... };

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.h...
media/midi/midi_manager.h:28: virtual void ReceiveMIDIData(
needs docs

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.h:24: // MIDIManager
 ... implementation

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_port_info.h
File media/midi/midi_port_info.h (right):

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_port_info...
media/midi/midi_port_info.h:33: // Would rather use typedef, but seems hard to
forward declare.
are you trying to avoid including this .h in multiple places?

[Chris Rogers, 2013-06-12 20:34:35]

Thanks Andrew, I've addressed all comments except for the IPC_STRUCT_TRAITS_
stuff (see comment below)

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:24: :
midi_manager_(midi_manager) {
On 2013/06/12 01:28:47, scherkus wrote:
> indent two more spaces

Done.

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:49: 
On 2013/06/12 01:28:47, scherkus wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:88: // Send to the renderer
On 2013/06/12 01:28:47, scherkus wrote:
> nit: comments end w/ periods

Done.

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.h:21: class CONTENT_EXPORT
MIDIHost :
On 2013/06/12 01:28:47, scherkus wrote:
> nit: ":" should be on next line

Done.

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.h:34: // MIDIManager::Client
On 2013/06/12 01:28:47, scherkus wrote:
> nit: "MIDIManager::Client implementation."

Done.

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:613: 
On 2013/06/12 01:28:47, scherkus wrote:
> nit: remove extra blank lines

Done.

https://codereview.chromium.org/16025005/diff/64001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:615: 
On 2013/06/12 01:28:47, scherkus wrote:
> nit: remove extra blank lines

Done.

https://codereview.chromium.org/16025005/diff/64001/content/common/media/medi...
File content/common/media/media_param_traits.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/common/media/medi...
content/common/media/media_param_traits.h:37: struct CONTENT_EXPORT
ParamTraits<media::MIDIPortInfoList> {
On 2013/06/12 01:28:47, scherkus wrote:
> hrm... if MIDIPortInfoList was a typedef would you still need all this stuff?
> 
> also you may want to take a look at the IPC_STRUCT_TRAITS_XXX() set of macros
--
> I believe they help autogenerate this kind of code

I took a look at IPC_STRUCT_TRAITS_BEGIN, IPC_STRUCT_TRAITS_MEMBER, etc. which
we use in many places, but couldn't find an example where it's a *list* of
structs that we want to send.  If you can help point me in the right direction
for how to do this for things which are vector<struct>, I'm happy to change it. 
For now, I'm not sure if it's possible, so that's why I wrote the pickling like
it is now.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:168: ));
On 2013/06/12 01:28:47, scherkus wrote:
> this looks a bit funky

I'm assuming you mean the style here :)

Done.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:107: base::MessageLoop*
client_message_loop_;
On 2013/06/12 01:28:47, scherkus wrote:
> is it possible to use scoped_refptr<base::MessageLoopProxy> instead?
> 
> advantages include not having to worry about the pointer becoming invalid
should
> the loop go away

Done.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/re...
File content/renderer/media/renderer_webmidiaccessor_impl.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/re...
content/renderer/media/renderer_webmidiaccessor_impl.h:19:
RendererWebMIDIAccessorImpl(WebKit::WebMIDIAccessor::Client* client);
On 2013/06/12 01:28:47, scherkus wrote:
> explicit

Done.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/media/re...
content/renderer/media/renderer_webmidiaccessor_impl.h:22: //
WebKit::WebMIDIAccessor
On 2013/06/12 01:28:47, scherkus wrote:
> nit: ... implementation.

Done.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:63: 
On 2013/06/12 01:28:47, scherkus wrote:
> remove blanks here

Done.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:398: 
On 2013/06/12 01:28:47, scherkus wrote:
> remove extra blanks

Done.

https://codereview.chromium.org/16025005/diff/64001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/16025005/diff/64001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:36: 
On 2013/06/12 01:28:47, scherkus wrote:
> remove extra blanks

Done.

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

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.c...
media/midi/midi_manager.cc:38: };
On 2013/06/12 01:28:47, scherkus wrote:
> remove extra  ;

Done.

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.h...
media/midi/midi_manager.h:25: class Client {
On 2013/06/12 01:28:47, scherkus wrote:
> although it is prevalent elsewhere in media code, inner classes like this are
> discouraged since they can't be forward declared
> 
> typical recommendation is to go with class MIDIManagerClient { ... };

Done.

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager.h...
media/midi/midi_manager.h:28: virtual void ReceiveMIDIData(
On 2013/06/12 01:28:47, scherkus wrote:
> needs docs

Added comments for this and similar methods in this file.

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.h:24: // MIDIManager
On 2013/06/12 01:28:47, scherkus wrote:
>  ... implementation

Done.

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_port_info.h
File media/midi/midi_port_info.h (right):

https://codereview.chromium.org/16025005/diff/64001/media/midi/midi_port_info...
media/midi/midi_port_info.h:33: // Would rather use typedef, but seems hard to
forward declare.
On 2013/06/12 01:28:47, scherkus wrote:
> are you trying to avoid including this .h in multiple places?

I think I was trying to avoid including it in "media_param_traits.h"
For now, I've turned this back into a typedef and am now including this header
in "media_param_traits.h"

[scherkus (not reviewing), 2013-06-13 02:05:41]

https://codereview.chromium.org/16025005/diff/64001/content/common/media/medi...
File content/common/media/media_param_traits.h (right):

https://codereview.chromium.org/16025005/diff/64001/content/common/media/medi...
content/common/media/media_param_traits.h:37: struct CONTENT_EXPORT
ParamTraits<media::MIDIPortInfoList> {
On 2013/06/12 20:34:35, Chris Rogers wrote:
> On 2013/06/12 01:28:47, scherkus wrote:
> > hrm... if MIDIPortInfoList was a typedef would you still need all this
stuff?
> > 
> > also you may want to take a look at the IPC_STRUCT_TRAITS_XXX() set of
macros
> --
> > I believe they help autogenerate this kind of code
> 
> I took a look at IPC_STRUCT_TRAITS_BEGIN, IPC_STRUCT_TRAITS_MEMBER, etc. which
> we use in many places, but couldn't find an example where it's a *list* of
> structs that we want to send.  If you can help point me in the right direction
> for how to do this for things which are vector<struct>, I'm happy to change
it. 
> For now, I'm not sure if it's possible, so that's why I wrote the pickling
like
> it is now.
> 

AFAIK the IPC system knows how to deal with std::vector so you only need to
provide a traits definition for the struct itself.

IPC_STRUCT_TRAITS_BEGIN(media::MIDIPortInfo)
  IPC_STRUCT_TRAITS_MEMBER(...)
IPC_STRUCT_TRAITS_END(media::MIDIPortInfo)

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_manager.h...
media/midi/midi_manager.h:28: // |length| is the number of bytes in |data|.
fix indenting

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_port_info.h
File media/midi/midi_port_info.h (right):

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_port_info...
media/midi/midi_port_info.h:27: std::string id_;
public struct members can leave off the trailing _

[Chris Rogers, 2013-06-14 00:47:33]

PTAL - latest patch uses IPC_STRUCT_TRAITS_BEGIN, which works as you explained
it - thanks

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_manager.h...
media/midi/midi_manager.h:28: // |length| is the number of bytes in |data|.
On 2013/06/13 02:05:41, scherkus wrote:
> fix indenting

Done.

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_port_info.h
File media/midi/midi_port_info.h (right):

https://codereview.chromium.org/16025005/diff/77001/media/midi/midi_port_info...
media/midi/midi_port_info.h:27: std::string id_;
On 2013/06/13 02:05:41, scherkus wrote:
> public struct members can leave off the trailing _

Done.

[scherkus (not reviewing), 2013-06-14 00:54:48]

few more tiny nits, but this is pretty much "ell gee tee emm"

what are the next steps / plans for landing everything?

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:127: UTF8ToUTF16(inputs[i].id),
nit: these should be indented by 2 more spaces

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:135: UTF8ToUTF16(outputs[i].id),
ditto

https://codereview.chromium.org/16025005/diff/99001/media/midi/midi_port_info.cc
File media/midi/midi_port_info.cc (right):

https://codereview.chromium.org/16025005/diff/99001/media/midi/midi_port_info...
media/midi/midi_port_info.cc:32: void MIDIPortInfo::Print() const {
OOC do you use this somewhere in production code or is this for debugging?

typically we don't include such methods

[ddorwin, 2013-06-18 05:02:16]

Drive-by nit

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/re...
File content/renderer/media/renderer_webmidiaccessor_impl.h (right):

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/re...
content/renderer/media/renderer_webmidiaccessor_impl.h:5: #ifndef
CONTENT_RENDERER_MEDIA_RENDERER_WEBMIDIACCESOR_IMPL_H_
s/ACCESOR/ACCESSOR/

[palmer, 2013-06-19 21:39:15]

LGTM, with nits/questions/observations.

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

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:71: client_id,
Nit: Indent one more space to align with (.

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:92: std::vector<uint8> v(data,
data + length);
Yeah, it would seem good to avoid this copy, if possible, by making the function
accept a const std::vector<uint8>&.

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
File content/browser/renderer_host/media/midi_host.h (right):

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.h:38: size_t length,
Does it make sense to subsume |data| and |length| into a const
std::vector<uint8>&, as used in OnSendData, below?

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:185: std::vector<uint8> v(data,
data + length);
Same copying-due-to-interface issue here. Perhaps it's necessary; just checking.

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
File content/renderer/media/renderer_webmidiaccessor_impl.cc (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
content/renderer/media/renderer_webmidiaccessor_impl.cc:29: const unsigned char*
data,
Same type name consistency thing

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
File content/renderer/media/renderer_webmidiaccessor_impl.h (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
content/renderer/media/renderer_webmidiaccessor_impl.h:27: const unsigned char*
data,
FWIW, you use uint8 elsewhere. It doesn't matter but it's good to be consistent.

https://codereview.chromium.org/16025005/diff/118001/media/midi/midi_manager_...
File media/midi/midi_manager_mac.cc (right):

https://codereview.chromium.org/16025005/diff/118001/media/midi/midi_manager_...
media/midi/midi_manager_mac.cc:158: if (port_index < 0 ||
Funny, I saw the same pattern in my previous (unrelated) code review. This code
is OK as-is, but for cleanliness You could either change post_index to be a
size_t or other unsigned type (thus obviating the port_index < 0 part of the
check), or use the range-checked std::vector::at function instead of operator[].

[Chris Rogers, 2013-06-20 17:41:32]

jam: OWNERS review for ipc and content?

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:127: UTF8ToUTF16(inputs[i].id),
On 2013/06/14 00:54:48, scherkus wrote:
> nit: these should be indented by 2 more spaces

Done.

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:135: UTF8ToUTF16(outputs[i].id),
On 2013/06/14 00:54:48, scherkus wrote:
> ditto

Done.

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/re...
File content/renderer/media/renderer_webmidiaccessor_impl.h (right):

https://codereview.chromium.org/16025005/diff/99001/content/renderer/media/re...
content/renderer/media/renderer_webmidiaccessor_impl.h:5: #ifndef
CONTENT_RENDERER_MEDIA_RENDERER_WEBMIDIACCESOR_IMPL_H_
On 2013/06/18 05:02:16, ddorwin wrote:
> s/ACCESOR/ACCESSOR/

Done.

https://codereview.chromium.org/16025005/diff/99001/media/midi/midi_port_info.cc
File media/midi/midi_port_info.cc (right):

https://codereview.chromium.org/16025005/diff/99001/media/midi/midi_port_info...
media/midi/midi_port_info.cc:32: void MIDIPortInfo::Print() const {
On 2013/06/14 00:54:48, scherkus wrote:
> OOC do you use this somewhere in production code or is this for debugging?
> 
> typically we don't include such methods

Done.

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

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:71: client_id,
On 2013/06/19 21:39:15, Chromium Palmer wrote:
> Nit: Indent one more space to align with (.

Done.

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:92: std::vector<uint8> v(data,
data + length);
On 2013/06/19 21:39:15, Chromium Palmer wrote:
> Yeah, it would seem good to avoid this copy, if possible, by making the
function
> accept a const std::vector<uint8>&.

At the earliest stages, we start with a pointer to raw data (at the OS-specific
level), so need to take the hit (converting to vector<>) at some point.

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
File content/browser/renderer_host/media/midi_host.h (right):

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.h:38: size_t length,
On 2013/06/19 21:39:15, Chromium Palmer wrote:
> Does it make sense to subsume |data| and |length| into a const
> std::vector<uint8>&, as used in OnSendData, below?

For this layer, I've chosen data and length to avoid another copy into a vector,
since the OS-specific APIs just have a pointer to the bytes directly.

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:185: std::vector<uint8> v(data,
data + length);
On 2013/06/19 21:39:15, Chromium Palmer wrote:
> Same copying-due-to-interface issue here. Perhaps it's necessary; just
checking.

Yeah, I think at some point we have to take the hit to convert into a vector<>,
since this data originates from Blink, and we can't use std::vector<> in the
public Blink API boundary...

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
File content/renderer/media/renderer_webmidiaccessor_impl.cc (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
content/renderer/media/renderer_webmidiaccessor_impl.cc:29: const unsigned char*
data,
On 2013/06/19 21:39:15, Chromium Palmer wrote:
> Same type name consistency thing

API-boundary issue

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
File content/renderer/media/renderer_webmidiaccessor_impl.h (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/r...
content/renderer/media/renderer_webmidiaccessor_impl.h:27: const unsigned char*
data,
On 2013/06/19 21:39:15, Chromium Palmer wrote:
> FWIW, you use uint8 elsewhere. It doesn't matter but it's good to be
consistent.

In this case it's because it's a Blink-API boundary where uint8 isn't allowed,
so I have to use this...

https://codereview.chromium.org/16025005/diff/118001/media/midi/midi_manager_...
File media/midi/midi_manager_mac.cc (right):

https://codereview.chromium.org/16025005/diff/118001/media/midi/midi_manager_...
media/midi/midi_manager_mac.cc:158: if (port_index < 0 ||
On 2013/06/19 21:39:15, Chromium Palmer wrote:
> Funny, I saw the same pattern in my previous (unrelated) code review. This
code
> is OK as-is, but for cleanliness You could either change post_index to be a
> size_t or other unsigned type (thus obviating the port_index < 0 part of the
> check), or use the range-checked std::vector::at function instead of
operator[].

Agreed that unsigned is better.  For now adding TODO,since this propagates all
the way through the Blink API, so will take some re-factoring...

[piman, 2013-06-20 20:16:19]

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

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:82:
midi_manager_->SendMIDIData(port, data.data(), data.size(), timestamp);
High-level question: this runs on the IO thread. Can sending to the midi manager
block? AFAIK MIDI interface data rate is very low (4k/s).
It would be a bad thing for one renderer that does MIDI to block the IO thread
and so block the other renderers, UI, etc.
Is the data directly coming from the page? Can a malicious site feed MBs of data
at once?
If the midi manager buffers the data, is there a risk for a malicious site to
OOM the browser?

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:84: Send(new
MIDIHostMsg_RequestAccess(client_id, access));
Which thread is this running on? The doc in the header suggests that
client_message_loop_ is the main thread, but Send expects this to be called on
the IO thread.

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:186: Send(new
MIDIHostMsg_SendData(port_index, v, timestamp));
Is this called on the client thread or IO thread?

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.h:103: static int next_available_id_;
nit: does this need to be static?

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.h:106: base::Lock clients_lock_;
I think you can do away with the lock:
channel_ is only accessed from the IO thread
io_message_loop_ is constant
clients_ is only accessed from the client thread
next_available_id_ is only accessed from the client thread
client_message_loop_ could be made constant if we took it in the constructor
(which happens on the main thread). Or you could even use
ChildThread::current()->message_loop().

[Chris Rogers, 2013-06-21 23:04:11]

piman: PTAL

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

https://codereview.chromium.org/16025005/diff/118001/content/browser/renderer...
content/browser/renderer_host/media/midi_host.cc:82:
midi_manager_->SendMIDIData(port, data.data(), data.size(), timestamp);
On 2013/06/20 20:16:20, piman wrote:
> High-level question: this runs on the IO thread. Can sending to the midi
manager
> block? AFAIK MIDI interface data rate is very low (4k/s).
> It would be a bad thing for one renderer that does MIDI to block the IO thread
> and so block the other renderers, UI, etc.
> Is the data directly coming from the page? Can a malicious site feed MBs of
data
> at once?
> If the midi manager buffers the data, is there a risk for a malicious site to
> OOM the browser?

According to our offline discussion, For now, I'm simply not allowing sending of
MIDI data for this CL,
and will work on that for a followup CL to address these types of problems.

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:84: Send(new
MIDIHostMsg_RequestAccess(client_id, access));
On 2013/06/20 20:16:20, piman wrote:
> Which thread is this running on? The doc in the header suggests that
> client_message_loop_ is the main thread, but Send expects this to be called on
> the IO thread.

Yes, good point. Fixed.

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:186: Send(new
MIDIHostMsg_SendData(port_index, v, timestamp));
On 2013/06/20 20:16:20, piman wrote:
> Is this called on the client thread or IO thread?

In this CL I have the code initially written to PostTask to
SendMIDIDataOnIOThread(), but I'm blocking allowing this code to actually
execute until a follow-up CL.

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.h:103: static int next_available_id_;
On 2013/06/20 20:16:20, piman wrote:
> nit: does this need to be static?

Done.

https://codereview.chromium.org/16025005/diff/118001/content/renderer/media/m...
content/renderer/media/midi_message_filter.h:106: base::Lock clients_lock_;
On 2013/06/20 20:16:20, piman wrote:
> I think you can do away with the lock:
> channel_ is only accessed from the IO thread
> io_message_loop_ is constant
> clients_ is only accessed from the client thread
> next_available_id_ is only accessed from the client thread
> client_message_loop_ could be made constant if we took it in the constructor
> (which happens on the main thread). Or you could even use
> ChildThread::current()->message_loop().

Yes you're right - removed it.

[piman, 2013-06-21 23:16:13]

Just one thing, LGTM after that.

https://codereview.chromium.org/16025005/diff/134001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/16025005/diff/134001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:71: client_message_loop_ =
base::MessageLoopProxy::current();
Do you mind moving this to the constructor?
As is it looks racy because it's set on the main thread and used on the IO
thread (OnAccessApproved).
If you set it in the constructor, it's easy to convince oneself that it's safe.

[scherkus (not reviewing), 2013-06-21 23:29:48]

oh crap I never gave my official blessing!

lgtm % piman's nit!

[Chris Rogers, 2013-06-22 00:55:57]

Committed patchset #18 manually as r207983.