[Android] Add ChromiumNetTestSupport.apk for the java EmbeddedTestServer.

[Android] Add ChromiumNetTestSupport.apk for the java EmbeddedTestServer.

BUG=488195
Committed: https://crrev.com/06215e9286247b08ff6c958f725c6e5185c4acd1
Cr-Commit-Position: refs/heads/master@{#365254}

[jbudorick, 2015-11-24 04:51:39]

jbudorick@chromium.org changed reviewers:
+ agrieve@chromium.org, mef@chromium.org, tedchoc@chromium.org,
yfriedman@chromium.org

[jbudorick, 2015-11-24 04:51:40]

This is a continuation of https://codereview.chromium.org/1218053003/ based on
cjhopman's last comment on that review. I've opted to pursue option 2 because
it's the simplest:
 - option 1 would require quite a few more infrastructure changes, and
 - option 3 involves some interesting issues because the main apk dex(es) aren't
loaded in the separate process, which makes calling the main LibraryLoader JNI
hook difficult :(

This CL does not deploy this server for any test. That would come in subsequent
CLs.

ptal:
tedchoc, yfriedman: base/android/ in particular, whatever else you feel like
agrieve: gyp + gn
mef: net/

[agrieve, 2015-11-24 16:27:14]

Just one question.

https://codereview.chromium.org/1465383003/diff/60001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1465383003/diff/60001/build/config/android/ru...
build/config/android/rules.gni:1884: "//base:base_java",
Shouldn't be necessary?

https://codereview.chromium.org/1465383003/diff/60001/net/test/android/javate...
File net/test/android/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/1465383003/diff/60001/net/test/android/javate...
net/test/android/javatests/AndroidManifest.xml:24:
android:process=":embedded_test_server"
nit: Probably unnecessary?

[jbudorick, 2015-11-24 18:19:28]

https://codereview.chromium.org/1465383003/diff/60001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1465383003/diff/60001/build/config/android/ru...
build/config/android/rules.gni:1884: "//base:base_java",
On 2015/11/24 16:27:14, agrieve wrote:
> Shouldn't be necessary?

nope, removed

https://codereview.chromium.org/1465383003/diff/60001/net/test/android/javate...
File net/test/android/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/1465383003/diff/60001/net/test/android/javate...
net/test/android/javatests/AndroidManifest.xml:24:
android:process=":embedded_test_server"
On 2015/11/24 16:27:14, agrieve wrote:
> nit: Probably unnecessary? 

Yeah, this was very necessary when I was trying a version in the test APK, but
it's now vestigial.

[agrieve, 2015-11-24 18:29:25]

lgtm

[Yaron, 2015-11-24 19:06:28]

https://codereview.chromium.org/1465383003/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1465383003/diff/80001/base/BUILD.gn#newcode1692
base/BUILD.gn:1692: "*/NativeTestLibraries##*.class",
Why isn't this needed for gyp?

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

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:78:
synchronized (mImplMonitor) {
Nit: add logs around this so we can see in logcat if this disappear here

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:65:
mHandlerThread = new HandlerThread("EmbeddedTestServer" +
sCount.getAndIncrement());
Wondering why you need a dedicated handler thread?

[mef, 2015-11-24 19:18:43]

On 2015/11/24 19:06:28, Yaron wrote:
> https://codereview.chromium.org/1465383003/diff/80001/base/BUILD.gn
> File base/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1465383003/diff/80001/base/BUILD.gn#newcode1692
> base/BUILD.gn:1692: "*/NativeTestLibraries##*.class",
> Why isn't this needed for gyp?
> 
>
https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
> File
> net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
> (right):
> 
>
https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
>
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:78:
> synchronized (mImplMonitor) {
> Nit: add logs around this so we can see in logcat if this disappear here
> 
>
https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
> File
>
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java
> (right):
> 
>
https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
>
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:65:
> mHandlerThread = new HandlerThread("EmbeddedTestServer" +
> sCount.getAndIncrement());
> Wondering why you need a dedicated handler thread?

Just to make sure that I understand, this is a wrapper of native
EmbeddedTestServer packaged as a separate APK, which is executed as service on
the device to replace Apache Http Server?

[jbudorick, 2015-11-25 02:43:40]

mef: yep, that's exactly what's going on.

https://codereview.chromium.org/1465383003/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1465383003/diff/80001/base/BUILD.gn#newcode1692
base/BUILD.gn:1692: "*/NativeTestLibraries##*.class",
On 2015/11/24 19:06:28, Yaron wrote:
> Why isn't this needed for gyp?

I don't think this is needed for GN any more. Removed.

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

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:78:
synchronized (mImplMonitor) {
On 2015/11/24 19:06:28, Yaron wrote:
> Nit: add logs around this so we can see in logcat if this disappear here

Done.

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:79:
// TODO(jbudorick): Add a fatal timeout here.
TODO removed and somewhat addressed.

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java
(right):

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:65:
mHandlerThread = new HandlerThread("EmbeddedTestServer" +
sCount.getAndIncrement());
On 2015/11/24 19:06:28, Yaron wrote:
> Wondering why you need a dedicated handler thread?

The embedded test server checks what thread it's on in a few locations. Without
a separate thread, we fail those checks because the calls get handled by
different threads in the pool. (Specifically, I was hitting this:
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_...)

[Yaron, 2015-11-25 16:04:55]

last round from me - just 2 main q's

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

https://codereview.chromium.org/1465383003/diff/80001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:79:
// TODO(jbudorick): Add a fatal timeout here.
On 2015/11/25 02:43:40, jbudorick wrote:
> TODO removed and somewhat addressed.

"somewhat" :)

https://codereview.chromium.org/1465383003/diff/100001/base/test/android/java...
File base/test/android/java/templates/NativeTestLibraries.template (right):

https://codereview.chromium.org/1465383003/diff/100001/base/test/android/java...
base/test/android/java/templates/NativeTestLibraries.template:1: #define
NATIVE_LIBRARIES_PACKAGE org.chromium.base.test.library_loader
copyright header? (don't actually know if it's needed but i assume it may as
well be)

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp#newcode1475
net/net.gyp:1475: 'target_name': 'require_net_test_support_apk',
Does anything depend on this? Something you want to guarantee it built? Is this
to avoid its classpath getting passed to the next target? Can you at least add a
comment for this

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

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:158:
mHandlerThread.join();
I'm wondering what's better in the case of failures to shut down? Is it better
to timeout and null out mHandler (to ensure you don't re-use a stuck server) or
fail like you currently do.

[mef, 2015-11-25 17:20:22]

https://codereview.chromium.org/1465383003/diff/80001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1465383003/diff/80001/net/net.gyp#newcode1417
net/net.gyp:1417: 'target_name': 'embedded_test_server_aidl',
FWIW I don't think calling this 'embedded' is appropriate. Should it be
something like 'ChromiumTestServerService'?

https://codereview.chromium.org/1465383003/diff/80001/net/net.gyp#newcode1464
net/net.gyp:1464: 'apk_name': 'ChromiumNetTestSupport',
Do we expect something in addition to embedded_test_server to go into
ChromiumNetTestSupport? If not, would it make sense to name this accordingly?

https://codereview.chromium.org/1465383003/diff/80001/net/net.gyp#newcode1475
net/net.gyp:1475: 'target_name': 'require_net_test_support_apk',
what does this target do?

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp
File net/net.gyp (left):

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp#oldcode1426
net/net.gyp:1426: 'net_test_support',
Where does |net_test_support| go now?

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

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:19:
* Java bindings for the native embedded test server.
This comment is no longer true.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:34:
public class EmbeddedTestServer {
Maybe call it LocalTestServer instead? It isn't embedded, is it?

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:138:
throw new EmbeddedTestServerFailure("Failed to start server: " + e.toString());
I think it makes sense to include |e| as the |cause| for new exception.

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

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:21:
* Java bindings for the native embedded test server.
This comment is wrong.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:73:
TestLibraryLoader.loadLibraries();
Is this Ok to run multiple times? Does it need to run on handler thread?

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:74:
if (mNativeEmbeddedTestServer == 0) nativeInit();
In what circumstances is mNativeEmbeddedTestServer != 0?

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:90:
nativeServeFilesFromDirectory(mNativeEmbeddedTestServer, directoryPath);
I think this should check for directory existence and accessibility.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl
(right):

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl:20:
*  @param directoryPath The path of the directory from which files should be
served.
what happens if directory is not accessible from server process?

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
File net/test/android/javatests/src_dummy/org/chromium/net/test/dummy/Dummy.java
(right):

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src_dummy/org/chromium/net/test/dummy/Dummy.java:9: *
TODO(newt): Delete after the switch to GN. This is only used in GYP.
Why is this needed?

[jbudorick, 2015-12-01 15:38:23]

Kept the EmbeddedTestServer name and held off on adding the directory checks on
top of the existing native behavior for now, but otherwise addressed comments.

https://codereview.chromium.org/1465383003/diff/100001/base/test/android/java...
File base/test/android/java/templates/NativeTestLibraries.template (right):

https://codereview.chromium.org/1465383003/diff/100001/base/test/android/java...
base/test/android/java/templates/NativeTestLibraries.template:1: #define
NATIVE_LIBRARIES_PACKAGE org.chromium.base.test.library_loader
On 2015/11/25 16:04:54, Yaron wrote:
> copyright header? (don't actually know if it's needed but i assume it may as
> well be)

Done.

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp#newcode1449
net/net.gyp:1449: 'net_test_support',
net_test_support goes here now.

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp#newcode1475
net/net.gyp:1475: 'target_name': 'require_net_test_support_apk',
On 2015/11/25 16:04:55, Yaron wrote:
> Does anything depend on this? Something you want to guarantee it built? Is
this
> to avoid its classpath getting passed to the next target? Can you at least add
a
> comment for this

Nothing depends on this currently, though targets will once they start using it.
It is to avoid the classpath getting passed, and I'll add a comment. I pulled
the same trick over in the instrumentation driver apk:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/android/on...

It's much cleaner in gn where I can just use data_deps.

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

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:19:
* Java bindings for the native embedded test server.
On 2015/11/25 17:20:22, mef wrote:
> This comment is no longer true.

Yeah, this first sentence is no longer accurate.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:34:
public class EmbeddedTestServer {
On 2015/11/25 17:20:22, mef wrote:
> Maybe call it LocalTestServer instead? It isn't embedded, is it?

It's not, but it is still a java endpoint for embedded_test_server.h/.cc and is
named to reflect that.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:138:
throw new EmbeddedTestServerFailure("Failed to start server: " + e.toString());
On 2015/11/25 17:20:22, mef wrote:
> I think it makes sense to include |e| as the |cause| for new exception.

Done here and in similar cases below.

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

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:21:
* Java bindings for the native embedded test server.
On 2015/11/25 17:20:22, mef wrote:
> This comment is wrong.

Here the first sentence is right, but everything else isn't.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:73:
TestLibraryLoader.loadLibraries();
On 2015/11/25 17:20:22, mef wrote:
> Is this Ok to run multiple times? Does it need to run on handler thread?

AFAICT it should be fine, but no, it doesn't need to run on the handler thread.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:74:
if (mNativeEmbeddedTestServer == 0) nativeInit();
On 2015/11/25 17:20:22, mef wrote:
> In what circumstances is mNativeEmbeddedTestServer != 0?

In normal use, it shouldn't be. If a client called initializeNative more than
once, it could be, and this would safely handle that case.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:90:
nativeServeFilesFromDirectory(mNativeEmbeddedTestServer, directoryPath);
On 2015/11/25 17:20:22, mef wrote:
> I think this should check for directory existence and accessibility.

I had intended to deviate as little as possible from the native
EmbeddedTestServer, which will handle both cases by returning 404s.

Surfacing this error from across processes will deviate from the native
EmbeddedTestServer behavior, will required redoing the aidl, and will be
inconsistent with the behavior of loadUrl unless I update that to check for
existence of that as well if we're serving from a directory (which I'm not sure
we want to do).

If all of that is ok with you, then I can try to do that.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:158:
mHandlerThread.join();
On 2015/11/25 16:04:55, Yaron wrote:
> I'm wondering what's better in the case of failures to shut down? Is it better
> to timeout and null out mHandler (to ensure you don't re-use a stuck server)
or
> fail like you currently do.

That actually raises a good point; EmbeddedTestServer should be unbinding the
service on calls to destroy() (which will also prevent reuse of a stuck server).

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl
(right):

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl:20:
*  @param directoryPath The path of the directory from which files should be
served.
On 2015/11/25 17:20:22, mef wrote:
> what happens if directory is not accessible from server process?

404, same as if the directory doesn't exist.

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
File net/test/android/javatests/src_dummy/org/chromium/net/test/dummy/Dummy.java
(right):

https://codereview.chromium.org/1465383003/diff/100001/net/test/android/javat...
net/test/android/javatests/src_dummy/org/chromium/net/test/dummy/Dummy.java:9: *
TODO(newt): Delete after the switch to GN. This is only used in GYP.
On 2015/11/25 17:20:22, mef wrote:
> Why is this needed?

java_in_dir (from build/java_apk.gypi) has to find something or else gyp chokes.
:(

[Yaron, 2015-12-01 22:43:31]

lgtm

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp#newcode1475
net/net.gyp:1475: 'target_name': 'require_net_test_support_apk',
On 2015/12/01 15:38:23, jbudorick wrote:
> On 2015/11/25 16:04:55, Yaron wrote:
> > Does anything depend on this? Something you want to guarantee it built? Is
> this
> > to avoid its classpath getting passed to the next target? Can you at least
add
> a
> > comment for this
> 
> Nothing depends on this currently, though targets will once they start using
it.
> It is to avoid the classpath getting passed, and I'll add a comment. I pulled
> the same trick over in the instrumentation driver apk:
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/android/on...
> 
> It's much cleaner in gn where I can just use data_deps.

sg but there's no comment ;)

[jbudorick, 2015-12-02 01:13:27]

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1465383003/diff/100001/net/net.gyp#newcode1475
net/net.gyp:1475: 'target_name': 'require_net_test_support_apk',
On 2015/12/01 22:43:31, Yaron wrote:
> On 2015/12/01 15:38:23, jbudorick wrote:
> > On 2015/11/25 16:04:55, Yaron wrote:
> > > Does anything depend on this? Something you want to guarantee it built? Is
> > this
> > > to avoid its classpath getting passed to the next target? Can you at least
> add
> > a
> > > comment for this
> > 
> > Nothing depends on this currently, though targets will once they start using
> it.
> > It is to avoid the classpath getting passed, and I'll add a comment. I
pulled
> > the same trick over in the instrumentation driver apk:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/android/on...
> > 
> > It's much cleaner in gn where I can just use data_deps.
> 
> sg but there's no comment ;)

oops, fixed

[mef, 2015-12-02 16:00:08]

lgtm with nits.

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
File net/test/android/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
net/test/android/javatests/AndroidManifest.xml:12: <uses-permission
android:name="android.permission.ACCESS_MOCK_LOCATION" />
Does it need all these permissions? Like Mock location?

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

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:52:
public void initializeNative() {
nit: Should use @Override annotation here and on other methods that extend
IEmbeddedTestServerImpl.Stub ?

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl
(right):

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl:12:
/** Starts the server.
nit: Starts -> Start

[jbudorick, 2015-12-03 00:03:43]

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

[jbudorick, 2015-12-03 00:03:44]

+thakis for base/BUILD.gn and base/base.gyp

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
File net/test/android/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
net/test/android/javatests/AndroidManifest.xml:12: <uses-permission
android:name="android.permission.ACCESS_MOCK_LOCATION" />
On 2015/12/02 16:00:08, mef wrote:
> Does it need all these permissions? Like Mock location?

ah, no, just external storage.

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

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java:52:
public void initializeNative() {
On 2015/12/02 16:00:08, mef wrote:
> nit: Should use @Override annotation here and on other methods that extend
> IEmbeddedTestServerImpl.Stub ?

Done.

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl
(right):

https://codereview.chromium.org/1465383003/diff/140001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/IEmbeddedTestServerImpl.aidl:12:
/** Starts the server.
On 2015/12/02 16:00:08, mef wrote:
> nit: Starts -> Start

Done.

[mef, 2015-12-03 00:16:44]

lgtm

[Nico, 2015-12-03 02:01:35]

lgtm

[jbudorick, 2015-12-03 03:51:00]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-12-03 03:51:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1465383003/#ps160001
(title: "mef comments 2")

[commit-bot: I haz the power, 2015-12-03 03:51:40]

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

[commit-bot: I haz the power, 2015-12-03 04:16:57]

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

[commit-bot: I haz the power, 2015-12-03 04:16:58]

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

[jbudorick, 2015-12-15 15:16:31]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-12-15 15:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, mef@chromium.org, agrieve@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1465383003/#ps240001
(title: "+landmine for GN")

[commit-bot: I haz the power, 2015-12-15 15:16:44]

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

[commit-bot: I haz the power, 2015-12-15 15:22:12]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-12-15 15:23:46]

Description was changed from

==========
[Android] Add ChromiumNetTestSupport.apk for the java EmbeddedTestServer.

BUG=488195
==========

to

==========
[Android] Add ChromiumNetTestSupport.apk for the java EmbeddedTestServer.

BUG=488195

Committed: https://crrev.com/06215e9286247b08ff6c958f725c6e5185c4acd1
Cr-Commit-Position: refs/heads/master@{#365254}
==========

[commit-bot: I haz the power, 2015-12-15 15:23:47]

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

[Nico, 2015-12-15 15:29:06]

https://codereview.chromium.org/1465383003/diff/240001/build/get_landmines.py
File build/get_landmines.py (right):

https://codereview.chromium.org/1465383003/diff/240001/build/get_landmines.py...
build/get_landmines.py:31: # landmine.
Did you see this? In general, you should fix things like issue crbug.com/569713
instead of adding landmines for it (especially without an explicit lgtm for the
landmine)

[jbudorick, 2015-12-15 15:31:48]

https://codereview.chromium.org/1465383003/diff/240001/build/get_landmines.py
File build/get_landmines.py (right):

https://codereview.chromium.org/1465383003/diff/240001/build/get_landmines.py...
build/get_landmines.py:31: # landmine.
On 2015/12/15 15:29:06, Nico wrote:
> Did you see this? In general, you should fix things like issue
crbug.com/569713
> instead of adding landmines for it (especially without an explicit lgtm for
the
> landmine)

Oh, no, I missed this. :(

I talked to agrieve@ offline about the landmine; going forward, I'll keep this
in mind. Sorry.

[danakj, 2015-12-15 20:09:47]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1526183002/ by danakj@chromium.org.

The reason for reverting is: Android compile failing with related error:
https://code.google.com/p/chromium/issues/detail?id=569991.