Add Cronet Test App that uses libcronet_tests.so with net test utilities.

Add Cronet Test App that uses libcronet_tests.so with net test utilities.
Move Cronet Java tests from Cronet Sample App to Cronet Test App.

Committed: https://crrev.com/2028ab74be7d4c9c2676f024394ddddb7c078be4
Cr-Commit-Position: refs/heads/master@{#294591}

[mef, 2014-09-11 16:55:22]

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

[mef, 2014-09-11 16:59:02]

Hi,

please take a look. This is CL to create separate Cronet Test app and native
library extracted from Helen's https://codereview.chromium.org/525613002/.

thanks,
-m

[mmenke, 2014-09-11 18:05:25]

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:388: {            'apk_name': 'CronetSampleTest',
nit:  Move { back to previous line

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:399:
'cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java',
I find the separation of "android/test/src/org/chromium/cronet_test_apk/" and
"android/test/javatests/src/org/chromium/cronet_test_apk/" more than a little
confusing.  The javatests one is the test fixture that launches the other, so
they're effectively two separate apps...But in the same APK?

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_jni.cc (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_jni.cc:8: // Checks the available version of
JNI. Also, caches Java reflection artifacts.
This should probably go in the body of the new CronetOnLoad function.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_loader.cc:33: namespace cronet {
Suggest moving the anonymous inside the cronet namespace as well, as it's a
little more typical, in net/ code, at least.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/cronet_tests_jni.cc:15: {"BaseAndroid",
base::android::RegisterJni},
nit: 2 space indent.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/cronet_tests_jni.cc:24: if
(vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) {
This is already in CronetOnLoad.  Should we duplicate it?

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java:22:
public class ChunkedWritableByteChannelTest extends InstrumentationTestCase {
Is there a reason we can't make all the tests in processes (In activity?) tests
like this?  No need to do in this CL, just think if we don't need to run another
activity, life is a little simpler.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:16:
ActivityInstrumentationTestCase2<CronetTestActivity> {
Now that we have this, do we still need CronetSampleTestBase?  Or anything else
in that directory, for that matter?

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:28:

nit:  Remove extra blank line.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:60:
return true;
Why is this different from the old version?  Is this needed at all?

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:36:

nit:  Remove blank line.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:173:

Think we generally don't have blank lines here?

[mef, 2014-09-11 18:37:54]

Thanks, PTAL.

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:388: {            'apk_name': 'CronetSampleTest',
On 2014/09/11 18:05:24, mmenke wrote:
> nit:  Move { back to previous line

Done.

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:399:
'cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java',
On 2014/09/11 18:05:24, mmenke wrote:
> I find the separation of "android/test/src/org/chromium/cronet_test_apk/" and
> "android/test/javatests/src/org/chromium/cronet_test_apk/" more than a little
> confusing.  The javatests one is the test fixture that launches the other, so
> they're effectively two separate apps...But in the same APK?
It is somewhat confusing, but they are in different APKs. Test App is in
CronetTest.apk and java tests are in CronetTestInstrumentation.apk.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_jni.cc (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_jni.cc:8: // Checks the available version of
JNI. Also, caches Java reflection artifacts.
On 2014/09/11 18:05:24, mmenke wrote:
> This should probably go in the body of the new CronetOnLoad function.

Done.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_loader.cc:33: namespace cronet {
On 2014/09/11 18:05:24, mmenke wrote:
> Suggest moving the anonymous inside the cronet namespace as well, as it's a
> little more typical, in net/ code, at least.

Done.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/cronet_tests_jni.cc:24: if
(vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) {
On 2014/09/11 18:05:24, mmenke wrote:
> This is already in CronetOnLoad.  Should we duplicate it?
This is needed to get env for the call to RegisterNativeMethods down below.
I suppose I could return env from CronetOnLoad, either way is fine with me.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java:22:
public class ChunkedWritableByteChannelTest extends InstrumentationTestCase {
On 2014/09/11 18:05:24, mmenke wrote:
> Is there a reason we can't make all the tests in processes (In activity?)
tests
> like this?  No need to do in this CL, just think if we don't need to run
another
> activity, life is a little simpler.
Yeah, no. The problem is that native library is NOT loaded in test APK, that's
why we have to do instrumentation tests and other fun stuff. This class OTOH is
pure java, so it doesn't need that...

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:16:
ActivityInstrumentationTestCase2<CronetTestActivity> {
On 2014/09/11 18:05:24, mmenke wrote:
> Now that we have this, do we still need CronetSampleTestBase?  Or anything
else
> in that directory, for that matter?
I'm going to prune that in separate CL. 
I would like to make Sample a simple sample :) AND use that for Proguard
application.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:28:

On 2014/09/11 18:05:24, mmenke wrote:
> nit:  Remove extra blank line.

Done.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:60:
return true;
On 2014/09/11 18:05:25, mmenke wrote:
> Why is this different from the old version?  Is this needed at all?
It *appears* to work fine without it, so I think we should remove it.
I think old version has inherited it from some other test that I've used for
inspiration. :)

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:36:

On 2014/09/11 18:05:25, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:173:

On 2014/09/11 18:05:25, mmenke wrote:
> Think we generally don't have blank lines here?

Done.

[mmenke, 2014-09-11 18:54:41]

Quick comments.  Want to get to another pass today.  Mostly just making sure I
understand the code.

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:399:
'cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java',
On 2014/09/11 18:37:54, mef wrote:
> On 2014/09/11 18:05:24, mmenke wrote:
> > I find the separation of "android/test/src/org/chromium/cronet_test_apk/"
and
> > "android/test/javatests/src/org/chromium/cronet_test_apk/" more than a
little
> > confusing.  The javatests one is the test fixture that launches the other,
so
> > they're effectively two separate apps...But in the same APK?
> It is somewhat confusing, but they are in different APKs. Test App is in
> CronetTest.apk and java tests are in CronetTestInstrumentation.apk.
> 
> 

So they're in the same package, but different apks?  Hrm...That is a bit funky.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/cronet_tests_jni.cc:24: if
(vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) {
On 2014/09/11 18:37:54, mef wrote:
> On 2014/09/11 18:05:24, mmenke wrote:
> > This is already in CronetOnLoad.  Should we duplicate it?
> This is needed to get env for the call to RegisterNativeMethods down below.
> I suppose I could return env from CronetOnLoad, either way is fine with me.

Ah, right...This is fine.  I hadn't realized we were using the env (I've been
thinking of base::android::AttachCurrentThread as the archetypical way to get
it).

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java:22:
public class ChunkedWritableByteChannelTest extends InstrumentationTestCase {
On 2014/09/11 18:37:54, mef wrote:
> On 2014/09/11 18:05:24, mmenke wrote:
> > Is there a reason we can't make all the tests in processes (In activity?)
> tests
> > like this?  No need to do in this CL, just think if we don't need to run
> another
> > activity, life is a little simpler.
> Yeah, no. The problem is that native library is NOT loaded in test APK, that's
> why we have to do instrumentation tests and other fun stuff. This class OTOH
is
> pure java, so it doesn't need that...
> 

Thanks for the explainer!  You probably explained that to me before, just find
this JNI stuff confusing.

And there's no way to load a native library in a test?

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:60:
return true;
On 2014/09/11 18:37:54, mef wrote:
> On 2014/09/11 18:05:25, mmenke wrote:
> > Why is this different from the old version?  Is this needed at all?
> It *appears* to work fine without it, so I think we should remove it.
> I think old version has inherited it from some other test that I've used for
> inspiration. :)

I don't see how testLoadUrl or testPostData aren't racy without that - they need
to wait a URL to load, and don't seem to have any other logic to actually do
that.

[mef, 2014-09-11 19:47:50]

Thanks, PTAL.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ChunkedWritableByteChannelTest.java:22:
public class ChunkedWritableByteChannelTest extends InstrumentationTestCase {
On 2014/09/11 18:54:41, mmenke wrote:
> On 2014/09/11 18:37:54, mef wrote:
> > On 2014/09/11 18:05:24, mmenke wrote:
> > > Is there a reason we can't make all the tests in processes (In activity?)
> > tests
> > > like this?  No need to do in this CL, just think if we don't need to run
> > another
> > > activity, life is a little simpler.
> > Yeah, no. The problem is that native library is NOT loaded in test APK,
that's
> > why we have to do instrumentation tests and other fun stuff. This class OTOH
> is
> > pure java, so it doesn't need that...
> > 
> 
> Thanks for the explainer!  You probably explained that to me before, just find
> this JNI stuff confusing.
> 
> And there's no way to load a native library in a test?
Not to my (admittedly limited) knowledge. In other words, instrumentation test
is a suggested way.

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java
(right):

https://codereview.chromium.org/560153002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:60:
return true;
On 2014/09/11 18:54:41, mmenke wrote:
> On 2014/09/11 18:37:54, mef wrote:
> > On 2014/09/11 18:05:25, mmenke wrote:
> > > Why is this different from the old version?  Is this needed at all?
> > It *appears* to work fine without it, so I think we should remove it.
> > I think old version has inherited it from some other test that I've used for
> > inspiration. :)
> 
> I don't see how testLoadUrl or testPostData aren't racy without that - they
need
> to wait a URL to load, and don't seem to have any other logic to actually do
> that.

Done. Excellent point.

[mmenke, 2014-09-11 21:10:17]

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:26:
mRequestContext = new ChromiumUrlRequestContext(
Why does this need to be moved?  Doesn't this break embedders that expect it?

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:63:
commandLineArgs);
nit:  Fix indent

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:178:
commandLineArgs);
Fix indent

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HttpUrlRequestFactoryTest.java:30:
commandLineArgs);
Fix indent.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:36:

nit:  Remove blank line.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:173:

nit:  Remove blank line.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java:16:
* failed.
@returns

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java:16:
* failed.
Also, maybe "Returns non-zero value on success." would be clearer (Otherwise,
I'm left wondering what it returns on success - is it a specific value or not)

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java:17:
*/
nit:  Fix indent

[mef, 2014-09-11 22:03:25]

Thanks, PTAL.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:26:
mRequestContext = new ChromiumUrlRequestContext(
On 2014/09/11 21:10:16, mmenke wrote:
> Why does this need to be moved?  Doesn't this break embedders that expect it?
Because now cronet_test app loads libcronet_tests.so instead.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:63:
commandLineArgs);
On 2014/09/11 21:10:16, mmenke wrote:
> nit:  Fix indent

Done.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlTest.java:178:
commandLineArgs);
On 2014/09/11 21:10:16, mmenke wrote:
> Fix indent

Done.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HttpUrlRequestFactoryTest.java:30:
commandLineArgs);
On 2014/09/11 21:10:16, mmenke wrote:
> Fix indent.

Done.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:36:

On 2014/09/11 21:10:16, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:173:

On 2014/09/11 21:10:16, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java:16:
* failed.
On 2014/09/11 21:10:17, mmenke wrote:
> Also, maybe "Returns non-zero value on success." would be clearer (Otherwise,
> I'm left wondering what it returns on success - is it a specific value or not)

Done.

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java:17:
*/
On 2014/09/11 21:10:17, mmenke wrote:
> nit:  Fix indent

Done.

[xunjieli, 2014-09-11 22:36:22]

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void JNIEXPORT
JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
I think JNIEXPORT and JNICALL are not defined in cronet. 
JNI_EXPORT from base/android/jni_android.h probably is the one we want
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_a...).

What does 'extern "C"' do? I was experimenting with getting rid of it, and
didn't see an effect.

[mmenke, 2014-09-11 22:55:06]

On 2014/09/11 22:36:22, xunjieli1 wrote:
>
https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
> File components/cronet/android/test/cronet_tests_jni.cc (right):
> 
>
https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
> components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void
JNIEXPORT
> JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
> I think JNIEXPORT and JNICALL are not defined in cronet. 
> JNI_EXPORT from base/android/jni_android.h probably is the one we want
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_a...).
> 
> What does 'extern "C"' do? I was experimenting with getting rid of it, and
> didn't see an effect.

extern "C" exports symbols with C linkage (No name mangling).  As a result, they
can be called by C code.

[mmenke, 2014-09-11 22:55:13]

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:26:
mRequestContext = new ChromiumUrlRequestContext(
On 2014/09/11 22:03:24, mef wrote:
> On 2014/09/11 21:10:16, mmenke wrote:
> > Why does this need to be moved?  Doesn't this break embedders that expect
it?
> Because now cronet_test app loads libcronet_tests.so instead.

And what about embedders?

[mef, 2014-09-12 13:32:47]

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(right):

https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:26:
mRequestContext = new ChromiumUrlRequestContext(
On 2014/09/11 22:55:13, mmenke wrote:
> On 2014/09/11 22:03:24, mef wrote:
> > On 2014/09/11 21:10:16, mmenke wrote:
> > > Why does this need to be moved?  Doesn't this break embedders that expect
> it?
> > Because now cronet_test app loads libcronet_tests.so instead.
> 
> And what about embedders?
They should call org.chromium.net.LibraryLoader.ensureInitialized():
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...

[mmenke, 2014-09-12 13:41:40]

On 2014/09/12 13:32:47, mef wrote:
>
https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
> File
>
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
> (right):
> 
>
https://codereview.chromium.org/560153002/diff/80001/components/cronet/androi...
>
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:26:
> mRequestContext = new ChromiumUrlRequestContext(
> On 2014/09/11 22:55:13, mmenke wrote:
> > On 2014/09/11 22:03:24, mef wrote:
> > > On 2014/09/11 21:10:16, mmenke wrote:
> > > > Why does this need to be moved?  Doesn't this break embedders that
expect
> > it?
> > > Because now cronet_test app loads libcronet_tests.so instead.
> > 
> > And what about embedders?
> They should call org.chromium.net.LibraryLoader.ensureInitialized():
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...

Ok, LGTM!

[xunjieli, 2014-09-12 13:46:19]

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void JNIEXPORT
JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
On 2014/09/11 22:36:22, xunjieli1 wrote:
> I think JNIEXPORT and JNICALL are not defined in cronet. 
> JNI_EXPORT from base/android/jni_android.h probably is the one we want
> 

I still have one question about JNIEXPORT and JNI_EXPORT.

[mef, 2014-09-12 13:47:21]

On 2014/09/12 13:46:19, xunjieli1 wrote:
>
https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
> File components/cronet/android/test/cronet_tests_jni.cc (right):
> 
>
https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
> components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void
JNIEXPORT
> JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
> On 2014/09/11 22:36:22, xunjieli1 wrote:
> > I think JNIEXPORT and JNICALL are not defined in cronet. 
> > JNI_EXPORT from base/android/jni_android.h probably is the one we want
> > 
> 
> I still have one question about JNIEXPORT and JNI_EXPORT.

Sure, go ahead.

[chromium-reviews, 2014-09-12 13:48:04]

It is in my comment. Could you reply to it?

On Fri, Sep 12, 2014 at 9:47 AM, <mef@chromium.org> wrote:

> On 2014/09/12 13:46:19, xunjieli1 wrote:
>
> https://codereview.chromium.org/560153002/diff/100001/
> components/cronet/android/test/cronet_tests_jni.cc
>
>> File components/cronet/android/test/cronet_tests_jni.cc (right):
>>
>
>
> https://codereview.chromium.org/560153002/diff/100001/
> components/cronet/android/test/cronet_tests_jni.cc#newcode41
>
>> components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void
>>
> JNIEXPORT
>
>> JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
>> On 2014/09/11 22:36:22, xunjieli1 wrote:
>> > I think JNIEXPORT and JNICALL are not defined in cronet.
>> > JNI_EXPORT from base/android/jni_android.h probably is the one we want
>> >
>>
>
>  I still have one question about JNIEXPORT and JNI_EXPORT.
>>
>
> Sure, go ahead.
>
> https://codereview.chromium.org/560153002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mef, 2014-09-12 13:54:31]

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void JNIEXPORT
JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
On 2014/09/11 22:36:22, xunjieli1 wrote:
> I think JNIEXPORT and JNICALL are not defined in cronet. 
> JNI_EXPORT from base/android/jni_android.h probably is the one we want
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_a...).
> 
> What does 'extern "C"' do? I was experimenting with getting rid of it, and
> didn't see an effect. 
I think you are right and  JNIEXPORT JNICALL are not needed (and are just empty
defines). Good news is that according to some sources JNI_OnUnLoad is never
called anyway. I'll remove those decorators.

[xunjieli, 2014-09-12 13:56:13]

On 2014/09/12 13:54:31, mef wrote:
>
https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
> File components/cronet/android/test/cronet_tests_jni.cc (right):
> 
>
https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
> components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void
JNIEXPORT
> JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
> On 2014/09/11 22:36:22, xunjieli1 wrote:
> > I think JNIEXPORT and JNICALL are not defined in cronet. 
> > JNI_EXPORT from base/android/jni_android.h probably is the one we want
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_a...).
> > 
> > What does 'extern "C"' do? I was experimenting with getting rid of it, and
> > didn't see an effect. 
> I think you are right and  JNIEXPORT JNICALL are not needed (and are just
empty
> defines). Good news is that according to some sources JNI_OnUnLoad is never
> called anyway. I'll remove those decorators.

lgtm

[mef, 2014-09-12 13:59:22]

Thanks!

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/560153002/diff/100001/components/cronet/andro...
components/cronet/android/test/cronet_tests_jni.cc:41: extern "C" void JNIEXPORT
JNICALL JNI_OnUnLoad(JavaVM* vm, void* reserved) {
On 2014/09/12 13:54:30, mef wrote:
> On 2014/09/11 22:36:22, xunjieli1 wrote:
> > I think JNIEXPORT and JNICALL are not defined in cronet. 
> > JNI_EXPORT from base/android/jni_android.h probably is the one we want
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_a...).
> > 
> > What does 'extern "C"' do? I was experimenting with getting rid of it, and
> > didn't see an effect. 
> I think you are right and  JNIEXPORT JNICALL are not needed (and are just
empty
> defines). Good news is that according to some sources JNI_OnUnLoad is never
> called anyway. I'll remove those decorators.

Done.

[mef, 2014-09-12 13:59:48]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-09-12 14:00:06]

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

[commit-bot: I haz the power, 2014-09-12 15:19:50]

Committed patchset #7 (id:120001) as ed20e777a193edb2553c94f2a2c3aa2f61682d2b

[commit-bot: I haz the power, 2014-09-12 15:22:50]

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