Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1975153003:
Enable NativeActivity based Android tests. (Closed)
DescriptionEnable NativeActivity based Android tests.
ANGLE Android tests use native OpenGL ES rendering,
and require NativeActivity to provide the rendering surface.
Existing tests should keep using Activity,
as they don't implement ANativeActivity_onCreate().
BUG=angleproject:1362
TEST=Compile angle_end2end_tests
Committed: https://crrev.com/389d9e445c27af560f0d370a2f9f3c69b0dd812d
Cr-Commit-Position: refs/heads/master@{#396361}
Patch Set 1 #Patch Set 2 : #
Total comments: 24
Patch Set 3 : #
Total comments: 13
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Messages
Total messages: 46 (15 generated)
The CQ bit was checked by ynovikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975153003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975153003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ynovikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975153003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975153003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ynovikov@chromium.org changed reviewers: + agrieve@chromium.org, yfriedman@chromium.org
https://codereview.chromium.org/1975153003/diff/20001/build/apk_test.gypi File build/apk_test.gypi (right): https://codereview.chromium.org/1975153003/diff/20001/build/apk_test.gypi#new... build/apk_test.gypi:30: ['OS == "android"', { I don't think we need to update GYP for Android templates anymore. Unless there's a reason we need GYP support for this, I think it'd be better to not touch .gypi files. https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:1793: if (defined(invoker.java_deps)) { A codesearch for java_deps shows no usages. I think the argument was merged with "deps" but the comments on the template were never updated. Could you delete this and the reference to it in the template args? https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:39: "deps", Adding "deps" here makes it apply to all targets defined below. This is really subtle, so could you add it explicitly to the targets rather than here? https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:2144: if (!defined(android_manifest)) { This should be "invoker.android_manifest" I think. (android_manifest is not forwarded, so will always be !defined). https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:2152: output = "${root_gen_dir}/${_apk_name}_jinja/AndroidManifest.xml" Better to use $target_gen_dir/$target_name/ rather than ${root_gen_dir}/${_apk_name}_jinja/ You can then also git rid of _apk_name https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:2173: "${root_gen_dir}/${_apk_name}_jinja/AndroidManifest.xml" nit: Make a variable for this in the outer scope. e.g.: _android_manifest_path https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... File testing/android/native_test/BUILD.gn (right): https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/BUILD.gn:40: output = "${root_gen_dir}/native_test_activity/org/chromium/native_test/NativeTestActivity.java" Please use $target_gen_dir/$target_name rather than $root_gen_dir (here and below) https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:20: static { nit: Add a comment about why this is done in a static initializer. https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:37: ContextUtils.initApplicationContextForNative(); nit: these two ContextUtils calls as well as the PathUtils call need to happen only once. If you're going to use a static initializer for the loadLibrary() call, then it makes sense to move these there as well. It may be even more appropriate though to keep them in onCreate and guard them behind an if (!sApplicationInitialized) flag. (I don't feel strongly about this though) https://codereview.chromium.org/1975153003/diff/20001/testing/test.gni File testing/test.gni (right): https://codereview.chromium.org/1975153003/diff/20001/testing/test.gni#newcod... testing/test.gni:158: defined(invoker.use_native_activity) && invoker.use_native_activity nit: it's a bit less code to just add "use_native_activity" to the list of forwarded variables in unittest_apk() below.
https://codereview.chromium.org/1975153003/diff/20001/build/apk_test.gypi File build/apk_test.gypi (right): https://codereview.chromium.org/1975153003/diff/20001/build/apk_test.gypi#new... build/apk_test.gypi:30: ['OS == "android"', { On 2016/05/19 00:42:50, agrieve wrote: > I don't think we need to update GYP for Android templates anymore. Unless > there's a reason we need GYP support for this, I think it'd be better to not > touch .gypi files. Done. https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:1793: if (defined(invoker.java_deps)) { On 2016/05/19 00:42:50, agrieve wrote: > A codesearch for java_deps shows no usages. I think the argument was merged with > "deps" but the comments on the template were never updated. Could you delete > this and the reference to it in the template args? I've used java_deps in testing/android/native_test/BUILD.gn. I couldn't use deps, as it broke build_config step. https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:39: "deps", On 2016/05/19 00:42:50, agrieve wrote: > Adding "deps" here makes it apply to all targets defined below. This is really > subtle, so could you add it explicitly to the targets rather than here? Done. https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:2144: if (!defined(android_manifest)) { On 2016/05/19 00:42:50, agrieve wrote: > This should be "invoker.android_manifest" I think. (android_manifest is not > forwarded, so will always be !defined). Done. https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:2152: output = "${root_gen_dir}/${_apk_name}_jinja/AndroidManifest.xml" On 2016/05/19 00:42:50, agrieve wrote: > Better to use $target_gen_dir/$target_name/ rather than > ${root_gen_dir}/${_apk_name}_jinja/ > > You can then also git rid of _apk_name Done. https://codereview.chromium.org/1975153003/diff/20001/build/config/android/ru... build/config/android/rules.gni:2173: "${root_gen_dir}/${_apk_name}_jinja/AndroidManifest.xml" On 2016/05/19 00:42:50, agrieve wrote: > nit: Make a variable for this in the outer scope. e.g.: _android_manifest_path Done. https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... File testing/android/native_test/BUILD.gn (right): https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/BUILD.gn:40: output = "${root_gen_dir}/native_test_activity/org/chromium/native_test/NativeTestActivity.java" On 2016/05/19 00:42:50, agrieve wrote: > Please use $target_gen_dir/$target_name rather than $root_gen_dir (here and > below) Done. https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:20: static { On 2016/05/19 00:42:50, agrieve wrote: > nit: Add a comment about why this is done in a static initializer. Done. https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:37: ContextUtils.initApplicationContextForNative(); On 2016/05/19 00:42:50, agrieve wrote: > nit: these two ContextUtils calls as well as the PathUtils call need to happen > only once. If you're going to use a static initializer for the loadLibrary() > call, then it makes sense to move these there as well. > > It may be even more appropriate though to keep them in onCreate and guard them > behind an if (!sApplicationInitialized) flag. (I don't feel strongly about this > though) Since getApplicationContext() is not static these must remain in onCreate. Can I leave sApplicationInitialized to another CL, as it seems unrelated? https://codereview.chromium.org/1975153003/diff/20001/testing/test.gni File testing/test.gni (right): https://codereview.chromium.org/1975153003/diff/20001/testing/test.gni#newcod... testing/test.gni:158: defined(invoker.use_native_activity) && invoker.use_native_activity On 2016/05/19 00:42:51, agrieve wrote: > nit: it's a bit less code to just add "use_native_activity" to the list of > forwarded variables in unittest_apk() below. Done.
The CQ bit was checked by ynovikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975153003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975153003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm but rebase required. https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:1793: if (defined(invoker.java_deps)) { On 2016/05/20 02:38:10, ynovikov wrote: > On 2016/05/19 00:42:50, agrieve wrote: > > A codesearch for java_deps shows no usages. I think the argument was merged > with > > "deps" but the comments on the template were never updated. Could you delete > > this and the reference to it in the template args? > > I've used java_deps in testing/android/native_test/BUILD.gn. I couldn't use > deps, as it broke build_config step. Looks like this was just a bug in the GN templates. I've fixed it here: https://codereview.chromium.org/2001573003/ Should be fine to put this as a regular dep now (and revert adding java_deps) https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:37: ContextUtils.initApplicationContextForNative(); On 2016/05/20 02:38:10, ynovikov wrote: > On 2016/05/19 00:42:50, agrieve wrote: > > nit: these two ContextUtils calls as well as the PathUtils call need to happen > > only once. If you're going to use a static initializer for the loadLibrary() > > call, then it makes sense to move these there as well. > > > > It may be even more appropriate though to keep them in onCreate and guard them > > behind an if (!sApplicationInitialized) flag. (I don't feel strongly about > this > > though) > > Since getApplicationContext() is not static these must remain in onCreate. Can I > leave sApplicationInitialized to another CL, as it seems unrelated? Ah, right, makes sense. Would it work to have loadLibraries() called in this onCreate, but before the call to super.onCreate()? Might even make sense to have super.onCreate() be the last thing to call in this method. https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java.jinja2 (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java.jinja2:149: mRunInSubThread = true; nit: Can you add a comment saying why these tests require being run off the UI thread?
agrieve@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@chromium.org: Please review changes in test.gni
https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/BUILD.gn (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/BUILD.gn:38: jinja_template("native_test_activity") { rename native_test_java_activity to help make it clearer what the distinction is from below https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java.jinja2 (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java.jinja2:36: public class NativeTestActivity extends NativeActivity { Truthfully, I think the templatized java files should be used sparingly and only for config-type files. I'd split this out into two separate classes and use inheritance/composition for re-use https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:22: // dependency libraries must be loaded before NativeActivity::OnCreate Can you elaborate (to me, not comments) why this is necessary? I'd like to avoid it
https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:22: // dependency libraries must be loaded before NativeActivity::OnCreate On 2016/05/20 14:39:54, Yaron wrote: > Can you elaborate (to me, not comments) why this is necessary? I'd like to avoid > it I think what's going on here is that NativeActivity calls through to JNI in its onCreate: https://github.com/android/platform_frameworks_base/blob/master/core/java/and... It's meant so that you don't need to write any .java, and NativeActivity loads your library via the android.app.lib_name metadata in the manifest. However, here we still manually load the libraries so that component mode will work. So yeah, I think just calling it before super.onCreate() will work out.
https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:22: // dependency libraries must be loaded before NativeActivity::OnCreate On 2016/05/20 15:43:16, agrieve wrote: > On 2016/05/20 14:39:54, Yaron wrote: > > Can you elaborate (to me, not comments) why this is necessary? I'd like to > avoid > > it > > I think what's going on here is that NativeActivity calls through to JNI in its > onCreate: > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > It's meant so that you don't need to write any .java, and NativeActivity loads > your library via the android.app.lib_name metadata in the manifest. However, > here we still manually load the libraries so that component mode will work. > > So yeah, I think just calling it before super.onCreate() will work out. You are right, but it's a bit more complicated. NativeActivity loads android.app.lib_name in NativeActivity::onCreate. However, Android / Java dependency search path is limited to /system/lib. We have dependencies in the apk, for example all our libraries depend on libc++_shared.so. For ANGLE tests, they will also depend on ANGLE's GLES / EGL libs. If those dependencies are not preloaded, loading in NativeActivity::onCreate will fail. I think it should be OK to load the dependencies before super.onCreate(), if you prefer it this way.
//testing lgtm. https://codereview.chromium.org/1975153003/diff/40001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1975153003/diff/40001/build/config/android/ru... build/config/android/rules.gni:2134: # This trivial assert is needed in case both android_manifest and !use_default_launcher nit: wrap long line?
https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:22: // dependency libraries must be loaded before NativeActivity::OnCreate On 2016/05/20 17:07:38, ynovikov wrote: > On 2016/05/20 15:43:16, agrieve wrote: > > On 2016/05/20 14:39:54, Yaron wrote: > > > Can you elaborate (to me, not comments) why this is necessary? I'd like to > > avoid > > > it > > > > I think what's going on here is that NativeActivity calls through to JNI in > its > > onCreate: > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > > It's meant so that you don't need to write any .java, and NativeActivity loads > > your library via the android.app.lib_name metadata in the manifest. However, > > here we still manually load the libraries so that component mode will work. > > > > So yeah, I think just calling it before super.onCreate() will work out. > > You are right, but it's a bit more complicated. NativeActivity loads > android.app.lib_name in NativeActivity::onCreate. However, Android / Java > dependency search path is limited to /system/lib. We have dependencies in the > apk, for example all our libraries depend on libc++_shared.so. For ANGLE tests, > they will also depend on ANGLE's GLES / EGL libs. If those dependencies are not > preloaded, loading in NativeActivity::onCreate will fail. I think it should be > OK to load the dependencies before super.onCreate(), if you prefer it this way. sure, ok to move to onCreate and before super.oncreate
https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1975153003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:1793: if (defined(invoker.java_deps)) { On 2016/05/20 14:13:04, agrieve wrote: > On 2016/05/20 02:38:10, ynovikov wrote: > > On 2016/05/19 00:42:50, agrieve wrote: > > > A codesearch for java_deps shows no usages. I think the argument was merged > > with > > > "deps" but the comments on the template were never updated. Could you delete > > > this and the reference to it in the template args? > > > > I've used java_deps in testing/android/native_test/BUILD.gn. I couldn't use > > deps, as it broke build_config step. > > Looks like this was just a bug in the GN templates. I've fixed it here: > https://codereview.chromium.org/2001573003/ > > Should be fine to put this as a regular dep now (and revert adding java_deps) Done. https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/20001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:37: ContextUtils.initApplicationContextForNative(); On 2016/05/20 14:13:04, agrieve wrote: > On 2016/05/20 02:38:10, ynovikov wrote: > > On 2016/05/19 00:42:50, agrieve wrote: > > > nit: these two ContextUtils calls as well as the PathUtils call need to > happen > > > only once. If you're going to use a static initializer for the loadLibrary() > > > call, then it makes sense to move these there as well. > > > > > > It may be even more appropriate though to keep them in onCreate and guard > them > > > behind an if (!sApplicationInitialized) flag. (I don't feel strongly about > > this > > > though) > > > > Since getApplicationContext() is not static these must remain in onCreate. Can > I > > leave sApplicationInitialized to another CL, as it seems unrelated? > > Ah, right, makes sense. > Would it work to have loadLibraries() called in this onCreate, but before the > call to super.onCreate()? Might even make sense to have super.onCreate() be the > last thing to call in this method. Done. https://codereview.chromium.org/1975153003/diff/40001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1975153003/diff/40001/build/config/android/ru... build/config/android/rules.gni:2134: # This trivial assert is needed in case both android_manifest and !use_default_launcher On 2016/05/20 17:18:41, Dirk Pranke (slow) wrote: > nit: wrap long line? Done. https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/BUILD.gn (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/BUILD.gn:38: jinja_template("native_test_activity") { On 2016/05/20 14:39:54, Yaron wrote: > rename native_test_java_activity to help make it clearer what the distinction is > from below Done. Removed entirely. https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java.jinja2 (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java.jinja2:36: public class NativeTestActivity extends NativeActivity { On 2016/05/20 14:39:54, Yaron wrote: > Truthfully, I think the templatized java files should be used sparingly and only > for config-type files. I'd split this out into two separate classes and use > inheritance/composition for re-use Done. https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java.jinja2:149: mRunInSubThread = true; On 2016/05/20 14:13:04, agrieve wrote: > nit: Can you add a comment saying why these tests require being run off the UI > thread? Done. https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java (right): https://codereview.chromium.org/1975153003/diff/40001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTestActivity.java:22: // dependency libraries must be loaded before NativeActivity::OnCreate On 2016/05/20 17:54:42, Yaron wrote: > On 2016/05/20 17:07:38, ynovikov wrote: > > On 2016/05/20 15:43:16, agrieve wrote: > > > On 2016/05/20 14:39:54, Yaron wrote: > > > > Can you elaborate (to me, not comments) why this is necessary? I'd like to > > > avoid > > > > it > > > > > > I think what's going on here is that NativeActivity calls through to JNI in > > its > > > onCreate: > > > > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > > > > It's meant so that you don't need to write any .java, and NativeActivity > loads > > > your library via the android.app.lib_name metadata in the manifest. However, > > > here we still manually load the libraries so that component mode will work. > > > > > > So yeah, I think just calling it before super.onCreate() will work out. > > > > You are right, but it's a bit more complicated. NativeActivity loads > > android.app.lib_name in NativeActivity::onCreate. However, Android / Java > > dependency search path is limited to /system/lib. We have dependencies in the > > apk, for example all our libraries depend on libc++_shared.so. For ANGLE > tests, > > they will also depend on ANGLE's GLES / EGL libs. If those dependencies are > not > > preloaded, loading in NativeActivity::onCreate will fail. I think it should be > > OK to load the dependencies before super.onCreate(), if you prefer it this > way. > > sure, ok to move to onCreate and before super.oncreate Done.
The CQ bit was checked by ynovikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975153003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975153003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
yfriedman@chromium.org changed reviewers: + jbudorick@chromium.org - yfriedman@chromium.org
So this structure now looks better to me but jbudorick should really review. Also taking myself off reviewer list since I'm about to disappear but I'm happy with this once John is https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java (right): https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java:31: "org.chromium.native_test.NativeTestActivity.CommandLineFile"; seems like all of these should be changed to NativeTest. This will require changes to the python scripts though
https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java (right): https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java:31: "org.chromium.native_test.NativeTestActivity.CommandLineFile"; On 2016/05/26 15:01:39, Yaron (OOO until June 27) wrote: > seems like all of these should be changed to NativeTest. This will require > changes to the python scripts though John, could you confirm if you want me to change these?
https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java (right): https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java:31: "org.chromium.native_test.NativeTestActivity.CommandLineFile"; On 2016/05/26 19:07:11, ynovikov wrote: > On 2016/05/26 15:01:39, Yaron (OOO until June 27) wrote: > > seems like all of these should be changed to NativeTest. This will require > > changes to the python scripts though > > John, could you confirm if you want me to change these? Yes, please. https://codereview.chromium.org/1975153003/diff/60001/testing/test.gni File testing/test.gni (right): https://codereview.chromium.org/1975153003/diff/60001/testing/test.gni#newcode66 testing/test.gni:66: # use_native_activity: Test implements ANativeActivity_onCreate(). nit: ANativeActivity -> NativeActivity?
https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java (right): https://codereview.chromium.org/1975153003/diff/60001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTest.java:31: "org.chromium.native_test.NativeTestActivity.CommandLineFile"; On 2016/05/26 20:59:10, jbudorick wrote: > On 2016/05/26 19:07:11, ynovikov wrote: > > On 2016/05/26 15:01:39, Yaron (OOO until June 27) wrote: > > > seems like all of these should be changed to NativeTest. This will require > > > changes to the python scripts though > > > > John, could you confirm if you want me to change these? > > Yes, please. Done. https://codereview.chromium.org/1975153003/diff/60001/testing/test.gni File testing/test.gni (right): https://codereview.chromium.org/1975153003/diff/60001/testing/test.gni#newcode66 testing/test.gni:66: # use_native_activity: Test implements ANativeActivity_onCreate(). On 2016/05/26 20:59:10, jbudorick wrote: > nit: ANativeActivity -> NativeActivity? No, it's actually ANativeActivity. https://developer.android.com/ndk/reference/native__activity_8h.html
The CQ bit was checked by ynovikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975153003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975153003/80001
lgtm w/ nit https://codereview.chromium.org/1975153003/diff/80001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java (right): https://codereview.chromium.org/1975153003/diff/80001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java:235: /** Starts the NativeTest. nit: this is still starting the native test Activity (though not the NativeTestActivity), whether that's NativeUnitTestActivity, NativeUnitTestNativeActivity, or NativeBrowserTestActivity.
Dry run: None
https://codereview.chromium.org/1975153003/diff/80001/testing/android/native_... File testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java (right): https://codereview.chromium.org/1975153003/diff/80001/testing/android/native_... testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java:235: /** Starts the NativeTest. On 2016/05/26 23:43:46, jbudorick wrote: > nit: this is still starting the native test Activity (though not the > NativeTestActivity), whether that's NativeUnitTestActivity, > NativeUnitTestNativeActivity, or NativeBrowserTestActivity. Done.
The CQ bit was checked by ynovikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, agrieve@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1975153003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975153003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975153003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Enable NativeActivity based Android tests. ANGLE Android tests use native OpenGL ES rendering, and require NativeActivity to provide the rendering surface. Existing tests should keep using Activity, as they don't implement ANativeActivity_onCreate(). BUG=angleproject:1362 TEST=Compile angle_end2end_tests ========== to ========== Enable NativeActivity based Android tests. ANGLE Android tests use native OpenGL ES rendering, and require NativeActivity to provide the rendering surface. Existing tests should keep using Activity, as they don't implement ANativeActivity_onCreate(). BUG=angleproject:1362 TEST=Compile angle_end2end_tests Committed: https://crrev.com/389d9e445c27af560f0d370a2f9f3c69b0dd812d Cr-Commit-Position: refs/heads/master@{#396361} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/389d9e445c27af560f0d370a2f9f3c69b0dd812d Cr-Commit-Position: refs/heads/master@{#396361} |
