[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom handler

[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom handler to support the java code.

Some tests in LoadUrlTest needs embedded_test_server custom handler.
Here to refactor the custom handler related tests, and implement custom
handler in embedded_test_server native code, and make it support
java code. (See bug 686247)

BUG=686247
Review-Url: https://codereview.chromium.org/2687573002
Cr-Commit-Position: refs/heads/master@{#479227}
Committed: https://chromium.googlesource.com/chromium/src/+/2b2fdbb4b96e498a7100bc07738f5406555bf466

[shenghuazhang, 2017-02-08 00:51:34]

shenghuazhang@chromium.org changed reviewers:
+ jbudorick@chromium.org, sgurun@chromium.org

[shenghuazhang, 2017-02-08 00:51:34]

This CL intends to add an AWEmbeddedTestServer.java, which could add custom
handlers for android webview tests. 
To make it work, registered the custom handler with the server in native side.
JNI bundles with native code at java side.

PS. found that the CL cannot avoid adding those several methods which also
implemented in CL https://codereview.chromium.org/2670643002/. I think could
close that one and merged into this to eliminate CL review cycles.

https://codereview.chromium.org/2687573002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/OWNERS (right):

https://codereview.chromium.org/2687573002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/OWNERS:1: per-file
*.aidl=set noparent
Add this file to avoid presubmit warning:

** Presubmit Warnings **
Found changes to IPC files without a security OWNER!

***************
net/test/android/javatests/src/org/chromium/net/test/OWNERS is missing the
following lines:

per-file *.aidl=set noparent
per-file *.aidl=file://ipc/SECURITY_OWNERS

for changed files:
 
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl

[jbudorick, 2017-02-08 01:11:43]

hmm, this isn't quite what I had in mind, but you've got some interesting ideas
here, particularly with having an optional android-specific group of handlers.
That said, I can imagine the net effect of this being that we have very specific
handlers custom handlers living in //net, far away from the code for which they
exist.

Can you write up a one-page design doc on this? I think it'd be good to hash out
possible design concerns there.

[shenghuazhang, 2017-02-11 02:33:44]

On 2017/02/08 01:11:43, jbudorick wrote:
> hmm, this isn't quite what I had in mind, but you've got some interesting
ideas
> here, particularly with having an optional android-specific group of handlers.
> That said, I can imagine the net effect of this being that we have very
specific
> handlers custom handlers living in //net, far away from the code for which
they
> exist.
> 
> Can you write up a one-page design doc on this? I think it'd be good to hash
out
> possible design concerns there.

Thanks for the comment. Wrote a design doc for it, please have a look at the
link below
https://docs.google.com/a/google.com/document/d/1o-_qMYEJLgqS13_KV7OVqqrEEwNg...

[shenghuazhang, 2017-03-04 00:22:21]

@john, can you please look into the patch 4?

https://codereview.chromium.org/2687573002/diff/20001/android_webview/embedde...
File android_webview/embeddedtestserver/aw_embedded_test_server_android.cc
(right):

https://codereview.chromium.org/2687573002/diff/20001/android_webview/embedde...
android_webview/embeddedtestserver/aw_embedded_test_server_android.cc:79:
test_server_.RegisterRequestHandler(base::Bind(&HandleClickRedirect));
Hard coded handler here.

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

https://codereview.chromium.org/2687573002/diff/40001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.h:45:
base::android::ScopedJavaLocalRef<jlongArray> GetCustomHandlers(
Have to add GetCustomHandlers and RegisterRequestHandler here for JNI use, due
to IEmbeddedTestServerImpl.aidl and EmbeddedTestServerImpl.java requesting them.

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

https://codereview.chromium.org/2687573002/diff/60001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.cc:7:
#include "android_webview/embeddedtestserver/custom_handlers.h"
Shows presubmit error:
Illegal include: "android_webview/embeddedtestserver/custom_handlers.h"
    Because of no rule applying.

Maybe need to add android_webview rule in //net/DEPS, but not sure if that's a
correct logic?

[sgurun-gerrit only, 2017-03-06 18:35:17]

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

https://codereview.chromium.org/2687573002/diff/60001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.cc:7:
#include "android_webview/embeddedtestserver/custom_handlers.h"
On 2017/03/04 00:22:21, shenghuazhang wrote:
> Shows presubmit error:
> Illegal include: "android_webview/embeddedtestserver/custom_handlers.h"
>     Because of no rule applying.
> 
> Maybe need to add android_webview rule in //net/DEPS, but not sure if that's a
> correct logic?

Not acceptable. Android webview is a layer on top of net.

[jbudorick, 2017-03-06 18:36:45]

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

https://codereview.chromium.org/2687573002/diff/60001/net/test/embedded_test_...
net/test/embedded_test_server/android/embedded_test_server_android.cc:7:
#include "android_webview/embeddedtestserver/custom_handlers.h"
On 2017/03/06 18:35:17, sgurun wrote:
> On 2017/03/04 00:22:21, shenghuazhang wrote:
> > Shows presubmit error:
> > Illegal include: "android_webview/embeddedtestserver/custom_handlers.h"
> >     Because of no rule applying.
> > 
> > Maybe need to add android_webview rule in //net/DEPS, but not sure if that's
a
> > correct logic?
> 
> Not acceptable. Android webview is a layer on top of net.

yeah, we discussed this & alternative mechanisms for handling it offline on
Friday. I forgot to publish my comment here, though :/

[shenghuazhang, 2017-03-09 00:06:00]

Patch #5 works and is ready to be reviewed. The structure of it is:

1. //android_webview/embeddedtestserver/custom_handlers.cc JNI binds with
CustomHandlers.java and passes in the custom handler function pointers (long[]
getHandlers()).
2. AwEmbeddedTestServerImpl.java extends from EmbeddedTestServerImpl.java and
gets the custom handlers pointers from CustomHandlers.java. It overrides the
method addDefaultHandlers s.t. it could register with both default and custom
handlers.
3. AwEmbeddedTestServer.java extends from EmbeddedTestServer.java. Replace the
package name for the service intent.
4. Bind AwEmbeddedTestServer with AwEmbeddedTestServerService and
AwEmbeddedTestServerImpl to make a new separate android apk
'aw_net_test_support_apk'. Set it as an additional_apks in test apk
'android_webview_test_apk', s.t. the test class is able to start AwETS which is
registered with the custom handlers.

Please have a review on it again. Thanks :)

[jbudorick, 2017-03-09 02:00:48]

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File android_webview/embeddedtestserver/aw_test_jni_onload.cc (right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/aw_test_jni_onload.cc:14: namespace net {
This should be android_webvew::test, not net::test. Similarly for everything in
//android_webview.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File android_webview/embeddedtestserver/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/AndroidManifest.xml:9:
package="org.chromium.net.test.awsupport">
org.chromium.android_webview.net.test.support? (It should definitely be
org.chromium.android_webview.*)

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/AndroidManifest.xml:15: <application
android:label="ChromiumNetTestAwsupport">
nit: ChromiumNetTestAwSupport

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServer.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServer.java:5:
package org.chromium.net.test;
Everything in this directory should be in org.chromium.android_webview.test.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServer.java:39:
public static AwEmbeddedTestServer createAndStartServer(Context context)
hmm, this logic is almost the same as the EmbeddedTestServer version. Is there a
way we can have the two share the common parts?

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServerImpl.java:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
nit: This shouldn't be android_webview/embeddedtestserver/...; maybe
android_webview/test/embedded_test_server/ ?

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServerImpl.java:18:
public AwEmbeddedTestServerImpl(Context context) {
I think this can be omitted.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File
android_webview/embeddedtestserver/java/src/org/chromium/net/test/CustomHandlers.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/CustomHandlers.java:20:
private native long[] nativeGetHandlers();
Does this need to be in its own class? Can it be in AwEmbeddedTestServerImpl?

also, I think this can/should be a static method.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/test/B...
File android_webview/test/BUILD.gn (right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/test/B...
android_webview/test/BUILD.gn:219: "//net/android:net_test_support_apk",
Do we need both?

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:79:
protected void setIntentClassName(Intent intent) {
nit: move this below initializeNative

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:40:
protected long mNativeEmbeddedTestServer;
This should be able to stay private.

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:220:
protected native void nativeAddDefaultHandlers(
Can we make these private and expose protected methods that the AW version can
call that don't require the native object?

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerInterface.aidl
(left):

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerInterface.aidl:5:
interface org.chromium.net.test.IEmbedddedTestServerImpl;
oops :)

https://codereview.chromium.org/2687573002/diff/100001/net/test/embedded_test...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/2687573002/diff/100001/net/test/embedded_test...
net/test/embedded_test_server/android/embedded_test_server_android.cc:65:
typedef std::unique_ptr<HttpResponse> (*HandlerUniquePtr)(
nit: maybe HandleRequestPtr?

[shenghuazhang, 2017-03-13 18:48:13]

shenghuazhang@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[shenghuazhang, 2017-03-13 18:48:15]

Refactored code based on John's comment.

+Phajdan

Hi Phajdan, this CL adds custom handlers to embedded_test_server_android under
//net/test, can you please have a review on it? Thanks!

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File android_webview/embeddedtestserver/aw_test_jni_onload.cc (right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/aw_test_jni_onload.cc:14: namespace net {
On 2017/03/09 02:00:47, jbudorick wrote:
> This should be android_webvew::test, not net::test. Similarly for everything
in
> //android_webview.

Done.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File android_webview/embeddedtestserver/custom_handlers.cc (right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/custom_handlers.cc:26: //
/click-redirect?url=URL&&echoheaders=HEADERS
Should be '/click-redirect?url=URL&echoheaders=HEADERS'

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File android_webview/embeddedtestserver/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/AndroidManifest.xml:9:
package="org.chromium.net.test.awsupport">
On 2017/03/09 02:00:47, jbudorick wrote:
> org.chromium.android_webview.net.test.support? (It should definitely be
> org.chromium.android_webview.*)

Done.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/AndroidManifest.xml:15: <application
android:label="ChromiumNetTestAwsupport">
On 2017/03/09 02:00:47, jbudorick wrote:
> nit: ChromiumNetTestAwSupport

Done.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServer.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServer.java:5:
package org.chromium.net.test;
On 2017/03/09 02:00:47, jbudorick wrote:
> Everything in this directory should be in org.chromium.android_webview.test.

Done.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServer.java:39:
public static AwEmbeddedTestServer createAndStartServer(Context context)
On 2017/03/09 02:00:47, jbudorick wrote:
> hmm, this logic is almost the same as the EmbeddedTestServer version. Is there
a
> way we can have the two share the common parts?

Yeah. Separated the common steps into a new method, and the return server type
of it would be generic that same as passed in.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServerImpl.java:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/03/09 02:00:47, jbudorick wrote:
> nit: This shouldn't be android_webview/embeddedtestserver/...; maybe
> android_webview/test/embedded_test_server/ ?

Done.

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/AwEmbeddedTestServerImpl.java:18:
public AwEmbeddedTestServerImpl(Context context) {
On 2017/03/09 02:00:47, jbudorick wrote:
> I think this can be omitted.

Hmm...Actually it would reveal constructor error without this.
http://stackoverflow.com/a/9370709

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
File
android_webview/embeddedtestserver/java/src/org/chromium/net/test/CustomHandlers.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/embedd...
android_webview/embeddedtestserver/java/src/org/chromium/net/test/CustomHandlers.java:20:
private native long[] nativeGetHandlers();
On 2017/03/09 02:00:47, jbudorick wrote:
> Does this need to be in its own class? Can it be in AwEmbeddedTestServerImpl?
> 
> also, I think this can/should be a static method.

Relocated them into AwEmbeddedTestServerImpl and removed this class.

But when I tried to define the native method as 'private static native', jni
will complain the method is used but never defined. Weird...

https://codereview.chromium.org/2687573002/diff/100001/android_webview/test/B...
File android_webview/test/BUILD.gn (right):

https://codereview.chromium.org/2687573002/diff/100001/android_webview/test/B...
android_webview/test/BUILD.gn:219: "//net/android:net_test_support_apk",
On 2017/03/09 02:00:48, jbudorick wrote:
> Do we need both?

No :) Removed it.

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:79:
protected void setIntentClassName(Intent intent) {
On 2017/03/09 02:00:48, jbudorick wrote:
> nit: move this below initializeNative

Done.

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:40:
protected long mNativeEmbeddedTestServer;
On 2017/03/09 02:00:48, jbudorick wrote:
> This should be able to stay private.

Done.

https://codereview.chromium.org/2687573002/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:220:
protected native void nativeAddDefaultHandlers(
On 2017/03/09 02:00:48, jbudorick wrote:
> Can we make these private and expose protected methods that the AW version can
> call that don't require the native object?

Done.

https://codereview.chromium.org/2687573002/diff/100001/net/test/embedded_test...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/2687573002/diff/100001/net/test/embedded_test...
net/test/embedded_test_server/android/embedded_test_server_android.cc:65:
typedef std::unique_ptr<HttpResponse> (*HandlerUniquePtr)(
On 2017/03/09 02:00:48, jbudorick wrote:
> nit: maybe HandleRequestPtr?

Done.

[jbudorick, 2017-03-13 23:32:54]

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/BUILD.gn (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/BUILD.gn:1: # Copyright 2017 The
Chromium Authors. All rights reserved.
nit: move this directory from android_webview/test/embeddedtestserver to
android_webview/test/embedded_test_server to match what //net does.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/BUILD.gn:14:
"../../../net/test/android/javatests/src/org/chromium/net/test/DummySpnegoAuthenticator.java",
This shouldn't be listing java files from //net. Instead, you should add
//net/android:net_java_test_support to deps below.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/BUILD.gn:46:
"../../../net/test/embedded_test_server/android/embedded_test_server_android.cc",
Similarly, this should depend on //net/android:java_test_native_support.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/aw_test_jni_onload.cc (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/aw_test_jni_onload.cc:21: return
net::test_server::EmbeddedTestServerAndroid::
hmm, can we have the net code handle this and just have ONJNIOnLoadRegisterJNI
call that?

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/aw_test_jni_onload.cc:25: bool
RegisterHandlersJNI(JNIEnv* env) {
w/ the change above, we could rename this to just RegisterJNI

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/custom_handlers.cc (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/custom_handlers.cc:29: //
/click-redirect?url=URL&echoheaders=HEADERS
Right now, you've got HEADERS as a URL-encoded list of headers. What if we just
allowed a user to specify echoheader= multiple times?

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/custom_handlers.cc:49: std::string
header_list = query.at("echoheaders").front();
w/ the above change, this code would just be iterating over
query.at("echoheader") -- we wouldn't need to get the string, then split it,
then iterate over that split.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/custom_handlers.h (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/custom_handlers.h:1: // Copyright 2017
The Chromium Authors. All rights reserved.
As these two files are the native side of AwEmbeddedTestServerImpl, they should
be named something like aw_embedded_test_server.h/.cc.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java:37:
callAddDefaultHandlers(directoryPath);
can this just call super.addDefaultHandlers(directoryPath)?

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java:40:
callRegisterRequestHandler(handler);
similarly, can this call registerRequestHandler(handler) (w/ the changes
suggested in EmbeddedTestServerImpl)

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java:47:
private native long[] nativeGetHandlers();
I think this should be private static native -- we don't need to pass the jobj
down.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/CustomHandlers.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/CustomHandlers.java:15:
public class CustomHandlers {
Do you still need this class? It doesn't look like it...

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:255:
public static <T extends EmbeddedTestServer> T createAndStartServer(T server,
Context context)
nit: maybe name this one "initializeAndStartServer" since it's not responsible
for creating the server.

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:262:
return (T) server;
server is already an instance of T. Why is the cast necessary?

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:47:
protected <V> V runOnHandlerThread(Callable<V> c) {
We shouldn't need to expose this to the subclass.

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:216:
protected void callRegisterRequestHandler(long handler) {
This should:
 - be public
 - be named registerRequestHandler
 - do the runOnHandlerThread dance that the other public methods do

https://codereview.chromium.org/2687573002/diff/120001/net/test/embedded_test...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/2687573002/diff/120001/net/test/embedded_test...
net/test/embedded_test_server/android/embedded_test_server_android.cc:72:
HandleRequestPtr& handler_ptr = reinterpret_cast<HandleRequestPtr&>(handler);
nit: I think this can (and should) just be HandleRequestPtr, not
HandleRequestPtr&.

[shenghuazhang, 2017-03-17 01:07:16]

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/BUILD.gn (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/BUILD.gn:1: # Copyright 2017 The
Chromium Authors. All rights reserved.
On 2017/03/13 23:32:54, jbudorick wrote:
> nit: move this directory from android_webview/test/embeddedtestserver to
> android_webview/test/embedded_test_server to match what //net does.

Done.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/BUILD.gn:14:
"../../../net/test/android/javatests/src/org/chromium/net/test/DummySpnegoAuthenticator.java",
On 2017/03/13 23:32:54, jbudorick wrote:
> This shouldn't be listing java files from //net. Instead, you should add
> //net/android:net_java_test_support to deps below.

Done.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/BUILD.gn:46:
"../../../net/test/embedded_test_server/android/embedded_test_server_android.cc",
On 2017/03/13 23:32:54, jbudorick wrote:
> Similarly, this should depend on //net/android:java_test_native_support.

'//net/android:java_test_native_support' involves the c++ file
'//net/test/android/net_test_entry_point.cc', it will cause multiple definition
of 'JNI_OnLoad' error with "aw_test_entry_point.cc". Have to add the source
files in '//net/android:java_test_native_support' individually here except for
'net_test_entry_point.cc' :(

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/aw_test_jni_onload.cc (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/aw_test_jni_onload.cc:21: return
net::test_server::EmbeddedTestServerAndroid::
On 2017/03/13 23:32:54, jbudorick wrote:
> hmm, can we have the net code handle this and just have ONJNIOnLoadRegisterJNI
> call that?

Done.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/aw_test_jni_onload.cc:25: bool
RegisterHandlersJNI(JNIEnv* env) {
On 2017/03/13 23:32:54, jbudorick wrote:
> w/ the change above, we could rename this to just RegisterJNI

Done.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/custom_handlers.cc (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/custom_handlers.cc:29: //
/click-redirect?url=URL&echoheaders=HEADERS
On 2017/03/13 23:32:54, jbudorick wrote:
> Right now, you've got HEADERS as a URL-encoded list of headers. What if we
just
> allowed a user to specify echoheader= multiple times?

Good suggestion! Also, now user need to encode the headers at test side:
mTestServer.getURL("/click-redirect?url=" + encodeUrl(redirected
     Url)
     +  "&echoheaders=" + encodeUrl(extraHeaders[0] + "&" + extraHeaders[2])

Using 'echoheader=' multiple times won't let user worry about the encoding
thing. (url still needs to be encoded though)

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File android_webview/test/embeddedtestserver/custom_handlers.h (right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/custom_handlers.h:1: // Copyright 2017
The Chromium Authors. All rights reserved.
On 2017/03/13 23:32:54, jbudorick wrote:
> As these two files are the native side of AwEmbeddedTestServerImpl, they
should
> be named something like aw_embedded_test_server.h/.cc.

Done.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java:37:
callAddDefaultHandlers(directoryPath);
On 2017/03/13 23:32:54, jbudorick wrote:
> can this just call super.addDefaultHandlers(directoryPath)?

Done.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java:40:
callRegisterRequestHandler(handler);
On 2017/03/13 23:32:54, jbudorick wrote:
> similarly, can this call registerRequestHandler(handler) (w/ the changes
> suggested in EmbeddedTestServerImpl)

Done.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/AwEmbeddedTestServerImpl.java:47:
private native long[] nativeGetHandlers();
On 2017/03/13 23:32:54, jbudorick wrote:
> I think this should be private static native -- we don't need to pass the jobj
> down.

Oh right! The native method GetHandlers was with parameter <jobject> s.t. the
method won't be a static one. Pass <jclass> instead and it works.

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
File
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/CustomHandlers.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/android_webview/test/e...
android_webview/test/embeddedtestserver/java/src/org/chromium/android_webview/test/CustomHandlers.java:15:
public class CustomHandlers {
On 2017/03/13 23:32:54, jbudorick wrote:
> Do you still need this class? It doesn't look like it...

Oops.. Should have removed this file in this patch.

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:255:
public static <T extends EmbeddedTestServer> T createAndStartServer(T server,
Context context)
On 2017/03/13 23:32:54, jbudorick wrote:
> nit: maybe name this one "initializeAndStartServer" since it's not responsible
> for creating the server.

Make sense! Will rename it.

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:262:
return (T) server;
On 2017/03/13 23:32:54, jbudorick wrote:
> server is already an instance of T. Why is the cast necessary?

Done.

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:47:
protected <V> V runOnHandlerThread(Callable<V> c) {
On 2017/03/13 23:32:54, jbudorick wrote:
> We shouldn't need to expose this to the subclass.

Done.

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:212:
protected void callAddDefaultHandlers(String directoryPath) {
removed this method since no class uses it.

https://codereview.chromium.org/2687573002/diff/120001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:216:
protected void callRegisterRequestHandler(long handler) {
On 2017/03/13 23:32:54, jbudorick wrote:
> This should:
>  - be public
>  - be named registerRequestHandler
>  - do the runOnHandlerThread dance that the other public methods do

Done.

https://codereview.chromium.org/2687573002/diff/120001/net/test/embedded_test...
File net/test/embedded_test_server/android/embedded_test_server_android.cc
(right):

https://codereview.chromium.org/2687573002/diff/120001/net/test/embedded_test...
net/test/embedded_test_server/android/embedded_test_server_android.cc:72:
HandleRequestPtr& handler_ptr = reinterpret_cast<HandleRequestPtr&>(handler);
On 2017/03/13 23:32:54, jbudorick wrote:
> nit: I think this can (and should) just be HandleRequestPtr, not
> HandleRequestPtr&.

Done.

[sgurun-gerrit only, 2017-03-17 08:03:03]

looked only a small portion.

https://codereview.chromium.org/2687573002/diff/140001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/2687573002/diff/140001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:163:
validateHeadersValue(awContents, contentsClient, extraHeaders, false);
the old test was testing that by loading an image url. I don't see how you are
doing it here. your url1 and url2 are same. look at the old tests carefully and
please do not regress the behavior.

https://codereview.chromium.org/2687573002/diff/140001/net/test/android/javat...
File net/test/android/javatests/src/org/chromium/net/test/OWNERS (right):

https://codereview.chromium.org/2687573002/diff/140001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/OWNERS:1: per-file
*.aidl=set noparent
was this requested by some reviewers?

[shenghuazhang, 2017-03-23 01:26:16]

@sgurun Patch #9 refactored all tests of LoadUrlTest.java which used 'apache'
package. Please have a review on it. Thanks!

https://codereview.chromium.org/2687573002/diff/140001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/2687573002/diff/140001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:163:
validateHeadersValue(awContents, contentsClient, extraHeaders, false);
On 2017/03/17 08:03:02, sgurun wrote:
> the old test was testing that by loading an image url. I don't see how you are
> doing it here. your url1 and url2 are same. look at the old tests carefully
and
> please do not regress the behavior. 

Oh yes you are right. I meant to focus this patch into infrastructure, and
refactor all the tests by adding more custom handlers for them in next patch/CL,
but this incompleted code seem like causing confusion :P. Sorry for the mistake!


In patch 9, I refactored the tests to match the origin behaviors, please have a
look at them again, thanks!

https://codereview.chromium.org/2687573002/diff/140001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:210:
public void testRedirectAndReloadWithExtraHeaders() throws Throwable {
Refactor this test as well in patch 9.

https://codereview.chromium.org/2687573002/diff/140001/net/test/android/javat...
File net/test/android/javatests/src/org/chromium/net/test/OWNERS (right):

https://codereview.chromium.org/2687573002/diff/140001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/OWNERS:1: per-file
*.aidl=set noparent
On 2017/03/17 08:03:02, sgurun wrote:
> was this requested by some reviewers? 

No. But there is a presubmit warnings for the changed file:
"//net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerInterface.aidl"

** Presubmit Warnings **
Found changes to IPC files without a security OWNER!

***************
net/test/android/javatests/src/org/chromium/net/test/OWNERS is missing the
following lines:

per-file *.aidl=set noparent
per-file *.aidl=file://ipc/SECURITY_OWNERS

for changed files:
 
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerInterface.aidl
***************

So have to create this OWNERS file following the warnings instruction.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:191:
assertFalse(textContent.contains(FOUND_EXTRA_HEADER));
Check the extra headers not exist in the image source url from native side -
handler 'HandleSetImageResponse' (see more comments in
//net/test/embedded_test_server/android/embedded_test_server_android.cc)
and check the result here.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:256:
awContents, contentsClient, extraHeaders, ECHO_CURRENT_URL_HEADER, true);
The page body text content is in json format, like,

  {
    "echo header": [header1, header2],
    "data content": [header3, header4];
  }

"echo header" contains the header results in the current request
(redirectedUrl).
"data content" contains the header results of the initialUrl.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_embedded_test_server.cc
(right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:159: if
(query.find("header") != query.end()) {
The test 'testLoadUrlWithExtraHeaders' uses this handler, which would check the
extra header NOT passed in the image resource.
The thing about checking header value result in the image resource request is,
since the image resource url is not supposed to be loaded, Java test side is
hardly able to check them directly by javascript (getting text content on loaded
page). So we check the extra headers in native code here, and if validation
failed, send out an error in the response.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:176: else {
if the extra header exist, set '404' status code to response instead of set the
decoded resource.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:217:
"src='%s' onError='setContent()'></div></body></html>",
If the image src url sent an error in response, call the function 'setContent()'
to set text content to html, s.t. Java side could know that something wrong with
the image url loading.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:217:
"src='%s' onError='setContent()'></div></body></html>",
This way is a little hacky. Since we expect the 'Found Extra Header' error, but
'onError' will handle whatever image url response error as the header error. So
if the test failed, have to dig into the native code and explore the details,
though I think this way would work.

[sgurun-gerrit only, 2017-03-29 18:19:27]

thanks for working on this. I would be happier if there was a way to simplify
things a little bit - unfortunately I think the "statefullness" of old test
server was making tests more readable. Now the logic is split between native and
Java as you would also acknowledge.

So I may ask for more changes in a next round if I can find a way to simplify.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:35:
private static final String ECHO_CURRENT_URL_HEADER = "echo header";
Move these two static strings to the method they are used in. They are related
to redirect case only.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:35:
private static final String ECHO_CURRENT_URL_HEADER = "echo header";
also rename ECHO_CURRENT_URL_HEADER to ECHO_REDIRECTED_URL_HEADER

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:137:
final TestAwContentsClient contentsClient, String[] extraHeader, boolean
realValue)
s/realValue/shouldHeaderExist/

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:148:
boolean realValue) throws Exception {
s/realValue/shouldHeaderExists/

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:179:
final String url1 = mTestServer.getURL(
the handler image-onload-html is very specific to this test. Url1 is not used
locally except for passing to the handler as a query param. Move it to the
handler, dropping imagesrc parameter.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:182:
final String url2 = mTestServer.getURL("/image-onload-html?imagesrc=" +
encodeUrl(url1)
rename url2 to url once you move url1 to handler

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:191:
assertFalse(textContent.contains(FOUND_EXTRA_HEADER));
On 2017/03/23 01:26:16, shenghuazhang wrote:
> Check the extra headers not exist in the image source url from native side -
> handler 'HandleSetImageResponse' (see more comments in
> //net/test/embedded_test_server/android/embedded_test_server_android.cc)
> and check the result here.

I think this is racy. You are relying on the JS code that fetches the image to
execute and finish its execution to confirm that image loading with extra
headers did not happen. this is probably why you added the title at the handler
js: you can wait for title to be updated to confirm js worked and use it to
assert here

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:256:
awContents, contentsClient, extraHeaders, ECHO_CURRENT_URL_HEADER, true);
On 2017/03/23 01:26:16, shenghuazhang wrote:
> The page body text content is in json format, like,
> 
>   {
>     "echo header": [header1, header2],
>     "data content": [header3, header4];
>   }
> 
> "echo header" contains the header results in the current request
> (redirectedUrl).
> "data content" contains the header results of the initialUrl.

Acknowledged.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/BUILD.gn (right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/BUILD.gn:36:
"../../../net/test/embedded_test_server/android/embedded_test_server_android.h",
should not be included this way. It should be part of a net library that we
depend on. you already have a test_support as deps, doesn't it include these
files?

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_embedded_test_server.cc
(right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:28: 
put the handlers in a anonymous namespace for internal linkage, i.e. namespace {

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:29: //
/click-redirect?url=URL&echoheader=HEADER
s/echoheader/header/

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:144: //
/set-image-response?resource=RESOURCE&header=HEADER
rename to image-response-if-header-not-exists

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:212: "{
document.title=document.getElementById('img').naturalHeight } "
where do you use the title?

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_test_jni_onload.cc (right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_test_jni_onload.cc:22:
TRACE_EVENT0("native", "android_webview::test::RegisterJNI");
why is it useful to have trace_event here?

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_test_jni_onload.h (right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_test_jni_onload.h:5: #ifndef
ANDROID_WERBVIEW_TEST_EMBEDDED_TEST_SERVER_ANDROID_WEBVIEW_TEST_JNI_ONLOAD_H_
ANDROID_WEBVIEW_
here and everywhere below.

[shenghuazhang, 2017-04-11 01:38:27]

Thanks for the suggestion! Refactored the test case and handler by verifying
title instead of JS code.

Still have some discussion in LoadUrlTest.java &
android_webview/test/embedded_test_server/BUILD.gn, please look at the comment
in patch #9.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:35:
private static final String ECHO_CURRENT_URL_HEADER = "echo header";
On 2017/03/29 18:19:27, sgurun wrote:
> also rename ECHO_CURRENT_URL_HEADER to ECHO_REDIRECTED_URL_HEADER

Done.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:137:
final TestAwContentsClient contentsClient, String[] extraHeader, boolean
realValue)
On 2017/03/29 18:19:27, sgurun wrote:
> s/realValue/shouldHeaderExist/

Done.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:148:
boolean realValue) throws Exception {
On 2017/03/29 18:19:27, sgurun wrote:
> s/realValue/shouldHeaderExists/

Done.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:179:
final String url1 = mTestServer.getURL(
On 2017/03/29 18:19:27, sgurun wrote:
> the handler image-onload-html is very specific to this test. Url1 is not used
> locally except for passing to the handler as a query param. Move it to the
> handler, dropping imagesrc parameter.

Hmm... If the image resource is the only thing we need, then I guess the url1
could be omitted, and simply make use of the image resource in the handler of
'image-onload-html' directly.

But in the test we will also verify the header values of the image resource
url(url1), and the header should be potentially embedded in the request handled
by handler 'set-image-response'(url1), which is not the one handled by handler
'image-onload-html'(url2).

S.t. we still need a real url request of the image. So the server instance here
to make it's own request:
    final String url1 = mTestServer.getURL("/set-image-response?...)

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:182:
final String url2 = mTestServer.getURL("/image-onload-html?imagesrc=" +
encodeUrl(url1)
On 2017/03/29 18:19:27, sgurun wrote:
> rename url2 to url once you move url1 to handler

Same reason as above.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:191:
assertFalse(textContent.contains(FOUND_EXTRA_HEADER));
On 2017/03/29 18:19:27, sgurun wrote:
> On 2017/03/23 01:26:16, shenghuazhang wrote:
> > Check the extra headers not exist in the image source url from native side -
> > handler 'HandleSetImageResponse' (see more comments in
> > //net/test/embedded_test_server/android/embedded_test_server_android.cc)
> > and check the result here.
> 
> I think this is racy. You are relying on the JS code that fetches the image to
> execute and finish its execution to confirm that image loading with extra
> headers did not happen. this is probably why you added the title at the
handler
> js: you can wait for title to be updated to confirm js worked and use it to
> assert here

Verifying the title here is a less flaky and more stable way than the JS loading
way as our offline discussion conclusion. Will refactor this test and the
handler to fit it.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/BUILD.gn (right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/BUILD.gn:36:
"../../../net/test/embedded_test_server/android/embedded_test_server_android.h",
On 2017/03/29 18:19:27, sgurun wrote:
> should not be included this way. It should be part of a net library that we
> depend on. you already have a test_support as deps, doesn't it include these
> files?

'//net/android:java_test_native_support' includes these files.

And it also involves the c++ file '//net/test/android/net_test_entry_point.cc',
it will cause multiple definition of 'JNI_OnLoad' error with
"aw_test_entry_point.cc". S.t have to add the source files in these separate
way.

If would like to not include //net files in this way, need to split the source
set 
'java_test_native_support' in //net/android/BUILD.gn to something like:

source_set("java_test_native_jni_support") {
  testonly = true
  sources = [
    "../test/android/net_test_jni_onload.cc",
    "../test/android/net_test_jni_onload.h",
    "../test/embedded_test_server/android/embedded_test_server_android.cc",
    "../test/embedded_test_server/android/embedded_test_server_android.h",
  ]

  deps = [
    "//net:test_support",
  ]
  public_deps = [
    "//net:net_test_jni_headers",
  ]
}

source_set("java_test_native_support") {
  testonly = true
  sources = [
    "../test/android/net_test_entry_point.cc",
  ]

  deps = [
    ":java_test_native_jni_support",
  ]
}

And add the '//net/android/java_test_native_jni_support' as one of the deps
here. But those are kind of weird looking as separate supporting sets in
//net/android/BUILD.gn.

The current way and the split way, which one is preferred?

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_embedded_test_server.cc
(right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:28: 
On 2017/03/29 18:19:27, sgurun wrote:
> put the handlers in a anonymous namespace for internal linkage, i.e. namespace
{

Done.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:29: //
/click-redirect?url=URL&echoheader=HEADER
On 2017/03/29 18:19:27, sgurun wrote:
> s/echoheader/header/

Done.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:144: //
/set-image-response?resource=RESOURCE&header=HEADER
On 2017/03/29 18:19:27, sgurun wrote:
> rename to image-response-if-header-not-exists
> 

Done.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_test_jni_onload.cc (right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_test_jni_onload.cc:22:
TRACE_EVENT0("native", "android_webview::test::RegisterJNI");
On 2017/03/29 18:19:27, sgurun wrote:
> why is it useful to have trace_event here?

Added for debugging reason. Will remove it.

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_test_jni_onload.h (right):

https://codereview.chromium.org/2687573002/diff/160001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_test_jni_onload.h:5: #ifndef
ANDROID_WERBVIEW_TEST_EMBEDDED_TEST_SERVER_ANDROID_WEBVIEW_TEST_JNI_ONLOAD_H_
On 2017/03/29 18:19:27, sgurun wrote:
> ANDROID_WEBVIEW_
> here and everywhere below.

Done.

https://codereview.chromium.org/2687573002/diff/180001/android_webview/test/e...
File android_webview/test/embedded_test_server/aw_embedded_test_server.cc
(right):

https://codereview.chromium.org/2687573002/diff/180001/android_webview/test/e...
android_webview/test/embedded_test_server/aw_embedded_test_server.cc:212: "{
document.title=document.getElementById('img').naturalHeight } "
Here set the page title to be the height of the image source if loaded
successfully. To check the header value not exist in the image source url, test
#testLoadUrlWithExtraHeaders will verify the page title loaded.

[shenghuazhang, 2017-04-13 19:23:51]

Description was changed from

==========
[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom
handler to support the java code.

Some tests in LoadUrlTest needs embedded_test_server custom handler.
Here to refactor the custom handler related tests, and implement custom
handler in embedded_test_server native code, and make it support
java code. (See bug 686247)

BUG=488194
==========

to

==========
[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom
handler to support the java code.

Some tests in LoadUrlTest needs embedded_test_server custom handler.
Here to refactor the custom handler related tests, and implement custom
handler in embedded_test_server native code, and make it support
java code. (See bug 686247)

BUG=707845
==========

[shenghuazhang, 2017-04-13 19:27:13]

Description was changed from

==========
[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom
handler to support the java code.

Some tests in LoadUrlTest needs embedded_test_server custom handler.
Here to refactor the custom handler related tests, and implement custom
handler in embedded_test_server native code, and make it support
java code. (See bug 686247)

BUG=707845
==========

to

==========
[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom
handler to support the java code.

Some tests in LoadUrlTest needs embedded_test_server custom handler.
Here to refactor the custom handler related tests, and implement custom
handler in embedded_test_server native code, and make it support
java code. (See bug 686247)

BUG=686247
==========

[sgurun-gerrit only, 2017-04-21 23:28:12]

lgtm

[shenghuazhang, 2017-04-21 23:37:06]

Hi @phajdan.jr, can you please have a review on the //net project? Thanks!

[Paweł Hajdan Jr., 2017-04-26 07:48:08]

net/test LGTM

[shenghuazhang, 2017-04-26 20:34:16]

The CQ bit was checked by shenghuazhang@chromium.org

[shenghuazhang, 2017-04-26 20:34:16]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2687573002/#ps200001
(title: "Rebase master")

[commit-bot: I haz the power, 2017-04-26 20:35:10]

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, 2017-04-26 21:28:21]

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

[commit-bot: I haz the power, 2017-04-26 21:28:23]

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

[shenghuazhang, 2017-04-27 01:01:11]

The CQ bit was checked by shenghuazhang@chromium.org

[shenghuazhang, 2017-04-27 01:01:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2687573002/#ps220001
(title: "Convert long long to int64_t")

[commit-bot: I haz the power, 2017-04-27 01:02:29]

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, 2017-04-27 02:20:31]

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

[commit-bot: I haz the power, 2017-04-27 02:20:32]

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

[shenghuazhang, 2017-04-28 18:24:53]

The CQ bit was checked by shenghuazhang@chromium.org

[commit-bot: I haz the power, 2017-04-28 18:26:02]

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, 2017-04-28 19:49:33]

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

[commit-bot: I haz the power, 2017-04-28 19:49:35]

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

[shenghuazhang, 2017-04-28 22:27:51]

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

[commit-bot: I haz the power, 2017-04-28 22:28:44]

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, 2017-04-28 23:49:35]

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

[commit-bot: I haz the power, 2017-04-28 23:49:36]

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

[shenghuazhang, 2017-06-13 20:49:55]

The CQ bit was checked by shenghuazhang@chromium.org

[shenghuazhang, 2017-06-13 20:49:56]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2687573002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-13 20:50:29]

CQ is trying da patch.

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

[shenghuazhang, 2017-06-13 22:52:22]

The CQ bit was unchecked by shenghuazhang@chromium.org

[shenghuazhang, 2017-06-13 22:59:21]

The CQ bit was checked by shenghuazhang@chromium.org

[shenghuazhang, 2017-06-13 22:59:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2687573002/#ps280001
(title: "Change AwETS intent class name")

[commit-bot: I haz the power, 2017-06-13 23:00:06]

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, 2017-06-14 00:30:39]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1497394761104550,
"parent_rev": "fb6d187db2e842f01db923c87d7958b52175c37f", "commit_rev":
"2b2fdbb4b96e498a7100bc07738f5406555bf466"}

[commit-bot: I haz the power, 2017-06-14 00:30:52]

Description was changed from

==========
[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom
handler to support the java code.

Some tests in LoadUrlTest needs embedded_test_server custom handler.
Here to refactor the custom handler related tests, and implement custom
handler in embedded_test_server native code, and make it support
java code. (See bug 686247)

BUG=686247
==========

to

==========
[Android Webview] Refactor LoadUrlTest and work on embedded_test_server custom
handler to support the java code.

Some tests in LoadUrlTest needs embedded_test_server custom handler.
Here to refactor the custom handler related tests, and implement custom
handler in embedded_test_server native code, and make it support
java code. (See bug 686247)

BUG=686247

Review-Url: https://codereview.chromium.org/2687573002
Cr-Commit-Position: refs/heads/master@{#479227}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b2fdbb4b96e498a7100bc07738f...
==========

[commit-bot: I haz the power, 2017-06-14 00:30:54]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/2b2fdbb4b96e498a7100bc07738f...