Convert DeviceManagerDelegate to PermissionProvider mojo interface.

Convert DeviceManagerDelegate to PermissionProvider mojo interface.

This changes the device::usb::DeviceManager service so that instead of
expecting a C++ implementation of the DeviceManagerDelegate interface it
depends on the client connecting to the devices app to provide an
implementation of the PermissionProvider mojo service interface. This
will allow the permissions checker to be injected by the FrameMojoShell
when a renderer requests a connection to the devices app.

BUG=492204
Committed: https://crrev.com/17b8a58c99537bab4d8944ec2f96023dac0a15e7
Cr-Commit-Position: refs/heads/master@{#347998}

[Reilly Grant (use Gerrit), 2015-09-02 01:13:55]

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

[Reilly Grant (use Gerrit), 2015-09-02 01:13:56]

Please take a look.

[Reilly Grant (use Gerrit), 2015-09-02 01:26:21]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2015-09-02 22:42:33]

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.cc (right):

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:189:
DeviceManagerImpl::~DeviceManagerImpl() {
nit: I totally missed this before, but could you please flush the
device_change_callbacks_ queue here with empty notifications? This is necessary
because dropping a mojo response callback without running it will result in a
DCHECK.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:275: if (!found) {
nit: eh, maybe no {}.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:290: size_t i;
nit: not a huge fan of a variable like |i| hanging around at function scope.
maybe rename to something more verbose, or enclose this little block in {}?

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:296: permission_request_pending_ =
true;
this doesn't appear to have any logical effect; i'm guessing that's
unintentional

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:300: weak_factory_.GetWeakPtr(),
base::Passed(&devices_added),
nit: No need to use a WeakPtr here. permission_provider_ can't outlive |this|
and so neither can this callback.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:326:
notification->devices_removed.push_back(guid);
Should we also omit any guid that would otherwise be present in both
devices_added and devices_removed?

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.h (right):

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.h:72:
mojo::StrongBinding<DeviceManager> binding_;
as discussed offline, this can become a mojo::Binding and we can leave
DeviceManagerImpl in control of its own lifetime

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/public/interfaces/permission_provider.mojom (right):

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/public/interfaces/permission_provider.mojom:10: //
filters a set of |requested_guids| down to the set of |allowed_guids| that
nit: Filters

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/public/interfaces/permission_provider.mojom:12:
HasDevicePermission(array<string> requested_guids) => (array<string>
allowed_guids);
nit: > 80 chars Preferred wrapping for mojom would be:

  HasDevicepermission(array<string> requested_guides)
      => (array<string> allowed_guids);

[Ken Rockot(use gerrit already), 2015-09-02 23:07:18]

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.h (right):

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.h:72:
mojo::StrongBinding<DeviceManager> binding_;
On 2015/09/02 22:42:33, Ken Rockot wrote:
> as discussed offline, this can become a mojo::Binding and we can leave
> DeviceManagerImpl in control of its own lifetime

to preserve what was also just discussed offline, you want this to occur after
device_change_callbacks_ so the Binding is destroyed before pending response
callbacks.

Then you can ignore my comment about flushing callbacks in the dtor

[Reilly Grant (use Gerrit), 2015-09-02 23:58:20]

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.cc (right):

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:189:
DeviceManagerImpl::~DeviceManagerImpl() {
On 2015/09/02 22:42:33, Ken Rockot wrote:
> nit: I totally missed this before, but could you please flush the
> device_change_callbacks_ queue here with empty notifications? This is
necessary
> because dropping a mojo response callback without running it will result in a
> DCHECK.

Done.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:275: if (!found) {
On 2015/09/02 22:42:33, Ken Rockot wrote:
> nit: eh, maybe no {}.

Done.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:290: size_t i;
On 2015/09/02 22:42:33, Ken Rockot wrote:
> nit: not a huge fan of a variable like |i| hanging around at function scope.
> maybe rename to something more verbose, or enclose this little block in {}?

Done.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:296: permission_request_pending_ =
true;
On 2015/09/02 22:42:33, Ken Rockot wrote:
> this doesn't appear to have any logical effect; i'm guessing that's
> unintentional 

Done.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:300: weak_factory_.GetWeakPtr(),
base::Passed(&devices_added),
On 2015/09/02 22:42:33, Ken Rockot wrote:
> nit: No need to use a WeakPtr here. permission_provider_ can't outlive |this|
> and so neither can this callback.

Done.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:326:
notification->devices_removed.push_back(guid);
On 2015/09/02 22:42:33, Ken Rockot wrote:
> Should we also omit any guid that would otherwise be present in both
> devices_added and devices_removed?

We know that devices_added and devices_removed are disjoint.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.h (right):

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.h:72:
mojo::StrongBinding<DeviceManager> binding_;
On 2015/09/02 23:07:18, Ken Rockot wrote:
> On 2015/09/02 22:42:33, Ken Rockot wrote:
> > as discussed offline, this can become a mojo::Binding and we can leave
> > DeviceManagerImpl in control of its own lifetime
> 
> to preserve what was also just discussed offline, you want this to occur after
> device_change_callbacks_ so the Binding is destroyed before pending response
> callbacks.
> 
> Then you can ignore my comment about flushing callbacks in the dtor

Done.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/public/interfaces/permission_provider.mojom (right):

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/public/interfaces/permission_provider.mojom:10: //
filters a set of |requested_guids| down to the set of |allowed_guids| that
On 2015/09/02 22:42:33, Ken Rockot wrote:
> nit: Filters

Done.

https://codereview.chromium.org/1316203006/diff/20001/device/devices_app/usb/...
device/devices_app/usb/public/interfaces/permission_provider.mojom:12:
HasDevicePermission(array<string> requested_guids) => (array<string>
allowed_guids);
On 2015/09/02 22:42:33, Ken Rockot wrote:
> nit: > 80 chars Preferred wrapping for mojom would be:
> 
>   HasDevicepermission(array<string> requested_guides)
>       => (array<string> allowed_guids);

Done.

[Ken Rockot(use gerrit already), 2015-09-03 00:06:11]

sweet, lgtm

https://codereview.chromium.org/1316203006/diff/40001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.cc (right):

https://codereview.chromium.org/1316203006/diff/40001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:234: for (size_t i = 0; i <
devices.size(); ++i) {
extra nitty nit: no {}

[Reilly Grant (use Gerrit), 2015-09-03 00:09:28]

https://codereview.chromium.org/1316203006/diff/40001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.cc (right):

https://codereview.chromium.org/1316203006/diff/40001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:234: for (size_t i = 0; i <
devices.size(); ++i) {
On 2015/09/03 00:06:11, Ken Rockot wrote:
> extra nitty nit: no {}

Done.

[Reilly Grant (use Gerrit), 2015-09-03 00:11:19]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-09-03 00:11:19]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1316203006/#ps60001
(title: "Fewer braces.")

[commit-bot: I haz the power, 2015-09-03 00:31:36]

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

[commit-bot: I haz the power, 2015-09-03 01:17:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 01:17:09]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Reilly Grant (use Gerrit), 2015-09-03 17:24:59]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2015-09-03 17:25:28]

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

[commit-bot: I haz the power, 2015-09-03 18:05:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 18:05:31]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Reilly Grant (use Gerrit), 2015-09-04 01:18:24]

Please take another look. I added a permission check for opening devices while I
was waiting for the patch to land.

[Reilly Grant (use Gerrit), 2015-09-04 01:18:25]

Please take another look. I added a permission check for opening devices while I
was waiting for the patch to land.

[Reilly Grant (use Gerrit), 2015-09-05 00:03:25]

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

[Reilly Grant (use Gerrit), 2015-09-05 00:04:22]

Combined this with my WebUSBPermissionProvider change to try to figure out why
one of the telemetry_perf_tests was failing.

thestig@, please review the addition of a new subdirectory in chrome/browser.

[Lei Zhang, 2015-09-05 00:24:20]

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.cc (right):

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:14:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
DCHECK_CURRENTLY_ON()

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:34:
ignore_result(render_frame_host_);
What's this suppose to do?

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.h (right):

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.h:17: namespace chrome {
no namespace chrome

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.h:21: static void Create(
Why have Create() and a public ctor?

[Reilly Grant (use Gerrit), 2015-09-05 00:56:25]

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.cc (right):

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:14:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2015/09/05 00:24:20, Lei Zhang wrote:
> DCHECK_CURRENTLY_ON()

Done.

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:34:
ignore_result(render_frame_host_);
On 2015/09/05 00:24:20, Lei Zhang wrote:
> What's this suppose to do?

It silences a warning that the private member is unused.

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.h (right):

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.h:17: namespace chrome {
On 2015/09/05 00:24:20, Lei Zhang wrote:
> no namespace chrome

Done.

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.h:21: static void Create(
On 2015/09/05 00:24:20, Lei Zhang wrote:
> Why have Create() and a public ctor?

The AddService method called in chrome_content_browser_client.cc requires a
closure and closures don't work over constructors.

[Lei Zhang, 2015-09-05 01:18:15]

lgtm

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.cc (right):

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:34:
ignore_result(render_frame_host_);
On 2015/09/05 00:56:25, Reilly Grant wrote:
> On 2015/09/05 00:24:20, Lei Zhang wrote:
> > What's this suppose to do?
> 
> It silences a warning that the private member is unused.

Add a comment?

// Make a dummy reference to the variable for now to make the compiler happy.

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.h (right):

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.h:21: static void Create(
On 2015/09/05 00:56:25, Reilly Grant wrote:
> On 2015/09/05 00:24:20, Lei Zhang wrote:
> > Why have Create() and a public ctor?
> 
> The AddService method called in chrome_content_browser_client.cc requires a
> closure and closures don't work over constructors.

Can you make the ctor private with a comment to use Create(), so all creation
happens in the same way via Create() ?

https://codereview.chromium.org/1316203006/diff/120001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.cc (right):

https://codereview.chromium.org/1316203006/diff/120001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:5: #include
"chrome/browser/usb/web_usb_permission_provider.h"
nit: blank line after.

https://codereview.chromium.org/1316203006/diff/120001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:12:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
Does WebUSBPermissionProvider live on the UI thread? May be good to add a
comment in the header and sprinkle more DCHECK_CURRENTLY_ON() calls in the impl
here.

[Reilly Grant (use Gerrit), 2015-09-05 01:56:47]

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.cc (right):

https://codereview.chromium.org/1316203006/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:34:
ignore_result(render_frame_host_);
On 2015/09/05 01:18:14, Lei Zhang wrote:
> On 2015/09/05 00:56:25, Reilly Grant wrote:
> > On 2015/09/05 00:24:20, Lei Zhang wrote:
> > > What's this suppose to do?
> > 
> > It silences a warning that the private member is unused.
> 
> Add a comment?
> 
> // Make a dummy reference to the variable for now to make the compiler happy.

Done.

https://codereview.chromium.org/1316203006/diff/120001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_provider.cc (right):

https://codereview.chromium.org/1316203006/diff/120001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:5: #include
"chrome/browser/usb/web_usb_permission_provider.h"
On 2015/09/05 01:18:14, Lei Zhang wrote:
> nit: blank line after.

Done.

https://codereview.chromium.org/1316203006/diff/120001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_provider.cc:12:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/09/05 01:18:15, Lei Zhang wrote:
> Does WebUSBPermissionProvider live on the UI thread? May be good to add a
> comment in the header and sprinkle more DCHECK_CURRENTLY_ON() calls in the
impl
> here.

Done.

[Reilly Grant (use Gerrit), 2015-09-05 02:09:13]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-09-05 02:09:14]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1316203006/#ps140001
(title: "Add comment and move ctor.")

[commit-bot: I haz the power, 2015-09-05 02:09:31]

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

[commit-bot: I haz the power, 2015-09-05 03:13:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-05 03:13:12]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Reilly Grant (use Gerrit), 2015-09-09 19:35:16]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2015-09-09 19:35:43]

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

[commit-bot: I haz the power, 2015-09-09 20:59:05]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-09-09 20:59:53]

Patchset 7 (id:??) landed as
https://crrev.com/17b8a58c99537bab4d8944ec2f96023dac0a15e7
Cr-Commit-Position: refs/heads/master@{#347998}

[commit-bot: I haz the power, 2015-09-23 12:03:06]

Patchset 7 (id:??) landed as
https://crrev.com/17b8a58c99537bab4d8944ec2f96023dac0a15e7
Cr-Commit-Position: refs/heads/master@{#347998}