Creating new gn arg: enable_all_proguard_optimizations.

build/config/android/rules.gni

Index: build/config/android/rules.gni
diff --git a/build/config/android/rules.gni b/build/config/android/rules.gni
index 381016414c43ff23d9eaaefea394b6e7802e436d..f37b84fa33842f9821dd14687ea4495fc517b959 100644
--- a/build/config/android/rules.gni
+++ b/build/config/android/rules.gni
@@ -1471,12 +1471,12 @@ if (enable_java_templates) {
     assert(defined(invoker.final_apk_path) || defined(invoker.apk_name))
     assert(defined(invoker.android_manifest))
     gen_dir = "$target_gen_dir/$target_name"
-    base_path = "$gen_dir/$target_name"
+    _base_path = "$gen_dir/$target_name"
     _build_config = "$target_gen_dir/$target_name.build_config"
-    resources_zip_path = "$base_path.resources.zip"
-    _all_resources_zip_path = "$base_path.resources.all.zip"
-    _jar_path = "$base_path.jar"
-    _lib_dex_path = "$base_path.dex.jar"
+    resources_zip_path = "$_base_path.resources.zip"
+    _all_resources_zip_path = "$_base_path.resources.all.zip"
+    _jar_path = "$_base_path.jar"
+    _lib_dex_path = "$_base_path.dex.jar"
     _rebased_lib_dex_path = rebase_path(_lib_dex_path, root_build_dir)
     _template_name = target_name
 
@@ -1616,8 +1616,19 @@ if (enable_java_templates) {
 
     _proguard_enabled =
         defined(invoker.proguard_enabled) && invoker.proguard_enabled
+    _is_testable_apk = _proguard_enabled && !defined(invoker.apk_under_test)

[Yaron, 2016/07/06 14:01:41]

Should at minimum have a comment but I feel like in general this is hard to grok
(or will be after the fact). Rather than assuming there will be other
differences, it might make it easier to understand if we just call this the
"unoptimized" apk.

[agrieve, 2016/07/06 14:18:49]

nit: non-proguarded apks are testable as well. How about "_create_apk_for_test"

This is probably also a good spot to add a comment saying why it's necessary to
do this.

[smaier, 2016/07/06 19:09:37]

Done.

     if (_proguard_enabled) {
-      _proguard_output_jar_path = "$base_path.proguard.jar"
+      _proguard_output_jar_path = "$_base_path.proguard.jar"
+    }
+    if (_is_testable_apk) {
+      _template_name_for_test = "${_template_name}_for_test"
+      _proguard_output_jar_for_test_path = "${_base_path}_for_test.proguard.jar"
+      final_dex_for_test_path = "$gen_dir/classes_for_test.dex"

[agrieve, 2016/07/06 14:18:49]

nit: prefix with _

[smaier, 2016/07/06 19:09:37]

Done.

+
+      assert(!defined(invoker.final_apk_path),
+             "Cannot set final_apk_path for tested apk")

[agrieve, 2016/07/06 14:18:49]

nit: this message misses the "why". How about:

"Setting final_apk_path for proguarded apks is not yet implemented."

[smaier, 2016/07/06 19:09:37]

Done.

+      _final_apk_for_test_path =
+          "$root_build_dir/apks/${invoker.apk_name}_for_test.apk"

[agrieve, 2016/07/06 14:18:49]

nit: apk_names are AllInCamelCase. So how about: ${invoker.apk_name}ForTest.apk

[smaier, 2016/07/06 19:09:37]

Done.

     }
 
     _emma_never_instrument = defined(invoker.testonly) && invoker.testonly
@@ -1629,6 +1640,9 @@ if (enable_java_templates) {
       jar_path = _jar_path
       dex_path = final_dex_path
       apk_path = _final_apk_path
+      if (_is_testable_apk) {
+        apk_for_test_path = _final_apk_for_test_path
+      }
       incremental_apk_path = "${_final_apk_path_no_ext}_incremental.apk"
       incremental_install_script_path = _incremental_install_script_path
       resources_zip = resources_zip_path
@@ -1667,7 +1681,7 @@ if (enable_java_templates) {
 
     _final_deps = []
 
-    _generated_proguard_config = "$base_path.resources.proguard.txt"
+    _generated_proguard_config = "$_base_path.resources.proguard.txt"
     process_resources_target = "${_template_name}__process_resources"
     process_resources(process_resources_target) {
       forward_variables_from(invoker,
@@ -1891,6 +1905,41 @@ if (enable_java_templates) {
           ":$_proguard_target",
         ]
       }
+
+      if (_is_testable_apk) {
+        _proguard_for_test_target = "${_template_name_for_test}__proguard"
+        proguard(_proguard_for_test_target) {
+          forward_variables_from(invoker,
+                                 [
+                                   "alternative_android_sdk_jar",
+                                   "proguard_jar_path",
+                                 ])
+          deps = [
+            ":$build_config_target",
+            ":$java_target",
+            ":$process_resources_target",
+          ]
+          inputs = [
+                     _build_config,
+                     _jar_path,
+                   ] + _proguard_configs
+
+          output_jar_path = _proguard_output_jar_for_test_path
+
+          # This Proguard flags file will apply only to tested apks - true
+          # release apks are not affected.
+          _proguard_configs +=
+              [ "//chrome/android/java/proguard_for_test.flags" ]
+          _rebased_proguard_configs =
+              rebase_path(_proguard_configs, root_build_dir)
+          args = [
+            "--proguard-configs=$_rebased_proguard_configs",
+            "--input-paths=@FileArg($_rebased_build_config:proguard:input_paths)",
+          ]
+        }
+        _dex_sources_for_test = [ _proguard_output_jar_for_test_path ]

[agrieve, 2016/07/06 14:18:49]

nit: these are both used only once. Would be better to inline them.

[smaier, 2016/07/06 19:09:37]

Done.

+        _dex_deps_for_test = [ ":$_proguard_for_test_target" ]
+      }
     } else {
       if (enable_multidex) {
         _dex_sources = [ _jar_path ]
@@ -1920,6 +1969,18 @@ if (enable_java_templates) {
       }
     }
 
+    if (_is_testable_apk) {
+      final_dex_for_test_target_name = "${final_dex_target_name}_for_test"

[agrieve, 2016/07/06 14:18:49]

nit: prefix with _

[smaier, 2016/07/06 19:09:37]

Done.

+      dex("$final_dex_for_test_target_name") {
+        deps = _dex_deps_for_test + [ ":$build_config_target" ]
+        inputs = [
+          _build_config,
+        ]
+        sources = _dex_sources_for_test
+        output = final_dex_for_test_path
+      }
+    }
+
     _native_libs_file_arg_dep = ":$build_config_target"
     _native_libs_file_arg = "@FileArg($_rebased_build_config:native:libraries)"
 
@@ -2013,6 +2074,7 @@ if (enable_java_templates) {
         deps = []
       }
       apk_path = _final_apk_path
+      base_path = _base_path
       android_manifest = _android_manifest
       assets_build_config = _build_config
       resources_zip = _all_resources_zip_path
@@ -2065,6 +2127,85 @@ if (enable_java_templates) {
       forward_variables_from(invoker, [ "native_lib_placeholders" ])
     }
 
+    if (_is_testable_apk) {
+      # We are going to create a second apk, very similiar to the above

[Yaron, 2016/07/06 14:01:41]

seems like an awful lot of copy/paste :(
is using a new template unwieldy?

[smaier, 2016/07/06 19:09:37]

Done.

+      # create_apk("${_template_name}__create"), but with this one being an
+      # externally visible rule which will build an .apk with the additional
+      # test-only proguard flags.
+      create_apk("${_template_name_for_test}") {

[agrieve, 2016/07/06 14:18:49]

We can eliminate this copy / paste using a nested template() I think (e.g.
define it at line 2055).

It would have to go something like:

template("create_main_apk") {
  _parent_invoker = invoker.invoker
  ...
}

create_main_apk(_template_name_for_test) {
  apk_path = ...
  base_path = ...
  dex_path = ...
  deps = ...
}

[smaier, 2016/07/06 19:09:37]

Done.

+        forward_variables_from(invoker,
+                               [
+                                 "alternative_android_sdk_jar",
+                                 "android_aapt_path",
+                                 "app_as_shared_lib",
+                                 "deps",
+                                 "extensions_to_not_compress",
+                                 "language_splits",
+                                 "page_align_shared_libraries",
+                                 "public_deps",
+                                 "secondary_native_libs",
+                                 "shared_resources",
+                                 "uncompress_shared_libraries",
+                                 "write_asset_list",
+                               ])
+        if (!defined(deps)) {
+          deps = []
+        }
+        apk_path = _final_apk_for_test_path
+        base_path = _base_path + "_for_test"
+        android_manifest = _android_manifest
+        assets_build_config = _build_config
+        resources_zip = _all_resources_zip_path
+        dex_path = final_dex_for_test_path
+        load_library_from_apk = _load_library_from_apk
+        create_density_splits = _create_density_splits
+        emma_instrument = emma_coverage && !_emma_never_instrument
+
+        if (!defined(extensions_to_not_compress)) {
+          # Allow icu data, v8 snapshots, and pak files to be loaded directly from
+          # the .apk.
+          # Note: These are actually suffix matches, not necessarily extensions.
+          extensions_to_not_compress = ".dat,.bin,.pak"
+        }
+
+        version_code = _version_code
+        version_name = _version_name
+
+        keystore_name = _keystore_name
+        keystore_path = _keystore_path
+        keystore_password = _keystore_password
+
+        # Incremental apk does not use native libs nor final dex.
+        incremental_deps = deps + _android_manifest_deps + [
+                             ":$build_config_target",
+                             ":$process_resources_target",
+                           ]
+
+        # This target generates the input file _all_resources_zip_path.
+        deps += _android_manifest_deps + [
+                  ":$build_config_target",
+                  ":$process_resources_target",
+                  ":$final_dex_for_test_target_name",
+                ]
+
+        if ((_native_libs_deps != [] ||
+             _extra_native_libs_even_when_incremental != []) &&
+            !_create_abi_split) {
+          deps += _native_libs_deps + _extra_native_libs_deps +
+                  _extra_native_libs_even_when_incremental_deps +
+                  [ _native_libs_file_arg_dep ]
+          native_libs_filearg = _native_libs_file_arg
+          native_libs = _extra_native_libs
+          native_libs_even_when_incremental =
+              _extra_native_libs_even_when_incremental
+        }
+
+        # Placeholders necessary for some older devices.
+        # http://crbug.com/395038
+        forward_variables_from(invoker, [ "native_lib_placeholders" ])

[agrieve, 2016/07/06 14:18:49]

nit: since you're here, can you put this in with the forward_variables_from()
all above, and move the note about http://crbug.com/395038 to appear in the
template argument comments on line 1429?

[smaier, 2016/07/06 19:09:37]

Done.

+      }
+    }
+
     if ((_native_libs_deps != [] ||
          _extra_native_libs_even_when_incremental != []) && _create_abi_split) {
       _manifest_rule =
@@ -2313,7 +2454,11 @@ if (enable_java_templates) {
         ":${_apk_target_name}_dist_ijar",
       ]
       if (defined(invoker.apk_under_test)) {
-        public_deps += [ invoker.apk_under_test ]
+        if (defined(invoker.proguard_enabled) && invoker.proguard_enabled) {
+          public_deps += [ "${invoker.apk_under_test}_for_test" ]
+        } else {
+          public_deps += [ invoker.apk_under_test ]
+        }
       }
       if (defined(invoker.isolate_file)) {
         isolate_values = exec_script("//build/gypi_to_gn.py",