Implement timing metrics for UrlRequest

components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
index 52afc3744099eafb412170514194f809a0133ce1..31a7d04c01339683d618200550342e8319a23aab 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
@@ -65,6 +65,8 @@ public final class CronetUrlRequest implements UrlRequest {
     @GuardedBy("mUrlRequestAdapterLock")
     @Nullable
     private final UrlRequestMetricsAccumulator mRequestMetricsAccumulator;
+    @GuardedBy("mUrlRequestAdapterLock")
+    private RequestFinishedInfo.Metrics mMetrics;
 
     /*
      * Synchronize access to mUrlRequestAdapter, mStarted, mWaitingOnRedirect,
@@ -201,7 +203,8 @@ public final class CronetUrlRequest implements UrlRequest {
             try {
                 mUrlRequestAdapter =
                         nativeCreateRequestAdapter(mRequestContext.getUrlRequestContextAdapter(),
-                                mInitialUrl, mPriority, mDisableCache, mDisableConnectionMigration);
+                                mInitialUrl, mPriority, mDisableCache, mDisableConnectionMigration,
+                                mRequestContext.hasRequestFinishedListener());

[xunjieli, 2016/09/20 21:53:33]

Can we just use the metricsCollectionEnabled field from the constructor?
Grabbing the mUrlRequestAdapterLock and another lock makes me nervous about
deadlock. Since this is a best effort thing, I think it's fine to use
metricsCollectionEnabled from the constructor.

[mgersh, 2016/09/30 23:47:14]

I was actually planning on deleting that field, so we can do the check a bit
later. We're already doing the multiple locks thing elsewhere, with the same two
locks, so we'll have to always acquire them in the same order anyway. Added a
comment about this on mFinishedListenerLock.

[xunjieli, 2016/10/01 13:43:49]

Why do you plan to delete that field? We don't win a whole lot by delaying the
check from constructor to when start() is called. 
However, if you planning to remove the synchronized block from
CronetUrlRequestContext#createRequest(), then this is fine with me. Just making
sure there won't be two synchronized blocks trying to do the same thing.

[mgersh, 2016/10/04 21:14:58]

Mostly because I prefer not having that information stored in an excessive
number of places, and using metricsCollectionEnabled from the constructor would
require adding it as a member variable on CronetUrlRequest in addition to the
native request adapter and the list of listeners in the CronetUrlRequestContext.
But yes, that block is getting deleted.

[xunjieli, 2016/10/04 22:29:23]

Acknowledged.

                 mRequestContext.onRequestStarted();
                 if (mInitialMethod != null) {
                     if (!nativeSetHttpMethod(mUrlRequestAdapter, mInitialMethod)) {
@@ -699,11 +702,30 @@ public final class CronetUrlRequest implements UrlRequest {
         postTaskToExecutor(task);
     }
 
+    /**
+     * Called by the native code to report metrics.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onMetricsCollected(long requestStartMs, long dnsStartMs, long dnsEndMs,
+            long connectStartMs, long connectEndMs, long sslStartMs, long sslEndMs,
+            long sendingStartMs, long sendingEndMs, long pushStartMs, long pushEndMs,
+            long responseStartMs, long responseEndMs, boolean socketReused, long sentBytesCount,
+            long receivedBytesCount) {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mMetrics != null) {
+                throw new IllegalStateException("Metrics collection should only happen once.");
+            }
+            mMetrics = new CronetMetrics(requestStartMs, dnsStartMs, dnsEndMs, connectStartMs,
+                    connectEndMs, sslStartMs, sslEndMs, sendingStartMs, sendingEndMs, pushStartMs,
+                    pushEndMs, responseStartMs, responseEndMs, socketReused, sentBytesCount,
+                    receivedBytesCount);
+        }
+    }
+
     RequestFinishedInfo getRequestFinishedInfo() {
         // TODO(mgersh): fill in real values for finishedReason and exception
-        return new RequestFinishedInfo(mInitialUrl, mRequestAnnotations,
-                (mRequestMetricsAccumulator != null ? mRequestMetricsAccumulator.getRequestMetrics()
-                                                    : EMPTY_METRICS),
+        return new RequestFinishedInfo(mInitialUrl, mRequestAnnotations, mMetrics,
                 RequestFinishedInfo.SUCCEEDED, mResponseInfo, null);
     }
 
@@ -751,7 +773,8 @@ public final class CronetUrlRequest implements UrlRequest {
     // Native methods are implemented in cronet_url_request_adapter.cc.
 
     private native long nativeCreateRequestAdapter(long urlRequestContextAdapter, String url,
-            int priority, boolean disableCache, boolean disableConnectionMigration);
+            int priority, boolean disableCache, boolean disableConnectionMigration,
+            boolean enableMetrics);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native boolean nativeSetHttpMethod(long nativePtr, String method);