[Cronet] Remove CronetEngine.Builder JSON serial/deserialization APIs

[Cronet] Remove CronetEngine.Builder JSON serial/deserialization APIs

This CL removes test dependencies on the serial/deserialization of
CronetEngine.Builders, and removes the related public APIs.
Internally CronetEngine.Builders are still serial/deserialized during
CronetUrlRequestContext construction but at least this doesn't
complicate the public APIs.
Perhaps in the future the internal serial/deserialization can be removed
in exchange for directly passing configuration options; this is a
large change however as the options are passed across several functions
and a thread hop.
Perhaps in the future the use of command line arguments to
CronetTestFramework can also be removed; for now I'm removing the
CONFIG_KEY argument.

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

[pauljensen, 2015-10-28 14:43:05]

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

[pauljensen, 2015-10-28 14:43:06]

Helen, PTAL.

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:135:
CronetEngine.Builder builder = new CronetEngine.Builder(getContext());
Had to switch to CronetEngine.Builder here so builder.mContext!=null

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java:26:
CronetEngine.Builder builder = new CronetEngine.Builder(getContext());
ditto

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
(right):

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:41:
builder.setStoragePath(CronetTestFramework.getTestStorage(getContext()));
The use of getTestStorage here without a CronetTestFramework instantiation
required making getTestStorage static.

[xunjieli, 2015-10-28 15:23:58]

nit: s/arguement/argument in the CL description.

Should we fix the race issue in
https://code.google.com/p/chromium/issues/detail?id=544976#c6 first? 

You could see the flake on the bot too:
http://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Buil...
("Failed steps failed instrumentation test cronettestinstrumentation")

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:135:
CronetEngine.Builder builder = new CronetEngine.Builder(getContext());
On 2015/10/28 14:43:06, pauljensen wrote:
> Had to switch to CronetEngine.Builder here so builder.mContext!=null

Acknowledged.

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/HttpUrlRequestFactoryTest.java:26:
CronetEngine.Builder builder = new CronetEngine.Builder(getContext());
On 2015/10/28 14:43:06, pauljensen wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:191:
cronetEngineBuilder.enableHTTP2(true).enableQUIC(true);
Overwriting enableHTTP2 and QUIC here seems odd. It should be probably moved to
the if statement above.

[pauljensen, 2015-10-29 13:27:48]

Description was changed from

==========
[Cronet] Remove CronetEngine.Builder JSON serial/deserialization APIs

This CL removes test dependencies on the serial/deserialization of
CronetEngine.Builders, and removes the related public APIs.
Internally CronetEngine.Builders are still serial/deserialized during
CronetUrlRequestContext construction but at least this doesn't
complicate the public APIs.
Perhaps in the future the internal serial/deserialization can be removed
in exchange for directly passing configuration options; this is a
large change however as the options are passed across several functions
and a thread hop.
Perhaps in the future the use of command line arguements to
CronetTestFramework can also be removed; for now I'm removing the
CONFIG_KEY arguement.

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

to

==========
[Cronet] Remove CronetEngine.Builder JSON serial/deserialization APIs

This CL removes test dependencies on the serial/deserialization of
CronetEngine.Builders, and removes the related public APIs.
Internally CronetEngine.Builders are still serial/deserialized during
CronetUrlRequestContext construction but at least this doesn't
complicate the public APIs.
Perhaps in the future the internal serial/deserialization can be removed
in exchange for directly passing configuration options; this is a
large change however as the options are passed across several functions
and a thread hop.
Perhaps in the future the use of command line arguments to
CronetTestFramework can also be removed; for now I'm removing the
CONFIG_KEY argument.

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

[pauljensen, 2015-10-29 13:28:22]

On 2015/10/28 15:23:58, xunjieli wrote:
> nit: s/arguement/argument in the CL description.
Done.

> 
> Should we fix the race issue in
> https://code.google.com/p/chromium/issues/detail?id=544976#c6 first?
Done.

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1424193003/diff/1/components/cronet/android/t...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:191:
cronetEngineBuilder.enableHTTP2(true).enableQUIC(true);
On 2015/10/28 15:23:58, xunjieli wrote:
> Overwriting enableHTTP2 and QUIC here seems odd. It should be probably moved
to
> the if statement above.

Done.

[xunjieli, 2015-10-29 15:55:49]

So no embedder relies on constructing a CronetEngine.Builder from a JSON string?
If this API is safe to get rid of, then I am totally for it.

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

https://codereview.chromium.org/1424193003/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:324: * Get
JSON string representation of the builder.
nit: s/Get/Gets.

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

https://codereview.chromium.org/1424193003/diff/20001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestApplication.java:12:
public class CronetTestApplication extends Application {
Do we need an empty application?

[pauljensen, 2015-10-29 16:20:48]

On 2015/10/29 15:55:49, xunjieli wrote:
> So no embedder relies on constructing a CronetEngine.Builder from a JSON
string?
> If this API is safe to get rid of, then I am totally for it.

No embedder relies on this.

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

https://codereview.chromium.org/1424193003/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:324: * Get
JSON string representation of the builder.
On 2015/10/29 15:55:49, xunjieli wrote:
> nit: s/Get/Gets.

Done, and added an "a" afterwards.

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

https://codereview.chromium.org/1424193003/diff/20001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestApplication.java:12:
public class CronetTestApplication extends Application {
On 2015/10/29 15:55:49, xunjieli wrote:
> Do we need an empty application?

Yes, unfortunately, until we completely switch from instrumentation tests to
junit tests we need this.  I briefly tried removing this and adjusting the
AndroidManifest.xml but the tests just failed complaining about not being able
to start the app.

[xunjieli, 2015-10-29 17:59:08]

lgtm

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

https://codereview.chromium.org/1424193003/diff/20001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestApplication.java:12:
public class CronetTestApplication extends Application {
On 2015/10/29 16:20:48, pauljensen wrote:
> On 2015/10/29 15:55:49, xunjieli wrote:
> > Do we need an empty application?
> 
> Yes, unfortunately, until we completely switch from instrumentation tests to
> junit tests we need this.  I briefly tried removing this and adjusting the
> AndroidManifest.xml but the tests just failed complaining about not being able
> to start the app.

Acknowledged. I see.

[pauljensen, 2015-10-30 11:05:55]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2015-10-30 11:06:11]

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

[commit-bot: I haz the power, 2015-10-30 11:27:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-30 11:28:24]

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