Web MIDI: Ask permissions of the content layer always

Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but with this series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181
Committed: https://crrev.com/71d21daf1f3b427b7dca74a536566def4a27e39e
Cr-Commit-Position: refs/heads/master@{#372044}

[Takashi Toyoshima, 2016-01-19 14:11:25]

Description was changed from

==========
WIP: Web MIDI: Ask permissions to the content layer always

Originally only sysex permission requests are exposed to the
content layer, but after this patch, all Web MIDI requests should be
exposed to the content layer so that each chromium embedder can decide
how to decide several permissions.

This does not change anything for Chrome, but each embedder can change
permission policy by adding own client code.

BUG=535181
==========

to

==========
WIP: Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but after this patch, all Web MIDI requests should be
exposed to the content layer so that each chromium embedder can decide
how to decide several permissions.

This does not change anything for Chrome, but each embedder can change
permission policy by adding own client code.

BUG=535181
==========

[Takashi Toyoshima, 2016-01-19 14:32:51]

Description was changed from

==========
WIP: Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but after this patch, all Web MIDI requests should be
exposed to the content layer so that each chromium embedder can decide
how to decide several permissions.

This does not change anything for Chrome, but each embedder can change
permission policy by adding own client code.

BUG=535181
==========

to

==========
Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but after a series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181
==========

[Takashi Toyoshima, 2016-01-20 10:51:25]

toyoshim@chromium.org changed reviewers:
+ yhirano@chromium.org

[Takashi Toyoshima, 2016-01-20 10:52:33]

this is ready for review, but it might be better to start after
public/*/modules/webmidi changes are submitted.

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

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

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

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.cc:31: return
WebMIDIPermissionRequest(request).setIsAllowed(true);
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]

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.cc:31: return
WebMIDIPermissionRequest(request).setIsAllowed(true);
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]

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.cc:31: return
WebMIDIPermissionRequest(request).setIsAllowed(true);
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?

[mlamouri (slow - plz ping), 2016-01-22 11:03:21]

On 2016/01/22 at 07:30:01, toyoshim wrote:
>
https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
> File content/renderer/media/midi_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
> content/renderer/media/midi_dispatcher.cc:31: return
WebMIDIPermissionRequest(request).setIsAllowed(true);
> 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?

That sounds reasonable indeed.

[mlamouri (slow - plz ping), 2016-01-22 11:54:43]

lgtm

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebMIDIClient.h (right):

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMIDIClient.h:34: #include "WebMIDIOptions.h"
nit: I think you can make this a forward declare.

[yhirano, 2016-01-25 11:20:16]

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.cc:10: #include
"third_party/WebKit/public/platform/WebString.h"
+#include ".../WebMidiOptions.h"

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.h (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.h:15: class WebMIDIPermissionRequest;
+ class WebMIDIOptions;

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebMIDIOptions.h (right):

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMIDIOptions.h:11: WebMIDIOptions(bool sysex)
+explicit

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMIDIOptions.h:11: WebMIDIOptions(bool sysex)
[optional] Could you replace this boolean with an enum?

Please see [names-enum-to-bool] in http://dev.chromium.org/blink/coding-style

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMIDIOptions.h:16: bool sysex;
const?

[Takashi Toyoshima, 2016-01-26 07:24:53]

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.cc:10: #include
"third_party/WebKit/public/platform/WebString.h"
On 2016/01/25 11:20:16, yhirano wrote:
> +#include ".../WebMidiOptions.h"

Done.

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.h (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.h:15: class WebMIDIPermissionRequest;
I prefer not declaring and classes and structures that should be apparently
handled in headers that's already included.

In this case, WebMIDIOptions is used in WebMIDIClient's virtual functions. So,
WebMIDIClient.h should be responsible for WebMIDIOptions's declaration.

But, actually, I'm not sure if there is a clear guideline on this.
In my experience, the policy seems to be different module by module. E.g., net/
suggested removing redundant headers and declarations as possible when I worked
on. WDYT? My preference is the middle, just omit apparently redundant
declarations and inclusion, and include and declare if it's ambiguous.

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebMIDIClient.h (right):

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMIDIClient.h:34: #include "WebMIDIOptions.h"
On 2016/01/22 11:54:43, Mounir Lamouri wrote:
> nit: I think you can make this a forward declare.

Done.

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebMIDIOptions.h (right):

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMIDIOptions.h:11: WebMIDIOptions(bool sysex)
On 2016/01/25 11:20:16, yhirano wrote:
> [optional] Could you replace this boolean with an enum?
> 
> Please see [names-enum-to-bool] in http://dev.chromium.org/blink/coding-style

Done.

https://codereview.chromium.org/1602703005/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMIDIOptions.h:16: bool sysex;
On 2016/01/25 11:20:16, yhirano wrote:
> const?

Done.

[Takashi Toyoshima, 2016-01-26 08:08:03]

https://codereview.chromium.org/1602703005/diff/40001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/1602703005/diff/40001/content/renderer/media/...
content/renderer/media/midi_dispatcher.cc:38: PermissionName permission_name =
Oops, I mistakenly merge this change into this patch set.
I'll remove this in the next rebased one.

[Takashi Toyoshima, 2016-01-26 08:12:28]

Description was changed from

==========
Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but after a series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181
==========

to

==========
Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but with this series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181
==========

[Takashi Toyoshima, 2016-01-26 12:09:46]

yhirano: PTAL.

[yhirano, 2016-01-27 14:14:56]

lgtm

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
File content/renderer/media/midi_dispatcher.h (right):

https://codereview.chromium.org/1602703005/diff/20001/content/renderer/media/...
content/renderer/media/midi_dispatcher.h:15: class WebMIDIPermissionRequest;
On 2016/01/26 07:24:52, Takashi Toyoshima wrote:
> I prefer not declaring and classes and structures that should be apparently
> handled in headers that's already included.
> 
> In this case, WebMIDIOptions is used in WebMIDIClient's virtual functions. So,
> WebMIDIClient.h should be responsible for WebMIDIOptions's declaration.
> 
> But, actually, I'm not sure if there is a clear guideline on this.
> In my experience, the policy seems to be different module by module. E.g.,
net/
> suggested removing redundant headers and declarations as possible when I
worked
> on. WDYT? My preference is the middle, just omit apparently redundant
> declarations and inclusion, and include and declare if it's ambiguous.

Fair enough. All I know is the Google style guide
(https://google.github.io/styleguide/cppguide.html#Self_contained_Headers).

[Takashi Toyoshima, 2016-01-28 06:10:36]

toyoshim@chromium.org changed reviewers:
+ tkent@chromium.org

[Takashi Toyoshima, 2016-01-28 06:10:36]

tkent@chromium.org: Please review changes in WebKit/Source/web

[tkent, 2016-01-28 06:24:04]

lgtm

[Takashi Toyoshima, 2016-01-28 06:29:22]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-01-28 06:29:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1602703005/#ps80001
(title: "revert layout_test_message_filter.cc that should be in the next CL")

[commit-bot: I haz the power, 2016-01-28 06:29:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1602703005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1602703005/80001

[commit-bot: I haz the power, 2016-01-28 07:47:18]

Description was changed from

==========
Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but with this series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181
==========

to

==========
Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but with this series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181
==========

[commit-bot: I haz the power, 2016-01-28 07:47:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-28 07:47:44]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-01-28 07:48:34]

Description was changed from

==========
Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but with this series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181
==========

to

==========
Web MIDI: Ask permissions of the content layer always

Originally only sysex permission requests are exposed to the
content layer, but with this series of patches, all Web MIDI requests
will be exposed to the content layer so that each chromium embedder
can decide how to decide several permissions.

First of all, expose all requests outside Blink.
Changes for the permission service will follow as a separate patch.

BUG=535181

Committed: https://crrev.com/71d21daf1f3b427b7dca74a536566def4a27e39e
Cr-Commit-Position: refs/heads/master@{#372044}
==========

[commit-bot: I haz the power, 2016-01-28 07:48:36]

Patchset 5 (id:??) landed as
https://crrev.com/71d21daf1f3b427b7dca74a536566def4a27e39e
Cr-Commit-Position: refs/heads/master@{#372044}

[Takashi Toyoshima, 2016-01-28 07:57:03]

hum.... CQ says something scary, but landed.
If this causes a build break, and someone notice it before I notice. Please feel
free to revert this. (I'll be on train for a while)