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

content/renderer/media/media_stream_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/media_stream_impl.h"
 
 #include <utility>
 
 #include "base/hash.h"
 #include "base/logging.h"
@@ skipping 181 matching lines @@
 }
 
 void MediaStreamImpl::cancelUserMediaRequest(
     const blink::WebUserMediaRequest& user_media_request) {
   DCHECK(CalledOnValidThread());
   UserMediaRequestInfo* request = FindUserMediaRequestInfo(user_media_request);
   if (request) {
     // 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(true,

[vrk (LEFT CHROMIUM), 2014/08/05 19:05:40]

In general, boolean parameters are not good practice. This function call is
pretty hard to understand unless you go to the definition of the call -- e.g.
what does "true" mean? See comment in LogUserMediaRequestWithNoResult -- you
should modify the logic so that you can simply say here:
LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_CANCELLED)

[andresp-chromium, 2014/08/06 10:13:56]

Done.

+                                    request->generated,
+                                    request->HasPendingSources());
     DeleteUserMediaRequestInfo(request);
   }
 }
 
 void MediaStreamImpl::requestMediaDevices(
     const blink::WebMediaDevicesRequest& media_devices_request) {
   UpdateWebRTCMethodCount(WEBKIT_GET_MEDIA_DEVICES);
   DCHECK(CalledOnValidThread());
 
   int audio_input_request_id = g_next_request_id++;
@@ skipping 54 matching lines @@
 void MediaStreamImpl::OnStreamGenerated(
     int request_id,
     const std::string& label,
     const StreamDeviceInfoArray& audio_array,
     const StreamDeviceInfoArray& video_array) {
   DCHECK(CalledOnValidThread());
   DVLOG(1) << "MediaStreamImpl::OnStreamGenerated stream:" << label;
 
   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";
-    return;
+    return OnStreamGeneratedForCancelledRequest(audio_array, video_array);

[vrk (LEFT CHROMIUM), 2014/08/05 19:05:40]

Why did you make this into its own function? Also shouldn't return the result of
a void function - I'm assuming that was a mistake?

[andresp-chromium, 2014/08/06 10:13:56]

This was re factoring when reading the code. Originally i was going to add an
histogram count for this scenario but then found out that those are counted as
Cancelled or NotGenerated already. Anyhow it might be interesting to add a
counter here and on OnStreamGenerationFailedForCancelledRequest, if we want to
investigate those buckets further.


Regarding return value:
 I think that contrary to Java, return void is valid in C++. I think I just
acquired this style once I spent too much time cleaning WebRTC with unused
return values. Since this seems to cause confusion I am switching to the most
common use case.

   }
   request_info->generated = true;
 
   // WebUserMediaRequest don't have an implementation in unit tests.
   // Therefore we need to check for isNull here and initialize the
   // constraints.
   blink::WebUserMediaRequest* request = &(request_info->request);
   blink::WebMediaConstraints audio_constraints;
   blink::WebMediaConstraints video_constraints;
   if (request->isNull()) {
@@ skipping 22 matching lines @@
   web_stream->setExtraData(
       new MediaStream(
           *web_stream));
 
   // Wait for the tracks to be started successfully or to fail.
   request_info->CallbackOnTracksStarted(
       base::Bind(&MediaStreamImpl::OnCreateNativeTracksCompleted,
                  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.
+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);
+  }
+}
+

[vrk (LEFT CHROMIUM), 2014/08/05 19:05:40]

nit: extra line

[andresp-chromium, 2014/08/06 10:13:56]

Done.

+
 // Callback from MediaStreamDispatcher.
 // The requested stream failed to be generated.
 void MediaStreamImpl::OnStreamGenerationFailed(
     int request_id,
     content::MediaStreamRequestResult result) {
   DCHECK(CalledOnValidThread());
   DVLOG(1) << "MediaStreamImpl::OnStreamGenerationFailed("
            << request_id << ")";
   UserMediaRequestInfo* request_info = FindUserMediaRequestInfo(request_id);
   if (!request_info) {
@@ skipping 349 matching lines @@
   UserMediaRequests::iterator it = user_media_requests_.begin();
   for (; it != user_media_requests_.end(); ++it) {
     if ((*it) == request) {
       user_media_requests_.erase(it);
       return;
     }
   }
   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());
+    }
+    LogUserMediaRequestWithNoResult(false,

[vrk (LEFT CHROMIUM), 2014/08/05 19:05:40]

As stated elsewhere, this call should instead look like 
if (!(*request_it)->generated) {
  DCHECK(!(*request_it)->HasPendingSources());
  LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_NOT_GENERATED)
} else {
  DCHECK((*request_it)->HasPendingSources());
  LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_PENDING_MEDIA_TRACKS)
}

(You could also do a DCHECK_EQ((*request_it)->HasPendingSources(),
(*request_it)->generated) before the if/else)

[andresp-chromium, 2014/08/06 10:13:56]

Done. I liked this suggestion.

+                                    (*request_it)->generated,
+                                    (*request_it)->HasPendingSources());
+    request_it = user_media_requests_.erase(request_it);
+  }
+}
+
 MediaStreamImpl::MediaDevicesRequestInfo*
 MediaStreamImpl::FindMediaDevicesRequestInfo(
     int request_id) {
   MediaDevicesRequests::iterator it = media_devices_requests_.begin();
   for (; it != media_devices_requests_.end(); ++it) {
     if ((*it)->audio_input_request_id == request_id ||
         (*it)->video_input_request_id == request_id ||
         (*it)->audio_output_request_id == request_id) {
       return (*it);
     }
@@ skipping 27 matching lines @@
 
       media_devices_requests_.erase(it);
       return;
     }
   }
   NOTREACHED();
 }
 
 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();

[vrk (LEFT CHROMIUM), 2014/08/05 19:05:40]

Why did you move this into its own function?

[andresp-chromium, 2014/08/06 10:13:56]

Re-factoring when reading the code.
I wanted to isolate all the places where user_media_requests_ vector gets
touched into simple methods. Thus allowing to more easily verify that all code
paths were logging MediaRequest with no result.

 
   // Loop through all current local sources and stop the sources.
   LocalStreamSources::iterator sources_it = local_sources_.begin();
   while (sources_it != local_sources_.end()) {
     StopLocalSource(*sources_it, true);
     sources_it = local_sources_.erase(sources_it);
   }
 }
 
 void MediaStreamImpl::OnLocalSourceStopped(
@@ skipping 129 matching lines @@
            sources_.begin();
        it != sources_.end(); ++it) {
     if (source.id() == it->id()) {
       sources_.erase(it);
       return;
     }
   }
 }
 
 }  // namespace content