Make CronetHttpURLConnection#setConnectTimeout silently fail

Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to
facilitate applications to try out Cronet. In order to
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting
any exceptions. We should only reserve the use of
exceptions when absolutely necessary (e.g. in matters of
correctness), but not for issues of
sub-optimality (e.g. HTTP cache being ignored).

BUG=611851
Committed: https://crrev.com/7130ccb5f2fa407a7dd54e221744978475d8b64e
Cr-Commit-Position: refs/heads/master@{#397450}

[xunjieli, 2016-06-02 13:27:36]

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

[xunjieli, 2016-06-02 13:27:36]

Paul, PTAL. Thanks!

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:37:
private static final String TAG = "cr_CronetHttpURLConn";
The linter says do not use dot in the log tag.
src/docs/android_logging.md

[pauljensen, 2016-06-02 14:12:56]

lgtm, you might want to provide some motivation in the CL description.  Making
things silently fail is normally a pretty bad thing, but in this case the goal
of our HttpURLConnection impl is to facilitate trying out Cronet, and to get to
that goal we want it to be as easy as possible to drop in our HttpURLConnection
impl without hitting any exceptions.  To this goal we reserve throwing
exceptions for functionality that will cause failures, but not sub-optimal
performance (e.g. we don't throw an exception if the user has configured an HTTP
cache that our HttpURLConnection impl isn't taking advantage of).

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:405:
Log.d(TAG, "setConnectTimeout is not supported by CronetHttpURLConnection");
It might be worth upgrading this to Log.e()

[xunjieli, 2016-06-02 14:23:05]

Description was changed from

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we don't crash applications
when this method is called.

BUG=611851
==========

to

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't  crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary.

BUG=611851
==========

[xunjieli, 2016-06-02 14:23:25]

Description was changed from

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't  crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary.

BUG=611851
==========

to

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary.

BUG=611851
==========

[xunjieli, 2016-06-02 14:24:17]

Thanks for the review! I have also adjusted the CL description.

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:405:
Log.d(TAG, "setConnectTimeout is not supported by CronetHttpURLConnection");
On 2016/06/02 14:12:56, pauljensen wrote:
> It might be worth upgrading this to Log.e()

I considered using Log.e() but I decided to use the debug version as Log.e()s
aren't stripped in release builds. We might get too many of these logs since
there might be one per request. I don't feel strongly though.

[pauljensen, 2016-06-02 14:56:54]

nit: you could expand the CL description even further:

Replace "absolutely necessary." with:
absolutely necessary (e.g. in matters of correctness), but not for issues of
sub-optimality (e.g. HTTP cache being ignored).

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:405:
Log.d(TAG, "setConnectTimeout is not supported by CronetHttpURLConnection");
On 2016/06/02 14:24:17, xunjieli wrote:
> On 2016/06/02 14:12:56, pauljensen wrote:
> > It might be worth upgrading this to Log.e()
> 
> I considered using Log.e() but I decided to use the debug version as Log.e()s
> aren't stripped in release builds. We might get too many of these logs since
> there might be one per request. I don't feel strongly though. 

I think log spamming is an equal punishment to calling this function and not
understanding that it's not doing anything.  If an embedder is going to ship a
product that calls this function, we don't want to break their app but at the
same time we'd rather they understand this function isn't doing anything.

[xunjieli, 2016-06-02 16:19:19]

Description was changed from

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary.

BUG=611851
==========

to

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary (e.g. in matters of 
correctness), but not for issues of
sub-optimality (e.g. HTTP cache being ignored).

BUG=611851
==========

[xunjieli, 2016-06-02 16:20:09]

> Replace "absolutely necessary." with:
> absolutely necessary (e.g. in matters of correctness), but > not for issues of
> sub-optimality (e.g. HTTP cache being ignored).

Done. Thanks!

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/2030933002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:405:
Log.d(TAG, "setConnectTimeout is not supported by CronetHttpURLConnection");
On 2016/06/02 14:56:54, pauljensen wrote:
> On 2016/06/02 14:24:17, xunjieli wrote:
> > On 2016/06/02 14:12:56, pauljensen wrote:
> > > It might be worth upgrading this to Log.e()
> > 
> > I considered using Log.e() but I decided to use the debug version as
Log.e()s
> > aren't stripped in release builds. We might get too many of these logs since
> > there might be one per request. I don't feel strongly though. 
> 
> I think log spamming is an equal punishment to calling this function and not
> understanding that it's not doing anything.  If an embedder is going to ship a
> product that calls this function, we don't want to break their app but at the
> same time we'd rather they understand this function isn't doing anything.

Done.

[xunjieli, 2016-06-02 16:20:26]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-06-02 16:20:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/2030933002/#ps20001
(title: "use Log.e")

[commit-bot: I haz the power, 2016-06-02 16:20:40]

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

[commit-bot: I haz the power, 2016-06-02 16:59:31]

Description was changed from

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary (e.g. in matters of 
correctness), but not for issues of
sub-optimality (e.g. HTTP cache being ignored).

BUG=611851
==========

to

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary (e.g. in matters of 
correctness), but not for issues of
sub-optimality (e.g. HTTP cache being ignored).

BUG=611851
==========

[commit-bot: I haz the power, 2016-06-02 16:59:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-02 17:01:24]

Description was changed from

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we 
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to 
facilitate applications to try out Cronet. In order to 
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting 
any exceptions. We should only reserve the use of 
exceptions when absolutely necessary (e.g. in matters of 
correctness), but not for issues of
sub-optimality (e.g. HTTP cache being ignored).

BUG=611851
==========

to

==========
Make CronetHttpURLConnection#setConnectTimeout silently fail

This CL makes setConnectTimeout to fail silently so we
don't crash applications when this method is called.

The goal of Cronet's HttpURLConnection impl is to
facilitate applications to try out Cronet. In order to
do so, we would like it to be as easy as possible
to drop in Cronet's HttpURLConnection impl without hitting
any exceptions. We should only reserve the use of
exceptions when absolutely necessary (e.g. in matters of
correctness), but not for issues of
sub-optimality (e.g. HTTP cache being ignored).

BUG=611851

Committed: https://crrev.com/7130ccb5f2fa407a7dd54e221744978475d8b64e
Cr-Commit-Position: refs/heads/master@{#397450}
==========

[commit-bot: I haz the power, 2016-06-02 17:01:25]

Patchset 2 (id:??) landed as
https://crrev.com/7130ccb5f2fa407a7dd54e221744978475d8b64e
Cr-Commit-Position: refs/heads/master@{#397450}