Modify presubmit check for AlertDialog

We introduced CompatibleAlertDialogBuilder to work around support
library issue at https://crrev/c/1626009

+ Modify the current presubmit check to check
  CompatibleAlertDialogBuilder
+ Add an error that CompatibleAlertDialogBuilder should be used
  instead of the AppCompat AlertDialog.Builder

Bug: 966101
Change-Id: Ic3280f3c2b2ab99d72086eaa26cbe9f6c57c69b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1625845Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664487}

chrome/android/java/src/PRESUBMIT.py

@@ -19,11 +19,20 @@ import re
 NEW_NOTIFICATION_BUILDER_RE = re.compile(
     r'\bnew\sNotification(Compat)?\.Builder\b')
 
+IMPORT_APP_COMPAT_ALERTDIALOG_RE = re.compile(
+    r'\bimport\sandroid\.support\.v7\.app\.AlertDialog;')
+
+NEW_COMPATIBLE_ALERTDIALOG_BUILDER_RE = re.compile(
+    r'\bnew\s+(UiUtils\s*\.)?CompatibleAlertDialogBuilder\b')
+
 NEW_ALERTDIALOG_BUILDER_RE = re.compile(
     r'\bnew\sAlertDialog\.Builder\b')
 
 COMMENT_RE = re.compile(r'^\s*(//|/\*|\*)')
 
+BROWSER_ROOT = 'chrome/android/java/src/org/chromium/chrome/browser/'
+
+
 def CheckChangeOnUpload(input_api, output_api):
   return _CommonChecks(input_api, output_api)
 
@@ -31,21 +40,24 @@ def CheckChangeOnUpload(input_api, output_api):
 def CheckChangeOnCommit(input_api, output_api):
   return _CommonChecks(input_api, output_api)
 
+
 def _CommonChecks(input_api, output_api):
   """Checks common to both upload and commit."""
   result = []
   result.extend(_CheckNotificationConstructors(input_api, output_api))
   result.extend(_CheckAlertDialogBuilder(input_api, output_api))
+  result.extend(_CheckCompatibleAlertDialogBuilder(input_api, output_api))
   # Add more checks here
   return result
 
+
 def _CheckNotificationConstructors(input_api, output_api):
   # "Blacklist" because the following files are excluded from the check.
   blacklist = (
       'chrome/android/java/src/org/chromium/chrome/browser/notifications/'
-          'NotificationBuilder.java',
+      'NotificationBuilder.java',
       'chrome/android/java/src/org/chromium/chrome/browser/notifications/'
-          'NotificationCompatBuilder.java'
+      'NotificationCompatBuilder.java'
   )
   error_msg = '''
   Android Notification Construction Check failed:
@@ -63,73 +75,133 @@ def _CheckNotificationConstructors(input_api, output_api):
 
 
 def _CheckAlertDialogBuilder(input_api, output_api):
-  browser_root = 'chrome/android/java/src/org/chromium/chrome/browser/'
-
-  # "Blacklist" because the following files are excluded from the check.
+  # "Blacklist" because the following files are excluded from the check. In
+  # general, preference and FRE related UIs are not relevant to VR mode.
   blacklist = (
-      browser_root + 'LoginPrompt.java',
-      browser_root + 'SSLClientCertificateRequest.java',
-      browser_root + 'RepostFormWarningDialog.java',
-      browser_root + 'autofill/AutofillPopupBridge.java',
-      browser_root + 'autofill/CardUnmaskPrompt.java',
-      browser_root + 'autofill/keyboard_accessory/'
-          'AutofillKeyboardAccessoryBridge.java',
-      browser_root + 'browserservices/ClearDataDialogActivity.java',
-      browser_root + 'datausage/DataUseTabUIManager.java',
-      browser_root + 'dom_distiller/DistilledPagePrefsView.java',
-      browser_root + 'dom_distiller/DomDistillerUIUtils.java',
-      browser_root + 'download/DownloadController.java',
-      browser_root + 'download/OMADownloadHandler.java',
-      browser_root + 'externalnav/ExternalNavigationDelegateImpl.java',
-      browser_root + 'init/InvalidStartupDialog.java',
-      browser_root + 'omnibox/SuggestionView.java',
-      browser_root + 'payments/AndroidPaymentApp.java',
-      browser_root + 'password_manager/AccountChooserDialog.java',
-      browser_root + 'password_manager/AutoSigninFirstRunDialog.java',
-      browser_root + 'permissions/AndroidPermissionRequester.java',
-      browser_root + r'preferences[\\\/].*',
-      browser_root + 'share/ShareHelper.java',
-      browser_root + 'signin/AccountAdder.java',
-      browser_root + 'signin/AccountPickerDialogFragment.java',
-      browser_root + 'signin/AccountSigninView.java',
-      browser_root + 'signin/ConfirmImportSyncDataDialog.java',
-      browser_root + 'signin/ConfirmManagedSyncDataDialog.java',
-      browser_root + 'signin/ConfirmSyncDataStateMachineDelegate.java',
-      browser_root + 'signin/SigninFragmentBase.java',
-      browser_root + 'signin/SignOutDialogFragment.java',
-      browser_root + 'sync/ui/PassphraseCreationDialogFragment.java',
-      browser_root + 'sync/ui/PassphraseDialogFragment.java',
-      browser_root + 'sync/ui/PassphraseTypeDialogFragment.java',
-      browser_root + 'util/AccessibilityUtil.java',
-      browser_root + 'webapps/AddToHomescreenDialog.java',
-      browser_root + 'webapps/WebappOfflineDialog.java',
+      BROWSER_ROOT + 'browserservices/ClearDataDialogActivity.java',
+      BROWSER_ROOT + 'init/InvalidStartupDialog.java',
+      BROWSER_ROOT + 'password_manager/AccountChooserDialog.java',
+      BROWSER_ROOT + 'password_manager/AutoSigninFirstRunDialog.java',
+      BROWSER_ROOT + r'preferences[\\\/].*',
+      BROWSER_ROOT + 'signin/AccountAdder.java',
+      BROWSER_ROOT + 'signin/AccountPickerDialogFragment.java',
+      BROWSER_ROOT + 'signin/AccountSigninView.java',
+      BROWSER_ROOT + 'signin/ConfirmImportSyncDataDialog.java',
+      BROWSER_ROOT + 'signin/ConfirmManagedSyncDataDialog.java',
+      BROWSER_ROOT + 'signin/ConfirmSyncDataStateMachineDelegate.java',
+      BROWSER_ROOT + 'signin/SigninFragmentBase.java',
+      BROWSER_ROOT + 'signin/SignOutDialogFragment.java',
+      BROWSER_ROOT + 'sync/ui/PassphraseCreationDialogFragment.java',
+      BROWSER_ROOT + 'sync/ui/PassphraseDialogFragment.java',
+      BROWSER_ROOT + 'sync/ui/PassphraseTypeDialogFragment.java',
   )
   error_msg = '''
   AlertDialog.Builder Check failed:
   Your new code added one or more calls to the AlertDialog.Builder, listed
   below.
 
-  This breaks browsing when in VR, please use ModalDialogView instead of
-  AlertDialog.
-  Contact asimjour@chromium.org if you have any questions.
+  We recommend you use ModalDialogProperties to show a dialog whenever possible
+  to support VR mode and Touchless mode. You could only keep the AlertDialog if
+  you are certain that
+    1) Your new AlertDialog is not used in VR mode (e.g. pereference, FRE)
+    2) You have handled Touchless display if Touchless mode is relevant
+
+  If you are in doubt, contact
+  //src/chrome/android/java/src/org/chromium/chrome/browser/vr/VR_JAVA_OWNERS
+  //src/chrome/android/touchless/OWNERS
+  '''
+  error_files = []
+  result = _CheckReIgnoreComment(input_api, output_api, error_msg, blacklist,
+                                 NEW_ALERTDIALOG_BUILDER_RE, error_files)
+
+  wrong_builder_errors = []
+  wrong_builder_error_msg = '''
+  Android Use of AppCompat AlertDialog.Builder Check failed:
+  Your new code added one or more calls to the AppCompat AlertDialog.Builder,
+  file listed below.
+
+  If you are keeping the new AppCompat AlertDialog.Builder, please use
+  CompatibleAlertDialogBuilder instead to work around support library issues.
+
+  See https://crbug.com/966101 for more information.
+  '''
+  for f in error_files:
+    contents = input_api.ReadFile(f)
+    if IMPORT_APP_COMPAT_ALERTDIALOG_RE.search(contents):
+      wrong_builder_errors.append('  %s' % (f.LocalPath()))
+  if wrong_builder_errors:
+    result.extend([output_api.PresubmitError(
+        wrong_builder_error_msg, wrong_builder_errors)])
+  return result
+
+
+def _CheckCompatibleAlertDialogBuilder(input_api, output_api):
+  # "Blacklist" because the following files are excluded from the check.
+  blacklist = (
+      BROWSER_ROOT + 'LoginPrompt.java',
+      BROWSER_ROOT + 'SSLClientCertificateRequest.java',
+      BROWSER_ROOT + 'autofill/AutofillPopupBridge.java',
+      BROWSER_ROOT + 'autofill/keyboard_accessory/'
+                     'AutofillKeyboardAccessoryBridge.java',
+      BROWSER_ROOT + 'dom_distiller/DistilledPagePrefsView.java',
+      BROWSER_ROOT + 'dom_distiller/DomDistillerUIUtils.java',
+      BROWSER_ROOT + 'download/DownloadController.java',
+      BROWSER_ROOT + 'download/OMADownloadHandler.java',
+      BROWSER_ROOT + 'externalnav/ExternalNavigationDelegateImpl.java',
+      BROWSER_ROOT + 'payments/AndroidPaymentApp.java',
+      BROWSER_ROOT + 'permissions/AndroidPermissionRequester.java',
+      BROWSER_ROOT + 'share/ShareHelper.java',
+      BROWSER_ROOT + 'util/AccessibilityUtil.java',
+      BROWSER_ROOT + 'webapps/AddToHomescreenDialog.java',
+      BROWSER_ROOT + 'webapps/WebappOfflineDialog.java',
+  )
+  error_msg = '''
+  Android Use of CompatibleAlertDialogBuilder Check failed:
+  Your new code added one or more calls to the CompatibleAlertDialogBuilder
+  constructors, listed below.
+
+  We recommend you use ModalDialogProperties to show a dialog whenever possible
+  to support VR mode and Touchless mode. You could only keep the AlertDialog if
+  you are certain that
+    1) Your new AlertDialog is not used in VR mode (e.g. pereference, FRE)
+    2) You have handled Touchless display if Touchless mode is relevant
+
+  If you are in doubt, contact
+  //src/chrome/android/java/src/org/chromium/chrome/browser/vr/VR_JAVA_OWNERS
+  //src/chrome/android/touchless/OWNERS
   '''
   return _CheckReIgnoreComment(input_api, output_api, error_msg, blacklist,
-                               NEW_ALERTDIALOG_BUILDER_RE)
+                               NEW_COMPATIBLE_ALERTDIALOG_BUILDER_RE)
+
 
 def _CheckReIgnoreComment(input_api, output_api, error_msg, blacklist,
-                          regular_expression):
+                          regular_expression, error_files=None):
+
+  def CheckLine(current_file, line_number, line, problems, error_files):
+    """Returns a boolean whether the line contains an error."""
+    if (regular_expression.search(line) and not COMMENT_RE.search(line)):
+      if error_files is not None:
+        error_files.append(current_file)
+      problems.append(
+          '  %s:%d\n    \t%s' %
+          (current_file.LocalPath(), line_number, line.strip()))
+      return True
+    return False
+
   problems = []
   sources = lambda x: input_api.FilterSourceFile(
       x, white_list=(r'.*\.java$',), black_list=blacklist)
   for f in input_api.AffectedFiles(include_deletes=False,
                                    file_filter=sources):
+    previous_line = ''
     for line_number, line in f.ChangedContents():
-      if (regular_expression.search(line)
-          and not COMMENT_RE.search(line)):
-        problems.append(
-          '  %s:%d\n    \t%s' % (f.LocalPath(), line_number, line.strip()))
+      if not CheckLine(f, line_number, line, problems, error_files):
+        if previous_line:
+          two_lines = '\n'.join([previous_line, line])
+          CheckLine(f, line_number, two_lines, problems, error_files)
+        previous_line = line
+      else:
+        previous_line = ''
   if problems:
-    return [output_api.PresubmitError(
-      error_msg,
-      problems)]
+    return [output_api.PresubmitError(error_msg, problems)]
   return []

chrome/android/java/src/PRESUBMIT_test.py

@@ -13,6 +13,7 @@ sys.path.append(os.path.abspath(os.path.dirname(os.path.abspath(__file__))
     + '/../../../..'))
 from PRESUBMIT_test_mocks import MockFile, MockInputApi, MockOutputApi
 
+
 class CheckNotificationConstructors(unittest.TestCase):
   """Test the _CheckNotificationConstructors presubmit check."""
 
@@ -20,41 +21,42 @@ class CheckNotificationConstructors(unittest.TestCase):
     """Examples of when Notification.Builder use is correctly flagged."""
     mock_input = MockInputApi()
     mock_input.files = [
-      MockFile('path/One.java', ['new Notification.Builder()']),
-      MockFile('path/Two.java', ['new NotificationCompat.Builder()']),
+        MockFile('path/One.java', ['new Notification.Builder()']),
+        MockFile('path/Two.java', ['new NotificationCompat.Builder()']),
     ]
     errors = PRESUBMIT._CheckNotificationConstructors(
         mock_input, MockOutputApi())
     self.assertEqual(1, len(errors))
     self.assertEqual(2, len(errors[0].items))
-    self.assertTrue('One.java' in errors[0].items[0])
-    self.assertTrue('Two.java' in errors[0].items[1])
+    self.assertIn('One.java', errors[0].items[0])
+    self.assertIn('Two.java', errors[0].items[1])
 
   def testFalsePositives(self):
     """Examples of when Notification.Builder should not be flagged."""
     mock_input = MockInputApi()
     mock_input.files = [
-      MockFile(
-          'chrome/android/java/src/org/chromium/chrome/browser/notifications/'
-              'NotificationBuilder.java',
-          ['new Notification.Builder()']),
-      MockFile(
-          'chrome/android/java/src/org/chromium/chrome/browser/notifications/'
-              'NotificationCompatBuilder.java',
-          ['new NotificationCompat.Builder()']),
-      MockFile('path/One.java', ['Notification.Builder']),
-      MockFile('path/Two.java', ['// do not: new Notification.Builder()']),
-      MockFile('path/Three.java', [
-          '/** ChromeNotificationBuilder',
-          ' * replaces: new Notification.Builder()']),
-      MockFile('path/PRESUBMIT.py', ['new Notification.Builder()']),
-      MockFile('path/Four.java', ['new NotificationCompat.Builder()'],
-          action='D'),
+        MockFile(
+            'chrome/android/java/src/org/chromium/chrome/browser/notifications/'
+            'NotificationBuilder.java',
+            ['new Notification.Builder()']),
+        MockFile(
+            'chrome/android/java/src/org/chromium/chrome/browser/notifications/'
+            'NotificationCompatBuilder.java',
+            ['new NotificationCompat.Builder()']),
+        MockFile('path/One.java', ['Notification.Builder']),
+        MockFile('path/Two.java', ['// do not: new Notification.Builder()']),
+        MockFile('path/Three.java',
+                 ['/** ChromeNotificationBuilder',
+                  ' * replaces: new Notification.Builder()']),
+        MockFile('path/PRESUBMIT.py', ['new Notification.Builder()']),
+        MockFile('path/Four.java', ['new NotificationCompat.Builder()'],
+                 action='D'),
     ]
     errors = PRESUBMIT._CheckNotificationConstructors(
         mock_input, MockOutputApi())
     self.assertEqual(0, len(errors))
 
+
 class CheckAlertDialogBuilder(unittest.TestCase):
   """Test the _CheckAlertDialogBuilder presubmit check."""
 
@@ -62,36 +64,121 @@ class CheckAlertDialogBuilder(unittest.TestCase):
     """Examples of when AlertDialog.Builder use is correctly flagged."""
     mock_input = MockInputApi()
     mock_input.files = [
-      MockFile('path/One.java', ['new AlertDialog.Builder()']),
-      MockFile('path/Two.java', ['new AlertDialog.Builder(context);']),
+        MockFile('path/One.java', ['new AlertDialog.Builder()']),
+        MockFile('path/Two.java', ['new AlertDialog.Builder(context);']),
     ]
-    errors = PRESUBMIT._CheckAlertDialogBuilder(
-        mock_input, MockOutputApi())
+    errors = PRESUBMIT._CheckAlertDialogBuilder(mock_input, MockOutputApi())
     self.assertEqual(1, len(errors))
     self.assertEqual(2, len(errors[0].items))
-    self.assertTrue('One.java' in errors[0].items[0])
-    self.assertTrue('Two.java' in errors[0].items[1])
+    self.assertIn('One.java', errors[0].items[0])
+    self.assertIn('Two.java', errors[0].items[1])
 
   def testFalsePositives(self):
     """Examples of when AlertDialog.Builder should not be flagged."""
     mock_input = MockInputApi()
     mock_input.files = [
-      MockFile(
-          'chrome/android/java/src/org/chromium/chrome/browser/payments/'
-              'AndroidPaymentApp.java',
-          ['new AlertDialog.Builder()']),
-      MockFile('path/One.java', ['AlertDialog.Builder']),
-      MockFile('path/Two.java', ['// do not: new AlertDialog.Builder()']),
-      MockFile('path/Three.java', [
-          '/** ChromeAlertDialogBuilder',
-          ' * replaces: new AlertDialog.Builder()']),
-      MockFile('path/PRESUBMIT.py', ['new AlertDialog.Builder()']),
-      MockFile('path/Four.java', ['new AlertDialog.Builder()'],
-          action='D'),
+        MockFile('chrome/android/java/src/org/chromium/chrome/browser/signin'
+                 '/AccountAdder.java',
+                 ['new AlertDialog.Builder()']),
+        MockFile('path/One.java', ['AlertDialog.Builder']),
+        MockFile('path/Two.java', ['// do not: new AlertDialog.Builder()']),
+        MockFile('path/Three.java',
+                 ['/** ChromeAlertDialogBuilder',
+                  ' * replaces: new AlertDialog.Builder()']),
+        MockFile('path/PRESUBMIT.py', ['new AlertDialog.Builder()']),
+        MockFile('path/Four.java', ['new AlertDialog.Builder()'],
+                 action='D'),
+    ]
+    errors = PRESUBMIT._CheckAlertDialogBuilder(
+        mock_input, MockOutputApi())
+    self.assertEqual(0, len(errors))
+
+  def testFailure_WrongBuilderCheck(self):
+    """Use of AppCompat AlertDialog.Builder is correctly flagged."""
+    mock_input = MockInputApi()
+    mock_input.files = [
+        MockFile('path/One.java',
+                 ['import android.support.v7.app.AlertDialog;',
+                  'new AlertDialog.Builder()']),
+        MockFile('path/Two.java',
+                 ['import android.app.AlertDialog;',
+                  'new AlertDialog.Builder(context);']),
     ]
     errors = PRESUBMIT._CheckAlertDialogBuilder(
         mock_input, MockOutputApi())
+    self.assertEqual(2, len(errors))
+    self.assertEqual(1, len(errors[1].items))
+    self.assertIn('One.java', errors[1].items[0])
+
+  def testSucess_WrongBuilderCheck(self):
+    """Use of OS-dependent AlertDialog should not be flagged."""
+    mock_input = MockInputApi()
+    mock_input.files = [
+        MockFile('path/One.java',
+                 ['import android.app.AlertDialog;',
+                  'new AlertDialog.Builder()']),
+        MockFile('path/Two.java',
+                 ['import android.app.AlertDialog;',
+                  'new AlertDialog.Builder(context);']),
+    ]
+    errors = PRESUBMIT._CheckAlertDialogBuilder(mock_input, MockOutputApi())
+    self.assertEqual(1, len(errors))
+    self.assertEqual(2, len(errors[0].items))
+
+
+class CheckCompatibleAlertDialogBuilder(unittest.TestCase):
+  """Test the _CheckCompatibleAlertDialogBuilder presubmit check."""
+
+  def testFailure(self):
+    """Use of CompatibleAlertDialogBuilder use is correctly flagged."""
+    mock_input = MockInputApi()
+    mock_input.files = [
+        MockFile('path/One.java',
+                 ['import '
+                  'org.chromium.ui.UiUtils.CompatibleAlertDialogBuilder;',
+                  'new CompatibleAlertDialogBuilder()',
+                  'A new line to make sure there is no duplicate error.']),
+        MockFile('path/Two.java',
+                 ['new UiUtils.CompatibleAlertDialogBuilder()']),
+        MockFile('path/Three.java',
+                 ['new UiUtils',
+                  '.CompatibleAlertDialogBuilder(context)']),
+        MockFile('path/Four.java',
+                 ['new UiUtils',
+                  '   .CompatibleAlertDialogBuilder()']),
+    ]
+    errors = PRESUBMIT._CheckCompatibleAlertDialogBuilder(
+        mock_input, MockOutputApi())
+    self.assertEqual(1, len(errors))
+    self.assertEqual(4, len(errors[0].items))
+    self.assertIn('One.java', errors[0].items[0])
+    self.assertIn('Two.java', errors[0].items[1])
+    self.assertIn('Three.java', errors[0].items[2])
+    self.assertIn('Four.java', errors[0].items[3])
+
+  def testSucess(self):
+    """Examples of when AlertDialog.Builder should not be flagged."""
+    mock_input = MockInputApi()
+    mock_input.files = [
+        MockFile('chrome/android/java/src/org/chromium/chrome/browser/payments/'
+                 'AndroidPaymentApp.java',
+                 ['new UiUtils.CompatibleAlertDialogBuilder()']),
+        MockFile('path/One.java', ['UiUtils.CompatibleAlertDialogBuilder']),
+        MockFile('path/Two.java',
+                 ['// do not: new UiUtils.CompatibleAlertDialogBuilder']),
+        MockFile('path/Three.java',
+                 ['/** ChromeAlertDialogBuilder',
+                  ' * replaces: new UiUtils.CompatibleAlertDialogBuilder()']),
+        MockFile('path/PRESUBMIT.py',
+                 ['new UiUtils.CompatibleAlertDialogBuilder()']),
+        MockFile('path/Four.java',
+                 ['new UiUtils.CompatibleAlertDialogBuilder()'],
+                 action='D'),
+    ]
+    errors = PRESUBMIT._CheckCompatibleAlertDialogBuilder(
+        mock_input, MockOutputApi())
     self.assertEqual(0, len(errors))
 
+
 if __name__ == '__main__':
   unittest.main()