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

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

Add a getErrorCode() method to UrlRequestException and define constant
values for the most common failures.  This allows embedders to make
decisions based on the error codes without worrying about Cronet's
internal error codes changing.
Add immediatelyRetryable() metod to UrlRequestException to help
embedders decide which requests should be retried.

BUG=531538
Committed: https://crrev.com/867dd4b5fc023427c6cc5b1dffd5f2a65cc9e901
Cr-Commit-Position: refs/heads/master@{#373042}

[pauljensen, 2015-10-09 20:23:12]

pauljensen@chromium.org changed reviewers:
+ xunjieli@chromium.org

[pauljensen, 2015-10-09 20:23:12]

Helen, PTAL.

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

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
(right):

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:732: *
@param nativeError native net error code.
nit: need to add "@param errorCode" to be consistent.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:24:
private final int mCronetError;
static variables should go before non-static ones. Maybe move these two down.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:31:
public static final int ERROR_LISTENER_THREW = UrlRequestError.LISTENER_THREW;
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.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:47:
public static final int ERROR_TIMED_OUT = UrlRequestError.TIMED_OUT;
What's the difference between TIMED_OUT and CONNECTION_TIMED_OUT? It's not very
clear from the comments.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:70:
* Error code indicating another type of error was encountered.  {@link
#netError} can be
nit: there are two spaces after the period.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:76:
* Construct exception wrapping {@code cause} thrown by {@link
UrlRequestListener}.
nit: s/Construct/Constructs. 
Maybe this changes to:
Constructs an exception that wraps {@code cause} thrown by a {@link
UrlRequestListener}.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:88:
* Construct exception coming from Cronet failure.
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.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:99:
mCronetError = netError;
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:29]

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
(right):

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:732: *
@param nativeError native net error code.
On 2015/10/13 23:07:10, xunjieli wrote:
> nit: need to add "@param errorCode" to be consistent.

Done.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:24:
private final int mCronetError;
On 2015/10/13 23:07:10, xunjieli wrote:
> static variables should go before non-static ones. Maybe move these two down.

Done.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:31:
public static final int ERROR_LISTENER_THREW = UrlRequestError.LISTENER_THREW;
On 2015/10/13 23:07:10, xunjieli wrote:
> 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.

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.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:47:
public static final int ERROR_TIMED_OUT = UrlRequestError.TIMED_OUT;
On 2015/10/13 23:07:11, xunjieli wrote:
> What's the difference between TIMED_OUT and CONNECTION_TIMED_OUT? It's not
very
> clear from the comments.

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.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:70:
* Error code indicating another type of error was encountered.  {@link
#netError} can be
On 2015/10/13 23:07:10, xunjieli wrote:
> nit: there are two spaces after the period.

Done.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:76:
* Construct exception wrapping {@code cause} thrown by {@link
UrlRequestListener}.
On 2015/10/13 23:07:11, xunjieli wrote:
> nit: s/Construct/Constructs. 
> Maybe this changes to:
> Constructs an exception that wraps {@code cause} thrown by a {@link
> UrlRequestListener}.

Done.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:88:
* Construct exception coming from Cronet failure.
On 2015/10/13 23:07:10, xunjieli wrote:
> 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.

Done.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:99:
mCronetError = netError;
On 2015/10/13 23:07:10, xunjieli wrote:
> 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?"

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.

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

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:31:
public static final int ERROR_LISTENER_THREW = UrlRequestError.LISTENER_THREW;
On 2015/11/05 18:35:29, pauljensen wrote:
> On 2015/10/13 23:07:10, xunjieli wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:15:
* <li>Cronet fails to process a network request. In this case
nit: This sentence seems a bit too short. Maybe move a few words from the next
line up.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:16:
*     {@link #getErrorCode} and {@link #netError} can be used to get more
nit: netError -> getCronetInternalErrorCode

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:36:
* Error code indicating that as the request was processed the network
configuration chnaged.
nit: typo in "changed"

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:66:
* Error code indicating another type of error was encountered. {@link #netError}
can be
nit: netError -> getCronetInternalErrorCode?

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:82:
public UrlRequestException(String message, Throwable cause) {
Suggest moving this constructor after UrlRequestException(String message, int
errorCode, int cronetInternalErrorCode), which is slightly easier to understand
and more general.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:97:
public UrlRequestException(String message, int errorCode, int
cronetInternalErrorCode) {
Why do we change the constructors from package protected to public?

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:51: // Convert most
popular net::ERR_* values to counterparts accessible in Java.
nit: Convert -> Converts

[pauljensen, 2016-01-25 01:43:51]

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:15:
* <li>Cronet fails to process a network request. In this case
On 2015/11/06 15:23:22, xunjieli wrote:
> nit: This sentence seems a bit too short. Maybe move a few words from the next
> line up.

Hmm, ya, it is a bit short.  I don't have any great rewording ideas.  What do
you mean bring up some words from the next line?  I thought the next line was
more discussion of details.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:16:
*     {@link #getErrorCode} and {@link #netError} can be used to get more
On 2015/11/06 15:23:22, xunjieli wrote:
> nit: netError -> getCronetInternalErrorCode

Done.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:36:
* Error code indicating that as the request was processed the network
configuration chnaged.
On 2015/11/06 15:23:22, xunjieli wrote:
> nit: typo in "changed"

Done.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:66:
* Error code indicating another type of error was encountered. {@link #netError}
can be
On 2015/11/06 15:23:22, xunjieli wrote:
> nit: netError -> getCronetInternalErrorCode?

Done.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:82:
public UrlRequestException(String message, Throwable cause) {
On 2015/11/06 15:23:22, xunjieli wrote:
> Suggest moving this constructor after UrlRequestException(String message, int
> errorCode, int cronetInternalErrorCode), which is slightly easier to
understand
> and more general.

Done.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:97:
public UrlRequestException(String message, int errorCode, int
cronetInternalErrorCode) {
On 2015/11/06 15:23:22, xunjieli wrote:
> Why do we change the constructors from package protected to public?

This was suggested by API review to allows users to create instances for
testing.

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:51: // Convert most
popular net::ERR_* values to counterparts accessible in Java.
On 2015/11/06 15:23:22, xunjieli wrote:
> nit: Convert -> Converts

Done.

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

PTAL.  I had to make a change to get javadocs building.

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestException.java:31:
public static final int ERROR_LISTENER_THREW = UrlRequestError.LISTENER_THREW;
On 2015/11/06 15:23:22, xunjieli wrote:
> On 2015/11/05 18:35:29, pauljensen wrote:
> > On 2015/10/13 23:07:10, xunjieli wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

Done; I changed it to ERROR_LISTENER_EXCEPTION_THROWN as suggested.

[xunjieli, 2016-01-25 14:37:02]

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:97:
public UrlRequestException(String message, int errorCode, int
cronetInternalErrorCode) {
On 2016/01/25 01:43:50, pauljensen wrote:
> On 2015/11/06 15:23:22, xunjieli wrote:
> > Why do we change the constructors from package protected to public?
> 
> This was suggested by API review to allows users to create instances for
> testing.

Acknowledged.

https://codereview.chromium.org/1393713005/diff/90009/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/90009/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:88:
// generation will fail because it is not passed the generated java constants.
Are we able to pass the generated java file to javadoc generator?  This
conversion seems an unnecessary step. (I looked up UrlRequest.Status, and that
is a subset of net::LoadState. But here these two sets are completely the same).

[pauljensen, 2016-02-02 18:45:45]

https://codereview.chromium.org/1393713005/diff/90009/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java
(right):

https://codereview.chromium.org/1393713005/diff/90009/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:88:
// generation will fail because it is not passed the generated java constants.
On 2016/01/25 14:37:02, xunjieli wrote:
> Are we able to pass the generated java file to javadoc generator?  This
> conversion seems an unnecessary step. (I looked up UrlRequest.Status, and that
> is a subset of net::LoadState. But here these two sets are completely the
same).
> 

Done.

[pauljensen, 2016-02-02 18:46:00]

PTAL, thank you!

[xunjieli, 2016-02-02 19:02:25]

On 2016/02/02 18:46:00, pauljensen wrote:
> PTAL, thank you!

lgtm. Thanks!

[pauljensen, 2016-02-02 19:57:11]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2016-02-02 19:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/1393713005/#ps310001
(title: "fix missing test annotation")

[commit-bot: I haz the power, 2016-02-02 20:00:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393713005/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393713005/310001

[commit-bot: I haz the power, 2016-02-02 21:23:03]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-02-02 21:24:14]

Description was changed from

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

Add a getErrorCode() method to UrlRequestException and define constant
values for the most common failures.  This allows embedders to make
decisions based on the error codes without worrying about Cronet's
internal error codes changing.
Add immediatelyRetryable() metod to UrlRequestException to help
embedders decide which requests should be retried.

BUG=531538
==========

to

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

Add a getErrorCode() method to UrlRequestException and define constant
values for the most common failures.  This allows embedders to make
decisions based on the error codes without worrying about Cronet's
internal error codes changing.
Add immediatelyRetryable() metod to UrlRequestException to help
embedders decide which requests should be retried.

BUG=531538

Committed: https://crrev.com/867dd4b5fc023427c6cc5b1dffd5f2a65cc9e901
Cr-Commit-Position: refs/heads/master@{#373042}
==========

[commit-bot: I haz the power, 2016-02-02 21:24:20]

Patchset 17 (id:??) landed as
https://crrev.com/867dd4b5fc023427c6cc5b1dffd5f2a65cc9e901
Cr-Commit-Position: refs/heads/master@{#373042}

[xunjieli, 2016-04-23 10:37:16]

https://codereview.chromium.org/1393713005/diff/310001/components/cronet/andr...
File components/cronet/android/url_request_error.h (right):

https://codereview.chromium.org/1393713005/diff/310001/components/cronet/andr...
components/cronet/android/url_request_error.h:14: enum UrlRequestError {
It turns out that there is already a java class with all NetError codes as
static ints
(https://code.google.com/p/chromium/codesearch#search/&q=NetError%20file:java$...).
Andrei told me about it.
We should probably get rid of this and do the mapping and retryable logic
entirely Java side. WDYT?

[pauljensen, 2016-04-29 14:39:05]

https://codereview.chromium.org/1393713005/diff/310001/components/cronet/andr...
File components/cronet/android/url_request_error.h (right):

https://codereview.chromium.org/1393713005/diff/310001/components/cronet/andr...
components/cronet/android/url_request_error.h:14: enum UrlRequestError {
On 2016/04/23 10:37:15, xunjieli wrote:
> It turns out that there is already a java class with all NetError codes as
> static ints
>
(https://code.google.com/p/chromium/codesearch#search/&q=NetError%20file:java$...).
> Andrei told me about it.
> We should probably get rid of this and do the mapping and retryable logic
> entirely Java side. WDYT?

I don't see a benefit to doing it Java-side.  I think there may actually be
benefits to doing it native side:
1. native code may be smaller, and is certainly faster
2. UrlRequestException is in the API side, rather than the implementation side,
and it may be much harder to make changes in the API side, e.g. if the
implementation side is auto-updated but the API side is linked into an app.