[Cronet] Add support to run quic_simple_server

[Cronet] Add support to run quic_simple_server

This CL added a wrapper on top of quic_simple_server so that
Cronet tests can spin up a local quic server on the device.

BUG=411010
Committed: https://crrev.com/c73b6a4647f82a3d7681457d00ab840456f2be17
Cr-Commit-Position: refs/heads/master@{#324001}

[xunjieli, 2015-03-31 14:27:39]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, rtenneti@chromium.org

[xunjieli, 2015-03-31 14:27:49]

Patchset #1 (id:1) has been deleted

[xunjieli, 2015-03-31 14:30:57]

Patchset #1 (id:20001) has been deleted

[ramant (doing other things), 2015-03-31 17:24:06]

LGTM. Please wait for approval from mef.

A general comment, is it possible to define constants for QUIC port (6121) and
host (127.0.0.1) (and/or other constants)?

[mef, 2015-03-31 17:33:18]

very cool!

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:142:
config.addQuicHint("127.0.0.1", 6121, 6121);
should those constants come from QuicTestServer?

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
File components/cronet/android/test/src/org/chromium/net/QuicTestServer.java
(right):

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/QuicTestServer.java:16:
public static String SERVER_URL = "http://127.0.0.1:6121";
maybe make a getServerURL() method, similar to NativeTestServer?
Also getServerHost() and getServerPort() for addQuicHint?

[xunjieli, 2015-03-31 17:50:16]

thanks for the review!

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:142:
config.addQuicHint("127.0.0.1", 6121, 6121);
On 2015/03/31 17:33:18, mef wrote:
> should those constants come from QuicTestServer?

Done.

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
File components/cronet/android/test/src/org/chromium/net/QuicTestServer.java
(right):

https://codereview.chromium.org/1039693003/diff/50001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/QuicTestServer.java:16:
public static String SERVER_URL = "http://127.0.0.1:6121";
On 2015/03/31 17:33:18, mef wrote:
> maybe make a getServerURL() method, similar to NativeTestServer?
> Also getServerHost() and getServerPort() for addQuicHint?

Done.

[pauljensen, 2015-03-31 18:16:34]

Can we move the "127.0.0.1" and 6121 constants to a location shared between Java
and C++? I'm not sure the best way to do it.  You could do it with the constant
at compile time via:
 // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.net
 enum QuicTestServer {
   PORT = 6121
 };
I'm not sure the recommended way of doing it with the string.  You could always
do it with JNI.

[xunjieli, 2015-03-31 18:20:58]

On 2015/03/31 18:16:34, pauljensen wrote:
> Can we move the "127.0.0.1" and 6121 constants to a location shared between
Java
> and C++? I'm not sure the best way to do it.  You could do it with the
constant
> at compile time via:
>  // GENERATED_JAVA_ENUM_PACKAGE: http://org.chromium.net
>  enum QuicTestServer {
>    PORT = 6121
>  };
> I'm not sure the recommended way of doing it with the string.  You could
always
> do it with JNI.

Hmm.. making the port into an enum seems a little weird. I guess I will put the
constants C++ side, and use JNI to retrieve the two constants on Java side.

[mef, 2015-03-31 20:21:48]

On 2015/03/31 18:20:58, xunjieli wrote:
> On 2015/03/31 18:16:34, pauljensen wrote:
> > Can we move the "127.0.0.1" and 6121 constants to a location shared between
> Java
> > and C++? I'm not sure the best way to do it.  You could do it with the
> constant
> > at compile time via:
> >  // GENERATED_JAVA_ENUM_PACKAGE: http://org.chromium.net
> >  enum QuicTestServer {
> >    PORT = 6121
> >  };
> > I'm not sure the recommended way of doing it with the string.  You could
> always
> > do it with JNI.
> 
> Hmm.. making the port into an enum seems a little weird. I guess I will put
the
> constants C++ side, and use JNI to retrieve the two constants on Java side.

I agree, getting those constants from native side through JNI call is cleaner.

[xunjieli, 2015-03-31 20:37:31]

On 2015/03/31 18:20:58, xunjieli wrote:
> On 2015/03/31 18:16:34, pauljensen wrote:
> > Can we move the "127.0.0.1" and 6121 constants to a location shared between
> Java
> > and C++? I'm not sure the best way to do it.  You could do it with the
> constant
> > at compile time via:
> >  // GENERATED_JAVA_ENUM_PACKAGE: http://org.chromium.net
> >  enum QuicTestServer {
> >    PORT = 6121
> >  };
> > I'm not sure the recommended way of doing it with the string.  You could
> always
> > do it with JNI.
> 
> Hmm.. making the port into an enum seems a little weird. I guess I will put
the
> constants C++ side, and use JNI to retrieve the two constants on Java side.

Moved constants to C++ side. PTAL. thanks!

[xunjieli, 2015-03-31 20:40:37]

Patchset #4 (id:90001) has been deleted

[mef, 2015-03-31 20:56:59]

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java
(right):

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:54:
// because the app will be re-installed and app data will be cleared.
Hmm, is app data cleared every time when app is installed?

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:69:
files = assetManager.list(path);
suggest: move this up to files[] declaration.

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:70:
if (files.length == 0) {
this logic is a bit unclear. Why files.length would ever be 0?

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:80:
install(context, path + "/" + files[i]);
Is this recursive install?

[xunjieli, 2015-03-31 21:20:26]

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java
(right):

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:54:
// because the app will be re-installed and app data will be cleared.
On 2015/03/31 20:56:58, mef wrote:
> Hmm, is app data cleared every time when app is installed?

Yes. app data is cleared when app is uninstalled. We always uninstall app before
we install a new one.

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:69:
files = assetManager.list(path);
On 2015/03/31 20:56:58, mef wrote:
> suggest: move this up to files[] declaration.

Done.

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:70:
if (files.length == 0) {
On 2015/03/31 20:56:58, mef wrote:
> this logic is a bit unclear. Why files.length would ever be 0?

When the path is a file instead of a directory. Added comments.

https://codereview.chromium.org/1039693003/diff/110001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/TestFilesInstaller.java:80:
install(context, path + "/" + files[i]);
On 2015/03/31 20:56:58, mef wrote:
> Is this recursive install?

Yes. We need to copy subdirectories and the files in subdirectorys. Added
comments.

[mef, 2015-04-06 20:30:56]

lgtm

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
File components/cronet/android/test/cronet_test_jni.cc (right):

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
components/cronet/android/test/cronet_test_jni.cc:21:
{"RegisterNativeTestServer", cronet::RegisterNativeTestServer},
nit: I think name is arbitrary, but for consistency and proper sorting I'd
suggest removing 'Register' from the name string.

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:140:
System.loadLibrary("cronet_tests");
Thanks for the comment! 
That's annoying that we have to do this, but I don't have better ideas.

Would it make sense to extract QUIC tests into separate test class, and load
library in its SetUp()?

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
File components/cronet/android/test/quic_test_server.h (right):

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
components/cronet/android/test/quic_test_server.h:5: #ifndef
CRONET_QUIC_TEST_SERVER_H_
I think this should be COMPONENTS_CRONET_ANDROID_TEST_QUIC_TEST_SERVER_H_, also
in some other files in same directory. 
Maybe do it in separate CL?

[xunjieli, 2015-04-06 21:23:13]

Patchset #7 (id:170001) has been deleted

[xunjieli, 2015-04-06 21:23:50]

Thanks for the review!

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
File components/cronet/android/test/cronet_test_jni.cc (right):

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
components/cronet/android/test/cronet_test_jni.cc:21:
{"RegisterNativeTestServer", cronet::RegisterNativeTestServer},
On 2015/04/06 20:30:56, mef wrote:
> nit: I think name is arbitrary, but for consistency and proper sorting I'd
> suggest removing 'Register' from the name string.

Done.

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:140:
System.loadLibrary("cronet_tests");
On 2015/04/06 20:30:56, mef wrote:
> Thanks for the comment! 
> That's annoying that we have to do this, but I don't have better ideas.
> 
> Would it make sense to extract QUIC tests into separate test class, and load
> library in its SetUp()?

Done. Makes sense, since we will be adding more tests later on.

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
File components/cronet/android/test/quic_test_server.h (right):

https://codereview.chromium.org/1039693003/diff/150001/components/cronet/andr...
components/cronet/android/test/quic_test_server.h:5: #ifndef
CRONET_QUIC_TEST_SERVER_H_
On 2015/04/06 20:30:56, mef wrote:
> I think this should be COMPONENTS_CRONET_ANDROID_TEST_QUIC_TEST_SERVER_H_,
also
> in some other files in same directory. 
> Maybe do it in separate CL?

Done. Right, will keep it in mind once we touch the other files.

[xunjieli, 2015-04-06 21:23:54]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-04-06 21:23:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1039693003/#ps190001
(title: "Address comments")

[commit-bot: I haz the power, 2015-04-06 21:24:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1039693003/190001

[commit-bot: I haz the power, 2015-04-06 22:32:55]

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

[commit-bot: I haz the power, 2015-04-06 22:32:56]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[xunjieli, 2015-04-07 00:29:06]

Patchset #8 (id:210001) has been deleted

[xunjieli, 2015-04-07 00:29:15]

Patchset #8 (id:230001) has been deleted

[xunjieli, 2015-04-07 00:47:52]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-04-07 00:47:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1039693003/#ps250001
(title: "Suppress fb warning")

[commit-bot: I haz the power, 2015-04-07 00:48:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1039693003/250001

[commit-bot: I haz the power, 2015-04-07 01:44:27]

Committed patchset #8 (id:250001)

[commit-bot: I haz the power, 2015-04-07 01:45:26]

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