[Cronet] Separate Cronet implementation and API by package name.

[Cronet] Separate Cronet implementation and API by package name.

They are already in different directories, now move them to different
packages.
Cronet API package is org.chromium.net
Cronet Impl package is org.chromium.net.{impl,urlconnection}

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

[pauljensen, 2016-05-31 18:21:54]

Description was changed from

==========
Separate Cronet implementation and API by package name.
==========

to

==========
[Cronet] Separate Cronet implementation and API by package name.
==========

[kapishnikov, 2016-06-02 17:27:00]

kapishnikov@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[kapishnikov, 2016-06-02 17:27:01]

This will probably require a lot of efforts but we could further split the
current API into the thinner API and the implementation parts. E.g., if we look
at the builder, there are internal classes such as QuicHint and Pkp that are
actually part of the implementation. The benefits we get from the splitting is
that we will not need hidden public methods and fields. Also, I think it will
help us to support backward compatibility in the future. E.g., let's say we want
to add a new field to QuicHint in a new version of Cronet. If QuicHint is loaded
by the app, the implementation code should contain checks to make sure that the
new field is present in QuicHint. If it is loaded by dynamite, the checks are
not necessary.

[pauljensen, 2016-06-03 13:31:50]

On 2016/06/02 17:27:01, kapishnikov wrote:
> This will probably require a lot of efforts but we could further split the
> current API into the thinner API and the implementation parts. E.g., if we
look
> at the builder, there are internal classes such as QuicHint and Pkp that are
> actually part of the implementation. The benefits we get from the splitting is
> that we will not need hidden public methods and fields. Also, I think it will
> help us to support backward compatibility in the future. E.g., let's say we
want
> to add a new field to QuicHint in a new version of Cronet. If QuicHint is
loaded
> by the app, the implementation code should contain checks to make sure that
the
> new field is present in QuicHint. If it is loaded by dynamite, the checks are
> not necessary.

QuicHint and Pkp are currently very tied to the API, in that they simply mirror
fields in the API.  I suppose we could load those classes from the impl and
offer default initializers for fields that are added in future API versions.  I
see your point that this would eliminate the need for version checks to access
new fields.  Is that what you're proposing?  Could we go alot further and make
Builder an interface and the impl of Builder be loaded from the impl?

[kapishnikov, 2016-06-03 15:56:37]

On 2016/06/03 13:31:50, pauljensen wrote:
> On 2016/06/02 17:27:01, kapishnikov wrote:
> > This will probably require a lot of efforts but we could further split the
> > current API into the thinner API and the implementation parts. E.g., if we
> look
> > at the builder, there are internal classes such as QuicHint and Pkp that are
> > actually part of the implementation. The benefits we get from the splitting
is
> > that we will not need hidden public methods and fields. Also, I think it
will
> > help us to support backward compatibility in the future. E.g., let's say we
> want
> > to add a new field to QuicHint in a new version of Cronet. If QuicHint is
> loaded
> > by the app, the implementation code should contain checks to make sure that
> the
> > new field is present in QuicHint. If it is loaded by dynamite, the checks
are
> > not necessary.
> 
> QuicHint and Pkp are currently very tied to the API, in that they simply
mirror
> fields in the API.  I suppose we could load those classes from the impl and
> offer default initializers for fields that are added in future API versions. 
I
> see your point that this would eliminate the need for version checks to access
> new fields.  Is that what you're proposing?  Could we go alot further and make
> Builder an interface and the impl of Builder be loaded from the impl?

Yes, I think splitting the builder to the interface and impl parts will give us
greater flexibility. The impl part can be loaded by Dynamite if we choose so. I
like your idea of having a method that constructs the builder in lieu of the
client instantiating the builder directly through the constructor.

[pauljensen, 2016-07-21 13:00:59]

Description was changed from

==========
[Cronet] Separate Cronet implementation and API by package name.
==========

to

==========
[Cronet] Separate Cronet implementation and API by package name.

They are already in different directories, now move them to different
packages.
Cronet API package is org.chromium.net
Cronet Impl package is org.chromium.net.{impl,urlconnection}

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

[pauljensen, 2016-07-21 13:01:43]

Andrei, would you like to review this?  I just synced it and I think it should
be ready for review.

[kapishnikov, 2016-07-21 13:59:36]

On 2016/07/21 13:01:43, pauljensen wrote:
> Andrei, would you like to review this?  I just synced it and I think it should
> be ready for review.

Sure, I will review it today.

[pauljensen, 2016-07-21 14:01:14]

On 2016/07/21 13:59:36, kapishnikov wrote:
> On 2016/07/21 13:01:43, pauljensen wrote:
> > Andrei, would you like to review this?  I just synced it and I think it
should
> > be ready for review.
> 
> Sure, I will review it today.

BTW my plan is to land this and then work on splitting the builders up in later
CLs.

[kapishnikov, 2016-07-22 05:54:13]

Paul, the changes look good. My only concern is that we are adding too many
@hide annotations. Will the functions and properties that are annotated with
@hide be obfuscated by proguard. If they cannot be obfuscated then they will be
visible to the client.

https://codereview.chromium.org/1849753002/diff/120001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
(right):

https://codereview.chromium.org/1849753002/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:66:
public BidirectionalStream createBidirectionalStream(String url,
Should it be hidden?

[pauljensen, 2016-07-22 11:10:33]

On 2016/07/22 05:54:13, kapishnikov wrote:
> Paul, the changes look good. My only concern is that we are adding too many
> @hide annotations. Will the functions and properties that are annotated with
> @hide be obfuscated by proguard. If they cannot be obfuscated then they will
be
> visible to the client.

@hide is extremely common in Java APIs, I'll send you an email demonstrating
this.  Even if these APIs are not obfuscated, there is no responsibility to
maintain or support them as they are not part of the documented APIs.  API
implementers are free to add/remove/modify them as they please with no concern
for breaking users of them.

https://codereview.chromium.org/1849753002/diff/120001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
(right):

https://codereview.chromium.org/1849753002/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:66:
public BidirectionalStream createBidirectionalStream(String url,
On 2016/07/22 05:54:13, kapishnikov wrote:
> Should it be hidden?

This class is package-private so it's hidden from javadocs.

[kapishnikov, 2016-07-22 14:32:02]

On 2016/07/22 11:10:33, pauljensen wrote:
> On 2016/07/22 05:54:13, kapishnikov wrote:
> > Paul, the changes look good. My only concern is that we are adding too many
> > @hide annotations. Will the functions and properties that are annotated with
> > @hide be obfuscated by proguard. If they cannot be obfuscated then they will
> be
> > visible to the client.
> 
> @hide is extremely common in Java APIs, I'll send you an email demonstrating
> this.  Even if these APIs are not obfuscated, there is no responsibility to
> maintain or support them as they are not part of the documented APIs.  API
> implementers are free to add/remove/modify them as they please with no concern
> for breaking users of them.
> 
>
https://codereview.chromium.org/1849753002/diff/120001/components/cronet/andr...
> File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
> (right):
> 
>
https://codereview.chromium.org/1849753002/diff/120001/components/cronet/andr...
> components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:66:
> public BidirectionalStream createBidirectionalStream(String url,
> On 2016/07/22 05:54:13, kapishnikov wrote:
> > Should it be hidden?
> 
> This class is package-private so it's hidden from javadocs.

Acknowledged.

[kapishnikov, 2016-07-22 14:33:33]

Paul, thanks for the explanation. If the change does not break the current build
LGTM.

[pauljensen, 2016-07-25 09:52:36]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2016-07-25 09:53:00]

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-07-25 10:43:47]

Description was changed from

==========
[Cronet] Separate Cronet implementation and API by package name.

They are already in different directories, now move them to different
packages.
Cronet API package is org.chromium.net
Cronet Impl package is org.chromium.net.{impl,urlconnection}

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

to

==========
[Cronet] Separate Cronet implementation and API by package name.

They are already in different directories, now move them to different
packages.
Cronet API package is org.chromium.net
Cronet Impl package is org.chromium.net.{impl,urlconnection}

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

[commit-bot: I haz the power, 2016-07-25 10:43:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-25 10:45:40]

Description was changed from

==========
[Cronet] Separate Cronet implementation and API by package name.

They are already in different directories, now move them to different
packages.
Cronet API package is org.chromium.net
Cronet Impl package is org.chromium.net.{impl,urlconnection}

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

to

==========
[Cronet] Separate Cronet implementation and API by package name.

They are already in different directories, now move them to different
packages.
Cronet API package is org.chromium.net
Cronet Impl package is org.chromium.net.{impl,urlconnection}

BUG=629299

Committed: https://crrev.com/13d259bf9ef1948c7e98154c4fdc6019ca9406d2
Cr-Commit-Position: refs/heads/master@{#407446}
==========

[commit-bot: I haz the power, 2016-07-25 10:45:42]

Patchset 7 (id:??) landed as
https://crrev.com/13d259bf9ef1948c7e98154c4fdc6019ca9406d2
Cr-Commit-Position: refs/heads/master@{#407446}