[Cronet] Add error code and immediatelyRetryable() to UrlRequestException

components/cronet/android/java/src/org/chromium/net/UrlRequestException.java

Index: components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
diff --git a/components/cronet/android/java/src/org/chromium/net/UrlRequestException.java b/components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
index 3557324c1e7b6d5d716131b5cf30fdafb215360f..5af8b1c440ebdc5a4501e88cff66556f1307b277 100644
--- a/components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
+++ b/components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
@@ -7,25 +7,149 @@ package org.chromium.net;
 import java.io.IOException;
 
 /**
- * Exception after UrlRequest start. Could be reported by network stack, in
- * which case netError() will contain native error code.
+ * Exception passed to {@link UrlRequestListener#onFailed UrlRequestListener.onFailed()} when:
+ * <ul>
+ * <li>{@link UrlRequestListener} or {@link UploadDataProvider} method throws an exception. In this
+ *     case {@link IOException#getCause getCause()} can be used to find the thrown exception.
+ *     {@link #getErrorCode} will return {@link #ERROR_LISTENER_THREW}.
+ * <li>Cronet fails to process a network request. In this case
+ *     {@link #getErrorCode} and {@link #netError} can be used to get more
+ *     information about the specific type of failure.
+ * </ul>
  */
 public class UrlRequestException extends IOException {
-    /** Net error code if exception is reported by native. */
-    final int mNetError;
+    // Error code, one of ERROR_*
+    private final int mErrorCode;
+    // Cronet error code.
+    private final int mCronetError;

[xunjieli, 2015/10/13 23:07:10]

static variables should go before non-static ones. Maybe move these two down.

[pauljensen, 2015/11/05 18:35:29]

Done.

 
-    UrlRequestException(String message, Throwable cause) {
+    /**
+     * Error code indicating this class wraps an exception thrown by {@link UrlRequestListener} or
+     * {@link UploadDataProvider}. Wrapped exception can be retrieved using
+     * {@link IOException#getCause}.
+     */
+    public static final int ERROR_LISTENER_THREW = UrlRequestError.LISTENER_THREW;

[xunjieli, 2015/10/13 23:07:10]

What about ERROR_LISTENER_EXCEPTION_THROWN?
The other names are mostly using past participles and passive voice. It is a bit
weird to have this one in active voice.

[pauljensen, 2015/11/05 18:35:29]

I was taught that passive voice is to be avoided whenever possible.  I think
this sentiment is shared by others:
https://en.wikipedia.org/wiki/English_passive_voice#Advice_against_the_passiv...
I do agree that it doesn't match the other errors well, but I think this error
is quite different from the other errors too, so I'm not sure that's a bad
thing.  In this case the error was caused by the listener rather than the
ethereal Cronet.
I'd tend to keep it in the active voice because active voice is preferred and to
differentiate it from the other errors.
I don't feel strongly though; if you feel strongly, I'm happy to change it. 
Your suggestion is a good passive voice rewording.

[xunjieli, 2015/11/06 15:23:22]

Hmm.. my grammar is failing me. I guess this isn't so much about passive vs
active. The other error names have a noun following ERROR_, and then a past
participle or an adjective to describe that noun (except ERROR_TIMED_OUT). The
word "THREW" is a bit strange, since it is a past tense but not a past
participle (THROWN). If we change THREW to THROWN, LISTENER_THROWN doesn't make
much sense, which led me to LISTENER_EXCEPTION_THROWN. For the sake of
consistency, I am fine with anything that is a noun followed by an adjective or
a past participle that describes that noun.

[pauljensen, 2016/01/25 02:08:16]

Done; I changed it to ERROR_LISTENER_EXCEPTION_THROWN as suggested.

+    /**
+     * Error code indicating the host being sent the request could not be resolved to an IP address.
+     */
+    public static final int ERROR_HOSTNAME_NOT_RESOLVED = UrlRequestError.HOSTNAME_NOT_RESOLVED;
+    /**
+     * Error code indicating the device was not connected to any network.
+     */
+    public static final int ERROR_INTERNET_DISCONNECTED = UrlRequestError.INTERNET_DISCONNECTED;
+    /**
+     * Error code indicating that as the request was processed the network configuration chnaged.
+     */
+    public static final int ERROR_NETWORK_CHANGED = UrlRequestError.NETWORK_CHANGED;
+    /**
+     * Error code indicating a timeout expired.
+     */
+    public static final int ERROR_TIMED_OUT = UrlRequestError.TIMED_OUT;

[xunjieli, 2015/10/13 23:07:11]

What's the difference between TIMED_OUT and CONNECTION_TIMED_OUT? It's not very
clear from the comments.

[pauljensen, 2015/11/05 18:35:28]

I think CONNECTION_TIMED_OUT is just TIMED_OUT occurring during the common
subcase of request phases, connection time.  I've adjusted the comments to try
to convey this.

+    /**
+     * Error code indicating the connection was closed unexpectedly.
+     */
+    public static final int ERROR_CONNECTION_CLOSED = UrlRequestError.CONNECTION_CLOSED;
+    /**
+     * Error code indicating the connection attempt timed out.
+     */
+    public static final int ERROR_CONNECTION_TIMED_OUT = UrlRequestError.CONNECTION_TIMED_OUT;
+    /**
+     * Error code indicating the connection attempt was refused.
+     */
+    public static final int ERROR_CONNECTION_REFUSED = UrlRequestError.CONNECTION_REFUSED;
+    /**
+     * Error code indicating the connection was unexpectedly reset.
+     */
+    public static final int ERROR_CONNECTION_RESET = UrlRequestError.CONNECTION_RESET;
+    /**
+     * Error code indicating the IP address being contacted is unreachable, meaning there is no
+     * route to the specified host or network.
+     */
+    public static final int ERROR_ADDRESS_UNREACHABLE = UrlRequestError.ADDRESS_UNREACHABLE;
+    /**
+     * Error code indicating another type of error was encountered.  {@link #netError} can be

[xunjieli, 2015/10/13 23:07:10]

nit: there are two spaces after the period.

[pauljensen, 2015/11/05 18:35:28]

Done.

+     * consulted to get a more specific cause.
+     */
+    public static final int ERROR_OTHER = UrlRequestError.OTHER;
+
+    /**
+     * Construct exception wrapping {@code cause} thrown by {@link UrlRequestListener}.

[xunjieli, 2015/10/13 23:07:11]

nit: s/Construct/Constructs. 
Maybe this changes to:
Constructs an exception that wraps {@code cause} thrown by a {@link
UrlRequestListener}.

[pauljensen, 2015/11/05 18:35:29]

Done.

+     *
+     * @param message explanation of failure.
+     * @param cause exception thrown by {@link UrlRequestListener} that's being wrapped.
+     */
+    public UrlRequestException(String message, Throwable cause) {
         super(message, cause);
-        mNetError = 0;
+        mErrorCode = ERROR_LISTENER_THREW;
+        mCronetError = 0;
     }
 
-    UrlRequestException(String message, int netError) {
+    /**
+     * Construct exception coming from Cronet failure.

[xunjieli, 2015/10/13 23:07:10]

nit: s/Construct/Constructs.
Embedder probably thinks that everything is Cronet, so it's unclear from the
comments what "Cronet" refers to.
Maybe change this to a slightly more general description? like: 
Constructs an exception with a specific error.

[pauljensen, 2015/11/05 18:35:28]

Done.

+     *
+     * @param message explanation of failure.
+     * @param errorCode error code, one of {@link #ERROR_LISTENER_THREW ERROR_*}.
+     * @param netError Cronet error code, one of
+     * <a href=https://chromium.googlesource.com/chromium/src/+/master/net/base/net_error_list.h>
+     * these</a>.
+     */
+    public UrlRequestException(String message, int errorCode, int netError) {
         super(message, null);
-        mNetError = netError;
+        mErrorCode = errorCode;
+        mCronetError = netError;

[xunjieli, 2015/10/13 23:07:10]

I don't really like that we call net error codes "cronet error code." This is a
bit confusing, since the ERROR_* codes are also defined and used by Cronet.
Should we keep netError as "net error code," and not calling them "cronet error
code?"

[pauljensen, 2015/11/05 18:35:28]

I don't like "net" as the ERROR_* codes are all also error codes indicative of
networking problems.  How about "Cronet internal"?  I've changed it accordingly,
PTAL.

+    }
+
+    /**
+     * Returns error code, one of {@link #ERROR_LISTENER_THREW ERROR_*}.
+     *
+     * @return error code, one of {@link #ERROR_LISTENER_THREW ERROR_*}.
+     */
+    public int getErrorCode() {
+        return mErrorCode;
     }
 
-    /** @return Error code if exception is reported by native. */
+    /**
+     * Returns a Cronet error code. This may provide more specific error
+     * diagnosis than {@link #getErrorCode}, but the constant values are not exposed to Java and
+     * may change over time. See
+     * <a href=https://chromium.googlesource.com/chromium/src/+/master/net/base/net_error_list.h>
+     * here</a> for the lastest list of values.
+     *
+     * @return Cronet error code.
+     */
     public int netError() {
-        return mNetError;
+        return mCronetError;
+    }
+
+    /**
+     * Returns {@code true} if retrying this request right away might succeed, {@code false}
+     * otherwise. For example returns {@code true} when {@link #getErrorCode} returns
+     * {@link #ERROR_NETWORK_CHANGED} because trying the request might succeed using the new
+     * network configuration, but {@code false} when {@code getErrorCode()} returns
+     * {@link #ERROR_INTERNET_DISCONNECTED} because retrying the request right away will
+     * encounter the same failure (instead retrying should be delayed until device regains
+     * network connectivity). Returns {@code false} when {@code getErrorCode()} returns
+     * {@link #ERROR_LISTENER_THREW}.
+     *
+     * @return {@code true} if retrying this request right away might succeed, {@code false}
+     *         otherwise.
+     */
+    public boolean immediatelyRetryable() {
+        switch (mErrorCode) {
+            case ERROR_LISTENER_THREW:
+            case ERROR_HOSTNAME_NOT_RESOLVED:
+            case ERROR_INTERNET_DISCONNECTED:
+            case ERROR_CONNECTION_REFUSED:
+            case ERROR_ADDRESS_UNREACHABLE:
+            case ERROR_OTHER:
+            default:
+                return false;
+            case ERROR_NETWORK_CHANGED:
+            case ERROR_TIMED_OUT:
+            case ERROR_CONNECTION_CLOSED:
+            case ERROR_CONNECTION_TIMED_OUT:
+            case ERROR_CONNECTION_RESET:
+                return true;
+        }
     }
 }