VideoCaptureManager: remove ScopedVector and other smaller cleanups

content/browser/renderer_host/media/video_capture_manager.cc

Index: content/browser/renderer_host/media/video_capture_manager.cc
diff --git a/content/browser/renderer_host/media/video_capture_manager.cc b/content/browser/renderer_host/media/video_capture_manager.cc
index e13e1573ccda9a1f71fc329db1415a1dfc71bb65..12f03c4387fca57342cf89022b5aeb5602301954 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -330,8 +330,8 @@ void VideoCaptureManager::Close(int capture_session_id) {
     return;
   }
 
-  DeviceEntry* const existing_device = GetDeviceEntryForMediaStreamDevice(
-      session_it->second);
+  DeviceEntry* const existing_device = GetDeviceEntryByTypeAndId(
+      session_it->second.type, session_it->second.id);
   if (existing_device) {
     // Remove any client that is still using the session. This is safe to call
     // even if there are no clients using the session.
@@ -363,7 +363,11 @@ void VideoCaptureManager::QueueStartDevice(
 
 void VideoCaptureManager::DoStopDevice(DeviceEntry* entry) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(std::find(devices_.begin(), devices_.end(), entry) != devices_.end());
+  DCHECK(
+      std::find_if(devices_.begin(), devices_.end(),
+                   [entry](const std::unique_ptr<DeviceEntry>& device_entry) {
+                     return device_entry.get() == entry;
+                   }) != devices_.end());

[chfremer, 2016/06/30 17:08:52]

Is there any reusable short-hand notation for finding a raw pointer in a
container storing unique_ptr<>? This seems like a fairly common need. If nothing
exists, can we create something and put it into ::base?

[mcasas, 2016/06/30 17:43:18]

Yeah, see my reply below.

 
   // Find the matching start request.
   for (DeviceStartQueue::reverse_iterator request =
@@ -403,13 +407,8 @@ void VideoCaptureManager::HandleQueuedStartRequest() {
     return;
 
   const int serial_id = request->serial_id();
-  DeviceEntries::iterator entry_it = std::find_if(
-      devices_.begin(), devices_.end(),
-      [serial_id] (const DeviceEntry* e) {
-        return e->serial_id == serial_id;
-      });
-  DCHECK(entry_it != devices_.end());
-  DeviceEntry* entry =  (*entry_it);
+  DeviceEntry* entry =  GetDeviceEntryBySerialId(serial_id);
+  DCHECK(entry);
 
 #if defined(OS_MACOSX)
   if (NeedToInitializeCaptureDeviceApi(entry->stream_type)) {
@@ -431,7 +430,7 @@ void VideoCaptureManager::HandleQueuedStartRequest() {
       // since the renderer does not have all the information that might be
       // held in the browser-side VideoCaptureDevice::Name structure.
       const media::VideoCaptureDeviceInfo* found =
-          FindDeviceInfoById(entry->id, devices_info_cache_);
+          GetDeviceInfoById(entry->id);
       if (found) {
         entry->video_capture_controller()->DoLogOnIOThread(base::StringPrintf(
             "Starting device: id: %s, name: %s, api: %s",
@@ -506,13 +505,8 @@ void VideoCaptureManager::OnDeviceStarted(
       device_ptr->StopAndDeAllocate();
     }
   } else {
-    DeviceEntries::iterator entry_it = std::find_if(
-        devices_.begin(), devices_.end(),
-        [serial_id] (const DeviceEntry* e) {
-          return e->serial_id == serial_id;
-        });
-    DCHECK(entry_it != devices_.end());
-    DeviceEntry* entry = *entry_it;
+    DeviceEntry* entry =  GetDeviceEntryBySerialId(serial_id);
+    DCHECK(entry);
     DCHECK(!entry->video_capture_device());
     entry->SetVideoCaptureDevice(std::move(device));
 
@@ -649,7 +643,7 @@ void VideoCaptureManager::StopCaptureForClient(
   DCHECK(controller);
   DCHECK(client_handler);
 
-  DeviceEntry* entry = GetDeviceEntryForController(controller);
+  DeviceEntry* entry = GetDeviceEntryByController(controller);
   if (!entry) {
     NOTREACHED();
     return;
@@ -693,11 +687,9 @@ void VideoCaptureManager::PauseCaptureForClient(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(controller);
   DCHECK(client_handler);
-  DeviceEntry* entry = GetDeviceEntryForController(controller);
-  if (!entry) {
-    NOTREACHED();
-    DVLOG(1) << "Got Null entry while pausing capture";
-  }
+  DeviceEntry* entry = GetDeviceEntryByController(controller);
+  if (!entry)
+    NOTREACHED() << "Got Null entry while pausing capture";
 
   // Do not pause Content Video Capture devices, e.g. Tab or Screen capture.
   if (entry->stream_type != MEDIA_DEVICE_VIDEO_CAPTURE)
@@ -716,11 +708,9 @@ void VideoCaptureManager::ResumeCaptureForClient(
   DCHECK(controller);
   DCHECK(client_handler);
 
-  DeviceEntry* entry = GetDeviceEntryForController(controller);
-  if (!entry) {
-    NOTREACHED();
-    DVLOG(1) << "Got Null entry while resuming capture";
-  }
+  DeviceEntry* entry = GetDeviceEntryByController(controller);
+  if (!entry)
+    NOTREACHED() << "Got Null entry while resuming capture";
 
   // Do not resume Content Video Capture devices, e.g. Tab or Screen capture.
   if (entry->stream_type != MEDIA_DEVICE_VIDEO_CAPTURE)
@@ -733,7 +723,7 @@ void VideoCaptureManager::RequestRefreshFrameForClient(
     VideoCaptureController* controller) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  if (DeviceEntry* entry = GetDeviceEntryForController(controller)) {
+  if (DeviceEntry* entry = GetDeviceEntryByController(controller)) {
     if (media::VideoCaptureDevice* device = entry->video_capture_device()) {
       device_task_runner_->PostTask(
           FROM_HERE,
@@ -760,7 +750,7 @@ bool VideoCaptureManager::GetDeviceSupportedFormats(
 
   // Return all available formats of the device, regardless its started state.
   media::VideoCaptureDeviceInfo* existing_device =
-      FindDeviceInfoById(it->second.id, devices_info_cache_);
+      GetDeviceInfoById(it->second.id);
   if (existing_device)
     *supported_formats = existing_device->supported_formats;
   return true;
@@ -779,7 +769,7 @@ bool VideoCaptureManager::GetDeviceFormatsInUse(
 
   // Return the currently in-use format(s) of the device, if it's started.
   DeviceEntry* device_in_use =
-      GetDeviceEntryForMediaStreamDevice(it->second);
+      GetDeviceEntryByTypeAndId(it->second.type, it->second.id);
   if (device_in_use) {
     // Currently only one format-in-use is supported at the VCC level.
     formats_in_use->push_back(
@@ -801,12 +791,11 @@ void VideoCaptureManager::SetDesktopCaptureWindowId(
 void VideoCaptureManager::MaybePostDesktopCaptureWindowId(
     media::VideoCaptureSessionId session_id) {
   SessionMap::iterator session_it = sessions_.find(session_id);
-  if (session_it == sessions_.end()) {
+  if (session_it == sessions_.end())
     return;
-  }
 
-  DeviceEntry* const existing_device =
-      GetDeviceEntryForMediaStreamDevice(session_it->second);
+  DeviceEntry* const existing_device = GetDeviceEntryByTypeAndId(
+      session_it->second.type, session_it->second.id);
   if (!existing_device) {
     DVLOG(2) << "Failed to find an existing screen capture device.";
     return;
@@ -846,7 +835,7 @@ void VideoCaptureManager::GetPhotoCapabilities(
     media::ScopedResultCallback<
         VideoCaptureDevice::GetPhotoCapabilitiesCallback> callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  VideoCaptureDevice* device = GetVideoCaptureDeviceFromSessionId(session_id);
+  VideoCaptureDevice* device = GetVideoCaptureDeviceBySessionId(session_id);
   if (!device)
     return;
   // Unretained(device) is safe to use here because |device| would be null if it
@@ -862,7 +851,7 @@ void VideoCaptureManager::TakePhoto(
     media::ScopedResultCallback<VideoCaptureDevice::TakePhotoCallback>
         callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  VideoCaptureDevice* device = GetVideoCaptureDeviceFromSessionId(session_id);
+  VideoCaptureDevice* device = GetVideoCaptureDeviceBySessionId(session_id);
   if (!device)
     return;
   // Unretained(device) is safe to use here because |device| would be null if it
@@ -968,61 +957,83 @@ void VideoCaptureManager::ConsolidateDevicesInfoOnDeviceThread(
   on_devices_enumerated_callback.Run(new_devices_info_cache);
 }
 
-VideoCaptureManager::DeviceEntry*
-VideoCaptureManager::GetDeviceEntryForMediaStreamDevice(
-    const MediaStreamDevice& device_info) {
+void VideoCaptureManager::DestroyDeviceEntryIfNoClients(DeviceEntry* entry) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  // Removal of the last client stops the device.
+  if (!entry->video_capture_controller()->HasActiveClient() &&
+      !entry->video_capture_controller()->HasPausedClient()) {
+    DVLOG(1) << "VideoCaptureManager stopping device (type = "
+             << entry->stream_type << ", id = " << entry->id << ")";
 
-  for (DeviceEntry* device : devices_) {
-    if (device_info.type == device->stream_type && device_info.id == device->id)
-      return device;
+    // The DeviceEntry is removed from |devices_| immediately. The controller is
+    // deleted immediately, and the device is freed asynchronously. After this
+    // point, subsequent requests to open this same device ID will create a new
+    // DeviceEntry, VideoCaptureController, and VideoCaptureDevice.
+    DoStopDevice(entry);
+    DeviceEntries::iterator device_it =
+        std::find_if(devices_.begin(), devices_.end(),
+                     [entry](const std::unique_ptr<DeviceEntry>& device_entry) {
+                       return device_entry.get() == entry;
+                     });

[chfremer, 2016/06/30 17:08:52]

See the other comment. Would be nice to have reusable short-hand form.

[mcasas, 2016/06/30 17:43:18]

I searched in stl_util.h and base/containers and didn't
find anything similar. Perhaps we could extend the logic
in ContainsKey()/ContainsValue() [1] to detect that the
Collection is a std::vector<std::unique_ptr<>> and the
Value would be a pointer, and search like we are doing 
here, WDYT? I'll make a bug and  leave it for another CL
though :)


[1]
https://cs.chromium.org/chromium/src/base/stl_util.h?sq=package:chromium&dr=C...

[chfremer, 2016/06/30 18:05:47]

Thanks for creating the bug entry. I think a new function ContainsPointer()
sounds like a reasonable solution. I am not a fan of overloading one of the
existing functions, because the name would not reveal the intent very well.

+    devices_.erase(device_it);
   }
-  return nullptr;
 }
 
 media::VideoCaptureDevice*
-VideoCaptureManager::GetVideoCaptureDeviceFromSessionId(int session_id) {
+VideoCaptureManager::GetVideoCaptureDeviceBySessionId(int session_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   SessionMap::const_iterator session_it = sessions_.find(session_id);
   if (session_it == sessions_.end())
     return nullptr;
 
-  DeviceEntry* const device_info =
-      GetDeviceEntryForMediaStreamDevice(session_it->second);
-  if (!device_info)
-    return nullptr;
-  return device_info->video_capture_device();
+  DeviceEntry* const device_info = GetDeviceEntryByTypeAndId(
+      session_it->second.type, session_it->second.id);
+  return device_info ? device_info->video_capture_device() : nullptr;
+}
+
+VideoCaptureManager::DeviceEntry*
+VideoCaptureManager::GetDeviceEntryByTypeAndId(MediaStreamType type,
+                                               const std::string& id) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  for (const std::unique_ptr<DeviceEntry>& device : devices_) {
+    if (type == device->stream_type && id == device->id)
+      return device.get();
+  }
+  return nullptr;
 }
 
 VideoCaptureManager::DeviceEntry*
-VideoCaptureManager::GetDeviceEntryForController(
+VideoCaptureManager::GetDeviceEntryByController(
     const VideoCaptureController* controller) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   // Look up |controller| in |devices_|.
-  for (DeviceEntry* device : devices_) {
+  for (const std::unique_ptr<DeviceEntry>& device : devices_) {
     if (device->video_capture_controller() == controller)
-      return device;
+      return device.get();
   }
   return nullptr;
 }
 
-void VideoCaptureManager::DestroyDeviceEntryIfNoClients(DeviceEntry* entry) {
+VideoCaptureManager::DeviceEntry*
+VideoCaptureManager::GetDeviceEntryBySerialId(int serial_id) const {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  // Removal of the last client stops the device.
-  if (!entry->video_capture_controller()->HasActiveClient() &&
-      !entry->video_capture_controller()->HasPausedClient()) {
-    DVLOG(1) << "VideoCaptureManager stopping device (type = "
-             << entry->stream_type << ", id = " << entry->id << ")";
 
-    // The DeviceEntry is removed from |devices_| immediately. The controller is
-    // deleted immediately, and the device is freed asynchronously. After this
-    // point, subsequent requests to open this same device ID will create a new
-    // DeviceEntry, VideoCaptureController, and VideoCaptureDevice.
-    DoStopDevice(entry);
-    DeviceEntries::iterator device_it = std::find(devices_.begin(),
-                                                  devices_.end(),
-                                                  entry);
-    devices_.erase(device_it);
+  for (const std::unique_ptr<DeviceEntry>& device : devices_) {
+    if (device->serial_id == serial_id)
+      return device.get();
   }
+  return nullptr;
+}
+
+media::VideoCaptureDeviceInfo* VideoCaptureManager::GetDeviceInfoById(
+    const std::string& id) {
+  for (auto& it : devices_info_cache_) {
+    if (it.name.id() == id)
+      return &(it);
+  }
+  return nullptr;
 }
 
 VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
@@ -1038,7 +1049,7 @@ VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
   // Check if another session has already opened this device. If so, just
   // use that opened device.
   DeviceEntry* const existing_device =
-      GetDeviceEntryForMediaStreamDevice(device_info);
+      GetDeviceEntryByTypeAndId(device_info.type, device_info.id);
   if (existing_device) {
     DCHECK_EQ(device_info.type, existing_device->stream_type);
     return existing_device;
@@ -1051,20 +1062,10 @@ VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
   DeviceEntry* new_device =
       new DeviceEntry(device_info.type, device_info.id,
                       std::move(video_capture_controller), params);
-  devices_.push_back(new_device);
+  devices_.emplace_back(new_device);
   return new_device;
 }
 
-media::VideoCaptureDeviceInfo* VideoCaptureManager::FindDeviceInfoById(
-    const std::string& id,
-    media::VideoCaptureDeviceInfos& device_vector) {
-  for (auto& it : device_vector) {
-    if (it.name.id() == id)
-      return &(it);
-  }
-  return nullptr;
-}
-
 void VideoCaptureManager::SetDesktopCaptureWindowIdOnDeviceThread(
     media::VideoCaptureDevice* device,
     gfx::NativeViewId window_id) {
@@ -1128,7 +1129,7 @@ void VideoCaptureManager::ReleaseDevices() {
     if (entry->stream_type != MEDIA_DEVICE_VIDEO_CAPTURE)
       continue;
 
-    DoStopDevice(entry);
+    DoStopDevice(entry.get());
   }
 }
 
@@ -1154,7 +1155,7 @@ void VideoCaptureManager::ResumeDevices() {
     if (!device_in_queue) {
       // Session ID is only valid for Screen capture. So we can fake it to
       // resume video capture devices here.
-      QueueStartDevice(kFakeSessionId, entry, entry->parameters);
+      QueueStartDevice(kFakeSessionId, entry.get(), entry->parameters);
     }
   }
 }