Resolve the problems where we can leak the system tray UI

content/browser/renderer_host/media/media_stream_device_settings.cc

Index: content/browser/renderer_host/media/media_stream_device_settings.cc
diff --git a/content/browser/renderer_host/media/media_stream_device_settings.cc b/content/browser/renderer_host/media/media_stream_device_settings.cc
index f701a1d179793d8033e550ccdda8888c8b8b0423..8af718488c90832f1d5358f2dd6c498bcd449032 100644
--- a/content/browser/renderer_host/media/media_stream_device_settings.cc
+++ b/content/browser/renderer_host/media/media_stream_device_settings.cc
@@ -99,9 +99,25 @@ class MediaStreamDeviceSettingsRequest : public MediaStreamRequest {
 
   ~MediaStreamDeviceSettingsRequest() {}
 
+  bool IsReadyForView() {
+    if (options.audio &&
+        !devices_full.count(content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE))

[mflodman_chromium_OOO, 2012/08/09 09:57:05]

Since it is counting, I would prefer '== 0' rather than '!'.

[no longer working on chromium, 2012/08/09 15:54:54]

Hope this is fine.

+      return false;
+
+    if (options.video &&
+        !devices_full.count(content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE))
+      return false;
+
+    // We have got all the requested devices, it is ready if it has not
+    // been posted for UI.
+    return !posted_task;
+  }
+
   // Request options.
   StreamOptions options;

[perkj_chrome, 2012/08/09 07:31:14]

options_

[no longer working on chromium, 2012/08/09 15:54:54]

Done.

   // Map containing available devices for the requested capture types.
+  // Note, never call devices_full[stream_type] before making sure the entry

[perkj_chrome, 2012/08/09 07:31:14]

nit- this comment seems irrelevant in this class

[no longer working on chromium, 2012/08/09 15:54:54]

It is relevant. One of the bug we are fixing is introduced by calling
devices_full_[type].empty() which creates an empty entry on the map.

+  // has been created.
   DeviceMap devices_full;

[perkj_chrome, 2012/08/09 07:31:14]

device_full_

[no longer working on chromium, 2012/08/09 15:54:54]

Done.

   // Whether or not a task was posted to make the call to
   // RequestMediaAccessPermission, to make sure that we never post twice to it.
@@ -148,7 +164,6 @@ void MediaStreamDeviceSettings::RequestCaptureDeviceUsage(
     const std::string& label, int render_process_id, int render_view_id,
     const StreamOptions& request_options, const GURL& security_origin) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   if (requests_.find(label) != requests_.end()) {

[mflodman_chromium_OOO, 2012/08/09 09:57:05]

This should never happen, can switch to a DCHECK instead?

[no longer working on chromium, 2012/08/09 15:54:54]

Good point. Done

     // Request with this id already exists.
     requester_->SettingsError(label);
@@ -163,22 +178,23 @@ void MediaStreamDeviceSettings::RequestCaptureDeviceUsage(
 void MediaStreamDeviceSettings::RemovePendingCaptureRequest(
     const std::string& label) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   SettingsRequests::iterator request_it = requests_.find(label);
   if (request_it != requests_.end()) {
-    // Proceed the next pending request for the same page.
     MediaStreamDeviceSettingsRequest* request = request_it->second;
-    std::string new_label = FindReadyRequestForView(request->render_view_id,
-                                                    request->render_process_id);
-    if (!new_label.empty()) {
-      PostRequestToUi(new_label);
-    }
-
+    int render_view_id = request->render_view_id;
+    int render_process_id = request->render_process_id;
     // TODO(xians): Post a cancel request on UI thread to dismiss the infobar

[perkj_chrome, 2012/08/09 07:31:14]

Is this todo rellevant still? Sounds important.

[no longer working on chromium, 2012/08/09 15:54:54]

No, they are not relevant to the issues we are fixing here. This should be
addressed by a separate CL.

     // if request has been sent to the UI.
     // Remove the request from the queue.
     requests_.erase(request_it);
     delete request;
+
+    // Proceed the next pending request for the same page.
+    std::string new_label = FindReadyRequestForView(render_view_id,

[mflodman_chromium_OOO, 2012/08/09 09:57:05]

This should only be done if the removed request has been posted to the ui, to
avoid posting two infobars at the same time.

[no longer working on chromium, 2012/08/09 15:54:54]

Done.

+                                                    render_process_id);
+    if (!new_label.empty()) {
+      PostRequestToUi(new_label);
+    }
   }
 }
 
@@ -190,23 +206,13 @@ void MediaStreamDeviceSettings::AvailableDevices(
 
   SettingsRequests::iterator request_it = requests_.find(label);
   DCHECK(request_it != requests_.end());
-
   // Add the answer for the request.
   MediaStreamDeviceSettingsRequest* request = request_it->second;
   DCHECK_EQ(request->devices_full.count(stream_type), static_cast<size_t>(0)) <<
       "This request already has a list of devices for this stream type.";
   request->devices_full[stream_type] = devices;
 
-  // Check if we're done.
-  size_t num_media_requests = 0;
-  if (request->options.audio) {
-    num_media_requests++;
-  }
-  if (request->options.video) {
-    num_media_requests++;
-  }
-
-  if (request->devices_full.size() == num_media_requests) {
+   if (request->IsReadyForView()) {
     // We have all answers needed.
     if (!use_fake_ui_) {
       // Abort if the task was already posted: wait for it to PostResponse.
@@ -226,26 +232,22 @@ void MediaStreamDeviceSettings::AvailableDevices(
       StreamDeviceInfoArray devices_to_use;
       for (DeviceMap::iterator it = request->devices_full.begin();
            it != request->devices_full.end(); ++it) {
-        for (StreamDeviceInfoArray::iterator device_it = it->second.begin();
-             device_it != it->second.end(); ++device_it) {
+        StreamDeviceInfoArray::iterator device_it = it->second.begin();
+        for (; device_it != it->second.end(); ++device_it) {
           if (!device_it->in_use) {
             devices_to_use.push_back(*device_it);
             break;
           }
         }
-      }
 
-      if (!request->devices_full[
-              content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE].empty() &&
-          num_media_requests != devices_to_use.size()) {
         // Not all requested device types were opened. This happens if all
         // video capture devices are already opened, |in_use| isn't set for

[perkj_chrome, 2012/08/09 07:31:14]

Is this comment valid? Can't you open the same device multiple times now?

[mflodman_chromium_OOO, 2012/08/09 09:57:05]

This was to be able to open several devices before the UI existed and there were
any selection. It should be moved inside the if below, and possibly rewritten,
considering the new code.

[no longer working on chromium, 2012/08/09 15:54:54]

No idea if the comment is valid or not. If will be great if someone can verify
it and tell me how this should be changed.

Done with moving the comments into the if case.

         // audio devices. Allow the first video capture device in the list to be
         // opened for this user too.
-        StreamDeviceInfoArray device_array =
-            request->devices_full[
-                content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE];
-        devices_to_use.push_back(*(device_array.begin()));
+        if (it->second.begin() != it->second.end() &&

[perkj_chrome, 2012/08/09 07:31:14]

How about using a temp variable above instead of it? 
Ie
const DeviceMap& device_map = it->second;
and here
if (!device_map.empty() && device_it == device_map.end()) {
...

[no longer working on chromium, 2012/08/09 15:54:54]

How about it->second.size() != 0 && device_it == it->second.end()
It fits in one line and does not need an temp variable.

and for some unknown reason empty() behaves differently with .size(), using it
destroys the logic and media_stream_dispatcher_host_unittests failed by hitting
a DCHECK.

+            device_it == it->second.end()) {
+          devices_to_use.push_back(*it->second.begin());
+        }
       }
 
       // Post result and delete request.
@@ -260,7 +262,6 @@ void MediaStreamDeviceSettings::PostResponse(
     const std::string& label,
     const content::MediaStreamDevices& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   SettingsRequests::iterator req = requests_.find(label);
   // Return if the request has been removed.
   if (req == requests_.end())
@@ -322,23 +323,8 @@ std::string MediaStreamDeviceSettings::FindReadyRequestForView(
         it->second->render_view_id == render_view_id) {
       // This request belongs to the given render view.
       MediaStreamDeviceSettingsRequest* request = it->second;
-      if (request->options.audio &&
-          request->devices_full[
-              content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE].empty()) {
-        // Audio requested, but no devices enumerated yet. Continue to next
-        // request.
-        continue;
-      }
-      if (request->options.video &&
-          request->devices_full[
-              content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE].empty()) {
-        // Video requested, but no devices enumerated yet. Continue to next
-        // request.
-        continue;
-      }
-      // This request belongs to the same view as the treated request and is
-      // ready to be requested. Return its label.
-      return it->first;
+      if (request->IsReadyForView())
+        return it->first;
     }
   }
   return std::string();