Reland: Add webusb permission bubble mojom

Reland: Add webusb permission bubble mojom

This patch added webusb permission bubble mojom.
It has function GetPermission to get a device after
user has granted permission to it. And this function
is used by class WebUSBClientImpl.

BUG=492204
Committed: https://crrev.com/9e3fbccf392bc87e66424051e94978d7802bba0e
Cr-Commit-Position: refs/heads/master@{#354442}

Committed: https://crrev.com/09f57e5938da96e39cc0739d78d52019fb686a82
Cr-Commit-Position: refs/heads/master@{#354597}

[juncai, 2015-10-14 21:03:03]

juncai@chromium.org changed reviewers:
+ nick@chromium.org, reillyg@chromium.org

[juncai, 2015-10-14 21:03:03]

nick@chromium.org: Please review changes in 
content/child/mojo/type_converters.h
content/content_child.gypi
content/content_renderer.gypi
content/renderer/BUILD.gn

reillyg@chromium.org: Please review changes in 
the remaining files.

[Reilly Grant (use Gerrit), 2015-10-14 21:08:48]

lgtm with nits

https://codereview.chromium.org/1404213004/diff/1/components/webusb/public/in...
File components/webusb/public/interfaces/webusb_permission_bubble.mojom (right):

https://codereview.chromium.org/1404213004/diff/1/components/webusb/public/in...
components/webusb/public/interfaces/webusb_permission_bubble.mojom:11:
GetPermission(array<device.usb.DeviceFilter> device_filters)
Please document what this interface is supposed to do.

https://codereview.chromium.org/1404213004/diff/1/content/renderer/usb/web_us...
File content/renderer/usb/web_usb_client_impl.cc (right):

https://codereview.chromium.org/1404213004/diff/1/content/renderer/usb/web_us...
content/renderer/usb/web_usb_client_impl.cc:115: Convert(options.filters);
You should be able to use mojo::Array::From() here.

[juncai, 2015-10-14 22:54:37]

https://codereview.chromium.org/1404213004/diff/1/components/webusb/public/in...
File components/webusb/public/interfaces/webusb_permission_bubble.mojom (right):

https://codereview.chromium.org/1404213004/diff/1/components/webusb/public/in...
components/webusb/public/interfaces/webusb_permission_bubble.mojom:11:
GetPermission(array<device.usb.DeviceFilter> device_filters)
On 2015/10/14 21:08:47, Reilly Grant wrote:
> Please document what this interface is supposed to do.

Done.

https://codereview.chromium.org/1404213004/diff/1/content/renderer/usb/web_us...
File content/renderer/usb/web_usb_client_impl.cc (right):

https://codereview.chromium.org/1404213004/diff/1/content/renderer/usb/web_us...
content/renderer/usb/web_usb_client_impl.cc:115: Convert(options.filters);
On 2015/10/14 21:08:47, Reilly Grant wrote:
> You should be able to use mojo::Array::From() here.

Done.

[ncarter (slow), 2015-10-15 20:41:15]

https://codereview.chromium.org/1404213004/diff/20001/content/renderer/usb/we...
File content/renderer/usb/web_usb_client_impl.cc (right):

https://codereview.chromium.org/1404213004/diff/20001/content/renderer/usb/we...
content/renderer/usb/web_usb_client_impl.cc:12: #include
"content/child/mojo/type_converters.h"
I'm not very familiar with mojo, and don't know much about the conventions. But
this seems to be the only place this new file is #included.

Why not add this new code to the existing file,
content/renderer/usb/type_converters.h?

[Reilly Grant (use Gerrit), 2015-10-15 20:48:24]

https://codereview.chromium.org/1404213004/diff/20001/content/renderer/usb/we...
File content/renderer/usb/web_usb_client_impl.cc (right):

https://codereview.chromium.org/1404213004/diff/20001/content/renderer/usb/we...
content/renderer/usb/web_usb_client_impl.cc:12: #include
"content/child/mojo/type_converters.h"
On 2015/10/15 20:41:15, ncarter wrote:
> I'm not very familiar with mojo, and don't know much about the conventions.
But
> this seems to be the only place this new file is #included.
> 
> Why not add this new code to the existing file,
> content/renderer/usb/type_converters.h?

We could, but converting between mojo::Array and WebVector is something I
imagined more than just this code would want to do some day so rockot@ suggested
content/child/mojo was the best place to put it.

[ncarter (slow), 2015-10-15 21:01:29]

https://codereview.chromium.org/1404213004/diff/20001/content/renderer/usb/we...
File content/renderer/usb/web_usb_client_impl.cc (right):

https://codereview.chromium.org/1404213004/diff/20001/content/renderer/usb/we...
content/renderer/usb/web_usb_client_impl.cc:12: #include
"content/child/mojo/type_converters.h"
On 2015/10/15 20:48:24, Reilly Grant wrote:
> On 2015/10/15 20:41:15, ncarter wrote:
> > I'm not very familiar with mojo, and don't know much about the conventions.
> But
> > this seems to be the only place this new file is #included.
> > 
> > Why not add this new code to the existing file,
> > content/renderer/usb/type_converters.h?
> 
> We could, but converting between mojo::Array and WebVector is something I
> imagined more than just this code would want to do some day so rockot@
suggested
> content/child/mojo was the best place to put it.

I see. content/child does seem like a good choice for this, then: There are uses
of WebVector both in content/renderer and content/child, but not in
content/browser.

lgtm from a content standpoint, but please make sure to get an ipc security
review too.

[Reilly Grant (use Gerrit), 2015-10-15 22:23:00]

reillyg@chromium.org changed reviewers:
+ tsepez@chromium.org

[Reilly Grant (use Gerrit), 2015-10-15 22:23:01]

As Nick pointed out we've been adding Mojo interfaces for WebUSB that cross the
renderer/browser boundary without getting an IPC security review so now we beg
for forgiveness. Adding tsepez@ to look at this new mojom. He should also take a
look at //device/devices_app/usb/public/interfaces/*.mojom when he gets a
chance.

[Tom Sepez, 2015-10-15 22:58:18]

On 2015/10/15 22:23:01, Reilly Grant wrote:
> As Nick pointed out we've been adding Mojo interfaces for WebUSB that cross
the
> renderer/browser boundary without getting an IPC security review so now we beg
> for forgiveness. Adding tsepez@ to look at this new mojom. He should also take
a
> look at //device/devices_app/usb/public/interfaces/*.mojom when he gets a
> chance.

LGTM, I'll take a look at the larger context soon.

[juncai, 2015-10-16 03:54:16]

The CQ bit was checked by juncai@chromium.org

[juncai, 2015-10-16 03:54:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, tsepez@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1404213004/#ps40001
(title: "set upstream to origin/master and rebase")

[commit-bot: I haz the power, 2015-10-16 03:54:35]

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

[commit-bot: I haz the power, 2015-10-16 03:59:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-16 04:00:26]

Patchset 3 (id:??) landed as
https://crrev.com/9e3fbccf392bc87e66424051e94978d7802bba0e
Cr-Commit-Position: refs/heads/master@{#354442}

[Mark P, 2015-10-16 04:26:24]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1409023002/ by mpearson@chromium.org.

The reason for reverting is: Causes compile failure on Windows builder:
http://build.chromium.org/p/chromium/builders/Win/builds/36553

c:\b\build\slave\win\build\src\out\release\gen\components\webusb\public\interfaces\webusb_permission_bubble.mojom-internal.h(13)
: fatalerror C1083: Cannot open include file:
'device/devices_app/usb/public/interfaces/device.mojom-internal.h': No such file
or directory

.

[juncai, 2015-10-16 21:05:35]

The CQ bit was checked by juncai@chromium.org

[juncai, 2015-10-16 21:05:35]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
nick@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/1404213004/#ps80001
(title: "updated webusb.gypi dependencies again")

[commit-bot: I haz the power, 2015-10-16 21:07:19]

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

[commit-bot: I haz the power, 2015-10-16 21:15:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-16 21:16:42]

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

[jam, 2016-01-21 17:32:32]

content can only depend on components shared with html_viewer, i.e. per this
comment
https://chromium.googlesource.com/chromium/src/+/master/content/renderer/DEPS#2

DEPS a few layers underneath top level ones in content can't override this.

This code appears to be just using components/webusb as a directory to get
around the fact that content can't include files from chrome. so the mojo
interface is put in components so that content can call it and chrome can
implement it.

with chrome IPCs, we had a strict rule from the beginning of the content/chrome
split that IPCs are not an API boundary between the two. for all these
permission style requests, content could call its embedder through the various
Request* or Allow* methods on ContentBrowserClient or ContentRendererClient
delegate interfaces.

Perhaps we would rethink this as we're switching code to use more mojo in
general, but that's beyond the scope of this cl and we still have to figure it
out. Until then, this code needs to quickly fix things by using the existing way
of doing things for consistency (i.e. go through CBC or CRC interfaces).

[jam, 2016-01-21 18:12:42]

On 2016/01/21 17:32:32, jam wrote:
> content can only depend on components shared with html_viewer, i.e. per this
> comment
>
https://chromium.googlesource.com/chromium/src/+/master/content/renderer/DEPS#2
> 
> DEPS a few layers underneath top level ones in content can't override this.
> 
> This code appears to be just using components/webusb as a directory to get
> around the fact that content can't include files from chrome. so the mojo
> interface is put in components so that content can call it and chrome can
> implement it.
> 
> with chrome IPCs, we had a strict rule from the beginning of the
content/chrome
> split that IPCs are not an API boundary between the two. for all these
> permission style requests, content could call its embedder through the various
> Request* or Allow* methods on ContentBrowserClient or ContentRendererClient
> delegate interfaces.
> 
> Perhaps we would rethink this as we're switching code to use more mojo in
> general, but that's beyond the scope of this cl and we still have to figure it
> out. Until then, this code needs to quickly fix things by using the existing
way
> of doing things for consistency (i.e. go through CBC or CRC interfaces).

ok i thought about this some more, and ben and i chatted about this as well.

where we want to get to is reduced layering and busy work. so it follows that
content/renderer can call chrome through a mojom, and we shouldn't have extra
layers in between. however it should still be clear where this api boundary is,
so this mojom should be in content/public.

webusb_permission_bubble.mojom should then be moved to content/public. Also, it
shouldn't say permission bubble anywhere, since that is a chrome UI concept and
other embedders of content can have different or no UI. so it should just be
something like WebUSBPermissionRequest.

[juncai, 2016-01-21 18:40:44]

On 2016/01/21 18:12:42, jam wrote:
> On 2016/01/21 17:32:32, jam wrote:
> > content can only depend on components shared with html_viewer, i.e. per this
> > comment
> >
>
https://chromium.googlesource.com/chromium/src/+/master/content/renderer/DEPS#2
> > 
> > DEPS a few layers underneath top level ones in content can't override this.
> > 
> > This code appears to be just using components/webusb as a directory to get
> > around the fact that content can't include files from chrome. so the mojo
> > interface is put in components so that content can call it and chrome can
> > implement it.
> > 
> > with chrome IPCs, we had a strict rule from the beginning of the
> content/chrome
> > split that IPCs are not an API boundary between the two. for all these
> > permission style requests, content could call its embedder through the
various
> > Request* or Allow* methods on ContentBrowserClient or ContentRendererClient
> > delegate interfaces.
> > 
> > Perhaps we would rethink this as we're switching code to use more mojo in
> > general, but that's beyond the scope of this cl and we still have to figure
it
> > out. Until then, this code needs to quickly fix things by using the existing
> way
> > of doing things for consistency (i.e. go through CBC or CRC interfaces).
> 
> ok i thought about this some more, and ben and i chatted about this as well.
> 
> where we want to get to is reduced layering and busy work. so it follows that
> content/renderer can call chrome through a mojom, and we shouldn't have extra
> layers in between. however it should still be clear where this api boundary
is,
> so this mojom should be in content/public.
> 
> webusb_permission_bubble.mojom should then be moved to content/public. Also,
it
> shouldn't say permission bubble anywhere, since that is a chrome UI concept
and
> other embedders of content can have different or no UI. so it should just be
> something like WebUSBPermissionRequest.

Thanks a lot! I'll make the code change in a new patch.