Add histogram WebRTC.UserMediaRequest.NoResultState to track user media requests with no result.

content/renderer/media/media_stream_impl.cc

Index: content/renderer/media/media_stream_impl.cc
diff --git a/content/renderer/media/media_stream_impl.cc b/content/renderer/media/media_stream_impl.cc
index 145da08becaafff25debdbd4808dcbf1316e1c20..fedd869ea5c34ed5d1600a7de43f941109d56120 100644
--- a/content/renderer/media/media_stream_impl.cc
+++ b/content/renderer/media/media_stream_impl.cc
@@ -199,6 +199,7 @@ void MediaStreamImpl::cancelUserMediaRequest(
     // We can't abort the stream generation process.
     // Instead, erase the request. Once the stream is generated we will stop the
     // stream if the request does not exist.
+    LogUserMediaRequestWithNoResult(MEDIA_STREAM_REQUEST_EXPLICITLY_CANCELLED);
     DeleteUserMediaRequestInfo(request);
   }
 }
@@ -273,22 +274,8 @@ void MediaStreamImpl::OnStreamGenerated(
 
   UserMediaRequestInfo* request_info = FindUserMediaRequestInfo(request_id);
   if (!request_info) {
-    // This can happen if the request is canceled or the frame reloads while
-    // MediaStreamDispatcher is processing the request.
-    // Only stop the device if the device is not used in another MediaStream.
-    for (StreamDeviceInfoArray::const_iterator device_it = audio_array.begin();
-         device_it != audio_array.end(); ++device_it) {
-      if (!FindLocalSource(*device_it))
-        media_stream_dispatcher_->StopStreamDevice(*device_it);
-    }
-
-    for (StreamDeviceInfoArray::const_iterator device_it = video_array.begin();
-         device_it != video_array.end(); ++device_it) {
-      if (!FindLocalSource(*device_it))
-        media_stream_dispatcher_->StopStreamDevice(*device_it);
-    }
-
     DVLOG(1) << "Request ID not found";
+    OnStreamGeneratedForCancelledRequest(audio_array, video_array);
     return;
   }
   request_info->generated = true;
@@ -332,6 +319,25 @@ void MediaStreamImpl::OnStreamGenerated(
                  weak_factory_.GetWeakPtr()));
 }
 
+// This can happen if the request is canceled or the frame reloads while
+// MediaStreamDispatcher is processing the request.
+// Only stop the device if the device is not used in another MediaStream.

[tommi (sloooow) - chröme, 2014/08/06 11:42:21]

nit: documentation for methods tends to stay in the header

[andresp-chromium, 2014/08/06 11:59:14]

Split documentation to the relevant places.

+void MediaStreamImpl::OnStreamGeneratedForCancelledRequest(
+    const StreamDeviceInfoArray& audio_array,
+    const StreamDeviceInfoArray& video_array) {
+  for (StreamDeviceInfoArray::const_iterator device_it = audio_array.begin();
+       device_it != audio_array.end(); ++device_it) {
+    if (!FindLocalSource(*device_it))
+      media_stream_dispatcher_->StopStreamDevice(*device_it);
+  }
+
+  for (StreamDeviceInfoArray::const_iterator device_it = video_array.begin();
+       device_it != video_array.end(); ++device_it) {
+    if (!FindLocalSource(*device_it))
+      media_stream_dispatcher_->StopStreamDevice(*device_it);
+  }
+}
+
 // Callback from MediaStreamDispatcher.
 // The requested stream failed to be generated.
 void MediaStreamImpl::OnStreamGenerationFailed(
@@ -701,6 +707,31 @@ void MediaStreamImpl::DeleteUserMediaRequestInfo(
   NOTREACHED();
 }
 
+void MediaStreamImpl::DeleteAllUserMediaRequests() {
+  UserMediaRequests::iterator request_it = user_media_requests_.begin();
+  while (request_it != user_media_requests_.end()) {
+    DVLOG(1) << "MediaStreamImpl@" << this << "::DeleteAllUserMediaRequests: "
+             << "Cancel user media request " << (*request_it)->request_id;
+    // If the request is not generated, it means that a request
+    // has been sent to the MediaStreamDispatcher to generate a stream
+    // but MediaStreamDispatcher has not yet responded and we need to cancel
+    // the request.
+    if (!(*request_it)->generated) {
+      media_stream_dispatcher_->CancelGenerateStream(
+          (*request_it)->request_id, weak_factory_.GetWeakPtr());
+    }
+    if ((*request_it)->generated) {

[tommi (sloooow) - chröme, 2014/08/06 11:42:21]

should this be an else?

[andresp-chromium, 2014/08/06 11:59:14]

Done.

+      DCHECK((*request_it)->HasPendingSources());
+      LogUserMediaRequestWithNoResult(
+          MEDIA_STREAM_REQUEST_PENDING_MEDIA_TRACKS);
+    } else {
+      DCHECK(!(*request_it)->HasPendingSources());

[tommi (sloooow) - chröme, 2014/08/06 11:42:21]

...and this looks like it should be in the scope where we call
CancelGenerateStream() (unless I'm missing something)

[andresp-chromium, 2014/08/06 11:59:14]

Done. Yeah you are right! Since I was looking as two diffs (one refactor  + one
adding logging) I never noticed I had the same condition above. Collapsing them
together now.

+      LogUserMediaRequestWithNoResult(MEDIA_STREAM_REQUEST_NOT_GENERATED);
+    }
+    request_it = user_media_requests_.erase(request_it);
+  }
+}
+
 MediaStreamImpl::MediaDevicesRequestInfo*
 MediaStreamImpl::FindMediaDevicesRequestInfo(
     int request_id) {
@@ -748,20 +779,7 @@ void MediaStreamImpl::CancelAndDeleteMediaDevicesRequest(
 
 void MediaStreamImpl::FrameWillClose() {
   // Cancel all outstanding UserMediaRequests.
-  UserMediaRequests::iterator request_it = user_media_requests_.begin();
-  while (request_it != user_media_requests_.end()) {
-    DVLOG(1) << "MediaStreamImpl@" << this << "::FrameWillClose: "
-             << "Cancel user media request " << (*request_it)->request_id;
-    // If the request is not generated, it means that a request
-    // has been sent to the MediaStreamDispatcher to generate a stream
-    // but MediaStreamDispatcher has not yet responded and we need to cancel
-    // the request.
-    if (!(*request_it)->generated) {
-      media_stream_dispatcher_->CancelGenerateStream(
-          (*request_it)->request_id, weak_factory_.GetWeakPtr());
-    }
-    request_it = user_media_requests_.erase(request_it);
-  }
+  DeleteAllUserMediaRequests();
 
   // Loop through all current local sources and stop the sources.
   LocalStreamSources::iterator sources_it = local_sources_.begin();
@@ -910,4 +928,11 @@ void MediaStreamImpl::UserMediaRequestInfo::RemoveSource(
   }
 }
 
+bool MediaStreamImpl::UserMediaRequestInfo::AreAllSourcesRemoved() const {

[tommi (sloooow) - chröme, 2014/08/06 11:42:21]

ugh, grammar :-/  not sure we should fix that now though

[andresp-chromium, 2014/08/06 11:59:14]

This method is never called. Removing.

+  return sources_.empty();
+}
+
+bool MediaStreamImpl::UserMediaRequestInfo::HasPendingSources() const {
+  return !sources_waiting_for_callback_.empty();
+}
 }  // namespace content