Refactor android_webview_shell and its tests

android_webview/android_webview_shell.gyp

Index: android_webview/android_webview_shell.gyp
diff --git a/android_webview/android_webview_shell.gyp b/android_webview/android_webview_shell.gyp
index a5d898f94f9961e6143ee2be917c2132aff3c538..7fa9ddaca0dfefeebbb7f00f8438765d3015cdcc 100644
--- a/android_webview/android_webview_shell.gyp
+++ b/android_webview/android_webview_shell.gyp
@@ -6,22 +6,64 @@
     {
       'target_name': 'android_webview_shell_apk',
       'type': 'none',
-      'dependencies': [
-        '../base/base.gyp:base_java_test_support',
-      ],
       'variables': {
         'apk_name': 'AndroidWebViewShell',
         'java_in_dir': 'tools/WebViewShell',
         'resource_dir': 'tools/WebViewShell/res',
+        'android_manifest_path': 'tools/WebViewShell/AndroidManifest.xml', # for lint
+      },
+      'includes': [
+        '../build/java_apk.gypi',
+      ],
+    },
+    {
+      # android_webview_apk creates a .jar as a side effect. Any java
+      # targets that need that .jar in their classpath should depend on this
+      # target. For more details see the content_shell_apk_java target.
+      'target_name': 'android_webview_shell_apk_java',

[timvolodine, 2015/09/23 15:46:29]

is this needed? can we just depend on android_webview_shell_apk in targets
below?

[boliu, 2015/09/23 16:59:07]

Yeah we looked this up. apk_fake_jar.gypi is a common pattern for splitting
foo_apk and foo_test_apk. apk_fake_jar.gypi pulls in some rules into dependent
targets, so it's not completley no-op.

+      'type': 'none',
+      'dependencies': [
+        'android_webview_shell_apk',
+      ],
+      'includes': [ '../build/apk_fake_jar.gypi' ],
+    },
+    {
+      'target_name': 'android_webview_shell_page_cycler_apk',
+      'type': 'none',
+      'dependencies': [
+        '../base/base.gyp:base_java_test_support',
+        'android_webview_shell_apk_java',
+      ],
+      'variables': {
+        'apk_name': 'AndroidWebViewShellPageCycler',
+        'java_in_dir': '../android_webview/tools/PageCycler',
         'is_test_apk': 1,
         'test_type': 'instrumentation',
-        'isolate_file': 'android_webview_shell_test_apk.isolate',
-        'android_manifest_path': 'tools/WebViewShell/AndroidManifest.xml', # for lint
+        'android_manifest_path': 'tools/PageCycler/AndroidManifest.xml',

[boliu, 2015/09/23 16:59:07]

Let's nitpick about paths.. tools now feels really really wrong. Maybe have
something like
android_webview/WebViewShell/apk (or /Shell?)
android_webview/WebViewShell/LayoutTest
android_webview/WebViewShell/PageCycler

Tim: WDYT?

[timvolodine, 2015/09/23 17:34:31]

looking at the naming in android_webview/ is lowercase so it should probably be
android_webview/webview_shell/ (and hope it'll not get confused with
android_webview/test/shell/)

/apk naming sounds good.
/LayoutTests (probably better -> /layout_tests) -- there are a couple of 'not
really layout tests' in the blink sense, but I guess it's close enough to call
it like that.

[boliu, 2015/09/23 17:48:28]

nitpick more... "webview_shell" vs "system_webview_shell", thoughts? If the
latter, then the target should renamed to "system_webview_shell_apk" to be
consistent. But on the other hand, the whole "system_webview" name is
unfortunate, because "android_webview" was already taken at the time.

Is the cost of renaming the shell target to system_webview high? I mean this
patch already needs bot-side changs.

[timvolodine, 2015/09/23 18:07:26]

Right, I thought about that as well ;)

My initial impression was that system_webview_shell is slightly too verbose, but
it does seem to distinguish it from 'non-system' webview i.e.
android_webview_apk, so would be more precise because we have the
system_webview_apk target. My concern is that system_webview_shell can be
interpreted as a 'system' webview_shell (instead of 'system_webview' shell), as
some kind of shell running in the system.. maybe that's too far fetched though
;)

yeah don't think renaming cost should be a concern in this case.

[Yoland Yan(Google), 2015/09/23 19:51:51]

Done

       },
       'includes': [
         '../build/java_apk.gypi',
         '../build/android/test_runner.gypi',
       ],
     },
+    {
+      'target_name': 'android_webview_shell_test_apk',
+      'type': 'none',
+      'dependencies': [
+        '../base/base.gyp:base_java_test_support',
+        'android_webview_shell_apk_java',
+      ],
+      'variables': {
+        'apk_name': 'AndroidWebViewShellTest',
+        'java_in_dir': '../android_webview/tools/WebViewShellTest',
+        'is_test_apk': 1,
+        'test_type': 'instrumentation',

[timvolodine, 2015/09/23 15:46:29]

add 'isolate_file' here as previously?

[Yoland Yan(Google), 2015/09/23 19:51:51]

Done

+        'android_manifest_path': 'tools/WebViewShellTest/AndroidManifest.xml',
+      },
+      'includes': [
+        '../build/java_apk.gypi',
+        '../build/android/test_runner.gypi',
+      ]
+    },
   ],
 }