Resolve the problems where we can leak the system tray UI

content/browser/renderer_host/media/media_stream_manager.cc

Index: content/browser/renderer_host/media/media_stream_manager.cc
diff --git a/content/browser/renderer_host/media/media_stream_manager.cc b/content/browser/renderer_host/media/media_stream_manager.cc
index 1329b97d8c7b1d28da84667c995ae40956eac700..1e4e30c3b099d040b3937b4563f9020e262e5505 100644
--- a/content/browser/renderer_host/media/media_stream_manager.cc
+++ b/content/browser/renderer_host/media/media_stream_manager.cc
@@ -178,37 +178,6 @@ void MediaStreamManager::GenerateStream(MediaStreamRequester* requester,
   StartEnumeration(&new_request, label);
 }
 
-void MediaStreamManager::CancelRequests(MediaStreamRequester* requester) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DeviceRequests::iterator it = requests_.begin();
-  while (it != requests_.end()) {
-    if (it->second.requester == requester && !RequestDone(it->second)) {
-      // The request isn't complete, but there might be some devices already
-      // opened -> close them.
-      DeviceRequest* request = &(it->second);
-      if (request->state[content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE] ==
-          DeviceRequest::kOpening) {
-        for (StreamDeviceInfoArray::iterator it =
-             request->audio_devices.begin(); it != request->audio_devices.end();
-             ++it) {
-          audio_input_device_manager()->Close(it->session_id);
-        }
-      }
-      if (request->state[content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE] ==
-          DeviceRequest::kOpening) {
-        for (StreamDeviceInfoArray::iterator it =
-             request->video_devices.begin(); it != request->video_devices.end();
-             ++it) {
-          video_capture_manager()->Close(it->session_id);
-        }
-      }
-      requests_.erase(it++);
-    } else {
-      ++it;
-    }
-  }
-}
-
 void MediaStreamManager::CancelGenerateStream(const std::string& label) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
@@ -256,7 +225,9 @@ void MediaStreamManager::StopGeneratedStream(const std::string& label) {
          video_it != it->second.video_devices.end(); ++video_it) {
       video_capture_manager()->Close(video_it->session_id);
     }
-    if (it->second.type == DeviceRequest::kGenerateStream) {
+
+    if (it->second.type == DeviceRequest::kGenerateStream &&
+        RequestDone(it->second)) {
       NotifyObserverDevicesClosed(&(it->second));
     }
     requests_.erase(it);
@@ -350,13 +321,11 @@ void MediaStreamManager::StartEnumeration(
   // Need to make an asynchronous call to make sure the |requester| gets the
   // |label| before it would receive any event.

[wjia(left Chromium), 2012/08/10 01:23:13]

remove the comments about posting task.

[no longer working on chromium, 2012/08/10 11:50:56]

Done.

   if (new_request->type == DeviceRequest::kGenerateStream) {
-    BrowserThread::PostTask(BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&MediaStreamDeviceSettings::RequestCaptureDeviceUsage,
-                   base::Unretained(device_settings_.get()),
-                   request_label, new_request->render_process_id,
-                   new_request->render_view_id, new_request->options,
-                   new_request->security_origin));
+    device_settings_->RequestCaptureDeviceUsage(request_label,
+                                                new_request->render_process_id,
+                                                new_request->render_view_id,
+                                                new_request->options,
+                                                new_request->security_origin);
   }
 
   (*label) = request_label;
@@ -494,11 +463,7 @@ void MediaStreamManager::DevicesEnumerated(
         }
         break;
       default:
-        BrowserThread::PostTask(BrowserThread::IO,
-            FROM_HERE,
-            base::Bind(&MediaStreamDeviceSettings::AvailableDevices,
-                       base::Unretained(device_settings_.get()),
-                       *it, stream_type, devices));
+        device_settings_->AvailableDevices(*it, stream_type, devices);
     }
   }
   label_list.clear();
@@ -537,17 +502,22 @@ void MediaStreamManager::Error(MediaStreamType stream_type,
             it->second.requester->VideoDeviceFailed(it->first, device_idx);
           }
           GetDeviceManager(stream_type)->Close(capture_session_id);
-          devices->erase(device_it);
-        } else if (it->second.audio_devices.size()
-            + it->second.video_devices.size() <= 1) {
-          // 2. Device not opened and no other devices for this request ->
-          //    signal stream error and remove the request.
-          it->second.requester->StreamGenerationFailed(it->first);
-          requests_.erase(it);
+          // We don't erase the devices here so that we can update the UI
+          // properly in StopGeneratedStream().
+          it->second.state[stream_type] = DeviceRequest::kError;
         } else {
-          // 3. Not opened but other devices exists for this request -> remove
-          //    device from list, but don't signal an error.
-          devices->erase(device_it);
+          // Request is not done, devices are not opened in this case.
+          if (it->second.audio_devices.size()
+              + it->second.video_devices.size() <= 1) {
+            // 2. Device not opened and no other devices for this request ->
+            //    signal stream error and remove the request.
+            it->second.requester->StreamGenerationFailed(it->first);
+            requests_.erase(it);
+          } else {
+            // 3. Not opened but other devices exists for this request -> remove
+            //    device from list, but don't signal an error.
+            devices->erase(device_it);
+          }

[wjia(left Chromium), 2012/08/10 01:23:13]

why this "else if" format change?

[no longer working on chromium, 2012/08/10 11:50:56]

It is more readable in this way, because:
The first "if" is checking the it->second.state[stream_type], and the second
"else if" is checking the sizes of the devices, they don't belong to the same
scope, so it should be better to separate them.

         }
         return;
       }
@@ -686,8 +656,12 @@ void MediaStreamManager::DevicesFromRequest(
 bool MediaStreamManager::RequestDone(const DeviceRequest& request) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // Check if all devices are opened.
-  if (Requested(request.options,
-                content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE)) {
+  MediaStreamType stream_type = content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE;
+  if (Requested(request.options, stream_type)) {
+    if (request.state[stream_type] != DeviceRequest::kDone &&
+        request.state[stream_type] != DeviceRequest::kError)
+      return false;
+
     for (StreamDeviceInfoArray::const_iterator it =

[mflodman_chromium_OOO, 2012/08/10 07:15:44]

See comment in patch set #2.

[no longer working on chromium, 2012/08/10 11:50:56]

After discussing offline, we decided to leave it as it is now, and remove the
in_use flag on a separate CL.

          request.audio_devices.begin(); it != request.audio_devices.end();
          ++it) {
@@ -696,8 +670,13 @@ bool MediaStreamManager::RequestDone(const DeviceRequest& request) const {
       }
     }
   }
-  if (Requested(request.options,
-                content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE)) {
+
+  stream_type = content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE;
+  if (Requested(request.options, stream_type)) {
+    if (request.state[stream_type] != DeviceRequest::kDone &&
+        request.state[stream_type] != DeviceRequest::kError)
+      return false;
+
     for (StreamDeviceInfoArray::const_iterator it =
          request.video_devices.begin(); it != request.video_devices.end();
          ++it) {
@@ -706,6 +685,7 @@ bool MediaStreamManager::RequestDone(const DeviceRequest& request) const {
       }
     }
   }
+
   return true;
 }