Update MidiDispatcher to use Mojo Permission Service.

Update MidiDispatcher to use Mojo Permission Service.

This is removing the need for MidiDispatcherHost.

BUG=430238
Committed: https://crrev.com/97de25e48eb98168b3d61c56a99522b1768e3ea5
Cr-Commit-Position: refs/heads/master@{#305614}

[mlamouri (slow - plz ping), 2014-11-24 17:31:04]

mlamouri@chromium.org changed reviewers:
+ ddorwin@chromium.org, nasko@chromium.org, tsepez@chromium.org

[mlamouri (slow - plz ping), 2014-11-24 17:31:04]

tsepez@chromium.org: Please review changes in:
 * content/common/media/midi_messages.h
 * content/common/permission_service.mojom
ddorwin@chromium.org: Please review changes in 
 * chrome/browser/media/midi_permission_context.cc
nasko@chromium.org: Please review changes in 
 * content/*

+CC timvolodine@ and miguelg@ FYI

[Tom Sepez, 2014-11-24 18:10:02]

https://codereview.chromium.org/755673002/diff/1/content/public/browser/child...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/755673002/diff/1/content/public/browser/child...
content/public/browser/child_process_security_policy.h:155: virtual void
GrantSendMidiSysExMessage(int child_id) = 0;
I'd expected to see a corresponding CanSendMidi...() method?

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
content/renderer/media/midi_dispatcher.cc:31: 
Can the connect fail? Do we need to handle that case?

[mlamouri (slow - plz ping), 2014-11-24 18:20:17]

https://codereview.chromium.org/755673002/diff/1/content/public/browser/child...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/755673002/diff/1/content/public/browser/child...
content/public/browser/child_process_security_policy.h:155: virtual void
GrantSendMidiSysExMessage(int child_id) = 0;
On 2014/11/24 18:10:02, Tom Sepez wrote:
> I'd expected to see a corresponding CanSendMidi...() method?

It's in the _impl.h It seems that ChildProcessSecurityPolicy only has methods
that need to be public and the *Impl has the methods that can stay inside
content/.

I would gladly move CanSendMidiSysExMessage to the public interface if you
prefer.

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
content/renderer/media/midi_dispatcher.cc:31: 
On 2014/11/24 18:10:02, Tom Sepez wrote:
> Can the connect fail? Do we need to handle that case?

My understanding is that permission_service_ can't be null after calling
ConnectToRemoteService.

[ddorwin, 2014-11-24 18:21:56]

chrome/browser/media/midi_permission_context.cc LGTM

[Tom Sepez, 2014-11-24 18:27:09]

LGTM otherwise.

https://codereview.chromium.org/755673002/diff/1/content/public/browser/child...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/755673002/diff/1/content/public/browser/child...
content/public/browser/child_process_security_policy.h:155: virtual void
GrantSendMidiSysExMessage(int child_id) = 0;
Ah, OK.  Its OK where it is, I was just surprised not to see it change too.

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
content/renderer/media/midi_dispatcher.cc:31: 
On 2014/11/24 18:20:17, Mounir Lamouri wrote:
> On 2014/11/24 18:10:02, Tom Sepez wrote:
> > Can the connect fail? Do we need to handle that case?
> 
> My understanding is that permission_service_ can't be null after calling
> ConnectToRemoteService.

OK, can we add a debug assert?

[mlamouri (slow - plz ping), 2014-11-24 18:29:40]

+qsr@

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
content/renderer/media/midi_dispatcher.cc:31: 
On 2014/11/24 18:27:09, Tom Sepez wrote:
> On 2014/11/24 18:20:17, Mounir Lamouri wrote:
> > On 2014/11/24 18:10:02, Tom Sepez wrote:
> > > Can the connect fail? Do we need to handle that case?
> > 
> > My understanding is that permission_service_ can't be null after calling
> > ConnectToRemoteService.
> 
> OK, can we add a debug assert?

qsr@, do you have some insights about this? It seems that no content/ calls
actually check that a service is null after connecting. Is that an oversight or
would an assert make sense?

[nasko, 2014-11-25 00:31:00]

LGTM with nits.

https://codereview.chromium.org/755673002/diff/1/content/common/permission_se...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/755673002/diff/1/content/common/permission_se...
content/common/permission_service.mojom:15: MIDI_SYSEX,
nit: MIDI_SYS_EX

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
content/renderer/media/midi_dispatcher.cc:31: 
On 2014/11/24 18:29:40, Mounir Lamouri wrote:
> On 2014/11/24 18:27:09, Tom Sepez wrote:
> > On 2014/11/24 18:20:17, Mounir Lamouri wrote:
> > > On 2014/11/24 18:10:02, Tom Sepez wrote:
> > > > Can the connect fail? Do we need to handle that case?
> > > 
> > > My understanding is that permission_service_ can't be null after calling
> > > ConnectToRemoteService.
> > 
> > OK, can we add a debug assert?
> 
> qsr@, do you have some insights about this? It seems that no content/ calls
> actually check that a service is null after connecting. Is that an oversight
or
> would an assert make sense?

I'm biased towards adding a (D)CHECK if we never expect it to fail.

[qsr, 2014-11-25 09:37:59]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2014-11-25 09:38:00]

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
content/renderer/media/midi_dispatcher.cc:31: 
On 2014/11/25 00:30:59, nasko wrote:
> On 2014/11/24 18:29:40, Mounir Lamouri wrote:
> > On 2014/11/24 18:27:09, Tom Sepez wrote:
> > > On 2014/11/24 18:20:17, Mounir Lamouri wrote:
> > > > On 2014/11/24 18:10:02, Tom Sepez wrote:
> > > > > Can the connect fail? Do we need to handle that case?
> > > > 
> > > > My understanding is that permission_service_ can't be null after calling
> > > > ConnectToRemoteService.
> > > 
> > > OK, can we add a debug assert?
> > 
> > qsr@, do you have some insights about this? It seems that no content/ calls
> > actually check that a service is null after connecting. Is that an oversight
> or
> > would an assert make sense?
> 
> I'm biased towards adding a (D)CHECK if we never expect it to fail.

 The DCHECK is not useful. The service cannot be null. What happens there is
that you create a pair of pipe, connect one to the proxy (the service), and send
the other one away.

 The proxy is initialized. Now, that doesn't mean there will be something on the
other side. If the other side close the pipe without connecting it to an
implementation, all the calls will just be lost.

[mlamouri (slow - plz ping), 2014-11-25 09:48:01]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-11-25 09:48:21]

https://codereview.chromium.org/755673002/diff/1/content/common/permission_se...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/755673002/diff/1/content/common/permission_se...
content/common/permission_service.mojom:15: MIDI_SYSEX,
On 2014/11/25 00:30:59, nasko wrote:
> nit: MIDI_SYS_EX

The current permission name is content::PERMISSION_MIDI_SYSEX. Also, MIDI_SYS_EX
is never used in the code base but MIDI_SYSEX is used some times (a couple
dozen).

I will keep the current name if you don't mind then.

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/755673002/diff/1/content/renderer/media/midi_...
content/renderer/media/midi_dispatcher.cc:31: 
On 2014/11/25 09:38:00, qsr wrote:
> On 2014/11/25 00:30:59, nasko wrote:
> > On 2014/11/24 18:29:40, Mounir Lamouri wrote:
> > > On 2014/11/24 18:27:09, Tom Sepez wrote:
> > > > On 2014/11/24 18:20:17, Mounir Lamouri wrote:
> > > > > On 2014/11/24 18:10:02, Tom Sepez wrote:
> > > > > > Can the connect fail? Do we need to handle that case?
> > > > > 
> > > > > My understanding is that permission_service_ can't be null after
calling
> > > > > ConnectToRemoteService.
> > > > 
> > > > OK, can we add a debug assert?
> > > 
> > > qsr@, do you have some insights about this? It seems that no content/
calls
> > > actually check that a service is null after connecting. Is that an
oversight
> > or
> > > would an assert make sense?
> > 
> > I'm biased towards adding a (D)CHECK if we never expect it to fail.
> 
>  The DCHECK is not useful. The service cannot be null. What happens there is
> that you create a pair of pipe, connect one to the proxy (the service), and
send
> the other one away.
> 
>  The proxy is initialized. Now, that doesn't mean there will be something on
the
> other side. If the other side close the pipe without connecting it to an
> implementation, all the calls will just be lost.

Thanks for the quick reply Benjamin! :)

[commit-bot: I haz the power, 2014-11-25 09:49:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/755673002/1

[commit-bot: I haz the power, 2014-11-25 10:50:32]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-11-25 10:51:11]

Patchset 1 (id:??) landed as
https://crrev.com/97de25e48eb98168b3d61c56a99522b1768e3ea5
Cr-Commit-Position: refs/heads/master@{#305614}