Convert DeviceManagerDelegate to PermissionProvider mojo interface.

device/devices_app/usb/device_manager_impl.cc

Index: device/devices_app/usb/device_manager_impl.cc
diff --git a/device/devices_app/usb/device_manager_impl.cc b/device/devices_app/usb/device_manager_impl.cc
index f438460ddd828dae87a4c28ca2320c41c9e8660a..62d860b6bad809134e6f51e06ea8431b2eef9c2a 100644
--- a/device/devices_app/usb/device_manager_impl.cc
+++ b/device/devices_app/usb/device_manager_impl.cc
@@ -14,7 +14,6 @@
 #include "base/thread_task_runner_handle.h"
 #include "device/core/device_client.h"
 #include "device/devices_app/usb/device_impl.h"
-#include "device/devices_app/usb/public/cpp/device_manager_delegate.h"
 #include "device/devices_app/usb/public/interfaces/device.mojom.h"
 #include "device/devices_app/usb/type_converters.h"
 #include "device/usb/usb_device.h"
@@ -108,6 +107,23 @@ void OpenDeviceOnServiceThread(
                           callback_task_runner));
 }
 
+void FilterDeviceListAndThen(
+    const DeviceManagerImpl::GetDevicesCallback& callback,
+    mojo::Array<DeviceInfoPtr> devices,
+    mojo::Array<mojo::String> allowed_guids) {
+  std::set<std::string> allowed_guid_set;
+  for (size_t i = 0; i < allowed_guids.size(); ++i)
+    allowed_guid_set.insert(allowed_guids[i]);
+
+  mojo::Array<DeviceInfoPtr> allowed_devices(0);
+  for (size_t i = 0; i < devices.size(); ++i) {
+    if (ContainsKey(allowed_guid_set, devices[i]->guid))
+      allowed_devices.push_back(devices[i].Pass());
+  }
+
+  callback.Run(allowed_devices.Pass());
+}
+
 }  // namespace
 
 class DeviceManagerImpl::ServiceThreadHelper
@@ -163,13 +179,12 @@ class DeviceManagerImpl::ServiceThreadHelper
 
 DeviceManagerImpl::DeviceManagerImpl(
     mojo::InterfaceRequest<DeviceManager> request,
-    scoped_ptr<DeviceManagerDelegate> delegate,
+    PermissionProviderPtr permission_provider,
     scoped_refptr<base::SequencedTaskRunner> service_task_runner)
     : binding_(this, request.Pass()),
-      delegate_(delegate.Pass()),
+      permission_provider_(permission_provider.Pass()),
       service_task_runner_(service_task_runner),
-      weak_factory_(this) {
-}
+      weak_factory_(this) {}
 
 DeviceManagerImpl::~DeviceManagerImpl() {

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

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.

[Reilly Grant (use Gerrit), 2015/09/02 23:58:18]

Done.

   if (helper_) {
@@ -197,19 +212,18 @@ void DeviceManagerImpl::GetDevices(EnumerationOptionsPtr options,
 
 void DeviceManagerImpl::GetDeviceChanges(
     const GetDeviceChangesCallback& callback) {
+  device_change_callbacks_.push(callback);
   if (helper_) {
-    device_change_callbacks_.push(callback);
     MaybeRunDeviceChangesCallback();
   } else {
     scoped_ptr<ServiceThreadHelper> helper(new ServiceThreadHelper(
         weak_factory_.GetWeakPtr(), base::ThreadTaskRunnerHandle::Get()));
     helper_ = helper.get();
-    auto get_devices_callback =
-        base::Bind(&DeviceManagerImpl::OnGetInitialDevices,
-                   weak_factory_.GetWeakPtr(), callback);
     service_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&ServiceThreadHelper::Start,
-                              base::Passed(&helper), get_devices_callback));
+        FROM_HERE,
+        base::Bind(&ServiceThreadHelper::Start, base::Passed(&helper),
+                   base::Bind(&DeviceManagerImpl::OnGetInitialDevices,
+                              weak_factory_.GetWeakPtr())));
   }
 }
 
@@ -225,25 +239,21 @@ void DeviceManagerImpl::OpenDevice(
 
 void DeviceManagerImpl::OnGetDevices(const GetDevicesCallback& callback,
                                      mojo::Array<DeviceInfoPtr> devices) {
-  mojo::Array<DeviceInfoPtr> allowed_devices(0);
+  mojo::Array<mojo::String> requested_guids(devices.size());
   for (size_t i = 0; i < devices.size(); ++i) {
-    if (delegate_->IsDeviceAllowed(*devices[i]))
-      allowed_devices.push_back(devices[i].Pass());
+    requested_guids[i] = devices[i]->guid;
   }
-  callback.Run(allowed_devices.Pass());
+  permission_provider_->HasDevicePermission(
+      requested_guids.Pass(),
+      base::Bind(&FilterDeviceListAndThen, callback, base::Passed(&devices)));
 }
 
 void DeviceManagerImpl::OnGetInitialDevices(
-    const GetDeviceChangesCallback& callback,
     mojo::Array<DeviceInfoPtr> devices) {
-  DeviceChangeNotificationPtr notification = DeviceChangeNotification::New();
-  notification->devices_added = mojo::Array<DeviceInfoPtr>::New(0);
-  notification->devices_removed = mojo::Array<mojo::String>::New(0);
-  for (size_t i = 0; i < devices.size(); ++i) {
-    if (delegate_->IsDeviceAllowed(*devices[i]))
-      notification->devices_added.push_back(devices[i].Pass());
-  }
-  callback.Run(notification.Pass());
+  DCHECK_EQ(0u, devices_added_.size());
+  DCHECK(devices_removed_.empty());
+  devices_added_.Swap(&devices);
+  MaybeRunDeviceChangesCallback();
 }
 
 void DeviceManagerImpl::OnDeviceAdded(DeviceInfoPtr device) {
@@ -262,24 +272,70 @@ void DeviceManagerImpl::OnDeviceRemoved(std::string device_guid) {
       devices_added.push_back(devices_added_[i].Pass());
   }
   devices_added.Swap(&devices_added_);
-  if (!found)
+  if (!found) {

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

nit: eh, maybe no {}.

[Reilly Grant (use Gerrit), 2015/09/02 23:58:19]

Done.

     devices_removed_.insert(device_guid);
+  }
   MaybeRunDeviceChangesCallback();
 }
 
 void DeviceManagerImpl::MaybeRunDeviceChangesCallback() {
   if (!device_change_callbacks_.empty()) {
+    mojo::Array<DeviceInfoPtr> devices_added;
+    devices_added.Swap(&devices_added_);
+    std::set<std::string> devices_removed;
+    devices_removed.swap(devices_removed_);
+
+    mojo::Array<mojo::String> requested_guids(devices_added.size() +
+                                              devices_removed.size());
+    size_t i;

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

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 {}?

[Reilly Grant (use Gerrit), 2015/09/02 23:58:18]

Done.

+    for (i = 0; i < devices_added.size(); ++i)
+      requested_guids[i] = devices_added[i]->guid;
+    for (const std::string& guid : devices_removed)
+      requested_guids[i++] = guid;
+
+    permission_request_pending_ = true;

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

this doesn't appear to have any logical effect; i'm guessing that's
unintentional 

[Reilly Grant (use Gerrit), 2015/09/02 23:58:19]

Done.

+    permission_provider_->HasDevicePermission(
+        requested_guids.Pass(),
+        base::Bind(&DeviceManagerImpl::OnPermissionCheckComplete,
+                   weak_factory_.GetWeakPtr(), base::Passed(&devices_added),

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

nit: No need to use a WeakPtr here. permission_provider_ can't outlive |this|
and so neither can this callback.

[Reilly Grant (use Gerrit), 2015/09/02 23:58:19]

Done.

+                   devices_removed));
+  }
+}
+
+void DeviceManagerImpl::OnPermissionCheckComplete(
+    mojo::Array<DeviceInfoPtr> devices_added,
+    const std::set<std::string>& devices_removed,
+    mojo::Array<mojo::String> allowed_guids) {
+  permission_request_pending_ = false;
+
+  if (allowed_guids.size() > 0 || first_notification_) {
+    std::set<std::string> allowed_guid_set;
+    for (size_t i = 0; i < allowed_guids.size(); ++i)
+      allowed_guid_set.insert(allowed_guids[i]);
+
     DeviceChangeNotificationPtr notification = DeviceChangeNotification::New();
-    notification->devices_added.Swap(&devices_added_);
+    notification->devices_added.resize(0);
+    for (size_t i = 0; i < devices_added.size(); ++i) {
+      if (ContainsKey(allowed_guid_set, devices_added[i]->guid))
+        notification->devices_added.push_back(devices_added[i].Pass());
+    }
+
     notification->devices_removed.resize(0);
-    for (const std::string& device : devices_removed_)
-      notification->devices_removed.push_back(device);
-    devices_removed_.clear();
+    for (const std::string& guid : devices_removed) {
+      if (ContainsKey(allowed_guid_set, guid))
+        notification->devices_removed.push_back(guid);

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

Should we also omit any guid that would otherwise be present in both
devices_added and devices_removed?

[Reilly Grant (use Gerrit), 2015/09/02 23:58:19]

We know that devices_added and devices_removed are disjoint.

+    }
 
+    DCHECK(!device_change_callbacks_.empty());
     const GetDeviceChangesCallback& callback = device_change_callbacks_.front();
+    first_notification_ = false;
     callback.Run(notification.Pass());
     device_change_callbacks_.pop();
   }
+
+  if (devices_added_.size() > 0 || !devices_removed_.empty()) {
+    MaybeRunDeviceChangesCallback();
+  }
 }
 
 }  // namespace usb