Cronet Java wrappers to fallback to HttpUrlConnection if Cronet is not available.

Cronet Java wrappers to fallback to HttpUrlConnection on Pre-ICS platform.

Based on https://codereview.chromium.org/145213003/.

BUG=354143
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258517

[mef, 2014-02-27 22:52:37]

Hi, please take a look at migrated Java wrappers.

[mmenke, 2014-03-07 17:02:38]

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/UsedByReflection.java (right):

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/UsedByReflection.java:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Should again maintain the copyright years on these.

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/UsedByReflection.java:12: * components
that would otherwise be removed by Proguard. Use the value parameter to mention
a file
nit:  Fix indent.

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/UsedByReflection.java:26: 
This comment is practically illegible.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:25: private
long mSize;
Entire file should be using 4-space indent (Goes for a bunch of these files).

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:27: public
ChromiumUrlRequest(ChromiumUrlRequestContext requestContext, String url,
Line too long (Goes for a bunch of lines in these files).

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:28: int
priority, Map<String, String> headers, HttpUrlRequestListener listener) {
All lines with arguments should be lined up.First argument should

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:98: // The
server may ignore the request for a byte range
Suggestion adding a period.  Android style guide doesn't require it, but my
opinion is that where the android style guide doesn't prohibit it, we should
follow Google style.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:153: //
expecting 200 to indicate success, we need to fake it.
Add a TODO about investigating this - I'm not sure if our network stack does
this.  If it does, it's done at that cache layer...And we're thinking about
getting rid of that for the library.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
File
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java
(right):

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:19:

I believe that since these are all from the same library, there should be no
blank line.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:23:
//import java.io.FileOutputStream;
Remove comment.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:73:
public SampleHttpUrlRequestListener() {}
4 space indent.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:77:
Log.i(TAG, "****** Request Complete, status code is " + getHttpStatusCode());
4 space indent.

[dplotnikov, 2014-03-07 20:01:12]

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:19: private
long mOffset;
Do you believe we should keep this resumable download functionality here or
break it out into something done by the clients who need it?  I would be in
favor of the latter approach.

[mmenke, 2014-03-07 20:47:28]

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/UsedByReflection.java (right):

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/UsedByReflection.java:26: 
On 2014/03/07 17:02:39, mmenke wrote:
> This comment is practically illegible.

To clarify:  A bunch of lines are missing asterisks, it uses a random HTML line
break, and it's not consistently indented.  Even if all that were fixed, I'd
probably have no idea what it means, of course.

[mef, 2014-03-07 20:54:12]

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/UsedByReflection.java (right):

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/UsedByReflection.java:33: public
@interface UsedByReflection {
The |UsedByReflection| annotation has not been used in Chromium before.
Dmitry, do you know whether it is necessary (I did find that it doesn't work
without it), and whether it could be replaced by some other annotation like
|UsedByNative|
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:19: private
long mOffset;
On 2014/03/07 20:01:12, dplotnikov wrote:
> Do you believe we should keep this resumable download functionality here or
> break it out into something done by the clients who need it?  I would be in
> favor of the latter approach.
Good question. At this point my primary goal was to upstream existing
functionality and use it as a starting point.
I'm totally fine with keeping resumable downloads out of cronet.
I think it is worth deciding on API that Cronet will initially support, and your
wrappers seemed like a good start.
Opinions?

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:25: private
long mSize;
On 2014/03/07 17:02:39, mmenke wrote:
> Entire file should be using 4-space indent (Goes for a bunch of these files).
Yeah, is there some tool for that? 'git cl format' doesn't seem to format java.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:27: public
ChromiumUrlRequest(ChromiumUrlRequestContext requestContext, String url,
On 2014/03/07 17:02:39, mmenke wrote:
> Line too long (Goes for a bunch of lines in these files).
Roger that.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:98: // The
server may ignore the request for a byte range
On 2014/03/07 17:02:39, mmenke wrote:
> Suggestion adding a period.  Android style guide doesn't require it, but my
> opinion is that where the android style guide doesn't prohibit it, we should
> follow Google style.

Done. Agreed.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:153: //
expecting 200 to indicate success, we need to fake it.
On 2014/03/07 17:02:39, mmenke wrote:
> Add a TODO about investigating this - I'm not sure if our network stack does
> this.  If it does, it's done at that cache layer...And we're thinking about
> getting rid of that for the library.

Done.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
File
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java
(right):

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:19:

On 2014/03/07 17:02:39, mmenke wrote:
> I believe that since these are all from the same library, there should be no
> blank line.

Done.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:23:
//import java.io.FileOutputStream;
On 2014/03/07 17:02:39, mmenke wrote:
> Remove comment.

Done.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:73:
public SampleHttpUrlRequestListener() {}
On 2014/03/07 17:02:39, mmenke wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:77:
Log.i(TAG, "****** Request Complete, status code is " + getHttpStatusCode());
On 2014/03/07 17:02:39, mmenke wrote:
> 4 space indent.

Done.

[mef, 2014-03-07 20:55:35]

On 2014/03/07 20:54:12, mef wrote:
>
https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
> File base/android/java/src/org/chromium/base/UsedByReflection.java (right):
> 
>
https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
> base/android/java/src/org/chromium/base/UsedByReflection.java:33: public
> @interface UsedByReflection {
> The |UsedByReflection| annotation has not been used in Chromium before.
> Dmitry, do you know whether it is necessary (I did find that it doesn't work
> without it), and whether it could be replaced by some other annotation like
> |UsedByNative|
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
> File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
> (right):
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
> net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:19:
private
> long mOffset;
> On 2014/03/07 20:01:12, dplotnikov wrote:
> > Do you believe we should keep this resumable download functionality here or
> > break it out into something done by the clients who need it?  I would be in
> > favor of the latter approach.
> Good question. At this point my primary goal was to upstream existing
> functionality and use it as a starting point.
> I'm totally fine with keeping resumable downloads out of cronet.
> I think it is worth deciding on API that Cronet will initially support, and
your
> wrappers seemed like a good start.
> Opinions?
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
> net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:25:
private
> long mSize;
> On 2014/03/07 17:02:39, mmenke wrote:
> > Entire file should be using 4-space indent (Goes for a bunch of these
files).
> Yeah, is there some tool for that? 'git cl format' doesn't seem to format
java.
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
> net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:27:
public
> ChromiumUrlRequest(ChromiumUrlRequestContext requestContext, String url,
> On 2014/03/07 17:02:39, mmenke wrote:
> > Line too long (Goes for a bunch of lines in these files).
> Roger that.
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
> net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:98: //
The
> server may ignore the request for a byte range
> On 2014/03/07 17:02:39, mmenke wrote:
> > Suggestion adding a period.  Android style guide doesn't require it, but my
> > opinion is that where the android style guide doesn't prohibit it, we should
> > follow Google style.
> 
> Done. Agreed.
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
> net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:153: //
> expecting 200 to indicate success, we need to fake it.
> On 2014/03/07 17:02:39, mmenke wrote:
> > Add a TODO about investigating this - I'm not sure if our network stack does
> > this.  If it does, it's done at that cache layer...And we're thinking about
> > getting rid of that for the library.
> 
> Done.
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
> File
>
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java
> (right):
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
>
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:19:
> 
> On 2014/03/07 17:02:39, mmenke wrote:
> > I believe that since these are all from the same library, there should be no
> > blank line.
> 
> Done.
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
>
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:23:
> //import java.io.FileOutputStream;
> On 2014/03/07 17:02:39, mmenke wrote:
> > Remove comment.
> 
> Done.
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
>
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:73:
> public SampleHttpUrlRequestListener() {}
> On 2014/03/07 17:02:39, mmenke wrote:
> > 4 space indent.
> 
> Done.
> 
>
https://codereview.chromium.org/183333002/diff/1/net/cronet/android/sample/sr...
>
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java:77:
> Log.i(TAG, "****** Request Complete, status code is " + getHttpStatusCode());
> On 2014/03/07 17:02:39, mmenke wrote:
> > 4 space indent.
> 
> Done.

It seems that my rebase went terribly wrong, I'm looking into it.

[Charles, 2014-03-07 21:31:14]

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/UsedByReflection.java (right):

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/UsedByReflection.java:26: 
You can actually get rid of these annotations, chromium has its own that serve
the same purpose (configuring proguard)

On 2014/03/07 20:47:28, mmenke wrote:
> On 2014/03/07 17:02:39, mmenke wrote:
> > This comment is practically illegible.
> 
> To clarify:  A bunch of lines are missing asterisks, it uses a random HTML
line
> break, and it's not consistently indented.  Even if all that were fixed, I'd
> probably have no idea what it means, of course.

https://codereview.chromium.org/183333002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/UsedByReflection.java:33: public
@interface UsedByReflection {
These should probably be replaced by Chromium's proguard annotations.

On 2014/03/07 20:54:12, mef wrote:
> The |UsedByReflection| annotation has not been used in Chromium before.
> Dmitry, do you know whether it is necessary (I did find that it doesn't work
> without it), and whether it could be replaced by some other annotation like
> |UsedByNative|
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...

[mef, 2014-03-14 20:03:24]

Hi, PTAL. I've applied Chromium (Android) Java formatting.

Also, it seems that Chromium on Android now uses ICU as a data file:
https://codereview.chromium.org/156333002/

Unfortunately that seems to cause ninja warning:
'multiple rules generate icudtl.dat. builds involving this target will not be
correct' 

Indeed Cronet Sample crashes on startup...

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/183333002/diff/1/net/cronet/android/java/src/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:25: private
long mSize;
On 2014/03/07 17:02:39, mmenke wrote:
> Entire file should be using 4-space indent (Goes for a bunch of these files).

Done.

[dplotnikov, 2014-03-17 18:01:00]

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:27: private static
final class ContextLock {
You don't have to use a new class for a lock. Traditionally, Object has been
used for that purpose.

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:41: public static
final int REQUEST_PRIORITY_HIGHEST = 4;
These should be code-genned from a common source.

[mef, 2014-03-17 18:19:05]

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:23:
public static synchronized ChromiumUrlRequestContext getInstance(
It seems that in several classes 'synchronized' static method is used to protect
access to static variable ('sInstance' in this case).
Could it be achieved by using an explicit static lock?

like:

---
private static Object sLock;

...
synchronized (sLock) {

}

---

If so, what's a good place to create the new sLock to avoid running into same
concurrency issues??

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:27: private static
final class ContextLock {
On 2014/03/17 18:01:01, dplotnikov wrote:
> You don't have to use a new class for a lock. Traditionally, Object has been
> used for that purpose.

Sounds good.

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:41: public static
final int REQUEST_PRIORITY_HIGHEST = 4;
On 2014/03/17 18:01:01, dplotnikov wrote:
> These should be code-genned from a common source.
Agreed. I am looking into using chromium jni generator to replace manually
created bindings.

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:95: private final
ContextLock mLock;
Is this a valid pattern to use a designated member variable for locking
purposes?

[dplotnikov, 2014-03-17 18:32:16]

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:23:
public static synchronized ChromiumUrlRequestContext getInstance(
If the concern is about performance of the lock, you may want to consider
double-checked locking:

http://stackoverflow.com/questions/1625118/java-double-checked-locking


On 2014/03/17 18:19:05, mef wrote:
> It seems that in several classes 'synchronized' static method is used to
protect
> access to static variable ('sInstance' in this case).
> Could it be achieved by using an explicit static lock?
> 
> like:
> 
> ---
> private static Object sLock;
> 
> ...
> synchronized (sLock) {
> 
> }
> 
> ---
> 
> If so, what's a good place to create the new sLock to avoid running into same
> concurrency issues??

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:95: private final
ContextLock mLock;
Absolutely.

On 2014/03/17 18:19:05, mef wrote:
> Is this a valid pattern to use a designated member variable for locking
> purposes?

[mef, 2014-03-17 18:50:18]

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:23:
public static synchronized ChromiumUrlRequestContext getInstance(
On 2014/03/17 18:32:17, dplotnikov wrote:
> If the concern is about performance of the lock, you may want to consider
> double-checked locking:
> 
> http://stackoverflow.com/questions/1625118/java-double-checked-locking
> 
> 
> On 2014/03/17 18:19:05, mef wrote:
> > It seems that in several classes 'synchronized' static method is used to
> protect
> > access to static variable ('sInstance' in this case).
> > Could it be achieved by using an explicit static lock?
> > 
> > like:
> > 
> > ---
> > private static Object sLock;
> > 
> > ...
> > synchronized (sLock) {
> > 
> > }
> > 
> > ---
> > 
> > If so, what's a good place to create the new sLock to avoid running into
same
> > concurrency issues?? 
> 
I don't think performance is a factor in this case as in all invocations after
initial it will skip sInstance initialization and should not have any meaningful
implications.

I think chromium 'findbugs' script may be overly sensitive to potentially
dangerous patterns, but it fails the build. :-(

Are there any problems with using explicit static lock? 

private static final Object sLock = new Object();
synchronized (sLock) {
}

This should make 'checkbugs' happy and work the same way as original code,
right?

[dplotnikov, 2014-03-17 23:51:18]

On 2014/03/17 18:50:18, mef wrote:
>
https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
> File
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
> (right):
> 
>
https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
>
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:23:
> public static synchronized ChromiumUrlRequestContext getInstance(
> On 2014/03/17 18:32:17, dplotnikov wrote:
> > If the concern is about performance of the lock, you may want to consider
> > double-checked locking:
> > 
> > http://stackoverflow.com/questions/1625118/java-double-checked-locking
> > 
> > 
> > On 2014/03/17 18:19:05, mef wrote:
> > > It seems that in several classes 'synchronized' static method is used to
> > protect
> > > access to static variable ('sInstance' in this case).
> > > Could it be achieved by using an explicit static lock?
> > > 
> > > like:
> > > 
> > > ---
> > > private static Object sLock;
> > > 
> > > ...
> > > synchronized (sLock) {
> > > 
> > > }
> > > 
> > > ---
> > > 
> > > If so, what's a good place to create the new sLock to avoid running into
> same
> > > concurrency issues?? 
> > 
> I don't think performance is a factor in this case as in all invocations after
> initial it will skip sInstance initialization and should not have any
meaningful
> implications.
> 
> I think chromium 'findbugs' script may be overly sensitive to potentially
> dangerous patterns, but it fails the build. :-(
> 
> Are there any problems with using explicit static lock? 
> 
> private static final Object sLock = new Object();
> synchronized (sLock) {
> }
> 
> This should make 'checkbugs' happy and work the same way as original code,
> right?

A static lock will work beautifully.

[mef, 2014-03-17 23:57:30]

On 2014/03/17 23:51:18, dplotnikov wrote:
> On 2014/03/17 18:50:18, mef wrote:
> >
>
https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
> > File
> net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/183333002/diff/80001/net/cronet/android/java/...
> >
>
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:23:
> > public static synchronized ChromiumUrlRequestContext getInstance(
> > On 2014/03/17 18:32:17, dplotnikov wrote:
> > > If the concern is about performance of the lock, you may want to consider
> > > double-checked locking:
> > > 
> > > http://stackoverflow.com/questions/1625118/java-double-checked-locking
> > > 
> > > 
> > > On 2014/03/17 18:19:05, mef wrote:
> > > > It seems that in several classes 'synchronized' static method is used to
> > > protect
> > > > access to static variable ('sInstance' in this case).
> > > > Could it be achieved by using an explicit static lock?
> > > > 
> > > > like:
> > > > 
> > > > ---
> > > > private static Object sLock;
> > > > 
> > > > ...
> > > > synchronized (sLock) {
> > > > 
> > > > }
> > > > 
> > > > ---
> > > > 
> > > > If so, what's a good place to create the new sLock to avoid running into
> > same
> > > > concurrency issues?? 
> > > 
> > I don't think performance is a factor in this case as in all invocations
after
> > initial it will skip sInstance initialization and should not have any
> meaningful
> > implications.
> > 
> > I think chromium 'findbugs' script may be overly sensitive to potentially
> > dangerous patterns, but it fails the build. :-(
> > 
> > Are there any problems with using explicit static lock? 
> > 
> > private static final Object sLock = new Object();
> > synchronized (sLock) {
> > }
> > 
> > This should make 'checkbugs' happy and work the same way as original code,
> > right?
> 
> A static lock will work beautifully.

Excellent, PTAL. I'm not sure why trybots are not going anywhere, but I think
I've addressed the 'findbugs' warnings.

[mef, 2014-03-18 18:15:11]

+nyquist as OWNER of base/android/java/src/org/chromium/base/

Tommy, is it OK to add a 'UsedByReflection.java' annotation to base/ for
Android?

Dmitry/Charles, is there some other way to create  ChromiumUrlRequestFactory
without using reflection?

[dplotnikov, 2014-03-18 18:40:41]

On 2014/03/18 18:15:11, mef wrote:
> +nyquist as OWNER of base/android/java/src/org/chromium/base/
> 
> Tommy, is it OK to add a 'UsedByReflection.java' annotation to base/ for
> Android?
> 
> Dmitry/Charles, is there some other way to create  ChromiumUrlRequestFactory
> without using reflection?

An alternative is to have two different implementations of HttpUrlRequestFactory
and actually build two different jars: one with the dependency on Chromium and
one without.

[mef, 2014-03-20 01:54:25]

Hi guys,

PTAL. Per yfriedman@ suggestion I've moved UsedByReflection annotation into
cronet package and addressed couple of presubmit warnings.

At this point I would like to land it as a base for further java interface work.

Please let me know if you have any comments/objections.

thanks,
-m

[mmenke, 2014-03-20 15:58:12]

LGTM

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:38:
context.getApplicationContext(), UserAgent.from(context),
nit:  Line over 80 characters

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:123:
"HttpUrlConnection #" + mCount.getAndIncrement());
nit:  Line over 80 characters

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:292:

nit:  -blank line

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:293:
@Override
nit:  -4 indent.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:24:
String CHROMIUM_URL_REQUEST_FACTORY =
"org.chromium.net.ChromiumUrlRequestFactory";
nit:  Line too long.  Maybe move "String CHROMIUM_URL_REQUEST_FACTORY = " up to
the previous line?

There are also some other long lines in this file...May not be able to do
anything with some of them (The 4 space indent + 80 chars per limit don't work
well together), but may be able to improve a couple of them.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/HttpUrlRequestListener.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlRequestListener.java:11: 
nit:  Remove blank line.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/ResponseTooLargeException.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/ResponseTooLargeException.java:15: 
nit:  Remove blank line.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:41: public static
final int REQUEST_PRIORITY_HIGHEST = 4;
Don't think we need blank lines between these.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:56: private static
final int UPLOAD_BYTE_BUFFER_SIZE = 32768;
Don't think we need blank lines between these.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:95: private final
ContextLock mLock;
Don't think we need blank lines between these.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/UsedByReflection.java (right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UsedByReflection.java:13: * that
calls this method.
nit:  long lines.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java:46:
&& SystemClock.uptimeMillis() - startTime < maxTimeoutMs) {
nit:  operators are generally placed on the previous line in Chromium, and the
Android style guide does not suggest otherwise.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSamplePreconditionsTest.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSamplePreconditionsTest.java:19:
"TestInfrastructure" })
nit:  Fit on single line?

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:20:
"Main" })
nit:  Fit on single line?  (x3)

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:57:
URL, commandLineArgs);
nit:  Long line.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:29:
"CronetSampleApplication.initializeApplicationParameters() success.");
nit:  -2 indent?

[mef, 2014-03-20 18:01:04]

Thanks, Matt!

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:38:
context.getApplicationContext(), UserAgent.from(context),
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Line over 80 characters

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:123:
"HttpUrlConnection #" + mCount.getAndIncrement());
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Line over 80 characters

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:292:

On 2014/03/20 15:58:12, mmenke wrote:
> nit:  -blank line

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequest.java:293:
@Override
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  -4 indent.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:24:
String CHROMIUM_URL_REQUEST_FACTORY =
"org.chromium.net.ChromiumUrlRequestFactory";
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Line too long.  Maybe move "String CHROMIUM_URL_REQUEST_FACTORY = " up
to
> the previous line?
> 
> There are also some other long lines in this file...May not be able to do
> anything with some of them (The 4 space indent + 80 chars per limit don't work
> well together), but may be able to improve a couple of them.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/HttpUrlRequestListener.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/HttpUrlRequestListener.java:11: 
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/ResponseTooLargeException.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/ResponseTooLargeException.java:15: 
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:41: public static
final int REQUEST_PRIORITY_HIGHEST = 4;
On 2014/03/20 15:58:12, mmenke wrote:
> Don't think we need blank lines between these.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:56: private static
final int UPLOAD_BYTE_BUFFER_SIZE = 32768;
On 2014/03/20 15:58:12, mmenke wrote:
> Don't think we need blank lines between these.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UrlRequest.java:95: private final
ContextLock mLock;
On 2014/03/20 15:58:12, mmenke wrote:
> Don't think we need blank lines between these.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
File net/cronet/android/java/src/org/chromium/net/UsedByReflection.java (right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/java...
net/cronet/android/java/src/org/chromium/net/UsedByReflection.java:13: * that
calls this method.
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  long lines.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CriteriaHelper.java:46:
&& SystemClock.uptimeMillis() - startTime < maxTimeoutMs) {
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  operators are generally placed on the previous line in Chromium, and the
> Android style guide does not suggest otherwise.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSamplePreconditionsTest.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSamplePreconditionsTest.java:19:
"TestInfrastructure" })
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Fit on single line?

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:20:
"Main" })
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Fit on single line?  (x3)

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:57:
URL, commandLineArgs);
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  Long line.

Done.

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
File
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java
(right):

https://codereview.chromium.org/183333002/diff/120001/net/cronet/android/samp...
net/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleApplication.java:29:
"CronetSampleApplication.initializeApplicationParameters() success.");
On 2014/03/20 15:58:12, mmenke wrote:
> nit:  -2 indent?

Done.

[mef, 2014-03-20 18:26:14]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:28:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/183333002/160001

[commit-bot: I haz the power, 2014-03-20 18:35:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:35:57]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[mef, 2014-03-20 21:20:04]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:20:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/183333002/160001

[commit-bot: I haz the power, 2014-03-21 11:12:33]

Change committed as 258517