Initial changes that will get the "async" capability of tab_ui module…

… to work

In summary, the changes revolve around making proguarding rules to keep foreign
symbols used by the module.

The following are the highlights:
- Investigate and add many missing "keep" rules to proguard file
- Fix R(esource) files to point to the appropriate location (base vs. module)
- Add build argument to build tab_ui as an async module
- Create file to keep proguarding rules that must be added manually
- Change proguard rule generation and add unit tests

Bug: 958026

Change-Id: I10904776d54a21bb292e2ef176750fdd338bcf40
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1627566
Commit-Queue: Fred Mello <fredmello@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665328}

build/android/constant_pool_refs_to_keep_rules.py

@@ -19,6 +19,17 @@
 # system APIs and are already included in ProGuard configs.
 _IGNORED_PACKAGES = ['java', 'android', 'org.w3c', 'org.xml', 'dalvik']
 
+# Classes in _WHITELIST_PACKAGES are support libraries compiled into chrome
+# that must bypass the _IGNORED_PACKAGES.
+_WHITELIST_PACKAGES = ['android.support']
+
+# TODO(https://crbug.com/968769): Filter may be too broad.
+# Classes in _DFM_FEATURES will be excluded from "keep all members" rule.
+_DFM_FEATURES = [
+    'org.chromium.chrome.autofill_assistant', 'org.chromium.chrome.tab_ui',
+    'org.chromium.chrome.browser.tasks.tab_management', 'org.chromium.chrome.vr'
+]
+
 # Mapping for translating Java bytecode type identifiers to source code type
 # identifiers.
 _TYPE_IDENTIFIER_MAP = {
@@ -91,6 +102,22 @@ def add_to_refs(class_name, keep_entry, dep_refs):
     dep_refs[class_name] = [keep_entry]
 
 
+def should_include_class_path(class_path):
+  """ Check whether a class_path should be added as keep rule.
+      Conditions:
+        - Class is auto-generated (Lambdas/Nested, for example $)
+        - Class is not in a DFM Module
+        - Class is not in a black/white listed package
+    """
+  nested_class = '$' in class_path
+  not_in_dfm = all(not class_path.startswith(f) for f in _DFM_FEATURES)
+  allowed_packages = not (any(
+      class_path.startswith(p)
+      for p in _IGNORED_PACKAGES) and all(not class_path.startswith(p)
+                                          for p in _WHITELIST_PACKAGES))
+  return nested_class or (not_in_dfm and allowed_packages)
+
+
 def main(argv):
   dep_refs = defaultdict(list)
   extended_and_implemented_classes = set()
@@ -109,8 +136,10 @@ def main(argv):
   with open(args.input_file, 'r') as constant_pool_refs:
     for line in constant_pool_refs:
       line = line.rstrip().replace('/', '.')
-      # Ignore any references specified by the list of _IGNORED_PACKAGES.
-      if any(line.startswith(package) for package in _IGNORED_PACKAGES):
+      # Ignore any references specified by the list of
+      # _IGNORED_PACKAGES and not in _WHITELIST_PACKAGES.
+      if (any(line.startswith(p) for p in _IGNORED_PACKAGES)
+          and all(not line.startswith(p) for p in _WHITELIST_PACKAGES)):
         continue
 
       reflist = line.split(',')
@@ -139,6 +168,11 @@ def main(argv):
       if class_name.startswith('['):
         continue
 
+      # Ignore R(esources) files that are from the same module.
+      if ('$' in class_name
+          and any(class_name.startswith(f) for f in _DFM_FEATURES)):
+        continue
+
       # If member_info starts with '(', member is a method, otherwise member
       # is a field.
       # Format keep entries as per ProGuard documentation
@@ -151,6 +185,18 @@ def main(argv):
           return_type = ''
         else:
           return_type = translate_single_type(return_type)[0]
+
+        # Include types of function arguments.
+        for arg_type in args_list:
+          if should_include_class_path(arg_type):
+            extended_and_implemented_classes.add(arg_type)
+
+        # Include the actual class when it's a constructor.
+        if member_name == '<init>':
+          if should_include_class_path(class_name):
+            extended_and_implemented_classes.add(class_name)
+          continue
+
         keep_entry = '%s %s(%s);' % (return_type, member_name,
                                      ', '.join(args_list))
       else:
@@ -162,14 +208,14 @@ def main(argv):
   with open(args.output_file, 'w') as keep_rules:
     # Write super classes and implemented interfaces to keep rules.
     for super_class in sorted(extended_and_implemented_classes):
-      keep_rules.write(
-          '-keep,allowobfuscation class %s { *; }\n' % (super_class.rstrip()))
+      keep_rules.write('-keep class %s { *; }\n' % (super_class.rstrip()))
       keep_rules.write('\n')
     # Write all other class references to keep rules.
     for c in sorted(dep_refs.iterkeys()):
+      if c in extended_and_implemented_classes:
+        continue
       class_keeps = '\n  '.join(dep_refs[c])
-      keep_rules.write(
-          '-keep,allowobfuscation class %s {\n  %s\n}\n' % (c, class_keeps))
+      keep_rules.write('-keep class %s {\n  %s\n}\n' % (c, class_keeps))
       keep_rules.write('\n')
 
 

build/android/constant_pool_refs_to_keep_rules_test.py

@@ -0,0 +1,91 @@
+# Copyright 2019 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import unittest
+import re
+import os
+
+
+class TestProguardRuleGeneration(unittest.TestCase):
+  """
+  This script is used to test a ProGuard keep rules for the purposes
+  of maintaining compatibility between async DFMs and synchronously
+  proguarded modules.
+
+  The rules are often generated by constant_pool_refs_to_keep_rules.py
+
+  This test can be run manually. Example:
+  python build/android/constant_pool_refs_to_keep_rules_test.py -v
+  """
+
+  # Make sure this variable is set accordingly.
+  # It should point to a proguard file.
+  PROGUARD_FILE_PATH = os.path.join(
+      os.path.dirname(__file__),
+      "../../chrome/android/features/tab_ui/proguard_async.flags")
+
+  def test_TabUI_HasRules(self):
+    """
+    Ensures that a few of the rules used in tabs_ui module are included.
+    Although this is far from 100% deterministic, these rules are
+    created by code that exercise different parts of the rule generation code.
+    """
+
+    rules = set()
+    with open(self.PROGUARD_FILE_PATH, 'r') as proguard_rules:
+      for line in proguard_rules:
+        if line.startswith('-keep'):
+          rule = re.search('class (.+?) {', line).group(1)
+          rules.add(rule)
+
+    # The following rules test most of the use cases for
+    # rules that can be added automatically.
+    self.assertIn('org.chromium.ui.modelutil.PropertyModel', rules)
+    self.assertIn('org.chromium.ui.modelutil.PropertyModel', rules)
+    self.assertIn('org.chromium.ui.modelutil.PropertyKey', rules)
+    self.assertIn('org.chromium.chrome.browser.toolbar.ToolbarManager', rules)
+    self.assertIn('org.chromium.base.Supplier', rules)
+    self.assertIn('android.support.v7.widget.helper.ItemTouchHelper', rules)
+    self.assertIn(
+        'android.support.v7.widget.helper.ItemTouchHelper$SimpleCallback',
+        rules)
+    self.assertIn('android.support.v7.widget.helper.ItemTouchHelper$Callback',
+                  rules)
+    self.assertIn('android.support.v4.content.ContextCompat', rules)
+    self.assertIn('android.support.v7.widget.GridLayoutManager', rules)
+    self.assertIn('android.support.v4.content.res.ResourcesCompat', rules)
+    self.assertIn(
+        'org.chromium.chrome.browser.tasks.tabgroup.TabGroupModelFilter', rules)
+    self.assertIn('android.support.v7.widget.RecyclerView$ViewHolder', rules)
+    self.assertIn('android.support.v7.widget.RecyclerView', rules)
+    self.assertIn('org.chromium.ui.modelutil.SimpleRecyclerViewMcpBase', rules)
+    self.assertIn('org.chromium.ui.modelutil.RecyclerViewAdapter', rules)
+
+    # The following rules need to be added manually.
+    self.assertNotIn(
+        'org.chromium.chrome.browser.fullscreen.ChromeFullscreenManager' +
+        '$FullscreenListener$$CC', rules)
+    self.assertNotIn(
+        'org.chromium.chrome.browser.widget.bottomsheet.BottomSheet' +
+        '$BottomSheetContent$$CC', rules)
+    self.assertNotIn('org.chromium.ui.widget.RoundedCornerImageView', rules)
+    self.assertNotIn(
+        'android.support.v4.graphics.drawable.RoundedBitmapDrawable', rules)
+
+  def test_TabUI_HasNoDuplicateRules(self):
+    """
+    Ensures that there are no duplicate keep rules
+    """
+
+    rules = set()
+    with open(self.PROGUARD_FILE_PATH, 'r') as proguard_rules:
+      for line in proguard_rules:
+        if line.startswith('-keep'):
+          rule = re.search('class (.+?) {', line).group(1)
+          self.assertNotIn(rule, rules)
+          rules.add(rule)
+
+
+if __name__ == '__main__':
+  unittest.main()

build/config/android/rules.gni

@@ -4353,9 +4353,11 @@ if (enable_java_templates) {
               build_config = _module.build_config
               repackage_classes = "ap${_async_package_number}"
 
-              # Pass mapping file of synchronous proguarding run to async module proguarding
-              # runs to preserve compatibility.
-              apply_mapping = _sync_proguard_mapping_path
+              # TODO(https://crbug.com/952858): R8 currently doesn't handle
+              # applying mapping files.
+              # Pass mapping file of synchronous proguarding run to async
+              # module proguarding runs to preserve compatibility.
+              # apply_mapping = _sync_proguard_mapping_path
 
               deps = [
                 _module.module_target,

chrome/android/BUILD.gn

@@ -12,6 +12,7 @@ import("//chrome/android/chrome_common_shared_library.gni")
 import("//chrome/android/chrome_public_apk_tmpl.gni")
 import(
     "//chrome/android/features/autofill_assistant/autofill_assistant_module_tmpl.gni")
+import("//chrome/android/features/tab_ui/buildflags.gni")
 import("//chrome/android/features/tab_ui/tab_management_java_sources.gni")
 import("//chrome/android/features/tab_ui/tab_ui_module_tmpl.gni")
 import("//chrome/android/features/vr/public_vr_java_sources.gni")
@@ -475,6 +476,12 @@ android_library("chrome_java") {
       "//chrome/android/features/vr/proguard_async_manual.flags",
     ]
   }
+  if (async_tab_ui) {
+    proguard_configs += [
+      "//chrome/android/features/tab_ui/proguard_async.flags",
+      "//chrome/android/features/tab_ui/proguard_async_manual.flags",
+    ]
+  }
 
   processor_args_javac = [ "dagger.fastInit=enabled" ]
 }
@@ -491,6 +498,10 @@ java_group("chrome_all_java") {
   if (disable_autofill_assistant_dfm) {
     deps += [ "//chrome/android/features/autofill_assistant:java" ]
   }
+
+  if (disable_tab_ui_dfm) {
+    deps += [ "//chrome/android/features/tab_ui:java" ]
+  }
 }
 
 # This is a list of all base module jni headers. New features should add their
@@ -2342,6 +2353,15 @@ template("monochrome_or_trichrome_public_bundle_tmpl") {
         },
       ]
     }
+    if (!disable_tab_ui_dfm) {
+      extra_modules += [
+        {
+          name = "tab_ui"
+          module_target = ":${target_name}__tab_ui_bundle_module"
+          proguard_async = async_tab_ui
+        },
+      ]
+    }
   }
 }
 

chrome/android/chrome_public_apk_tmpl.gni

@@ -255,10 +255,7 @@ template("chrome_public_common_apk_or_module_tmpl") {
       deps += [ ":$_unwind_asset" ]
     }
 
-    deps += [
-      "//chrome/android:chrome_all_java",
-      "//chrome/android/features/tab_ui:java",
-    ]
+    deps += [ "//chrome/android:chrome_all_java" ]
 
     if (!defined(version_code)) {
       if (_is_trichrome) {

chrome/android/features/tab_ui/BUILD.gn

@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 import("//build/config/android/rules.gni")
+import("//chrome/android/features/tab_ui/buildflags.gni")
 import("//chrome/common/features.gni")
 
 android_resources("java_resources") {
@@ -14,6 +15,10 @@ android_resources("java_resources") {
 }
 
 android_library("java") {
+  # Add this flag to prevent build hooks to be included in this module
+  # as it's already included in the base module
+  no_build_hooks = true
+
   java_files = [
     "java/src/org/chromium/chrome/browser/tasks/tab_groups/TabGroupUtils.java",
     "java/src/org/chromium/chrome/browser/tasks/tab_management/GridTabSwitcherCoordinator.java",
@@ -73,4 +78,8 @@ android_library("java") {
     "//third_party/android_deps:com_android_support_recyclerview_v7_java",
     "//ui/android:ui_java",
   ]
+
+  if (async_tab_ui) {
+    proguard_configs = [ "//base/android/proguard/chromium_code.flags" ]
+  }
 }

chrome/android/features/tab_ui/buildflags.gni

@@ -0,0 +1,11 @@
+# Copyright 2019 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+declare_args() {
+  # Controls the feature being a DFM or not.
+  disable_tab_ui_dfm = true
+
+  # Whether to create tab_ui module as an asynchronous DFM.
+  async_tab_ui = false
+}

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/GridTabSwitcherCoordinator.java

@@ -85,8 +85,10 @@ public GridTabSwitcherCoordinator(Context context,
                 mMediator::getCreateGroupButtonOnClickListener, gridCardOnClickListenerProvider,
                 compositorViewHolder, compositorViewHolder.getDynamicResourceLoader(), true,
                 org.chromium.chrome.tab_ui.R.layout.grid_tab_switcher_layout, COMPONENT_NAME);
-        HistoryNavigationLayout navigation =
-                compositorViewHolder.findViewById(R.id.history_navigation);
+
+        HistoryNavigationLayout navigation = compositorViewHolder.findViewById(
+                org.chromium.chrome.tab_ui.R.id.history_navigation);
+
         navigation.setNavigationDelegate(HistoryNavigationDelegate.createForTabSwitcher(
                 context, backPress, tabModelSelector::getCurrentTab));
         mContainerViewChangeProcessor = PropertyModelChangeProcessor.create(containerViewModel,

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogParent.java

@@ -75,7 +75,7 @@ private void setupDialogContent(Context context) {
         mDialogContainerView = new LinearLayout(context);
         mDialogContainerView.setLayoutParams(mContainerParams);
         mDialogContainerView.setBackgroundColor(ApiCompatibilityUtils.getColor(
-                context.getResources(), R.color.modern_primary_color));
+                context.getResources(), org.chromium.chrome.R.color.modern_primary_color));
         mDialogContainerView.setOrientation(LinearLayout.VERTICAL);
         mScrimView = new ScrimView(context, null, backgroundView);
         mPopupWindow = new PopupWindow(backgroundView, 0, 0);

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridViewHolder.java

@@ -39,8 +39,8 @@ private TabGridViewHolder(View itemView) {
         this.thumbnail = itemView.findViewById(R.id.tab_thumbnail);
         this.title = itemView.findViewById(R.id.tab_title);
         // TODO(yuezhanggg): Remove this when the strip is properly tinted. (crbug/939915)
-        title.setTextColor(
-                ContextCompat.getColor(itemView.getContext(), R.color.default_text_color_dark));
+        title.setTextColor(ContextCompat.getColor(
+                itemView.getContext(), org.chromium.chrome.R.color.default_text_color_dark));
         this.favicon = itemView.findViewById(R.id.tab_favicon);
         this.closeButton = itemView.findViewById(R.id.close_button);
         this.createGroupButton = itemView.findViewById(R.id.create_group_button);
@@ -49,8 +49,8 @@ private TabGridViewHolder(View itemView) {
         if (sCloseButtonBitmapWeakRef == null || sCloseButtonBitmapWeakRef.get() == null) {
             int closeButtonSize =
                     (int) itemView.getResources().getDimension(R.dimen.tab_grid_close_button_size);
-            Bitmap bitmap =
-                    BitmapFactory.decodeResource(itemView.getResources(), R.drawable.btn_close);
+            Bitmap bitmap = BitmapFactory.decodeResource(
+                    itemView.getResources(), org.chromium.chrome.R.drawable.btn_close);
             sCloseButtonBitmapWeakRef = new WeakReference<>(
                     Bitmap.createScaledBitmap(bitmap, closeButtonSize, closeButtonSize, true));
             bitmap.recycle();