Job Controller: clean up metrics logging about the job racing result

net/http/http_stream_factory_impl_job_controller.cc

Index: net/http/http_stream_factory_impl_job_controller.cc
diff --git a/net/http/http_stream_factory_impl_job_controller.cc b/net/http/http_stream_factory_impl_job_controller.cc
index 4186e9698917c80820ae2ff8f04d8c67c63545a7..bd3d8256b86624c6fcb5682639a0be567c87d25e 100644
--- a/net/http/http_stream_factory_impl_job_controller.cc
+++ b/net/http/http_stream_factory_impl_job_controller.cc
@@ -741,10 +741,8 @@ void HttpStreamFactoryImpl::JobController::OnJobSucceeded(Job* job) {
     ReportBrokenAlternativeService();
 
   if (!bound_job_) {
-    if (main_job_ && alternative_job_) {
-      job->ReportJobSucceededForRequest();
-      MaybeRecordAlternativeProxyServerUsage(job);
-    }
+    if (main_job_ && alternative_job_)
+      ReportAlternateProtocolUsage(job);
     BindJob(job);
     return;
   }
@@ -901,7 +899,7 @@ HttpStreamFactoryImpl::JobController::GetAlternativeServiceForInternal(
       quic_advertised = true;
     if (http_server_properties.IsAlternativeServiceBroken(
             alternative_service)) {
-      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_BROKEN);
+      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_BROKEN, false);
       continue;
     }
 
@@ -1050,37 +1048,25 @@ bool HttpStreamFactoryImpl::JobController::
   return true;
 }
 
-void HttpStreamFactoryImpl::JobController::
-    MaybeRecordAlternativeProxyServerUsage(Job* job) const {
-  if (is_preconnect_ ||
-      !alternative_job_->alternative_proxy_server().is_quic()) {
-    return;
-  }
-  DCHECK(main_job_.get() == job || alternative_job_.get() == job);
-
-  enum AlternativeProxyUsage {
-    ALTERNATIVE_PROXY_USAGE_NO_RACE = 0,
-    ALTERNATIVE_PROXY_USAGE_WON_RACE,
-    ALTERNATIVE_PROXY_USAGE_LOST_RACE,
-    ALTERNATIVE_PROXY_USAGE_MAX,
-  };
-  AlternativeProxyUsage alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_MAX;
-
-  if (alternative_job_->using_existing_quic_session()) {
-    // If an existing session was used, then no TCP connection was
-    // started.
-    alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_NO_RACE;
-  } else if (job->alternative_proxy_server().is_quic()) {
-    // |job| was the alternative Job, and hence won the race.
-    alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_WON_RACE;
+void HttpStreamFactoryImpl::JobController::ReportAlternateProtocolUsage(
+    Job* job) const {
+  DCHECK(main_job_ && alternative_job_);

[tbansal1, 2016/09/22 17:53:02]

Curious why use a weaker DCHECK?
Is it possible that DCHECK(main_job_.get() == job || alternative_job_.get() ==
job); is actually false?

[Zhongyi Shi, 2016/09/22 23:56:53]

The new DCHECK was to make sure both jobs are alive when we figure out
reporting. And the if and DCHECK @1066 enforces:
DCHECK(main_job_.get() == job || alternative_job_.get() ==
job).
 

[tbansal1, 2016/09/23 00:02:07]

Acknowledged.

+  bool proxy_server_used =
+      alternative_job_->alternative_proxy_server().is_quic();
+
+  if (job == alternative_job_.get()) {
+    if (job->using_existing_quic_session()) {
+      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE,
+                                      proxy_server_used);
+    } else {
+      HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE,
+                                      proxy_server_used);
+    }
   } else {
-    // |job| was the normal Job, and hence the alternative Job lost the race.
-    alternative_proxy_usage = ALTERNATIVE_PROXY_USAGE_LOST_RACE;
+    DCHECK_EQ(main_job_.get(), job);
+    HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE,
+                                    proxy_server_used);
   }

[Ryan Hamilton, 2016/09/22 21:46:33]

nit: I think we can use some early returns:

if (job == main_job_) {
 HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE,
                                 proxy_server_used);
  return;
}

DCHECK_EQ(alternative_job_.get(), job);
if (job->using_existing_quic_session()) {
  HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE, 
                                  proxy_server_used);
  return;
}

HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE,
                                proxy_server_used);

[Zhongyi Shi, 2016/09/22 23:56:53]

Done.

-  DCHECK_NE(ALTERNATIVE_PROXY_USAGE_MAX, alternative_proxy_usage);
-  UMA_HISTOGRAM_ENUMERATION("Net.QuicAlternativeProxy.Usage",
-                            alternative_proxy_usage,
-                            ALTERNATIVE_PROXY_USAGE_MAX);
 }
 
 void HttpStreamFactoryImpl::JobController::StartAlternativeProxyServerJob() {