[Cronet] Test the libcronet that's shipped, not libcronet_test

[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means, rather than forcing testing to replace the implementation in an artificial manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be tested without libcronet.  Now libcronet contains Cronet and the tests are contained within libcronet_test.  During testing libcronet and libcronet_test are loaded.  This presents some complications because libcronet exposes no symbols so direct native testing is not possible (there are no symbols to call), but this is easily addressed:  Most testing just involves actuating the Cronet API (we've good encapsulation boundaries) and what native components need to be accessed can be accessed through a couple simple @hide JNI APIs, namely CronetUrlRequestContext.getUrlRequestContextAdapter and CronetUrlRequest.getUrlRequestAdapterForTesting.

There are a few static data items that cannot be directly manipulated within libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with URLRequestFilter.  This was remedied by inserting a URLRequestInterceptingJobFactory into the URLRequestContext under test.  The URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more comprehensive system integration test that did not require direct native manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g. URLRequestInterceptors) are run on libcronet's network threads they cannot access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're static and, like all static data, are not shared between libcronet and libcronet_test.  To remedy this I simply instantiate a MessageLoop in libcronet_test via CronetTestUtil.PrepareNetworkThread(), and set it to use libcronet's SingleThreadTaskRunner.

TestUploadDataStreamHandler was also slightly modified to run on a thread from libcronet rather than create its own thread.  This way there was a MessageLoop and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299
Committed: https://crrev.com/fd031f17d903f22300f08f46610e1f6418275708
Cr-Commit-Position: refs/heads/master@{#433525}

[pauljensen, 2016-10-19 18:21:58]

Description was changed from

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

BUG=
==========

to

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to use replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop - 
4. ThreadTaskRunnerHandle

BUG=629299
==========

[pauljensen, 2016-10-19 18:32:00]

Description was changed from

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to use replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop - 
4. ThreadTaskRunnerHandle

BUG=629299
==========

to

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to use replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop and
SingleThreadTaskRunner in libcronet_test via
CronetTestUtil.prepareNetworkThreadForTesting().  Posting tasks to libcronet's
threads is a little tricky, but is done by using JNI and a UrlRequest with a
direct-executor.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299
==========

[pauljensen, 2016-10-19 19:12:20]

Description was changed from

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to use replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop and
SingleThreadTaskRunner in libcronet_test via
CronetTestUtil.prepareNetworkThreadForTesting().  Posting tasks to libcronet's
threads is a little tricky, but is done by using JNI and a UrlRequest with a
direct-executor.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299
==========

to

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to use replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop and
SingleThreadTaskRunner in libcronet_test via
CronetTestUtil.prepareNetworkThreadForTesting().  Posting tasks to libcronet's
threads is a little tricky, but is done by using JNI and a UrlRequest with a
direct-executor.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

SdchTest was also slightly modified to post to the network thread in Java rather
than in native code (which is no longer possible as the
CronetUrlRequestContextAdapter is no longer callable by libcronet_test).

BUG=629299
==========

[pauljensen, 2016-10-19 19:12:20]

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

[pauljensen, 2016-10-19 19:13:59]

Helen & Andrei (Andrei suggested having both of you review), PTAL, thank you!
This change unfortunately got rather complicated.  I tried to document
motivations and working in the commit description.

[pauljensen, 2016-10-19 19:15:03]

Description was changed from

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to use replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop and
SingleThreadTaskRunner in libcronet_test via
CronetTestUtil.prepareNetworkThreadForTesting().  Posting tasks to libcronet's
threads is a little tricky, but is done by using JNI and a UrlRequest with a
direct-executor.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

SdchTest was also slightly modified to post to the network thread in Java rather
than in native code (which is no longer possible as the
CronetUrlRequestContextAdapter is no longer callable by libcronet_test).

BUG=629299
==========

to

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop and
SingleThreadTaskRunner in libcronet_test via
CronetTestUtil.prepareNetworkThreadForTesting().  Posting tasks to libcronet's
threads is a little tricky, but is done by using JNI and a UrlRequest with a
direct-executor.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

SdchTest was also slightly modified to post to the network thread in Java rather
than in native code (which is no longer possible as the
CronetUrlRequestContextAdapter is no longer callable by libcronet_test).

BUG=629299
==========

[kapishnikov, 2016-10-20 17:25:46]

The solution is nice and very creative. My only concern is that we introduce a
bunch of static variables and methods that access or modify global state. I
think we should try to avoid doing it as much as possible. Agree, that it will
require a significant amount of work/refactoring but it should be possible.
WDYT?

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

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/cronet_test_jni.cc:51: if
(!base::android::OnJNIOnLoadRegisterJNI(vm, register_callbacks) ||
Do we need to call this since Cronet::CronetOnLoad() will do it?
|register_callbacks| and |init_callbacks| are empty vectors now. Should we add
an empty cronet_test::RegisterJNI & cronet_test::Init to follow the pattern?

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:25: class JavaTaskRunner :
public base::SingleThreadTaskRunner {
Would it be better if we had a non-static CronetTestContext that would keep a
reference to all libcronet objects that it needs, e.g. net::URLRequestContext or
CronetURLRequestContextAdapter? Would it eliminate the need for jumping from C
to Java and back? I think this solution is good but it looks a little hackish
and introduces even more static object/methods, e.g. the static
postClosureToNetworkThread method in CronetTestUtils.java. What if in the future
we need to test multiple engines simultaneously?

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:60:
for (int i = 0; i < 4; i++) {
Why do we need 4 requests? Do we care about the fate of the first three
requests?

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:62:
request = new UrlRequest
Use mCronetEngine.newUrlRequestBuilder(...)

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:94:
assertEquals(-21, callback.mError.getCronetInternalErrorCode());
-21 => NetError.ERR_NETWORK_CHANGED

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:27:
private static CronetEngine sCronetEngineWithPreparedNetworkThread;
It would be great if we can avoid 'static' here.

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:183:
private static native void nativePrepareNetworkThreadForTesting();
A static method with no context. This limits us to one engine.

[pauljensen, 2016-10-21 02:27:33]

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

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/cronet_test_jni.cc:51: if
(!base::android::OnJNIOnLoadRegisterJNI(vm, register_callbacks) ||
On 2016/10/20 17:25:46, kapishnikov wrote:
> Do we need to call this since Cronet::CronetOnLoad() will do it?

Yes, we need to call base::android::InitVM() to init libcronet_test.so, and
OnJNIOnLoadRegisterJNI() does this for us.

> |register_callbacks| and |init_callbacks| are empty vectors now. Should we add
> an empty cronet_test::RegisterJNI & cronet_test::Init to follow the pattern?

Adding empty functions doesn't seem purposeful.

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:25: class JavaTaskRunner :
public base::SingleThreadTaskRunner {
On 2016/10/20 17:25:46, kapishnikov wrote:
> Would it be better if we had a non-static CronetTestContext that would keep a
> reference to all libcronet objects that it needs, e.g. net::URLRequestContext
or
> CronetURLRequestContextAdapter? Would it eliminate the need for jumping from C
> to Java and back?

It would not eliminate the jumping back and forth between C++ and Java.  To
implement SingleThreadTaskRunner without access to the actual
SingleThreadTaskRunner managing this thread, we have to jump from C++ to Java
because we have the ability to post to the network thread in Java but not in
C++.  Likewise we have to jump from Java to C++ to execute the Closure because
Closure is a C++ concept.

> I think this solution is good but it looks a little hackish
> and introduces even more static object/methods, e.g. the static
> postClosureToNetworkThread method in CronetTestUtils.java. What if in the
future
> we need to test multiple engines simultaneously?

I'm not a fan of the static stuff either, but I think it's somewhat simpler than
the alternative of creating matched C++ and Java objects.  The C++ and Java
objects each need to hold a pointer to the other, and we'd need to add some JNI
to create the objects.  I don't feel strongly though.  Do you think it's worth
it to change to non-static paired Java and C++ objects?

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:60:
for (int i = 0; i < 4; i++) {
On 2016/10/20 17:25:46, kapishnikov wrote:
> Why do we need 4 requests?

We just need to fill up the connect queue.  I try to set the queue length to 0
in the listen() call above, but as I say in my comment on line 56, for whatever
reason it takes four requests before the queue overflows.  I think the linux
kernel interprets the backlog value how it pleases, I'm told a value of 1
actually results in a queue of more than 1 depending on your kernel version...

> Do we care about the fate of the first three
> requests? 

No, they're just filling the connect queue.

[pauljensen, 2016-10-23 03:26:39]

I'm working on a new patch-set that cleans up a lot of things.

[xunjieli, 2016-10-24 13:28:58]

On 2016/10/23 03:26:39, pauljensen wrote:
> I'm working on a new patch-set that cleans up a lot of things.

This is very cool! Not compiling cronet in libcronet_tests.so will give us a lot
of flexibility in the future. I will take a look when the new PS is ready.

[pauljensen, 2016-10-24 18:49:49]

Description was changed from

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextForTesting and
CronetUrlRequest.getUrlRequestForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop and
SingleThreadTaskRunner in libcronet_test via
CronetTestUtil.prepareNetworkThreadForTesting().  Posting tasks to libcronet's
threads is a little tricky, but is done by using JNI and a UrlRequest with a
direct-executor.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

SdchTest was also slightly modified to post to the network thread in Java rather
than in native code (which is no longer possible as the
CronetUrlRequestContextAdapter is no longer callable by libcronet_test).

BUG=629299
==========

to

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextAdapter and
CronetUrlRequest.getUrlRequestAdapterForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop in
libcronet_test via CronetTestUtil.PrepareNetworkThread(), and set it to use
libcronet's SingleThreadTaskRunner.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299
==========

[pauljensen, 2016-10-24 18:52:53]

Ok, PTAL, I think I'm done with the cleanup.  My idea to friend the adapter
classes worked well and allowed me to remove a lot of my thread hopping magic. 
I also de-static'ed my network thread preparation and MockUrlRequestJobFactory
test utilities.

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:62:
request = new UrlRequest
On 2016/10/20 17:25:46, kapishnikov wrote:
> Use mCronetEngine.newUrlRequestBuilder(...)

Done.

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:94:
assertEquals(-21, callback.mError.getCronetInternalErrorCode());
On 2016/10/20 17:25:46, kapishnikov wrote:
> -21 => NetError.ERR_NETWORK_CHANGED

Done.

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2406273002/diff/320001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:27:
private static CronetEngine sCronetEngineWithPreparedNetworkThread;
On 2016/10/20 17:25:46, kapishnikov wrote:
> It would be great if we can avoid 'static' here.

Done.

[kapishnikov, 2016-10-24 21:17:34]

Thanks for getting rid of static members and the hopping between Java and C. The
change looks good. I have some theoretical questions regarding C++ inner
workings. See below.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:24: return
TestUtil::getURLRequest(jurl_request_adapter)->load_flags();
Is it safe to call load_flags() here? As I understand this works because
load_flags() is inline and the definition of URLRequest is the same in libcronet
& libcronet_test. But what will happen if the method is not inline for some
reason; or libcronettest is compiled with different flags which cause different
field alignment?

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:32: return
context_adapter->network_thread_->task_runner();
The task_runner() is inline but if DCHECK is enabled, it calls a regular
non-virtual method:
id_event_.IsSignaled()

How will it find the address of that function if DEBUG is enabled?

[pauljensen, 2016-10-24 22:06:06]

load_flags() and task_runner() are functions in net/ not Cronet.  libcronet and
libcronet_test both have copies of net/.  The test code that calls these
functions will call the versions in libcronet_test.

On 2016/10/24 21:17:34, kapishnikov wrote:
> Thanks for getting rid of static members and the hopping between Java and C.
The
> change looks good. I have some theoretical questions regarding C++ inner
> workings. See below.
> 
>
https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
> File components/cronet/android/test/cronet_test_util.cc (right):
> 
>
https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
> components/cronet/android/test/cronet_test_util.cc:24: return
> TestUtil::getURLRequest(jurl_request_adapter)->load_flags();
> Is it safe to call load_flags() here? As I understand this works because
> load_flags() is inline and the definition of URLRequest is the same in
libcronet
> & libcronet_test. But what will happen if the method is not inline for some
> reason; or libcronettest is compiled with different flags which cause
different
> field alignment?
> 
>
https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
> components/cronet/android/test/cronet_test_util.cc:32: return
> context_adapter->network_thread_->task_runner();
> The task_runner() is inline but if DCHECK is enabled, it calls a regular
> non-virtual method:
> id_event_.IsSignaled()
> 
> How will it find the address of that function if DEBUG is enabled?

[pauljensen, 2016-10-25 11:55:38]

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:24: return
TestUtil::getURLRequest(jurl_request_adapter)->load_flags();
On 2016/10/24 21:17:34, kapishnikov wrote:
> Is it safe to call load_flags() here? As I understand this works because
> load_flags() is inline and the definition of URLRequest is the same in
libcronet
> & libcronet_test. But what will happen if the method is not inline for some
> reason; or libcronettest is compiled with different flags which cause
different
> field alignment? 

If the method isn't inline, as I said, it's fine because load_flags() is in
net/, not Cronet, and net/ is built into libcronet_test.so so it should be fine.
 We get a linker unresolved symbol error if we try to use anything in Cronet
from the tests.

If libcronet and libcronet_test are compiled with different flags we're in
trouble, but the tests will definitely catch this.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:32: return
context_adapter->network_thread_->task_runner();
On 2016/10/24 21:17:34, kapishnikov wrote:
> The task_runner() is inline but if DCHECK is enabled, it calls a regular
> non-virtual method:
> id_event_.IsSignaled()
> 
> How will it find the address of that function if DEBUG is enabled?

As I said, this is all calling stuff in net/, not Cronet, and net/ is built into
libcronet_test.so so it should be fine.  We get a linker unresolved symbol error
if we try to use anything in Cronet from the tests.

[xunjieli, 2016-10-25 17:51:38]

This is very clever! I need to look at it a bit more. Here is a question and a
few nits in the meanwhile.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/BUILD.gn:452:
"//components/cronet/android/cronet_url_request_adapter.h",
Could you add a comment here on why these three non-test files are required?

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:46:
(CronetURLRequestContextAdapter*)jcontext_adapter;
reinterpret_cast<CronetURLRequestContextAdapter*> ?

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:61: static void
PrepareNetworkThreadOnNetworkThread(jlong jcontext_adapter) {
Can this be a file local function (in an anonymous space) ?

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:81: static void
CleanupNetorkThreadOnNetworkThread() {
file local function? Also there is a typo in Network.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.h (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.h:27: static
scoped_refptr<base::SingleThreadTaskRunner> getTaskRunner(
nit: CamelCase here and below.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/mock_url_request_job_factory.cc (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:53: void
initOnNetworkThread() {
nit: CamelCase.

[pauljensen, 2016-10-26 17:55:18]

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/BUILD.gn:452:
"//components/cronet/android/cronet_url_request_adapter.h",
On 2016/10/25 17:51:38, xunjieli wrote:
> Could you add a comment here on why these three non-test files are required?

Done.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:46:
(CronetURLRequestContextAdapter*)jcontext_adapter;
On 2016/10/25 17:51:38, xunjieli wrote:
> reinterpret_cast<CronetURLRequestContextAdapter*> ?

Done.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:61: static void
PrepareNetworkThreadOnNetworkThread(jlong jcontext_adapter) {
On 2016/10/25 17:51:38, xunjieli wrote:
> Can this be a file local function (in an anonymous space) ?

It's static, which means file local.  I'd rather not hoist it up into an
anonymous namespace, as that would move it far away from its pair,
PrepareNetworkThread, and I'd prefer to have it reside beside its pair.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:81: static void
CleanupNetorkThreadOnNetworkThread() {
On 2016/10/25 17:51:38, xunjieli wrote:
> file local function? Also there is a typo in Network.

Same comment about file local.  I fixed the typo in Network.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.h (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.h:27: static
scoped_refptr<base::SingleThreadTaskRunner> getTaskRunner(
On 2016/10/25 17:51:38, xunjieli wrote:
> nit: CamelCase here and below.

Done.

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
File components/cronet/android/test/mock_url_request_job_factory.cc (right):

https://codereview.chromium.org/2406273002/diff/440001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:53: void
initOnNetworkThread() {
On 2016/10/25 17:51:38, xunjieli wrote:
> nit: CamelCase.

Done.

[xunjieli, 2016-10-27 14:49:43]

I think this change looks good. I still don't quite understand the MessageLoop
part. 

If libcronet_tests.so grabs the TaskRunner of the CronetURLRequestContextAdapter
object, and create a new MessageLoop on it. What happens to the old message
loop? Will libcronet.so still posts task to the old message loop, or will both
libcronet.so and libcronet_tests.so use the new message loop?

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:13: #include
"components/cronet/android/test/native_test_server.h"
not used?  (not sure why the original code has it)

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.h (right):

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.h:21: class TestUtil {
I guess it doesn't make sense for this to have a copy and assignment
constructor.

Could you add a
DISALLOW_COPY_AND_ASSIGN(TestUtil)?

and #include "base/macros.h"

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.h:44: static void
RunAfterContextInitOnNetworkThread(jlong jcontext_adapter,
The style guide encourages making this file local, see:

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-st...

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
File components/cronet/android/test/mock_url_request_job_factory.cc (right):

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:45: void
Shutdown() {
nit: Matt told me that Shutdown() is a noun, and ShutDown() is a verb phrase, so
s/Shutdown/ShutDown.

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:81: jlong
AddUrlInterceptorJobFactory(JNIEnv* env,
cleanup idea: maybe combine AddUrlInterceptorJobFactory and AddUrlInterceptors
into one function. It took me some time to differentiate those two. Since they
are almost doing the same thing, maybe we can use one JNI function.

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:89: void
RemoveUrlInterceptorJobFactory(JNIEnv* env,
Optional nit:

Use @NativeClassQualifiedName() and directly invoke Shutdown() on the handle.
That way, we can have one fewer static functions.

[pauljensen, 2016-10-27 18:43:26]

On 2016/10/27 14:49:43, xunjieli wrote:
> If libcronet_tests.so grabs the TaskRunner of the
CronetURLRequestContextAdapter
> object, and create a new MessageLoop on it. What happens to the old message
> loop? Will libcronet.so still posts task to the old message loop, or will both
> libcronet.so and libcronet_tests.so use the new message loop?

I don't think you can post tasks to a MessageLoop, I think the
MessageLoop::task_runner() function is just used to extract the
SingleThreadTaskRunner pointer (or alternatively ThreadTaskRunnerHandle::Get()),
and that is used to post tasks.

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:13: #include
"components/cronet/android/test/native_test_server.h"
On 2016/10/27 14:49:42, xunjieli wrote:
> not used?  (not sure why the original code has it)

Done.

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
File components/cronet/android/test/mock_url_request_job_factory.cc (right):

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:45: void
Shutdown() {
On 2016/10/27 14:49:43, xunjieli wrote:
> nit: Matt told me that Shutdown() is a noun, and ShutDown() is a verb phrase,
so
> s/Shutdown/ShutDown.

Done.  I should mention that Shutdown() appears 25x more than ShutDown() in
Chromium, but Matt's correct that shutdown is a noun.

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:81: jlong
AddUrlInterceptorJobFactory(JNIEnv* env,
On 2016/10/27 14:49:43, xunjieli wrote:
> cleanup idea: maybe combine AddUrlInterceptorJobFactory and AddUrlInterceptors
> into one function. It took me some time to differentiate those two. Since they
> are almost doing the same thing, maybe we can use one JNI function.

Done.

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:89: void
RemoveUrlInterceptorJobFactory(JNIEnv* env,
On 2016/10/27 14:49:43, xunjieli wrote:
> Optional nit:
> 
> Use @NativeClassQualifiedName() and directly invoke Shutdown() on the handle.
> That way, we can have one fewer static functions.

I tried to get this to work for a while, but it appears
@NativeClassQualifiedName doesn't really work with classes defined in .cc files
:(

[pauljensen, 2016-10-28 15:24:42]

Oops, I forgot to address two comments, done. PTAL, thank you!

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.h (right):

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.h:21: class TestUtil {
On 2016/10/27 14:49:42, xunjieli wrote:
> I guess it doesn't make sense for this to have a copy and assignment
> constructor.
> 
> Could you add a
> DISALLOW_COPY_AND_ASSIGN(TestUtil)?
> 
> and #include "base/macros.h"

I've gone one step further and used DISALLOW_IMPLICIT_CONSTRUCTORS().

https://codereview.chromium.org/2406273002/diff/460001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.h:44: static void
RunAfterContextInitOnNetworkThread(jlong jcontext_adapter,
On 2016/10/27 14:49:42, xunjieli wrote:
> The style guide encourages making this file local, see:
> 
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-st...

This function needs to be a member of TestUtil because it accesses internals of
CronetURLRequestContext.  I've added a note at the top of this class to clarify.

[kapishnikov, 2016-10-28 18:09:24]

Paul, thanks for making it happen. There are no extra comments from my side.
LGTM pending on Helen's approval.

[xunjieli, 2016-10-28 22:47:49]

Sorry for the delay. I was scrambling to fix some bugs earlier this week. I
talked to Andrei a bit more about this change and understood the message loop
part better. I still have some concern about change. Let's resume the discussion
on Monday in person!

[pauljensen, 2016-11-11 15:31:36]

Ping.  This has sat idle for 2 weeks now.  I talked to davidben@ a bit about
this last week.  He was concerned that it is fragile (i.e. some base/ or net/
code could add more static data and potentially break our tests).  He didn't
want this blocking the CQ.  I agree with his concern, but am not particularly
worried as the native interaction of our tests is extremely small (i.e. just
inserting our URLRequest interceptors and the TestUploadDataStreamHandler use of
net thread) and because there is a policy of avoiding static singletons in
Chromium (see https://cs.chromium.org/chromium/src/base/memory/singleton.h). 
For now Cronet test failures are not blocking the CQ.  I think we should move
forward with this CL.  We can see how often this fragility causes failures, I'm
guessing almost never as our tests rarely need to fall back to native
interaction.

[xunjieli, 2016-11-11 16:43:32]

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

[xunjieli, 2016-11-11 16:43:33]

I am deferring this to Misha.

[pauljensen, 2016-11-15 10:18:54]

Ping.

[mef, 2016-11-16 18:45:44]

Mostly nits and I *think* one bug. To be continued...

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/BUILD.gn:452: # While "cronet_tests" cannot depend on
"cronet_static", and hence cannot
Does it make sense to extract these into separate target?

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/mock_url_request_job_factory.cc (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:38: // Should
only be called on network thread; other threads should use
Can we have DCHECK for this?

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/test_upload_data_stream_handler.cc (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/test_upload_data_stream_handler.cc:47:
network_thread_ = nullptr;
I think there is a bug here. If |this| gets destroyed on network thread before
line 47 executed, then this assignment will override released memory.

https://codereview.chromium.org/2406273002/diff/500001/net/url_request/url_re...
File net/url_request/url_request_intercepting_job_factory.h (right):

https://codereview.chromium.org/2406273002/diff/500001/net/url_request/url_re...
net/url_request/url_request_intercepting_job_factory.h:40:
URLRequestInterceptingJobFactory(URLRequestJobFactory* job_factory,
This is weird and I think deserves a comment WRT why is it needed.

[mef, 2016-11-16 20:01:27]

Few more comments about naming and commenting.
I think overall approach is sound.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:81: void
PrepareNetworkThread(JNIEnv* env,
nit: maybe add a comment that this is called from Java CronetTestUtil class?

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:93: void
CleanupNetworkThread(JNIEnv* env,
nit: maybe add a comment that this is called from Java CronetTestUtil class?

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:50:
mMockUrlRequestJobFactory = new
MockUrlRequestJobFactory(mTestFramework.mCronetEngine);
Hrm, how would this work if mTestFramework.mCronetEngine is JavaCronetEngine?

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:61:
public static class PrepareNetworkThread {
I think the name is a bit confusing, maybe call it NetworkThreadTestConnector or
something like that??

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:84:
private static native void nativePrepareNetworkThread(long contextAdapter);
nit: maybe these should be class methods as opposed to standalone functions on
native side?

[pauljensen, 2016-11-18 18:12:10]

I addressed your comments, Misha, PTAL, thank you!

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/BUILD.gn:452: # While "cronet_tests" cannot depend on
"cronet_static", and hence cannot
On 2016/11/16 18:45:43, mef wrote:
> Does it make sense to extract these into separate target?

I don't see the motivation to do so.  I don't think we want to encourage other
targets to rely on this odd grouping of a few header files.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/cronet_test_util.cc (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:81: void
PrepareNetworkThread(JNIEnv* env,
On 2016/11/16 20:01:27, mef wrote:
> nit: maybe add a comment that this is called from Java CronetTestUtil class?

Done.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/cronet_test_util.cc:93: void
CleanupNetworkThread(JNIEnv* env,
On 2016/11/16 20:01:27, mef wrote:
> nit: maybe add a comment that this is called from Java CronetTestUtil class?

Done.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:50:
mMockUrlRequestJobFactory = new
MockUrlRequestJobFactory(mTestFramework.mCronetEngine);
On 2016/11/16 20:01:27, mef wrote:
> Hrm, how would this work if mTestFramework.mCronetEngine is JavaCronetEngine?

It won't; it'll get a ClassCastException trying to cast JavaCronetEngine to
CronetUrlRequestContext.  For better or worse we only run setUp() once prior to
running our tests with the native impl and then the Java impl.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/mock_url_request_job_factory.cc (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/mock_url_request_job_factory.cc:38: // Should
only be called on network thread; other threads should use
On 2016/11/16 18:45:44, mef wrote:
> Can we have DCHECK for this?

Done.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:61:
public static class PrepareNetworkThread {
On 2016/11/16 20:01:27, mef wrote:
> I think the name is a bit confusing, maybe call it NetworkThreadTestConnector
or
> something like that??

Done.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:84:
private static native void nativePrepareNetworkThread(long contextAdapter);
On 2016/11/16 20:01:27, mef wrote:
> nit: maybe these should be class methods as opposed to standalone functions on
> native side?

@NativeClassQualifiedName only works for non-static native functions.  The
native pointer these functions take is to the contextAdapter, and I don't want
to move test-only functions to the contextAdapter if I don't need to.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/test_upload_data_stream_handler.cc (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/test_upload_data_stream_handler.cc:47:
network_thread_ = nullptr;
On 2016/11/16 18:45:44, mef wrote:
> I think there is a bug here. If |this| gets destroyed on network thread before
> line 47 executed, then this assignment will override released memory.

Done.

https://codereview.chromium.org/2406273002/diff/500001/net/url_request/url_re...
File net/url_request/url_request_intercepting_job_factory.h (right):

https://codereview.chromium.org/2406273002/diff/500001/net/url_request/url_re...
net/url_request/url_request_intercepting_job_factory.h:40:
URLRequestInterceptingJobFactory(URLRequestJobFactory* job_factory,
On 2016/11/16 18:45:44, mef wrote:
> This is weird and I think deserves a comment WRT why is it needed.

Done.

[mef, 2016-11-18 21:45:50]

lg tm mod broken test.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/BUILD.gn:452: # While "cronet_tests" cannot depend on
"cronet_static", and hence cannot
On 2016/11/18 18:12:10, pauljensen wrote:
> On 2016/11/16 18:45:43, mef wrote:
> > Does it make sense to extract these into separate target?
> 
> I don't see the motivation to do so.  I don't think we want to encourage other
> targets to rely on this odd grouping of a few header files.

Acknowledged.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:50:
mMockUrlRequestJobFactory = new
MockUrlRequestJobFactory(mTestFramework.mCronetEngine);
On 2016/11/18 18:12:10, pauljensen wrote:
> On 2016/11/16 20:01:27, mef wrote:
> > Hrm, how would this work if mTestFramework.mCronetEngine is
JavaCronetEngine?
> 
> It won't; it'll get a ClassCastException trying to cast JavaCronetEngine to
> CronetUrlRequestContext.  For better or worse we only run setUp() once prior
to
> running our tests with the native impl and then the Java impl.

Acknowledged.

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java
(right):

https://codereview.chromium.org/2406273002/diff/500001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestUtil.java:84:
private static native void nativePrepareNetworkThread(long contextAdapter);
On 2016/11/18 18:12:10, pauljensen wrote:
> On 2016/11/16 20:01:27, mef wrote:
> > nit: maybe these should be class methods as opposed to standalone functions
on
> > native side?
> 
> @NativeClassQualifiedName only works for non-static native functions.  The
> native pointer these functions take is to the contextAdapter, and I don't want
> to move test-only functions to the contextAdapter if I don't need to.

Acknowledged. Thanks for explanation!

[pauljensen, 2016-11-19 02:05:41]

On 2016/11/18 21:45:50, mef wrote:
> lg tm mod broken test.

I fixed the test, PTAL, thank you!

[mef, 2016-11-19 03:30:15]

lgtm

[pauljensen, 2016-11-21 12:55:47]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2016-11-21 12:55:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
kapishnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2406273002/#ps580001
(title: "fix shutdown race")

[commit-bot: I haz the power, 2016-11-21 12:56:05]

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, 2016-11-21 13:53:01]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1479732947477770,
"parent_rev": ["c0d445b00e98c24ac2cb9f7b2b516eb7bd0a54cf", null], "commit_rev":
["a7b0496eb033e068451e450176f87a9c491ad300", null]}

[commit-bot: I haz the power, 2016-11-21 13:53:24]

Description was changed from

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextAdapter and
CronetUrlRequest.getUrlRequestAdapterForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop in
libcronet_test via CronetTestUtil.PrepareNetworkThread(), and set it to use
libcronet's SingleThreadTaskRunner.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299
==========

to

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextAdapter and
CronetUrlRequest.getUrlRequestAdapterForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop in
libcronet_test via CronetTestUtil.PrepareNetworkThread(), and set it to use
libcronet's SingleThreadTaskRunner.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299
==========

[commit-bot: I haz the power, 2016-11-21 13:53:25]

Committed patchset #30 (id:580001)

[commit-bot: I haz the power, 2016-11-21 13:57:39]

Description was changed from

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextAdapter and
CronetUrlRequest.getUrlRequestAdapterForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop in
libcronet_test via CronetTestUtil.PrepareNetworkThread(), and set it to use
libcronet's SingleThreadTaskRunner.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299
==========

to

==========
[Cronet] Test the libcronet that's shipped, not libcronet_test

The motivation is two-fold:
1. It's always best to test what you ship.
2. This facilitates testing an implementation (libcronet) loaded by other means,
rather than forcing testing to replace the implementation in an artificial
manner (e.g. substituting libcronet_test for libcronet).

Previously libcronet_test contained both Cronet and the tests, so it could be
tested without libcronet.  Now libcronet contains Cronet and the tests are
contained within libcronet_test.  During testing libcronet and libcronet_test
are loaded.  This presents some complications because libcronet exposes no
symbols so direct native testing is not possible (there are no symbols to call),
but this is easily addressed:  Most testing just involves actuating the Cronet
API (we've good encapsulation boundaries) and what native components need to be
accessed can be accessed through a couple simple @hide JNI APIs, namely
CronetUrlRequestContext.getUrlRequestContextAdapter and
CronetUrlRequest.getUrlRequestAdapterForTesting.

There are a few static data items that cannot be directly manipulated within
libcronet and required some additional changes for testing:

1. URLRequestFilter - A few tests registered URLRequest interceptors with
URLRequestFilter.  This was remedied by inserting a
URLRequestInterceptingJobFactory into the URLRequestContext under test.  The
URLRequestInterceptingJobFactory delegates to libcronet_test's URLRequestFilter
so interceptors work like normal.

2. NetworkChangeNotifier - I rewrote NetworkChangeNotifierTest to perform a more
comprehensive system integration test that did not require direct native
manipulation, and instead used pre-existing Java testing APIs.

3. MessageLoop and ThreadTaskRunnerHandle - When libcronet_test code (e.g.
URLRequestInterceptors) are run on libcronet's network threads they cannot
access MessageLoop::current() and ThreadTaskRunnerHandle::Get() as they're
static and, like all static data, are not shared between libcronet and
libcronet_test.  To remedy this I simply instantiate a MessageLoop in
libcronet_test via CronetTestUtil.PrepareNetworkThread(), and set it to use
libcronet's SingleThreadTaskRunner.

TestUploadDataStreamHandler was also slightly modified to run on a thread from
libcronet rather than create its own thread.  This way there was a MessageLoop
and ThreadTaskRunnerHandle already accessible to libcronet native code.

BUG=629299

Committed: https://crrev.com/fd031f17d903f22300f08f46610e1f6418275708
Cr-Commit-Position: refs/heads/master@{#433525}
==========

[commit-bot: I haz the power, 2016-11-21 13:57:41]

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