[Android] Add a Java version of net::test_server::EmbeddedTestServer.

[Android] Add a Java version of net::test_server::EmbeddedTestServer.

Notably, this CL does not:
 - convert ChromeInstrumentationTestRunner's TestHttpServer to use this
 - convert cronet's NativeTestServer to use this

Those can be done separately.

BUG=488192
Committed: https://crrev.com/d58755d3bb7ead19d8ad909e4455b14857fe6d10
Cr-Commit-Position: refs/heads/master@{#336159}

[jbudorick, 2015-06-09 17:29:51]

jbudorick@chromium.org changed reviewers:
+ mef@chromium.org

[jbudorick, 2015-06-09 17:29:52]

FWIW, I started working on converting the cronet server to use this. Once I
started throwing function pointers through the JNI, I figured it'd be better to
do that in a separate review.

[mef, 2015-06-17 21:55:56]

Thanks for doing this!

https://codereview.chromium.org/1174613002/diff/20001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1174613002/diff/20001/net/net.gyp#newcode648
net/net.gyp:648: 'test/spawned_test_server/remote_test_server.cc',
it seems that these were removed unless building for android, so this section
isn't needed anymore (if we remove corresponding main list).

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:26:
public void initializeNative() {
should this method be private?

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:28:
assert mNativeEmbeddedTestServer != 0;
I'm unfamiliar with this notation, how does it work in regards to debug /
release?
Should it be before 'if'?

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:67:
/** Shut down the server.
nit: Shutdown as one word?

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:76:
public void destroy() {
what if it is destroyed without shutdownAndWaitForComplete?

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:101:
private native void nativeRegisterRequestHandler(
unused?

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:103:
private native void nativeRegisterRequestHandlers(
unused?

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/net_te...
File net/test/android/net_test_jni_onload.h (right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/net_te...
net/test/android/net_test_jni_onload.h:1: #ifndef
NET_TEST_ANDROID_NET_TEST_JNI_ONLOAD_H_
need copyright header.

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.cc:27: return
test_server_.InitializeAndWaitUntilReady();
what if ServeFilesFromDirectory was not called?

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
File net/test/embedded_test_server/android/embedded_test_server_android.h
(right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.h:1: #ifndef
NET_TEST_EMBEDDED_TEST_SERVER_ANDROID_EMBEDDED_TEST_SERVER_ANDROID_H_
need copyright comment

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.h:29: jstring
relative_url) const;
I'm not sure whether this is recommended or required, but we've adopted the
naming pattern for params passed from jni to start with 'j'.

For example
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.h:35: void
RegisterRequestHandler(JNIEnv* env, jobject obj, jlong request_handler);
How will request_handlers be created?

[jbudorick, 2015-06-18 17:44:56]

re calling X without calling Y: my goal with this is to provide a thin Java
wrapper around the C++ version of EmbeddedTestServer, so this doesn't provide
any additional protection in those cases.

https://codereview.chromium.org/1174613002/diff/20001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1174613002/diff/20001/net/net.gyp#newcode648
net/net.gyp:648: 'test/spawned_test_server/remote_test_server.cc',
On 2015/06/17 at 21:55:55, mef wrote:
> it seems that these were removed unless building for android, so this section
isn't needed anymore (if we remove corresponding main list).

Moved remote_test_server.{cc,h} and spawner_communicator.{cc,h} from the main
list to the 'sources' section above, and removed the OS == 'android' else
clause.

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:26:
public void initializeNative() {
On 2015/06/17 at 21:55:55, mef wrote:
> should this method be private?

No. Clients will call this explicitly to initialize the native side.

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:28:
assert mNativeEmbeddedTestServer != 0;
On 2015/06/17 at 21:55:55, mef wrote:
> I'm unfamiliar with this notation, how does it work in regards to debug /
release?
> Should it be before 'if'?

I had based this on Tab, which uses assertions similarly. However, assertions
are somewhat problematic, especially with art (see
https://code.google.com/p/chromium/issues/detail?id=462676). I'm honestly not
sure if it's better to leave the asserts for now and revise it when we address
462676 or do a one-off here.

Regardless, this should not be before the if. nativeInit will set
mNativeEmbeddedTestServer if it's 0 to start with. The assertion just makes sure
that it has done so.

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:67:
/** Shut down the server.
On 2015/06/17 at 21:55:55, mef wrote:
> nit: Shutdown as one word?

done

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:76:
public void destroy() {
On 2015/06/17 at 21:55:55, mef wrote:
> what if it is destroyed without shutdownAndWaitForComplete?

We'll hit this:
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_...

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:101:
private native void nativeRegisterRequestHandler(
On 2015/06/17 at 21:55:55, mef wrote:
> unused?

oops, these are leftovers from my experimentation with supporting cronet's
NativeTestServer in this CL.

Removed.

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/net_te...
File net/test/android/net_test_jni_onload.h (right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/net_te...
net/test/android/net_test_jni_onload.h:1: #ifndef
NET_TEST_ANDROID_NET_TEST_JNI_ONLOAD_H_
On 2015/06/17 at 21:55:56, mef wrote:
> need copyright header.

done

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.cc:27: return
test_server_.InitializeAndWaitUntilReady();
On 2015/06/17 at 21:55:56, mef wrote:
> what if ServeFilesFromDirectory was not called?

The server will hit this when attempting to handle a request:
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_...

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
File net/test/embedded_test_server/android/embedded_test_server_android.h
(right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.h:1: #ifndef
NET_TEST_EMBEDDED_TEST_SERVER_ANDROID_EMBEDDED_TEST_SERVER_ANDROID_H_
On 2015/06/17 at 21:55:56, mef wrote:
> need copyright comment

done

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.h:29: jstring
relative_url) const;
On 2015/06/17 at 21:55:56, mef wrote:
> I'm not sure whether this is recommended or required, but we've adopted the
naming pattern for params passed from jni to start with 'j'.
> 
> For example
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...

I've seen both, but I'm happy to do it in net/ if that's what you've done
elsewhere.

https://codereview.chromium.org/1174613002/diff/20001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.h:35: void
RegisterRequestHandler(JNIEnv* env, jobject obj, jlong request_handler);
On 2015/06/17 at 21:55:56, mef wrote:
> How will request_handlers be created?

As you noted in the java file, these are unused in this CL, so I'm removing
them.

However, if we need to support java clients adding a request handler, I had been
considering this:
 - have the native side of the client declare & define the request handler
 - have the java side of the callback call across the JNI to get a pointer to
that handler as a jlong
 - pass that jlong to this, which would cast it back to a
scoped_ptr<HttpResponse> (*)(const HttpRequest&)

[mef, 2015-06-19 19:26:08]

I think it is probably ok not to have robust safeguards in test utility, but it
still would be really useful to describe expected sequence.

Does this look right?

1. Construct java object
2. initNative
3. serveFilesFromDirectory
4. initializeAndWait
5. <serve requests>
6. shutdownAndWait
7. Destroy
8. release java object.

Could/should this be simplified?

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:26:
public void initializeNative() {
On 2015/06/18 17:44:56, jbudorick wrote:
> On 2015/06/17 at 21:55:55, mef wrote:
> > should this method be private?
> 
> No. Clients will call this explicitly to initialize the native side.

Acknowledged.

https://codereview.chromium.org/1174613002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:28:
assert mNativeEmbeddedTestServer != 0;
On 2015/06/18 17:44:56, jbudorick wrote:
> On 2015/06/17 at 21:55:55, mef wrote:
> > I'm unfamiliar with this notation, how does it work in regards to debug /
> release?
> > Should it be before 'if'?
> 
> I had based this on Tab, which uses assertions similarly. However, assertions
> are somewhat problematic, especially with art (see
> https://code.google.com/p/chromium/issues/detail?id=462676). I'm honestly not
> sure if it's better to leave the asserts for now and revise it when we address
> 462676 or do a one-off here.
> 
> Regardless, this should not be before the if. nativeInit will set
> mNativeEmbeddedTestServer if it's 0 to start with. The assertion just makes
sure
> that it has done so.

Acknowledged.

https://codereview.chromium.org/1174613002/diff/60001/net/test/embedded_test_...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/1174613002/diff/60001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.cc:3:
#include "base/android/jni_array.h"
unused?

[jbudorick, 2015-06-22 22:01:11]

Your sample use is correct; I've added it to the comments for
EmbeddedTestServer.

The only way it could conceivably be simplified while maintaining the same
interface as the C++ one is by combining 1 & 2. I'm not sure if we construct the
native side object in the java constructor anywhere else, though; I'll have to
check.

https://codereview.chromium.org/1174613002/diff/60001/net/test/embedded_test_...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/1174613002/diff/60001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.cc:3:
#include "base/android/jni_array.h"
On 2015/06/19 at 19:26:07, mef wrote:
> unused?

Yes. Removed.

[jbudorick, 2015-06-24 15:20:49]

Friendly ping?

[mef, 2015-06-24 15:24:11]

lgtm

[jbudorick, 2015-06-24 15:29:10]

On 2015/06/24 at 15:24:11, mef wrote:
> lgtm

Thanks!

[jbudorick, 2015-06-24 15:30:12]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-06-24 15:30:28]

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

[commit-bot: I haz the power, 2015-06-24 15:36:59]

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

[commit-bot: I haz the power, 2015-06-24 15:37:01]

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

[jbudorick, 2015-06-24 22:02:08]

jbudorick@chromium.org changed reviewers:
+ yfriedman@chromium.org

[jbudorick, 2015-06-24 22:02:09]

+yfriedman for owners review of base/

[Yaron, 2015-06-25 14:14:02]

On 2015/06/24 22:02:09, jbudorick wrote:
> +yfriedman for owners review of base/

lgtm

[jbudorick, 2015-06-25 14:14:46]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-06-25 14:14:46]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1174613002/#ps120001
(title: "add @VisibleForTesting to fix chrome_public_test_apk")

[commit-bot: I haz the power, 2015-06-25 14:15:19]

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

[commit-bot: I haz the power, 2015-06-25 15:22:02]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-25 15:22:54]

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

[jbudorick, 2015-06-25 15:43:28]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1210973002/ by jbudorick@chromium.org.

The reason for reverting is: broke main waterfall:
http://build.chromium.org/p/chromium/builders/Android/builds/42187.