Add API for new Cronet metrics

components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java

Index: components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java
diff --git a/components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java b/components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java
index 6f1c9ab457d0dc222d0a9b3ba33c6153c9ae122d..8d59e2a87bc42fcc944463c32bb256d7e75ea601 100644
--- a/components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java
+++ b/components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java
@@ -4,9 +4,13 @@
 
 package org.chromium.net;
 
+import android.support.annotation.IntDef;
 import android.support.annotation.Nullable;
 
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.Collection;
+import java.util.Date;
 import java.util.concurrent.Executor;
 
 /**
@@ -49,76 +53,207 @@ public final class RequestFinishedInfo {
     /**
      * Metrics collected for a single request.

[pauljensen, 2016/09/06 17:22:23]

Can we elaborate a bit more here?  Like by adding a statement of what these
metrics generally comprise?  We could also provide a segway into the event
ordering list.  Something like "The metrics provide timing details for different
events that are required to process a {@link UrlRequest}. These metrics can be
used to investigate and improve performance."

[mgersh, 2016/09/07 22:09:58]

Done.

      *
+     * Most timing metrics are taken from
+     * {@link https://cs.chromium.org/chromium/src/net/base/load_timing_info.h LoadTimingInfo},
+     * which holds the information for {@link http://w3c.github.io/navigation-timing/} and
+     * {@link https://www.w3.org/TR/resource-timing/}.

[pauljensen, 2016/09/06 17:22:23]

This is kind of an implementation detail.  Perhaps could we move this down to
the end of the description as sort of a foot-note?

[mgersh, 2016/09/07 22:09:57]

Done.

+     *
+     * Events happen in this order:
+     * request start

[pauljensen, 2016/09/06 17:22:23]

Use <ol> and <li>'s

[pauljensen, 2016/09/06 17:22:23]

I feel like we should include links from these list items to the getters below. 
Perhaps like:
Request start (see {@link Metrics#getRequestStart})

[mgersh, 2016/09/07 22:09:58]

Done.

[mgersh, 2016/09/07 22:09:58]

Done.

+     * dns start

[pauljensen, 2016/09/06 17:22:23]

dns->DNS
here and below

[mgersh, 2016/09/07 22:09:57]

Done.

+     * dns end
+     * connect start
+     * ssl start

[pauljensen, 2016/09/06 17:22:23]

ssl->SSL
here and below

[mgersh, 2016/09/07 22:09:58]

Done.

+     * ssl end
+     * connect end
+     * send start
+     * send end
+     * response start
+     * response end
+     *
+     * Start times are reported as the time when a request started blocking on event, not when the
+     * event actually occurred, with the exception of push start and end. If a metric is not
+     * meaningful or not available, including cases when a request finished before reaching that
+     * stage, start and end times will be null. If no time was spent blocking on an event, start

[pauljensen, 2016/09/06 17:22:23]

null->{@code null}

[mgersh, 2016/09/07 22:09:58]

Done.

+     * and end will be the same time.
+     *
+     * Timestamps are recorded using a clock that is guaranteed not to run backwards. All timestamps
+     * are correct relative to the system clock at the time of request start, and taking the
+     * difference between two timestamps will give the correct difference between the events. In
+     * order to preserve this property, timestamps for events other than request start are not
+     * guaranteed to match the system clock at the times they represent.

[pauljensen, 2016/09/06 17:22:23]

Could we simplify this paragraph or preface it with something like "In the
unlikely event that the system clock is adjusted during the request, these
{@link #Date} values may not match the system clock adjustment."
This doesn't seem like an issue that would affect many users, so I'd hate to
force many users to grok all this.

[mgersh, 2016/09/07 22:09:58]

Done.

+     *
      * {@hide} as it's a prototype.
      */
-    public static class Metrics {
+    public abstract static class Metrics {
+        /**
+         * @return {@link java.util.Date} representing when the request started from the perspective

[pauljensen, 2016/09/06 17:22:23]

All of these methods need a method description in addition to the @return tag.

Some guidance: "The @return tag is required for every method that returns
something other than void, even if it is redundant with the method description.
(Whenever possible, find something non-redundant (ideally, more specific) to use
for the tag comment.)"
from:
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html
which is linked from
https://source.android.com/source/code-style.html#use-javadoc-standard-comments
which is linked from http://dev.chromium.org/developers/coding-style/java

[mgersh, 2016/09/07 22:09:58]

Done.

+         * of the native URLRequest layer. This timestamp will match the system clock at the time it

[xunjieli, 2016/09/05 23:24:17]

nit: Avoid implementation details in the Javadoc.  Consumers don't really know
what "URLRequest" is, and they don't need to know. Could simply say "the native
request" instead of "the native URLRequest layer." Or you could say that this
represents the time when the request actually started. That'll give us room to
adjust the implementation if needed. 

[mgersh, 2016/09/07 22:09:57]

Done.

+         * represents.
+         */
+        @Nullable
+        public abstract Date getRequestStart();
+
+        /**
+         * @return {@link java.util.Date} representing when DNS lookup started. Null if a proxy is

[pauljensen, 2016/09/06 17:22:23]

What do you mean by "a proxy" here?

[pauljensen, 2016/09/06 17:22:23]

Null->{@code null}
here and below

[mgersh, 2016/09/07 22:09:57]

I think I mean the thing that Helen said doesn't apply to us, but forgot to
delete it here.

[mgersh, 2016/09/07 22:09:58]

Done.

+         * used to determine the DNS address.

[pauljensen, 2016/09/06 17:22:23]

doesn't this and some of the other methods return null when the socket is
reused?

[mgersh, 2016/09/07 22:09:58]

Done.

+         */
         @Nullable
-        private final Long mTtfbMs;
+        public abstract Date getDnsStart();
+
+        /**
+         * @return {@link java.util.Date} representing when DNS lookup finished. Null if a proxy is
+         * used to determine the DNS address.
+         */
         @Nullable
-        private final Long mTotalTimeMs;
+        public abstract Date getDnsEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when connection establishment started,
+         * typically when DNS resolution finishes if no proxy is used.
+         */
         @Nullable
-        private final Long mSentBytesCount;
+        public abstract Date getConnectStart();
+
+        /**
+         * @return {@link java.util.Date} representing when connection establishment finished,
+         * after TCP connection is established and, if using HTTPS, SSL handshake is completed.
+         */
         @Nullable
-        private final Long mReceivedBytesCount;
+        public abstract Date getConnectEnd();
 
-        public Metrics(@Nullable Long ttfbMs, @Nullable Long totalTimeMs,
-                @Nullable Long sentBytesCount, @Nullable Long receivedBytesCount) {
-            mTtfbMs = ttfbMs;
-            mTotalTimeMs = totalTimeMs;
-            mSentBytesCount = sentBytesCount;
-            mReceivedBytesCount = receivedBytesCount;
-        }
+        /**
+         * @return {@link java.util.Date} representing when SSL handshake started. Null if SSL is
+         * not used.
+         */
+        @Nullable
+        public abstract Date getSslStart();
+
+        /**
+         * @return {@link java.util.Date} representing when SSL handshake finished. Null if SSL is
+         * not used.
+         */
+        @Nullable
+        public abstract Date getSslEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when sending HTTP request headers started.
+         */
+        @Nullable
+        public abstract Date getSendingStart();
+
+        /**
+         * @return {@link java.util.Date} representing when sending HTTP request body finished.

[pauljensen, 2016/09/06 17:22:23]

It's a little odd that getSendingStart() is documented to be the start of
something other than what getSendingEnd() is documented to be the end of.  Can
we elaborate to mention that the request body follows the request headers?

[mgersh, 2016/09/07 22:09:58]

Done.

+         */
+        @Nullable
+        public abstract Date getSendingEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when the first byte of an HTTP2 server push

[pauljensen, 2016/09/06 17:22:23]

HTTP2->HTTP/2
here and below
so as to match things like CronetEngine.Builder.enableHTTP2 documentation

[mgersh, 2016/09/07 22:09:58]

Done.

+         * was received. Null if server push is not used.
+         */
+        @Nullable
+        public abstract Date getPushStart();
+
+        /**
+         * @return {@link java.util.Date} representing when the last byte of an HTTP2 server push
+         * was received. Null if server push is not used.
+         */
+        @Nullable
+        public abstract Date getPushEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when the first byte of the response was
+         * received.
+         */
+        @Nullable
+        public abstract Date getResponseStart();
+
+        /**
+         * @return {@link java.util.Date} representing when the last byte of the response was
+         * received.
+         */
+        @Nullable
+        public abstract Date getResponseEnd();
+
+        /**
+         * @return whether this request reused a socket from a previous request. When true, DNS,

[pauljensen, 2016/09/06 17:22:23]

true->{@code true}

[mgersh, 2016/09/07 22:09:58]

Done.

+         * connection, and SSL times will be null.
+         */
+        @Nullable
+        public abstract boolean getSocketReused();
 
         /**
          * Returns milliseconds between request initiation and first byte of response headers,
          * or null if not collected.
+         * TODO(mgersh): Remove once new API works http://crbug.com/629194
          */
         @Nullable
-        public Long getTtfbMs() {
-            return mTtfbMs;
-        }
+        public abstract Long getTtfbMs();
 
         /**
          * Returns milliseconds between request initiation and finish,
          * including a failure or cancellation, or null if not collected.
+         * TODO(mgersh): Remove once new API works http://crbug.com/629194
          */
         @Nullable
-        public Long getTotalTimeMs() {
-            return mTotalTimeMs;
-        }
+        public abstract Long getTotalTimeMs();
 
         /**
          * Returns total bytes sent over the network transport layer, or null if not collected.
          */
         @Nullable
-        public Long getSentBytesCount() {
-            return mSentBytesCount;
-        }
+        public abstract Long getSentBytesCount();
 
         /**
          * Returns total bytes received over the network transport layer, or null if not collected.
          */
         @Nullable
-        public Long getReceivedBytesCount() {
-            return mReceivedBytesCount;
-        }
+        public abstract Long getReceivedBytesCount();
     }
 
     private final String mUrl;
     private final Collection<Object> mAnnotations;
     private final Metrics mMetrics;
+
+    /** @hide */
+    @IntDef({SUCCEEDED, FAILED, CANCELED})
+    @Retention(RetentionPolicy.SOURCE)
+    public @interface FinishedReason {}
+
+    /**
+     * The request succeeded. Returned from {@link #getFinishedReason}.

[pauljensen, 2016/09/06 17:22:23]

do you need to qualify the class, so Metrics#getFinishedReason?

[pauljensen, 2016/09/06 17:22:23]

should we prefix this description with "Reason value indicating "

[mgersh, 2016/09/07 22:09:58]

Done.

[mgersh, 2016/09/07 22:09:58]

No, both this and getFinishedReason() are in RequestFinishedInfo, not a nested
class.

+     */
+    public static final int SUCCEEDED = 0;
+    /**
+     * The request failed or errored. Returned from {@link #getFinishedReason}.
+     */
+    public static final int FAILED = 1;
+    /**
+     * The request was canceled. Returned from {@link #getFinishedReason}.
+     */
+    public static final int CANCELED = 2;
+
+    @FinishedReason
+    private final int mFinishedReason;
+
     @Nullable
     private final UrlResponseInfo mResponseInfo;
+    @Nullable
+    private final UrlRequestException mException;
 
     /**
      * @hide only used by internal implementation.
      */
     public RequestFinishedInfo(String url, Collection<Object> annotations, Metrics metrics,
-            @Nullable UrlResponseInfo responseInfo) {
+            @FinishedReason int finishedReason, @Nullable UrlResponseInfo responseInfo,
+            @Nullable UrlRequestException exception) {
         mUrl = url;
         mAnnotations = annotations;
         mMetrics = metrics;
+        mFinishedReason = finishedReason;
         mResponseInfo = responseInfo;
+        mException = exception;
     }
 
     /** Returns the request's original URL. */
@@ -132,6 +267,7 @@ public final class RequestFinishedInfo {
     }
 
     // TODO(klm): Collect and return a chain of Metrics objects for redirect responses.
+    // TODO(mgersh): Update this javadoc when new metrics are fully implemented
     /**
      * Returns metrics collected for this request.
      *
@@ -149,6 +285,15 @@ public final class RequestFinishedInfo {
     }
 
     /**
+     * Returns the reason why the request finished.
+     * @return one of {@link #SUCCEEDED}, {@link #FAILED}, or {@link #CANCELED}
+     */
+    @FinishedReason
+    public int getFinishedReason() {
+        return mFinishedReason;
+    }
+
+    /**
      * Returns a {@link UrlResponseInfo} for the request, if its response had started.
      * @return {@link UrlResponseInfo} for the request, if its response had started.
      */
@@ -156,4 +301,15 @@ public final class RequestFinishedInfo {
     public UrlResponseInfo getResponseInfo() {
         return mResponseInfo;
     }
+
+    /**
+     * If the request failed, returns the same {@link UrlRequestException} provided to
+     * {@link UrlRequest.Callback#onFailed}.
+     *
+     * @return the request's {@link UrlRequestException}, if the request failed
+     */
+    @Nullable
+    public UrlRequestException getException() {
+        return mException;
+    }
 }