Cronet API Refactoring

Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the API layer and the implementation. This is a step toward a pluggable architecture that would allow loading the implementation independently from the API. E.g., this would potentially allow having multiple Cronet implementation and load them using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given functionality/features are experimental and can be changed in the future. This replaces hidden javadoc methods that can still be visible in Android Studio and therefore used by the developers without realization that this is the experimental API. The developer has to explicitly cast a class to the experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

4. Added auto-generated files to srcjars.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
Committed: https://crrev.com/aa8f338c7107e3d45279e4f62e6145478964ab1c
Cr-Commit-Position: refs/heads/master@{#428390}

[kapishnikov, 2016-09-14 22:08:00]

Description was changed from

==========
Cronet API Refactoring
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Removed deprecated HttpUrlConnection API. Affected files:

Java Implementation:
ChromiumUrlRequest.java
ChromiumUrlRequestContext.java
ChromiumUrlRequestFactory.java
ChunkedWritableByteChannel.java
HttpUrlConnectionUrlRequest.java
HttpUrlConnectionUrlRequestFactory.java
HttpUrlRequest.java
HttpUrlRequestFactory.java
HttpUrlRequestListener.java
ResponseTooLargeException.java

Java Tests:
ChromiumUrlRequestTest.java
ChunkedWritableByteChannelTest.java
ContextInitTest.java
CronetUrlTest.java
HttpUrlRequestFactoryTest.java
TestHttpUrlRequestListener.java
UploadTest.java

C++ Layer:
chromium_url_request.cc
chromium_url_request.h
chromium_url_request_context.cc
chromium_url_request_context.h


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?
==========

[kapishnikov, 2016-09-14 22:08:01]

kapishnikov@chromium.org changed reviewers:
+ mef@chromium.org, pauljensen@chromium.org

[pauljensen, 2016-09-15 12:10:39]

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:300: ":load_states_list",
you're going to move this down to cronet_java, right?

[kapishnikov, 2016-09-15 14:38:56]

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:300: ":load_states_list",
On 2016/09/15 12:10:39, pauljensen wrote:
> you're going to move this down to cronet_java, right?

Yes. Good catch.

[kapishnikov, 2016-09-15 15:07:40]

The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 15:07:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 15:17:17]

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

[commit-bot: I haz the power, 2016-09-15 15:17:18]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pauljensen, 2016-09-20 19:01:38]

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:300: ":load_states_list",
On 2016/09/15 14:38:56, kapishnikov wrote:
> On 2016/09/15 12:10:39, pauljensen wrote:
> > you're going to move this down to cronet_java, right?
> 
> Yes. Good catch.

Still waiting on this.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:14:
* Created using {@link BidirectionalStream.Builder}.
This needs to be updated.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:19:
public abstract class BidirectionalStream {
can this now be an interface?

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:24:
public abstract static class Builder {
ditto

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:19:
public abstract class ExperimentalCronetEngine extends CronetEngine {
can this be an interface?

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:22:
* {@hide} since this class exposes experimental features that should be hidden.
Is the inner @hide necessary?  I wouldn't have thought so

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:81:
public abstract BidirectionalStream.Builder newBidirectionalStreamBuilder(
this needs a comment

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:81:
public abstract BidirectionalStream.Builder newBidirectionalStreamBuilder(
this should either:
1. be moved to CronetEngine, or
2. BiDirectionalStream should be @hide'd

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:171:
* TODO(pauljensen): Expose once implemented, http://crbug.com/418111
Move this comment to just below the "*/" and use a "//" comment.  This way if it
gets unhidden, we don't end up with a TODO in the javadocs.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:207:
public abstract UrlRequest.Builder newUrlRequestBuilder(
I think this should be removed, it's redundant with the one in the parent class

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:13:
public interface ICronetEngineBuilder {
as discussed, all experimental methods might want to have default impls so we
don't have to support them forever

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:12: * Created
using {@link UrlRequest.Builder}.
This needs to be updated.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:15: public
abstract class UrlRequest {
Can this now be an interface?

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:20: public
abstract static class Builder {
ditto

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:20:
public abstract class UrlRequestBase extends ExperimentalUrlRequest {
can this be an interface?

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:98:
*/
all these comments should be removed and replaced with simply @Override

[pauljensen, 2016-09-21 11:15:02]

Andrei, can you please rebase this when you have a chance.  It's such a big
change I'd like to start basing some other patches off it and it has merge
conflicts.

[pauljensen, 2016-09-21 12:12:03]

Please add BUG=629299 line to CL description

[kapishnikov, 2016-09-21 17:26:44]

Description was changed from

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Removed deprecated HttpUrlConnection API. Affected files:

Java Implementation:
ChromiumUrlRequest.java
ChromiumUrlRequestContext.java
ChromiumUrlRequestFactory.java
ChunkedWritableByteChannel.java
HttpUrlConnectionUrlRequest.java
HttpUrlConnectionUrlRequestFactory.java
HttpUrlRequest.java
HttpUrlRequestFactory.java
HttpUrlRequestListener.java
ResponseTooLargeException.java

Java Tests:
ChromiumUrlRequestTest.java
ChunkedWritableByteChannelTest.java
ContextInitTest.java
CronetUrlTest.java
HttpUrlRequestFactoryTest.java
TestHttpUrlRequestListener.java
UploadTest.java

C++ Layer:
chromium_url_request.cc
chromium_url_request.h
chromium_url_request_context.cc
chromium_url_request_context.h


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Removed deprecated HttpUrlConnection API. Affected files:

Java Implementation:
ChromiumUrlRequest.java
ChromiumUrlRequestContext.java
ChromiumUrlRequestFactory.java
ChunkedWritableByteChannel.java
HttpUrlConnectionUrlRequest.java
HttpUrlConnectionUrlRequestFactory.java
HttpUrlRequest.java
HttpUrlRequestFactory.java
HttpUrlRequestListener.java
ResponseTooLargeException.java

Java Tests:
ChromiumUrlRequestTest.java
ChunkedWritableByteChannelTest.java
ContextInitTest.java
CronetUrlTest.java
HttpUrlRequestFactoryTest.java
TestHttpUrlRequestListener.java
UploadTest.java

C++ Layer:
chromium_url_request.cc
chromium_url_request.h
chromium_url_request_context.cc
chromium_url_request_context.h


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

[kapishnikov, 2016-09-21 17:49:51]

On 2016/09/21 11:15:02, pauljensen wrote:
> Andrei, can you please rebase this when you have a chance.  It's such a big
> change I'd like to start basing some other patches off it and it has merge
> conflicts.

Rebased

[kapishnikov, 2016-09-22 02:56:17]

The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 02:56:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 02:59:24]

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

[commit-bot: I haz the power, 2016-09-22 02:59:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kapishnikov, 2016-09-22 03:04:36]

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:300: ":load_states_list",
On 2016/09/20 19:01:36, pauljensen wrote:
> On 2016/09/15 14:38:56, kapishnikov wrote:
> > On 2016/09/15 12:10:39, pauljensen wrote:
> > > you're going to move this down to cronet_java, right?
> > 
> > Yes. Good catch.
> 
> Still waiting on this.

Paul, I have removed the dependencies on the generated files except
CronetApiVersion.java. The change is rebased onto
624c32473d6d601e104bca2d91d68a0fa0266c0e commit, the commit you were also
waiting for (Cronet API Javadoc). There are changes after that commit that cause
a decent amount of merge conflicts. All tests pass locally. They don't pass on
the bot because of the merge conflicts.

[pauljensen, 2016-09-22 11:45:30]

On 2016/09/22 03:04:36, kapishnikov wrote:
>
https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
> File components/cronet/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
> components/cronet/android/BUILD.gn:300: ":load_states_list",
> On 2016/09/20 19:01:36, pauljensen wrote:
> > On 2016/09/15 14:38:56, kapishnikov wrote:
> > > On 2016/09/15 12:10:39, pauljensen wrote:
> > > > you're going to move this down to cronet_java, right?
> > > 
> > > Yes. Good catch.
> > 
> > Still waiting on this.
> 
> Paul, I have removed the dependencies on the generated files except
> CronetApiVersion.java. The change is rebased onto
> 624c32473d6d601e104bca2d91d68a0fa0266c0e commit, the commit you were also
> waiting for (Cronet API Javadoc). There are changes after that commit that
cause
> a decent amount of merge conflicts. All tests pass locally. They don't pass on
> the bot because of the merge conflicts.

Ok, I'm waiting on you addressing the merge conflicts and my previous comments,
before I review again.

[pauljensen, 2016-09-22 17:34:32]

https://codereview.chromium.org/2339223002/diff/100001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/100001/components/cronet/andr...
components/cronet/android/BUILD.gn:303: srcjar_deps = [
":cronet_api_version_srcjar" ]
can we create a new top level variable like so:
  cronet_api_srcjar_deps = [ ":cronet_api_version_srcjar" ]
and then here say:
  srcjar_deps = cronet_api_srcjar_deps
and also do the same in jar_cronet_api_source target?

https://codereview.chromium.org/2339223002/diff/100001/components/cronet/andr...
components/cronet/android/BUILD.gn:869: srcjar_deps = [
":effective_connection_type_java" ]
I don't think effective_connection_type_java should be in the API srcjar as it's
not in the API anymore.  Anyhow, as I recommended above we could make this
simply:
 srcjar_deps = cronet_api_srcjar_deps

[kapishnikov, 2016-09-22 21:29:07]

The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 21:29:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kapishnikov, 2016-09-22 21:32:04]

Paul, thanks for the review. I think I have address all the comments except the
"interface vs abstract class" one (see below).

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:14:
* Created using {@link BidirectionalStream.Builder}.
On 2016/09/20 19:01:37, pauljensen wrote:
> This needs to be updated.

Done. Also added {@hide} since this is an experimental class.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:19:
public abstract class BidirectionalStream {
On 2016/09/20 19:01:37, pauljensen wrote:
> can this now be an interface?

Making it an interface will prohibit us from adding new methods in the future
without potentially breaking the client code. E.g., a client may wrap
BidirectionalStream methods in order to add some logging, like this:

class VerboseBidirectionalStream implements BidirectionalStream {
  void start() {
    Log("Start");
    delegate.start();
  }
  ...
}

If we add a new method to BidirectionalStream, VerboseBidirectionalStream won't
compile.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:24:
public abstract static class Builder {
On 2016/09/20 19:01:36, pauljensen wrote:
> ditto

Same reason.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:19:
public abstract class ExperimentalCronetEngine extends CronetEngine {
On 2016/09/20 19:01:37, pauljensen wrote:
> can this be an interface?

See the previous comment.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:22:
* {@hide} since this class exposes experimental features that should be hidden.
On 2016/09/20 19:01:37, pauljensen wrote:
> Is the inner @hide necessary?  I wouldn't have thought so

Removed.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:81:
public abstract BidirectionalStream.Builder newBidirectionalStreamBuilder(
On 2016/09/20 19:01:37, pauljensen wrote:
> this should either:
> 1. be moved to CronetEngine, or
> 2. BiDirectionalStream should be @hide'd

Made BidirectionalStream hidden since it is considered to be experimental for
now.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:81:
public abstract BidirectionalStream.Builder newBidirectionalStreamBuilder(
On 2016/09/20 19:01:37, pauljensen wrote:
> this needs a comment

Done.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:171:
* TODO(pauljensen): Expose once implemented, http://crbug.com/418111
On 2016/09/20 19:01:37, pauljensen wrote:
> Move this comment to just below the "*/" and use a "//" comment.  This way if
it
> gets unhidden, we don't end up with a TODO in the javadocs.

Good point. Done.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:207:
public abstract UrlRequest.Builder newUrlRequestBuilder(
On 2016/09/20 19:01:37, pauljensen wrote:
> I think this should be removed, it's redundant with the one in the parent
class

Done.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:13:
public interface ICronetEngineBuilder {
On 2016/09/20 19:01:37, pauljensen wrote:
> as discussed, all experimental methods might want to have default impls so we
> don't have to support them forever

That is a very good point. Changed it to an abstract class and added the default
implementation of the experimental method.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:12: * Created
using {@link UrlRequest.Builder}.
On 2016/09/20 19:01:37, pauljensen wrote:
> This needs to be updated.

Done.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:20:
public abstract class UrlRequestBase extends ExperimentalUrlRequest {
On 2016/09/20 19:01:37, pauljensen wrote:
> can this be an interface?

It could be an interface but I think we should keep ExperimentalUrlRequest an
abstract class.

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:98:
*/
On 2016/09/20 19:01:37, pauljensen wrote:
> all these comments should be removed and replaced with simply @Override

Done. Same for BidirectionalStreamBuilderImpl. Kept comments in
CronetEngineBuilderImpl.

https://codereview.chromium.org/2339223002/diff/100001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/100001/components/cronet/andr...
components/cronet/android/BUILD.gn:303: srcjar_deps = [
":cronet_api_version_srcjar" ]
On 2016/09/22 17:34:31, pauljensen wrote:
> can we create a new top level variable like so:
>   cronet_api_srcjar_deps = [ ":cronet_api_version_srcjar" ]
> and then here say:
>   srcjar_deps = cronet_api_srcjar_deps
> and also do the same in jar_cronet_api_source target?

Done.

https://codereview.chromium.org/2339223002/diff/100001/components/cronet/andr...
components/cronet/android/BUILD.gn:869: srcjar_deps = [
":effective_connection_type_java" ]
On 2016/09/22 17:34:31, pauljensen wrote:
> I don't think effective_connection_type_java should be in the API srcjar as
it's
> not in the API anymore.  Anyhow, as I recommended above we could make this
> simply:
>  srcjar_deps = cronet_api_srcjar_deps

Done.

[commit-bot: I haz the power, 2016-09-22 21:41:30]

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

[commit-bot: I haz the power, 2016-09-22 21:41:31]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pauljensen, 2016-09-23 18:56:45]

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:62: ":effective_connection_type_java",
this should go as it's not referenced from the javadocs anymore

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:63: ":url_request_error_java",
this should go too, I guess this whole target can go

[pauljensen, 2016-09-23 19:00:54]

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:50: //
Reference to  the actual builder implementation.
extra spaces after "to"

[kapishnikov, 2016-09-23 21:26:39]

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:62: ":effective_connection_type_java",
On 2016/09/23 18:56:45, pauljensen wrote:
> this should go as it's not referenced from the javadocs anymore

Done.

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:63: ":url_request_error_java",
On 2016/09/23 18:56:45, pauljensen wrote:
> this should go too, I guess this whole target can go

Done.

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/160001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:50: //
Reference to  the actual builder implementation.
On 2016/09/23 19:00:54, pauljensen wrote:
> extra spaces after "to"

Done.

[kapishnikov, 2016-09-23 21:32:20]

The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 21:32:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 22:33:14]

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

[commit-bot: I haz the power, 2016-09-23 22:33:16]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2016-09-26 14:51:22]

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:943: public
abstract void startNetLogToDisk(String dirPath, boolean logAll, int maxSize);
why did you hide this?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:24: public
abstract class CronetEngine {
should we put a big comment here (using // so it doesn't appear in javadocs)
about the rules of modifying the API?  or perhaps somewhere else?  I just think
it might be pertinent to mention that no new abstract methods can be added to
any of our APIs without dummy impls.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:186: *
@return the builder to facilitate chaining.
You are unhiding this.  I don't think we want to do that.  See my comment here:
https://codereview.chromium.org/2296793002/#msg4

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:334:
ICronetEngineBuilder getBuilderDelegate() {
If this is only for internal testing, can we move it to
ExperimentalCronetEngine.Builder to hide it better, and add @VisibleForTesting?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:461: *
Creates a builder for {@link UrlRequest} objects. All callbacks for
for->for the

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:463: *
{@code executor}'s thread. {@code executor} must not run tasks on the
thread->thread(s)

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:464: *
current thread to prevent blocking networking operations and causing
current thread->thread calling {@link Executor#execute}

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:467: *
@param url {@link java.net.URL} for the generated requests.
delinkify URL as it's not a java.net.URL

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:29:
}
So I've an idea:
We were wondering how to deal with the fact that Experimental Builders will
return the base Builder class from all setters.  What about using covariant
return types to address this?  We could @Override every Builder method in the
Experimental.Builder classes and make them return the Experimental.Builder
classes exploiting covariant return types.  This way clients only need to cast
the Builder to the Experimental.Builder class once.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:16:
* instance of this class.
should we mention that every instance of a CronetEngine can be casted to an
ExperimentalCronetEngine? or is that not going to be true?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:31:
public abstract Builder addRequestAnnotation(Object annotation);
how come this class doesn't have a build() method, like
ExperimentalCronetEngine.Builder?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:11:
* {@code CronetEngine.Builder} uses this interface to delegate the calls.
add comment mentioning that documentation is at identically named
CronetEngine.Builder methods

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:33:
// Experimental API methods.
mention that these methods must all have default implementations, as
experimental APIs may be remove in the future.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:48:
public abstract CronetEngine build();
perhaps move this up to the "Public API methods" section, as that's what it is.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
(left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: final
class JavaUrlRequest implements UrlRequest {
do we need a separate build target (i.e. a jar) containing JavaUrlRequest and
JavaCronetEngine?  I guess you'll know the answer when you try and convert our
existing embedders to use this new API.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:164: public
@interface RequestPriority {}
how come you removed this?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:415: public
@interface StatusValues {}
ditto?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:12: * Created
using {@link CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.
you replaced a type with a method that returns that type, I think you might want
to go back to the type as the method you reference does not create a UrlRequest
but the builder for it instead.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:13: * Note:
All methods must be called on the {@link Executor} passed in during creation.
in during creation->to {@link CronetEngine#newUrlRequestBuilder}

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:335:
protected Status() {}
why the transition to protected?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:60:
boolean disableCache, boolean disableConnectionMigration, boolean
allowDirectExecutor);
I'm not a fan of these odd functions that try and take every builder option as
an argument.  Seems ripe for an accidental argument order swap.  Also, the
JavaUrlRequest might not need many of these options.  I feel like we should
remove all these options and just offer getters on the Builders.  Perhaps not
for this CL.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:103:
CronetEngineBase possibleEngine = constructor.newInstance(builder);
eh what's the point of this?  I don't think we should need any reflection in the
impl.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:118:
*/
Add @Override to all these functions and remove comments

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:229:
*/
Remove comment on @Override method

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:38:
public final class JavaCronetEngine extends CronetEngineBase {
why public?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:89:
"The bidirectional stream is not supported by the Java implementation "
stream->stream API

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:51:
protected abstract void setUploadDataProvider(
why is this one protected but setHttpMethod and addHeader are public?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:57:
public static class Status extends UrlRequest.Status {
why this subclass?  why not just a static function on UrlRequestBase?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java:108:
builder.append(ApiVersion.CRONET_VERSION);
Change to ImplVersion and remove TODO

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java:28:
public CronetHttpURLStreamHandler(CronetEngineBase cronetEngine) {
why the change to CronetEngineBase?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:117:
@OnlyRunNativeCronet
perhaps we should run with the java-only impl and test that the appropriate
exception is thrown when newBidirectionalStreamBuilder is called?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:204:
public CronetEngineBase initCronetEngine() {
why does this return an impl class now?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:78:
public static CronetEngineBuilderImpl
getCronetEngineBuilderImpl(CronetEngine.Builder builder) {
is this used outside this class?  if not, can we make private?

[pauljensen, 2016-09-26 18:58:42]

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:13:
public abstract class ICronetEngineBuilder {
this should be @hide...otherwise it appears in javadocs

[pauljensen, 2016-09-27 12:52:32]

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:22: *
available on the current platform.
We need a sentence explaining how to create a CronetEngine, something like:
Created using {@link CronetEngine#Builder}.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:18: * with
{@link Builder#build}.
We need a sentence explaining how to create a UrlRequest.Builder, something
like:
Created by {@link CronetEngine#newUrlRequestBuilder}.

[pauljensen, 2016-09-27 14:42:26]

While we're changing all our interfaces to abstract classes, do we want to do
the same to UploadDataSink?  I think it's the only remaining interface.

[kapishnikov, 2016-09-27 18:38:26]

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:943: public
abstract void startNetLogToDisk(String dirPath, boolean logAll, int maxSize);
On 2016/09/26 14:51:21, pauljensen wrote:
> why did you hide this?

When I started the refactoring, the consensus was that this API is experimental.
Should we deprecate startNetLogToFile and replace it with this one?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:24: public
abstract class CronetEngine {
On 2016/09/26 14:51:21, pauljensen wrote:
> should we put a big comment here (using // so it doesn't appear in javadocs)
> about the rules of modifying the API?  or perhaps somewhere else?  I just
think
> it might be pertinent to mention that no new abstract methods can be added to
> any of our APIs without dummy impls.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:186: *
@return the builder to facilitate chaining.
On 2016/09/26 14:51:21, pauljensen wrote:
> You are unhiding this.  I don't think we want to do that.  See my comment
here:
> https://codereview.chromium.org/2296793002/#msg4

I was under impression that we are going to change the method name and make it
public; so, I kept it here as the placeholder.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:334:
ICronetEngineBuilder getBuilderDelegate() {
Good idea.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:461: *
Creates a builder for {@link UrlRequest} objects. All callbacks for
On 2016/09/26 14:51:21, pauljensen wrote:
> for->for the

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:463: *
{@code executor}'s thread. {@code executor} must not run tasks on the
On 2016/09/26 14:51:21, pauljensen wrote:
> thread->thread(s)

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:464: *
current thread to prevent blocking networking operations and causing
On 2016/09/26 14:51:21, pauljensen wrote:
> current thread->thread calling {@link Executor#execute}

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:467: *
@param url {@link java.net.URL} for the generated requests.
On 2016/09/26 14:51:21, pauljensen wrote:
> delinkify URL as it's not a java.net.URL

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:29:
}
On 2016/09/26 14:51:21, pauljensen wrote:
> So I've an idea:
> We were wondering how to deal with the fact that Experimental Builders will
> return the base Builder class from all setters.  What about using covariant
> return types to address this?  We could @Override every Builder method in the
> Experimental.Builder classes and make them return the Experimental.Builder
> classes exploiting covariant return types.  This way clients only need to cast
> the Builder to the Experimental.Builder class once.

Done. Same for ExperimentalCronetEngine & ExperimentalUrlRequest.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:16:
* instance of this class.
On 2016/09/26 14:51:21, pauljensen wrote:
> should we mention that every instance of a CronetEngine can be casted to an
> ExperimentalCronetEngine? or is that not going to be true?

In case of ExperimentalCronetEngine, we can change it without breaking the
implementation but we cannot do it for ExperimentalUrlRequest and
ExperimentalBidirectionalStream since the implementation directly inherits from
them.

Nevertheless, should we mention the experimental classes? We hide them in the
JavaDoc.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:31:
public abstract Builder addRequestAnnotation(Object annotation);
On 2016/09/26 14:51:21, pauljensen wrote:
> how come this class doesn't have a build() method, like
> ExperimentalCronetEngine.Builder?

build() is inherited from UrlRequest.Builder. The actual implementation of the
method is in UrlRequestBuilderImpl.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:11:
* {@code CronetEngine.Builder} uses this interface to delegate the calls.
On 2016/09/26 14:51:21, pauljensen wrote:
> add comment mentioning that documentation is at identically named
> CronetEngine.Builder methods

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:13:
public abstract class ICronetEngineBuilder {
On 2016/09/26 18:58:42, pauljensen wrote:
> this should be @hide...otherwise it appears in javadocs

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:33:
// Experimental API methods.
On 2016/09/26 14:51:21, pauljensen wrote:
> mention that these methods must all have default implementations, as
> experimental APIs may be remove in the future.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:48:
public abstract CronetEngine build();
On 2016/09/26 14:51:21, pauljensen wrote:
> perhaps move this up to the "Public API methods" section, as that's what it
is.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
(left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: final
class JavaUrlRequest implements UrlRequest {
On 2016/09/26 14:51:20, pauljensen wrote:
> do we need a separate build target (i.e. a jar) containing JavaUrlRequest and
> JavaCronetEngine?  I guess you'll know the answer when you try and convert our
> existing embedders to use this new API.

This is a good question that we should think about. We should be able to
repackage the classes the way that is suitable for the embedders. From the
architecture point of view JavaUrlRequest & JavaCronetEngine is one of the
UrlRequest & CronetEngine implementations and should be treated accordingly.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:164: public
@interface RequestPriority {}
On 2016/09/26 14:51:21, pauljensen wrote:
> how come you removed this?

I moved it to CronetEngineBase since it is used there. Should we make
@RequestPriority annotation public and add it to the Javadoc; othwrwise, it has
limited use in the API?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:12: * Created
using {@link CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.
On 2016/09/26 14:51:21, pauljensen wrote:
> you replaced a type with a method that returns that type, I think you might
want
> to go back to the type as the method you reference does not create a
UrlRequest
> but the builder for it instead.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:13: * Note:
All methods must be called on the {@link Executor} passed in during creation.
On 2016/09/26 14:51:21, pauljensen wrote:
> in during creation->to {@link CronetEngine#newUrlRequestBuilder}

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:18: * with
{@link Builder#build}.
On 2016/09/27 12:52:32, pauljensen wrote:
> We need a sentence explaining how to create a UrlRequest.Builder, something
> like:
> Created by {@link CronetEngine#newUrlRequestBuilder}.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:335:
protected Status() {}
On 2016/09/26 14:51:21, pauljensen wrote:
> why the transition to protected?

The status is defined in the public API (org.chromium.net pacgkage) but is
extended by UrlRequestBase.Status which is in the impl package. I am not really
sure if we should extend it.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:60:
boolean disableCache, boolean disableConnectionMigration, boolean
allowDirectExecutor);
On 2016/09/26 14:51:22, pauljensen wrote:
> I'm not a fan of these odd functions that try and take every builder option as
> an argument.  Seems ripe for an accidental argument order swap.  Also, the
> JavaUrlRequest might not need many of these options.  I feel like we should
> remove all these options and just offer getters on the Builders.  Perhaps not
> for this CL.

Acknowledged. I agree.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:103:
CronetEngineBase possibleEngine = constructor.newInstance(builder);
On 2016/09/26 14:51:22, pauljensen wrote:
> eh what's the point of this?  I don't think we should need any reflection in
the
> impl.

Agree. I will fix it.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:118:
*/
On 2016/09/26 14:51:22, pauljensen wrote:
> Add @Override to all these functions and remove comments

Should we add Javadocs to ICronetEngineBuilder in that case? Otherwise, Javadoc
for this class will not contain any documentation.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:229:
*/
On 2016/09/26 14:51:22, pauljensen wrote:
> Remove comment on @Override method

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:38:
public final class JavaCronetEngine extends CronetEngineBase {
On 2016/09/26 14:51:22, pauljensen wrote:
> why public?

We are referencing it from the tests that are in a different Java package. I
think we should move the tests to the impl package. This will improve the test
quality since the code that uses Cronet will be in a different package.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:89:
"The bidirectional stream is not supported by the Java implementation "
On 2016/09/26 14:51:22, pauljensen wrote:
> stream->stream API

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:51:
protected abstract void setUploadDataProvider(
On 2016/09/26 14:51:22, pauljensen wrote:
> why is this one protected but setHttpMethod and addHeader are public?

Changed to protected.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:57:
public static class Status extends UrlRequest.Status {
On 2016/09/26 14:51:22, pauljensen wrote:
> why this subclass?  why not just a static function on UrlRequestBase?

Agree. I will fix it.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java:108:
builder.append(ApiVersion.CRONET_VERSION);
On 2016/09/26 14:51:22, pauljensen wrote:
> Change to ImplVersion and remove TODO

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java:28:
public CronetHttpURLStreamHandler(CronetEngineBase cronetEngine) {
On 2016/09/26 14:51:22, pauljensen wrote:
> why the change to CronetEngineBase?

This is internal implementation. I think the less we reference the API classes
the better. Besides, we may need some internal CronetEngineBase methods that are
not exposed by the public API.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:117:
@OnlyRunNativeCronet
On 2016/09/26 14:51:22, pauljensen wrote:
> perhaps we should run with the java-only impl and test that the appropriate
> exception is thrown when newBidirectionalStreamBuilder is called?

I will add a new test.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:204:
public CronetEngineBase initCronetEngine() {
On 2016/09/26 14:51:22, pauljensen wrote:
> why does this return an impl class now?

This method is used by tests that access internal state of the engine that are
not exposed to the API.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:78:
public static CronetEngineBuilderImpl
getCronetEngineBuilderImpl(CronetEngine.Builder builder) {
On 2016/09/26 14:51:22, pauljensen wrote:
> is this used outside this class?  if not, can we make private?

Yes, it is used in CronetUrlRequestContextTest.

[pauljensen, 2016-09-27 19:08:10]

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:186: *
@return the builder to facilitate chaining.
On 2016/09/27 18:38:25, kapishnikov wrote:
> On 2016/09/26 14:51:21, pauljensen wrote:
> > You are unhiding this.  I don't think we want to do that.  See my comment
> here:
> > https://codereview.chromium.org/2296793002/#msg4
> 
> I was under impression that we are going to change the method name and make it
> public; so, I kept it here as the placeholder.

Consensus is that we'll hide this one (move to experimental).

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:16:
* instance of this class.
On 2016/09/27 18:38:25, kapishnikov wrote:
> On 2016/09/26 14:51:21, pauljensen wrote:
> > should we mention that every instance of a CronetEngine can be casted to an
> > ExperimentalCronetEngine? or is that not going to be true?
> 
> In case of ExperimentalCronetEngine, we can change it without breaking the
> implementation but we cannot do it for ExperimentalUrlRequest and
> ExperimentalBidirectionalStream since the implementation directly inherits
from
> them.
> 
> Nevertheless, should we mention the experimental classes? We hide them in the
> JavaDoc.

I think it couldn't hurt to mention...esp considering it's hidden anyhow.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:31:
public abstract Builder addRequestAnnotation(Object annotation);
On 2016/09/27 18:38:25, kapishnikov wrote:
> On 2016/09/26 14:51:21, pauljensen wrote:
> > how come this class doesn't have a build() method, like
> > ExperimentalCronetEngine.Builder?
> 
> build() is inherited from UrlRequest.Builder. The actual implementation of the
> method is in UrlRequestBuilderImpl.

Agreed to add it and make it abstract.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
(left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:37: final
class JavaUrlRequest implements UrlRequest {
On 2016/09/27 18:38:25, kapishnikov wrote:
> On 2016/09/26 14:51:20, pauljensen wrote:
> > do we need a separate build target (i.e. a jar) containing JavaUrlRequest
and
> > JavaCronetEngine?  I guess you'll know the answer when you try and convert
our
> > existing embedders to use this new API.
> 
> This is a good question that we should think about. We should be able to
> repackage the classes the way that is suitable for the embedders. From the
> architecture point of view JavaUrlRequest & JavaCronetEngine is one of the
> UrlRequest & CronetEngine implementations and should be treated accordingly.

Agreed, we'll add the new build target later if there is demand.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:335:
protected Status() {}
On 2016/09/27 18:38:26, kapishnikov wrote:
> On 2016/09/26 14:51:21, pauljensen wrote:
> > why the transition to protected?
> 
> The status is defined in the public API (http://org.chromium.net pacgkage) but
is
> extended by UrlRequestBase.Status which is in the impl package. I am not
really
> sure if we should extend it.

Maybe just not extend Status in the impl.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:118:
*/
On 2016/09/27 18:38:26, kapishnikov wrote:
> On 2016/09/26 14:51:22, pauljensen wrote:
> > Add @Override to all these functions and remove comments
> 
> Should we add Javadocs to ICronetEngineBuilder in that case? Otherwise,
Javadoc
> for this class will not contain any documentation.

Definitly add @Override; I'd prefer no docs, perhaps consult style guide.

[kapishnikov, 2016-09-27 23:27:18]

On 2016/09/27 14:42:26, pauljensen wrote:
> While we're changing all our interfaces to abstract classes, do we want to do
> the same to UploadDataSink?  I think it's the only remaining interface.

Done.

[kapishnikov, 2016-09-28 00:14:12]

The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 00:14:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kapishnikov, 2016-09-28 00:20:50]

Paul, thanks for the thorough review.

Still pending:
1) Remove reflection in CronetEngineBase.
2) New unit test for JavaCronetEngine.newBidirectionalStreamBuilder().

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:22: *
available on the current platform.
On 2016/09/27 12:52:32, pauljensen wrote:
> We need a sentence explaining how to create a CronetEngine, something like:
> Created using {@link CronetEngine#Builder}.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:186: *
@return the builder to facilitate chaining.
On 2016/09/27 19:08:09, pauljensen wrote:
> On 2016/09/27 18:38:25, kapishnikov wrote:
> > On 2016/09/26 14:51:21, pauljensen wrote:
> > > You are unhiding this.  I don't think we want to do that.  See my comment
> > here:
> > > https://codereview.chromium.org/2296793002/#msg4
> > 
> > I was under impression that we are going to change the method name and make
it
> > public; so, I kept it here as the placeholder.
> 
> Consensus is that we'll hide this one (move to experimental).

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:16:
* instance of this class.
On 2016/09/27 19:08:09, pauljensen wrote:
> On 2016/09/27 18:38:25, kapishnikov wrote:
> > On 2016/09/26 14:51:21, pauljensen wrote:
> > > should we mention that every instance of a CronetEngine can be casted to
an
> > > ExperimentalCronetEngine? or is that not going to be true?
> > 
> > In case of ExperimentalCronetEngine, we can change it without breaking the
> > implementation but we cannot do it for ExperimentalUrlRequest and
> > ExperimentalBidirectionalStream since the implementation directly inherits
> from
> > them.
> > 
> > Nevertheless, should we mention the experimental classes? We hide them in
the
> > JavaDoc.
> 
> I think it couldn't hurt to mention...esp considering it's hidden anyhow.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:31:
public abstract Builder addRequestAnnotation(Object annotation);
On 2016/09/27 19:08:09, pauljensen wrote:
> On 2016/09/27 18:38:25, kapishnikov wrote:
> > On 2016/09/26 14:51:21, pauljensen wrote:
> > > how come this class doesn't have a build() method, like
> > > ExperimentalCronetEngine.Builder?
> > 
> > build() is inherited from UrlRequest.Builder. The actual implementation of
the
> > method is in UrlRequestBuilderImpl.
> 
> Agreed to add it and make it abstract.

Done. Same for ExperimentalBidirectionalStream.Builder

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:335:
protected Status() {}
On 2016/09/27 19:08:09, pauljensen wrote:
> On 2016/09/27 18:38:26, kapishnikov wrote:
> > On 2016/09/26 14:51:21, pauljensen wrote:
> > > why the transition to protected?
> > 
> > The status is defined in the public API (http://org.chromium.net pacgkage)
but
> is
> > extended by UrlRequestBase.Status which is in the impl package. I am not
> really
> > sure if we should extend it.
> 
> Maybe just not extend Status in the impl.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:118:
*/
On 2016/09/27 19:08:09, pauljensen wrote:
> On 2016/09/27 18:38:26, kapishnikov wrote:
> > On 2016/09/26 14:51:22, pauljensen wrote:
> > > Add @Override to all these functions and remove comments
> > 
> > Should we add Javadocs to ICronetEngineBuilder in that case? Otherwise,
> Javadoc
> > for this class will not contain any documentation.
> 
> Definitly add @Override; I'd prefer no docs, perhaps consult style guide.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:57:
public static class Status extends UrlRequest.Status {
On 2016/09/27 18:38:26, kapishnikov wrote:
> On 2016/09/26 14:51:22, pauljensen wrote:
> > why this subclass?  why not just a static function on UrlRequestBase?
> 
> Agree. I will fix it.

Done.

[commit-bot: I haz the power, 2016-09-28 01:30:33]

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

[commit-bot: I haz the power, 2016-09-28 01:30:35]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2016-09-30 12:25:43]

Can you update the CL description to remove the part about removing the legacy
API?

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(left):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:943: public
abstract void startNetLogToDisk(String dirPath, boolean logAll, int maxSize);
On 2016/09/27 18:38:25, kapishnikov wrote:
> On 2016/09/26 14:51:21, pauljensen wrote:
> > why did you hide this?
> 
> When I started the refactoring, the consensus was that this API is
experimental.

Ok, makes sense.

> Should we deprecate startNetLogToFile and replace it with this one?

Not in this CL.

[pauljensen, 2016-09-30 13:19:25]

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:15:
* Executor)}.
nit: you can remove the "(String, Callback, Executor)" stuff

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:31: //
NOTE: In order to avoid breaking the existing API clients, all future methods
added to
NOTE->NOTE(kapishnikov)

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:54:
protected final ICronetEngineBuilder mBuilderDelegate;
don't protected members end up in the javadocs?  we should definitely @hide this

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:168: *
@return the builder to facilitate chaining.
I think this should be moved to ExperimentalCrontEngine.Builder as I think it's
experimental, at least judging by setDataReductionProxyOptions() being @hide. 
You can ask bengr@ to verify.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:367: public
abstract String getCertVerifierData(long timeout);
this should be moved to Experimental, like you did with the setCertVerifierData
in the Builder.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:9:
* Created using {@link ExperimentalBidirectionalStream.Builder}.
should we add "Every instance of {@link BidirectionalStream} can be cast to
{@link ExperimentalBidirectionalStream}." like you did for
ExperimentalCronetEngine?

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:34:
public abstract Builder setHttpMethod(String method);
how come there are abstract but ExperimentalCronetEngine.Builder overrides are
not?

[kapishnikov, 2016-09-30 16:00:16]

Description was changed from

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Removed deprecated HttpUrlConnection API. Affected files:

Java Implementation:
ChromiumUrlRequest.java
ChromiumUrlRequestContext.java
ChromiumUrlRequestFactory.java
ChunkedWritableByteChannel.java
HttpUrlConnectionUrlRequest.java
HttpUrlConnectionUrlRequestFactory.java
HttpUrlRequest.java
HttpUrlRequestFactory.java
HttpUrlRequestListener.java
ResponseTooLargeException.java

Java Tests:
ChromiumUrlRequestTest.java
ChunkedWritableByteChannelTest.java
ContextInitTest.java
CronetUrlTest.java
HttpUrlRequestFactoryTest.java
TestHttpUrlRequestListener.java
UploadTest.java

C++ Layer:
chromium_url_request.cc
chromium_url_request.h
chromium_url_request_context.cc
chromium_url_request_context.h


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

[kapishnikov, 2016-09-30 16:01:43]

Paul, thanks again. I uploaded a new patch. All comments should be resolved.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:103:
CronetEngineBase possibleEngine = constructor.newInstance(builder);
On 2016/09/26 14:51:22, pauljensen wrote:
> eh what's the point of this?  I don't think we should need any reflection in
the
> impl.

Done.

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2339223002/diff/180001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:117:
@OnlyRunNativeCronet
On 2016/09/26 14:51:22, pauljensen wrote:
> perhaps we should run with the java-only impl and test that the appropriate
> exception is thrown when newBidirectionalStreamBuilder is called?

Done.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:15:
* Executor)}.
On 2016/09/30 13:19:24, pauljensen wrote:
> nit: you can remove the "(String, Callback, Executor)" stuff

Done.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:31: //
NOTE: In order to avoid breaking the existing API clients, all future methods
added to
On 2016/09/30 13:19:24, pauljensen wrote:
> NOTE->NOTE(kapishnikov)

Done.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:54:
protected final ICronetEngineBuilder mBuilderDelegate;
On 2016/09/30 13:19:25, pauljensen wrote:
> don't protected members end up in the javadocs?  we should definitely @hide
this

Done.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:168: *
@return the builder to facilitate chaining.
On 2016/09/30 13:19:24, pauljensen wrote:
> I think this should be moved to ExperimentalCrontEngine.Builder as I think
it's
> experimental, at least judging by setDataReductionProxyOptions() being @hide. 
> You can ask bengr@ to verify.

Done. Misha confirmed that it should be experimental.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:367: public
abstract String getCertVerifierData(long timeout);
On 2016/09/30 13:19:24, pauljensen wrote:
> this should be moved to Experimental, like you did with the
setCertVerifierData
> in the Builder.

Done.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:9:
* Created using {@link ExperimentalBidirectionalStream.Builder}.
On 2016/09/30 13:19:25, pauljensen wrote:
> should we add "Every instance of {@link BidirectionalStream} can be cast to
> {@link ExperimentalBidirectionalStream}." like you did for
> ExperimentalCronetEngine?

Done.

https://codereview.chromium.org/2339223002/diff/200001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:34:
public abstract Builder setHttpMethod(String method);
On 2016/09/30 13:19:25, pauljensen wrote:
> how come there are abstract but ExperimentalCronetEngine.Builder overrides are
> not?

ExperimentalCronetEngine.Builder is a concrete class that can be instantiated
using 'new' operator; thus, it cannot have abstract methods.

[pauljensen, 2016-10-03 15:22:39]

After this round of comments are addressed I'm pretty happy with landing this.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:14:
* Created using {@link ExperimentalCronetEngine#newBidirectionalStreamBuilder}.
you replaced a type with a method that returns that type, I think you might want
to go back to the type as the method you reference does not create a
BidirectionalStream
but the builder for it instead.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:24:
* it via {@link Builder#build}.
We need a sentence explaining how to create a BidirectionalStream.Builder,
something
like:
Created by {@link ExperimentalCronetEngine#newBidirectionalStreamBuilder}.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:54: //
{@hide exclude from JavaDoc}.
Does @hide work in // comments?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:415: *
Creates a builder for the {@link UrlRequest} objects. All callbacks for
remove "the"

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:9:
* Created using {@link ExperimentalBidirectionalStream.Builder}. Every instance
remove "ExperimentalBidirectionalStream."?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:17:
* constructing it via {@link Builder#build}.
add sentence describing how to create this

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:26:
* {@hide since this class exposes experimental features that should be hidden.}
is this @hide necessary considering the parent class is already @hide?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:68:
public Builder setDataReductionProxyOptions(
eh where did the comment for this method go?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:80:
public ExperimentalCronetEngine build() {
remove comment, add @Override, move to the end of the class after the comment
about chaining

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:117:
ICronetEngineBuilder getBuilderDelegate() {
@VisibleForTesting

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:199:
* @param url the URL for the generated stream.
this description differs in "the" and plurality of stream, as compared to
newUrlRequestBuilder.  please make them similar

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:315:

extra newline

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:17:
* Builder for building {@link UrlRequest}.
add comment about how one creates an instance of this class

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:37:
public abstract CronetEngine build();
make this return ExperimentalCronetEngine and change
ExperimentalCronetEngine.Builder.build() to return directly from the delegate,
thus avoiding the need for a run-time downcast.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:44:
public ICronetEngineBuilder enableNetworkQualityEstimator(boolean value) {
insert a new line between the comment and method, so it's clearer that this
comment applies to all later methods

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:13: * {@link
CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.
remove "(String, Callback, Executor)" like you did on line 15

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:21: * {@link
CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.
ditto

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:76: * The
request is given {@link #REQUEST_PRIORITY_MEDIUM} priority by default.
can we go back to the old wording?  I think it was clearer

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamBuilderImpl.java:25:
// All fields are temporary storage of BidirectionalStream configuration to be
BidirectionalStream->ExperimentalBidirectionalStream

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamBuilderImpl.java:26:
// copied to BidirectionalStream.
BidirectionalStream->CronetBidirectionalStream

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:56:
Executor executor, int priority, Collection<Object> requestAnnotations,
@RequestPriority?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:80:
public abstract ExperimentalBidirectionalStream createBidirectionalStream(String
url,
how come this is public but createRequest() is protected?  override of
createBidiStream looks to be public

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:89:
public abstract boolean isEnabled();
is this used anymore?  can we get rid of it and the overrides?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:193:
String getDefaultQuicUserAgentId(Context context) {
is this function used anymore? if yes, can we at least get rid of the |context|
param as per the TODO

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:262:
static final int HTTP_CACHE_DISABLED = 0;
we shouldn't have a copy of these constants here, we should be able to just
import them from the API

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:437:
public CronetEngineBuilderImpl setMockCertVerifierForTesting(long
mockCertVerifier) {
@Override

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:475:
public Context getContext() {
why public?  why not package-private like other accessors?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:484:
CronetEngine cronetEngine;
final

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:74:
public static class StatusConverter {
why wrap in a class?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:20:
* Implements {@link org.chromium.net.UrlRequest.Builder} and all experimental
features.
why not just say ExperimentalUrlRequest.Builder, rather than include the "and
all experimental features" fragment?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:26:
// All fields are temporary storage of UrlRequest configuration to be
UrlRequest->ExperimentalUrlRequest

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:27:
// copied to built UrlRequests.
ditto

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:29:
// CronetEngine to execute request.
CronetEngine->CronetEngineBase

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:64:
* @param url {@link java.net.URL} for the generated requests.
remove incorrect @link tag

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java:101: //
version. The implementation version may be more appropriate for the
um you got rid of the first line of the comment...the whole thing needs to go

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:117:
@OnlyRunNativeCronet
remove

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1338:
testFramework.createCronetEngineBuilder(getContext())));
please add back "new ContextWrapper("

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java:28:
private static class TestStatusListener extends UrlRequest.StatusListener {
why this change?  this file imports StatusListener directly

[kapishnikov, 2016-10-03 23:49:29]

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:14:
* Created using {@link ExperimentalCronetEngine#newBidirectionalStreamBuilder}.
On 2016/10/03 15:22:36, pauljensen wrote:
> you replaced a type with a method that returns that type, I think you might
want
> to go back to the type as the method you reference does not create a
> BidirectionalStream
> but the builder for it instead.

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:24:
* it via {@link Builder#build}.
On 2016/10/03 15:22:36, pauljensen wrote:
> We need a sentence explaining how to create a BidirectionalStream.Builder,
> something
> like:
> Created by {@link ExperimentalCronetEngine#newBidirectionalStreamBuilder}.

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:54: //
{@hide exclude from JavaDoc}.
On 2016/10/03 15:22:36, pauljensen wrote:
> Does @hide work in // comments?

Probably not. Fixed.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:415: *
Creates a builder for the {@link UrlRequest} objects. All callbacks for
On 2016/10/03 15:22:36, pauljensen wrote:
> remove "the"

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:9:
* Created using {@link ExperimentalBidirectionalStream.Builder}. Every instance
On 2016/10/03 15:22:36, pauljensen wrote:
> remove "ExperimentalBidirectionalStream."?

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalBidirectionalStream.java:17:
* constructing it via {@link Builder#build}.
On 2016/10/03 15:22:36, pauljensen wrote:
> add sentence describing how to create this

Done. Same for ExperimentalUrlRequest.Builder.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:26:
* {@hide since this class exposes experimental features that should be hidden.}
On 2016/10/03 15:22:36, pauljensen wrote:
> is this @hide necessary considering the parent class is already @hide?

Removed.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:68:
public Builder setDataReductionProxyOptions(
On 2016/10/03 15:22:36, pauljensen wrote:
> eh where did the comment for this method go?

Restored the comment back.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:80:
public ExperimentalCronetEngine build() {
On 2016/10/03 15:22:37, pauljensen wrote:
> remove comment, add @Override, move to the end of the class after the comment
> about chaining

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:117:
ICronetEngineBuilder getBuilderDelegate() {
On 2016/10/03 15:22:37, pauljensen wrote:
> @VisibleForTesting

Added. I used @android.support.annotation.VisibleForTesting instead of
@org.chromium.base.VisibleForTesting to avoid adding API dependency on
"//base:base_java".

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:199:
* @param url the URL for the generated stream.
On 2016/10/03 15:22:37, pauljensen wrote:
> this description differs in "the" and plurality of stream, as compared to
> newUrlRequestBuilder.  please make them similar

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:315:

On 2016/10/03 15:22:37, pauljensen wrote:
> extra newline

Removed.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:17:
* Builder for building {@link UrlRequest}.
On 2016/10/03 15:22:37, pauljensen wrote:
> add comment about how one creates an instance of this class

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:37:
public abstract CronetEngine build();
On 2016/10/03 15:22:37, pauljensen wrote:
> make this return ExperimentalCronetEngine and change
> ExperimentalCronetEngine.Builder.build() to return directly from the delegate,
> thus avoiding the need for a run-time downcast.

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/ICronetEngineBuilder.java:44:
public ICronetEngineBuilder enableNetworkQualityEstimator(boolean value) {
On 2016/10/03 15:22:37, pauljensen wrote:
> insert a new line between the comment and method, so it's clearer that this
> comment applies to all later methods

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:13: * {@link
CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.
On 2016/10/03 15:22:37, pauljensen wrote:
> remove "(String, Callback, Executor)" like you did on line 15

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:21: * {@link
CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.
On 2016/10/03 15:22:37, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/UrlRequest.java:76: * The
request is given {@link #REQUEST_PRIORITY_MEDIUM} priority by default.
On 2016/10/03 15:22:37, pauljensen wrote:
> can we go back to the old wording?  I think it was clearer

Done, but replaced the reference to itself by "this method". Same for
BidirectionalStream.setPriority().

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamBuilderImpl.java:25:
// All fields are temporary storage of BidirectionalStream configuration to be
On 2016/10/03 15:22:37, pauljensen wrote:
> BidirectionalStream->ExperimentalBidirectionalStream

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamBuilderImpl.java:26:
// copied to BidirectionalStream.
On 2016/10/03 15:22:37, pauljensen wrote:
> BidirectionalStream->CronetBidirectionalStream

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:56:
Executor executor, int priority, Collection<Object> requestAnnotations,
On 2016/10/03 15:22:37, pauljensen wrote:
> @RequestPriority?
I am actually getting a lint error if I do so:

------
/tmp/tmpKBxsvT/SRC_ROOT1/components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:174
Must be one of: 0, 1, 2, 3, 4: WrongConstant [warning]
                mPriority, mRequestAnnotations, mDisableCache,
mDisableConnectionMigration,
                ~~~~~~~~~
-----

Both mPriority and the input parameter are annotated with the same
@RequestPriority annotation. Need to figure it out.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:80:
public abstract ExperimentalBidirectionalStream createBidirectionalStream(String
url,
On 2016/10/03 15:22:37, pauljensen wrote:
> how come this is public but createRequest() is protected?  override of
> createBidiStream looks to be public

Good catch. Changed to 'protected'.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:89:
public abstract boolean isEnabled();
On 2016/10/03 15:22:37, pauljensen wrote:
> is this used anymore?  can we get rid of it and the overrides?

Another good catch. It is not used anymore.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:193:
String getDefaultQuicUserAgentId(Context context) {
On 2016/10/03 15:22:38, pauljensen wrote:
> is this function used anymore? if yes, can we at least get rid of the
|context|
> param as per the TODO

This method is used and the returned value is passed to
nativeCreateRequestContextConfig() call. The context is used to retrieve the app
package/name. If we don't retrieve the app name from the context, what value
should we pass as the user agent? Should the client set it explicitly?

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:262:
static final int HTTP_CACHE_DISABLED = 0;
On 2016/10/03 15:22:38, pauljensen wrote:
> we shouldn't have a copy of these constants here, we should be able to just
> import them from the API

Good point. Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:437:
public CronetEngineBuilderImpl setMockCertVerifierForTesting(long
mockCertVerifier) {
On 2016/10/03 15:22:38, pauljensen wrote:
> @Override

This method is actually used by the unit tests directly and is not exposed
through the experimental API.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:475:
public Context getContext() {
On 2016/10/03 15:22:37, pauljensen wrote:
> why public?  why not package-private like other accessors?

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:484:
CronetEngine cronetEngine;
On 2016/10/03 15:22:37, pauljensen wrote:
> final

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java:74:
public static class StatusConverter {
On 2016/10/03 15:22:38, pauljensen wrote:
> why wrap in a class?

Good point. Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:20:
* Implements {@link org.chromium.net.UrlRequest.Builder} and all experimental
features.
On 2016/10/03 15:22:38, pauljensen wrote:
> why not just say ExperimentalUrlRequest.Builder, rather than include the "and
> all experimental features" fragment?

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:26:
// All fields are temporary storage of UrlRequest configuration to be
On 2016/10/03 15:22:38, pauljensen wrote:
> UrlRequest->ExperimentalUrlRequest

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:27:
// copied to built UrlRequests.
On 2016/10/03 15:22:38, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:29:
// CronetEngine to execute request.
On 2016/10/03 15:22:38, pauljensen wrote:
> CronetEngine->CronetEngineBase

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:64:
* @param url {@link java.net.URL} for the generated requests.
On 2016/10/03 15:22:38, pauljensen wrote:
> remove incorrect @link tag

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/UserAgent.java:101: //
version. The implementation version may be more appropriate for the
On 2016/10/03 15:22:38, pauljensen wrote:
> um you got rid of the first line of the comment...the whole thing needs to go

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:117:
@OnlyRunNativeCronet
On 2016/10/03 15:22:38, pauljensen wrote:
> remove

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1338:
testFramework.createCronetEngineBuilder(getContext())));
On 2016/10/03 15:22:38, pauljensen wrote:
> please add back "new ContextWrapper("

Done.

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/GetStatusTest.java:28:
private static class TestStatusListener extends UrlRequest.StatusListener {
On 2016/10/03 15:22:38, pauljensen wrote:
> why this change?  this file imports StatusListener directly

Fixed.

[pauljensen, 2016-10-04 21:50:46]

Please also update the README.md

[kapishnikov, 2016-10-05 17:16:31]

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java:56:
Executor executor, int priority, Collection<Object> requestAnnotations,
On 2016/10/03 23:49:28, kapishnikov wrote:
> On 2016/10/03 15:22:37, pauljensen wrote:
> > @RequestPriority?
> I am actually getting a lint error if I do so:
> 
> ------
>
/tmp/tmpKBxsvT/SRC_ROOT1/components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java:174
> Must be one of: 0, 1, 2, 3, 4: WrongConstant [warning]
>                 mPriority, mRequestAnnotations, mDisableCache,
> mDisableConnectionMigration,
>                 ~~~~~~~~~
> -----
> 
> Both mPriority and the input parameter are annotated with the same
> @RequestPriority annotation. Need to figure it out.

Done but had to add @SuppressLint("WrongConstant").

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2339223002/diff/220001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:193:
String getDefaultQuicUserAgentId(Context context) {
On 2016/10/03 23:49:28, kapishnikov wrote:
> On 2016/10/03 15:22:38, pauljensen wrote:
> > is this function used anymore? if yes, can we at least get rid of the
> |context|
> > param as per the TODO
> 
> This method is used and the returned value is passed to
> nativeCreateRequestContextConfig() call. The context is used to retrieve the
app
> package/name. If we don't retrieve the app name from the context, what value
> should we pass as the user agent? Should the client set it explicitly?

Removed context from the input parameter list. Changed to use Builder.mContext
instead.

[kapishnikov, 2016-10-05 17:16:51]

On 2016/10/04 21:50:46, pauljensen wrote:
> Please also update the README.md

Done.

[pauljensen, 2016-10-07 18:07:31]

https://codereview.chromium.org/2339223002/diff/300001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2339223002/diff/300001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:14:
import org.chromium.net.impl.CronetUrlRequest;
um it looks like the import order got broken in lots of files...

[kapishnikov, 2016-10-07 18:12:20]

https://codereview.chromium.org/2339223002/diff/300001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2339223002/diff/300001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:14:
import org.chromium.net.impl.CronetUrlRequest;
On 2016/10/07 18:07:31, pauljensen wrote:
> um it looks like the import order got broken in lots of files...

The ordering has been changed recently. See
https://codereview.chromium.org/2052823002/diff/20001/tools/android/checkstyl...

[pauljensen, 2016-10-07 18:20:13]

I don't think I have any more comments on this CL.  I think we need to wait to
land until we update embedders.

[kapishnikov, 2016-10-10 18:23:38]

Description was changed from

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size:
63971 cronet_api.jar - Cronet API.
41180 cronet_impl_common_java.jar - common Cronet code that is shared by the
Cronet engine implementations.
104730 cronet_impl_java.jar - Java based implementation of the Cronet engine.
195425 cronet_impl_native_java.jar - native implementation of the Cronet engine.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

[kapishnikov, 2016-10-10 20:56:47]

Description was changed from

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size:
63971 cronet_api.jar - Cronet API.
41180 cronet_impl_common_java.jar - common Cronet code that is shared by the
Cronet engine implementations.
104730 cronet_impl_java.jar - Java based implementation of the Cronet engine.
195425 cronet_impl_native_java.jar - native implementation of the Cronet engine.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the
Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet
engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

[kapishnikov, 2016-10-10 21:43:40]

Description was changed from

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the
Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet
engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the
Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet
engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

4. Added auto-generated files to srcjars.


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

[kapishnikov, 2016-10-21 22:56:55]

The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 22:57:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-22 02:28:02]

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

[commit-bot: I haz the power, 2016-10-22 02:28:04]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2016-10-28 12:15:18]

lgtm

[pauljensen, 2016-10-28 12:20:34]

https://codereview.chromium.org/2339223002/diff/400001/components/cronet/andr...
File components/cronet/android/proguard.cfg (right):

https://codereview.chromium.org/2339223002/diff/400001/components/cronet/andr...
components/cronet/android/proguard.cfg:6: # avoid the dependency on
Chromium-Base Java classes.
Can we use @Keep:
https://developer.android.com/reference/android/support/annotation/Keep.html
Java 1.8 has @Native, but I don't think Android has that yet.

[kapishnikov, 2016-10-28 13:56:42]

https://codereview.chromium.org/2339223002/diff/400001/components/cronet/andr...
File components/cronet/android/proguard.cfg (right):

https://codereview.chromium.org/2339223002/diff/400001/components/cronet/andr...
components/cronet/android/proguard.cfg:6: # avoid the dependency on
Chromium-Base Java classes.
On 2016/10/28 12:20:34, pauljensen wrote:
> Can we use @Keep:
> https://developer.android.com/reference/android/support/annotation/Keep.html
> Java 1.8 has @Native, but I don't think Android has that yet.

I can try and see if it works.

[kapishnikov, 2016-10-28 15:17:31]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2016-10-28 15:17:32]

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/2339223002/#ps420001
(title: "Rebase & Conflict Resolution")

[commit-bot: I haz the power, 2016-10-28 15:18:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 16:18:02]

Description was changed from

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the
Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet
engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

4. Added auto-generated files to srcjars.


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the
Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet
engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

4. Added auto-generated files to srcjars.


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

[commit-bot: I haz the power, 2016-10-28 16:18:04]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-10-28 16:51:05]

Description was changed from

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the
Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet
engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

4. Added auto-generated files to srcjars.


The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299
==========

to

==========
Cronet API Refactoring

1. Refactored Cronet Java implementation to create clean separation between the
API layer and the implementation. This is a step toward a pluggable architecture
that would allow loading the implementation independently from the API. E.g.,
this would potentially allow having multiple Cronet implementation and load them
using different class loaders.

2. Introduced "Experimental" subclasses that clearly indicate that given
functionality/features are experimental and can be changed in the future. This
replaces hidden javadoc methods that can still be visible in Android Studio and
therefore used by the developers without realization that this is the
experimental API. The developer has to explicitly cast a class to the
experimental version in order to be able to use the features.

3. Split the Cronet API and the implementation into 4 separate jars:

Size unpacked:
 63K cronet_api.jar                - Cronet API.
 41K cronet_impl_common_java.jar   - common Cronet code that is shared by the
Cronet engine implementations.
103K cronet_impl_platform_java.jar - Java based implementation of the Cronet
engine.
623K cronet_impl_native_java.jar   - native implementation of the Cronet engine.

4. Added auto-generated files to srcjars.

The following work should be done in a separate CL:

Break the following classes to the API and implementation parts.
* UrlResponseInfo
* UploadDataProviders
* RequestFinishedInfo
* NetworkQualityRttListener?
* NetworkQualityThroughputListener?

BUG=629299

Committed: https://crrev.com/aa8f338c7107e3d45279e4f62e6145478964ab1c
Cr-Commit-Position: refs/heads/master@{#428390}
==========

[commit-bot: I haz the power, 2016-10-28 16:51:08]

Patchset 22 (id:??) landed as
https://crrev.com/aa8f338c7107e3d45279e4f62e6145478964ab1c
Cr-Commit-Position: refs/heads/master@{#428390}