Web MIDI: add RequestMIDISysExPermission to BrowserContext

Web MIDI: add RequestMIDISysExPermission to BrowserContext

Add RequestMIDISysExPermission to handle Web MIDI API permissions for
system exclusive messages in content embedder.

BUG=163795, 257618
TEST=none
TBR=benm@chromium.org

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

[Takashi Toyoshima, 2013-07-03 15:47:13]

Hi jam,

I'd like to add new interface into Content API.
Can you review this change?

Background is that we are implementing Web MIDI API, and it needs UI to get
permission for full control of MIDI devices.

I already implement major part of it and now start upstreaming.
See also the following full change list for Web MIDI permission handling.
It will be handled as a member of content settings.
https://codereview.chromium.org/18004004/

[jam, 2013-07-08 20:18:56]

hi, sorry for the delay in getting to this. a few comments:

-it doesn't look like you need a separate class? i.e. you can have a method on
BrowserContext for the embedder to implement
-i don't see CancelMIDISysExPermissionRequest being called in the other change
that you linked to. are you sure this is needed?

[Takashi Toyoshima, 2013-07-11 09:04:53]

> -it doesn't look like you need a separate class? i.e. you can have a method on
> BrowserContext for the embedder to implement

OK, I'll try the method approach.

> -i don't see CancelMIDISysExPermissionRequest being called in the other change
> that you linked to. are you sure this is needed?

Adding blink API needs cancelation on navigating another page.
https://codereview.chromium.org/18325007/
But, now I think renderer can terminate the cancelation and contents API doesn't
need it.
I'll remove Cancel API.

[Takashi Toyoshima, 2013-07-11 10:12:27]

I update the API change in Patch Set 2.
Can you take a look again?

[jam, 2013-07-12 00:16:31]

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
content/public/browser/browser_context.h:54: typedef base::Callback<void(int,
int, int, bool)> MIDISysExPermissionCallback;
it looks like you're sending the embedder a bunch of values and then expecting
them to pass them back in. it would be simpler if you use currying to
transparently send these parameters back to content. even if chrome also needs
some of them (i.e. rph/rvh), then you can put it in RequestMIDISysExPermission,
and also use currying


nit: convention in content api (which existing method above doesn't follow :( )
is to put the callback typedef right above the method which uses it

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
content/public/browser/browser_context.h:138: virtual void
RequestMIDISysExPermission(
what is "SysEx"? see style guide re abbreviations

[Takashi Toyoshima, 2013-07-12 12:45:49]

Thanks, currying looks pretty nice to use here.
Can you give me another advice on abbreviation since it comes from the Web MIDI
spec.

Anyway, I uploaded Patch Set 5 using currying for the callback.

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
content/public/browser/browser_context.h:54: typedef base::Callback<void(int,
int, int, bool)> MIDISysExPermissionCallback;
On 2013/07/12 00:16:31, jam wrote:
> it looks like you're sending the embedder a bunch of values and then expecting
> them to pass them back in. it would be simpler if you use currying to
> transparently send these parameters back to content. even if chrome also needs
> some of them (i.e. rph/rvh), then you can put it in
RequestMIDISysExPermission,
> and also use currying
> 
> 
> nit: convention in content api (which existing method above doesn't follow :(
)
> is to put the callback typedef right above the method which uses it

Done.

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
content/public/browser/browser_context.h:138: virtual void
RequestMIDISysExPermission(
SysEx is a abbreviation of system exclusive messages, but it is used in Web MIDI
API as a parameter name. So, I think using SysEx here is straight-forward way
and consistent with the spec though style guide say don't use well-known
abbreviations. What do you think about this?
FYI, we already use "SysEx" and "sysex" in some parts of Web MIDI related
codebases.

https://codereview.chromium.org/18647002/diff/26001/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/18647002/diff/26001/content/public/browser/br...
content/public/browser/browser_context.h:58: typedef
base::Callback<void(StoragePartition*)> StoragePartitionCallback;
Move here to follow style guide, and remove comments because this is apparently
used in the next function, and description about arguments looks out of date.

[jam, 2013-07-12 16:06:35]

lgtm with comments

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/18647002/diff/16001/content/public/browser/br...
content/public/browser/browser_context.h:138: virtual void
RequestMIDISysExPermission(
On 2013/07/12 12:45:49, Takashi Toyoshima (chromium) wrote:
> SysEx is a abbreviation of system exclusive messages, but it is used in Web
MIDI
> API as a parameter name. So, I think using SysEx here is straight-forward way
> and consistent with the spec though style guide say don't use well-known
> abbreviations. What do you think about this?
> FYI, we already use "SysEx" and "sysex" in some parts of Web MIDI related
> codebases.

ok

https://codereview.chromium.org/18647002/diff/26001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/18647002/diff/26001/android_webview/browser/a...
android_webview/browser/aw_browser_context.cc:188: // Always reject requests for
testing.
? This is used for android_webview. why do you say testing?

https://codereview.chromium.org/18647002/diff/26001/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/18647002/diff/26001/content/public/browser/br...
content/public/browser/browser_context.h:129: // Requests a permission to use
system exclusive messages in MIDI events.
nit: a blank line above this would aid readability

https://codereview.chromium.org/18647002/diff/26001/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/18647002/diff/26001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:179: callback.Run(false);
this means that someone running content_shell won't see this feature. in
general, we try to always enable stuff in content_shell and ignore asking the
user for permission, so this should be callback.Run(true). if for layout tests
you want different behavior, you can set that programmatically.

[Takashi Toyoshima, 2013-07-12 16:37:30]

Thanks!
I'm going to submit this change.

https://codereview.chromium.org/18647002/diff/26001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/18647002/diff/26001/android_webview/browser/a...
android_webview/browser/aw_browser_context.cc:188: // Always reject requests for
testing.
Oops.
Sorry for my poor copy & paste.
Fixed.

https://codereview.chromium.org/18647002/diff/26001/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/18647002/diff/26001/content/public/browser/br...
content/public/browser/browser_context.h:129: // Requests a permission to use
system exclusive messages in MIDI events.
On 2013/07/12 16:06:35, jam wrote:
> nit: a blank line above this would aid readability

Done.

https://codereview.chromium.org/18647002/diff/26001/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/18647002/diff/26001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:179: callback.Run(false);
Currently layout test assumes content_shell always reject SysEx permission
request to verify such a request causes SecurityError. In the future, we'll make
this can be set programmatically later.

[commit-bot: I haz the power, 2013-07-12 16:38:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/18647002/36001

[Takashi Toyoshima, 2013-07-12 16:42:33]

Add OWNERS of API users to TBR line.

[commit-bot: I haz the power, 2013-07-12 16:44:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/18647002/36001

[commit-bot: I haz the power, 2013-07-12 19:19:13]

Change committed as 211437

[jam, 2013-07-17 00:00:13]

https://codereview.chromium.org/18647002/diff/26001/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/18647002/diff/26001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:179: callback.Run(false);
On 2013/07/12 16:37:30, Takashi Toyoshima (chromium) wrote:
> Currently layout test assumes content_shell always reject SysEx permission
> request to verify such a request causes SecurityError. In the future, we'll
make
> this can be set programmatically later.

content_shell is used as both a small browser to try out content & blink, and
also for running layout tests. so you shouldn't disable stuff by default unless
it's running in layout test mode, i.e. change this to

callback.Run(!CommandLine::ForCurrentProcess()->HasSwitch(switches::kDumpRenderTree));