Web MIDI: introduce WebMIDIClient API to request a permission

Web MIDI: introduce WebMIDIClient API to request a permission

Add WebMIDIClient API to request a permission for system exclusive messages.
Also make |access| obsolete in WebMIDIAccessor to delegate it to the Client API.

TEST=none
BUG=163795
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153989

[Takashi Toyoshima, 2013-07-01 13:19:24]

Hi Chris,
This is a blink side change to pass security origin.

I'll land this change once following chromium side change is granted and
submitted.
https://codereview.chromium.org/18309004/

[abarth-chromium, 2013-07-01 17:42:20]

https://codereview.chromium.org/18325007/diff/1/public/platform/WebMIDIAccess...
File public/platform/WebMIDIAccessor.h (right):

https://codereview.chromium.org/18325007/diff/1/public/platform/WebMIDIAccess...
public/platform/WebMIDIAccessor.h:44: virtual void requestAccess(const
WebString& origin, bool access) { }
What do you do with the origin on the Chromium side?  Should we pass a
WebSecurityOrigin instead?  I realize that will cause a layering violation. 
Perhaps |requestAccess| should be part of the client API rather than the
platform API.  Most of the permission requests get routed through the client
rather than the platform.

[Takashi Toyoshima, 2013-07-01 18:57:11]

OK, I try to do this by a client API.

FYI, I'll use it for
chrome/browser/content_settings/tab_specific_content_settings.cc because content
settings manage various permissions by per-origin based.
So, basically, I don't need security origin class features, but need just a URL
string, or GURL object, here.

[abarth-chromium, 2013-07-01 19:19:25]

Yeah, if this is going to content settings, you should make it part of
WebPermissionClient.  That's where all the other content settings bits are
plumbed.

[Takashi Toyoshima, 2013-07-09 11:31:57]

Sorry for late response. I'm visiting another office there persons working on
content settings.

I checked WebPermissionClient. It looks like each permission is enabled or
disabled by default and the default value can be decided almost statically when
the page is loaded.
In my case, default action is to ask users by prompting infobar or something
when the API is called with a special option. So, it doesn't match with my case.
This behavior is very similar to geolocation and WebRTC's devices 
Since they are also members of content settings and don't rely on
WebPermissionClient, I'll follow how these APIs does.

[Takashi Toyoshima, 2013-07-09 11:54:59]

Sorry, just a confirmation.
Does the client API mean WebKit/public/[web|platform]/*Client classes,
And does the platform API mean other classes in these directories, right?

We already have a class WebMIDIAccessorClient. So changing origin parameter of
String to one of WebSecurityOrigin works as others do.

[Takashi Toyoshima, 2013-07-09 17:18:21]

OK, I added a client API to ask security origin instead of passing it in a
request of platform API.

Adam, can you take a look this again?
It isn't a part of WebPermissionClient, but consistent with other APIs using
devices with an user permission.

[abarth-chromium, 2013-07-09 18:01:50]

On 2013/07/09 11:54:59, Takashi Toyoshima (chromium) wrote:
> Does the client API mean WebKit/public/[web|platform]/*Client classes,

Client API means WebKit/public/web/*Client.

> And does the platform API mean other classes in these directories, right?

Platform API means WebKit/public/platform/*

> We already have a class WebMIDIAccessorClient. So changing origin parameter of
> String to one of WebSecurityOrigin works as others do.

public/platform/WebMIDIAccessorClient.h is in public/platform and therefore
cannot see the definition of WebSecurityOrigin.

[abarth-chromium, 2013-07-09 18:04:30]

https://codereview.chromium.org/18325007/diff/11001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/18325007/diff/11001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:37: class WebSecurityOrigin;
This is a bad dependency.  You need to route the permission question "up" to the
public/web directory rather than "down" to the public/platform directory.  The
layering diagram looks like the following:

-- User Interface --
Code in Chromium
public/web
Code in Blink
public/platform
Code in Chromium
-- Operating System Services --

Permission questions are related to the user interface so they need to be routed
"up" the layering diagram.  Interfacing with MIDI devices is related to
operating system services, so they need to be routed "down" the layering
diagram.

[Takashi Toyoshima, 2013-07-10 15:44:39]

Thank you for information.
I removed |access| parameter from Platform API requestAccess, and added the
WebMIDIClient for permission requests as a Client API.
I still use requestAccess for device enumeration and accessing.

Can you take another look?

[abarth-chromium, 2013-07-10 19:43:35]

https://codereview.chromium.org/18325007/diff/27001/Source/modules/webmidi/MI...
File Source/modules/webmidi/MIDIController.cpp (right):

https://codereview.chromium.org/18325007/diff/27001/Source/modules/webmidi/MI...
Source/modules/webmidi/MIDIController.cpp:59: 
you've got an extra blank line

[abarth-chromium, 2013-07-10 19:45:39]

https://codereview.chromium.org/18325007/diff/27001/public/web/WebMIDIPermiss...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/18325007/diff/27001/public/web/WebMIDIPermiss...
public/web/WebMIDIPermissionRequest.h:64: WebCore::MIDIAccess* m_private;
MIDIAccess is RefCounted.  Should this be a WebPrivatePtr?

[abarth-chromium, 2013-07-10 19:47:18]

LGTM module the lifetime issue above.

https://codereview.chromium.org/18325007/diff/27001/Source/web/MIDIClientImpl.h
File Source/web/MIDIClientImpl.h (right):

https://codereview.chromium.org/18325007/diff/27001/Source/web/MIDIClientImpl...
Source/web/MIDIClientImpl.h:48: MIDIClientImpl(WebViewImpl*);
Please mark one-argument constructors explicit

[abarth-chromium, 2013-07-10 19:47:28]

s/module/modulo/

[Chris Rogers, 2013-07-10 23:05:48]

lgtm with naming nits

https://codereview.chromium.org/18325007/diff/27001/Source/core/platform/midi...
File Source/core/platform/midi/MIDIAccessor.h (right):

https://codereview.chromium.org/18325007/diff/27001/Source/core/platform/midi...
Source/core/platform/midi/MIDIAccessor.h:49: void requestAccess();
See other comments about name change -- maybe should be "startSession()" or
"getAccess()" or "initializeAccess()"

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessor.h (right):

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessor.h:42: virtual void requestAccess() { }
Because access is now being handled in another place, it seems like we need a
better name for this API -- maybe something like "startSession()" or
"getAccess()" or "initializeAccess()" -- anything which doesn't have the word
"request" seems better.

A TODO seems OK for now.

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:43: virtual void didAllowAccess() = 0;
Now that permissions is being handled in another place, didAllowAccess() should
probably be re-named to something like "didAddPorts()" or "didStartSession()" or
something which doesn't have the word "allow" in it.

Seems ok to add this as a TODO for now.

[Takashi Toyoshima, 2013-07-10 23:14:55]

Upload Patch Set 5.
I'll address Chris's comment in the next set.

https://codereview.chromium.org/18325007/diff/27001/Source/modules/webmidi/MI...
File Source/modules/webmidi/MIDIController.cpp (right):

https://codereview.chromium.org/18325007/diff/27001/Source/modules/webmidi/MI...
Source/modules/webmidi/MIDIController.cpp:59: 
On 2013/07/10 19:43:36, abarth wrote:
> you've got an extra blank line

Done.

https://codereview.chromium.org/18325007/diff/27001/Source/web/MIDIClientImpl.h
File Source/web/MIDIClientImpl.h (right):

https://codereview.chromium.org/18325007/diff/27001/Source/web/MIDIClientImpl...
Source/web/MIDIClientImpl.h:48: MIDIClientImpl(WebViewImpl*);
On 2013/07/10 19:47:18, abarth wrote:
> Please mark one-argument constructors explicit

Done.

https://codereview.chromium.org/18325007/diff/27001/public/web/WebMIDIPermiss...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/18325007/diff/27001/public/web/WebMIDIPermiss...
public/web/WebMIDIPermissionRequest.h:64: WebCore::MIDIAccess* m_private;
On 2013/07/10 19:45:39, abarth wrote:
> MIDIAccess is RefCounted.  Should this be a WebPrivatePtr?

Done.

[Takashi Toyoshima, 2013-07-10 23:52:38]

https://codereview.chromium.org/18325007/diff/27001/Source/core/platform/midi...
File Source/core/platform/midi/MIDIAccessor.h (right):

https://codereview.chromium.org/18325007/diff/27001/Source/core/platform/midi...
Source/core/platform/midi/MIDIAccessor.h:49: void requestAccess();
Renamed to startSession.

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessor.h (right):

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessor.h:42: virtual void requestAccess() { }
Renamed to startSession.

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/18325007/diff/27001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:43: virtual void didAllowAccess() = 0;
Renamed to didStartSession.

[commit-bot: I haz the power, 2013-07-11 01:26:46]

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

[commit-bot: I haz the power, 2013-07-11 02:29:52]

Retried try job too often on linux_layout_rel for step(s) webkit_python_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-07-11 05:59:41]

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

[commit-bot: I haz the power, 2013-07-11 08:28:21]

Change committed as 153989