Web MIDI: Ask permissions of the content layer always

content/renderer/media/midi_dispatcher.cc

Index: content/renderer/media/midi_dispatcher.cc
diff --git a/content/renderer/media/midi_dispatcher.cc b/content/renderer/media/midi_dispatcher.cc
index 40fca8472bec453b3211554cd592d05736df47d9..5f07e991a4a01b8d64f3478092cc5257bd938d14 100644
--- a/content/renderer/media/midi_dispatcher.cc
+++ b/content/renderer/media/midi_dispatcher.cc
@@ -13,6 +13,7 @@
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 
 using blink::WebMIDIPermissionRequest;
+using blink::WebMIDIOptions;
 using blink::WebSecurityOrigin;
 
 namespace content {
@@ -23,8 +24,12 @@ MidiDispatcher::MidiDispatcher(RenderFrame* render_frame)
 
 MidiDispatcher::~MidiDispatcher() {}
 
-void MidiDispatcher::requestSysexPermission(
-      const WebMIDIPermissionRequest& request) {
+void MidiDispatcher::requestPermission(const WebMIDIPermissionRequest& request,
+                                       const WebMIDIOptions& options) {
+  // TODO(crbug.com/535181): Expose all requests to the content layer.
+  if (!options.sysex)
+    return WebMIDIPermissionRequest(request).setIsAllowed(true);

[mlamouri (slow - plz ping), 2016/01/21 11:50:57]

What about doing:
PermissionName permission_name = options.sysex ? PERMISSION_NAME_MIDI_SYSEX :
PERMISSION_NAME_MIDI;

(The non-sysex midi permission is already part of the permission system so you
really just need to use it here.)

[Takashi Toyoshima, 2016/01/22 04:33:22]

Thank you for jumping in! I thought I will need your help in the next changes.

I tried PERMISSION_NAME_MIDI, but it makes webmidi layout tests fail. Then, I
decided to split changes outside blink for the next patch. But if I can fix it
easily, I'd merge it to this change.

By the way, I'll add one more option 'software' in addition to 'sysex' soon. In
that case, what's the best practice in terms of current psermission service and
content settings's design?
Should I passed an array of permissions via RequestPermissions()?

[Takashi Toyoshima, 2016/01/22 07:30:01]

Some additional notes for this:

- PERMISSION_NAME_MIDI is rejected by default on layout tests, then need to call
testRunner.setPermission.
- setPermission({name:"midi"}) crashes at NOTREACHED in
LayoutTestMessageFiler::OnSetPermission for lack of "midi" handling.
- Even after handling it, setPermission call won't invoke result.onchange of
permissions.query in permissions-helper.js

So, I feel it's reasonable to split the layout test trick change to another CL,
and leave this TODO as is to make review easy. WDYT?

+
   if (!permission_service_.get()) {
     render_frame()->GetServiceRegistry()->ConnectToRemoteService(
         mojo::GetProxy(&permission_service_));
@@ -34,15 +39,13 @@ void MidiDispatcher::requestSysexPermission(
       requests_.Add(new WebMIDIPermissionRequest(request));
 
   permission_service_->RequestPermission(
-      PERMISSION_NAME_MIDI_SYSEX,
-      request.securityOrigin().toString().utf8(),
+      PERMISSION_NAME_MIDI_SYSEX, request.securityOrigin().toString().utf8(),
       blink::WebUserGestureIndicator::isProcessingUserGesture(),
-      base::Bind(&MidiDispatcher::OnSysExPermissionSet,
-                 base::Unretained(this),
+      base::Bind(&MidiDispatcher::OnPermissionSet, base::Unretained(this),
                  permission_request_id));
 }
 
-void MidiDispatcher::cancelSysexPermissionRequest(
+void MidiDispatcher::cancelPermissionRequest(
     const WebMIDIPermissionRequest& request) {
   for (Requests::iterator it(&requests_); !it.IsAtEnd(); it.Advance()) {
     WebMIDIPermissionRequest* value = it.GetCurrentValue();
@@ -53,8 +56,7 @@ void MidiDispatcher::cancelSysexPermissionRequest(
   }
 }
 
-void MidiDispatcher::OnSysExPermissionSet(int request_id,
-                                          PermissionStatus status) {
+void MidiDispatcher::OnPermissionSet(int request_id, PermissionStatus status) {
   // |request| can be NULL when the request is canceled.
   WebMIDIPermissionRequest* request = requests_.Lookup(request_id);
   if (!request)