[Android WebAPK] Rename webapk/libs/common/res_splash to avoid merging

This CL:
- Renames the resources in webapk/libs/common/res_splash to avoid
the need for merging resources with the same name prior to uploading
them to the Google storage build bucket.
- Adds a PRESUBMIT check to ensure that resource files names in
webapk/shell_apk/res differ from the resource file names in
webapk/libs/common/res_splash

BUG=969591

Change-Id: I8aa6062e8f9b37d008d185ea45dc4f04606d2c03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1642976
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666066}

chrome/android/webapk/PRESUBMIT.py

@@ -7,12 +7,18 @@
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into depot_tools.
 
-This presubmit checks for two rules:
+This presubmit checks for three rules:
 1. If anything in the webapk/libs/common or the webapk/shell_apk directories
 has changed (excluding test files), $CURRENT_VERSION_VARIABLE should be updated.
 2. If $REQUEST_UPDATE_FOR_VERSION_VARIABLE in
 $REQUEST_UPDATE_FOR_VERSION_LOCAL_PATH is changed, the variable change should
 be the only change in the CL.
+3. If a file in a res/ directory has been added, its' file name should be
+unique to the res/ directory.
+  res/values/dimens.xml and res/values-v17/dimens.xml -> OK
+  res/values/dimens.xml and libs/common/res_splash/values/dimens.xml -> BAD
+This requirement is needed to upload the resources to the Google storage
+build bucket.
 """
 
 CURRENT_VERSION_VARIABLE = 'current_shell_apk_version'
@@ -24,11 +30,14 @@ REQUEST_UPDATE_FOR_VERSION_LOCAL_PATH = (
 
 TRIGGER_CURRENT_VERSION_UPDATE_LOCAL_PATHS = [
     'libs/common/src/',
+    'libs/common/res_splash/',
     'shell_apk/AndroidManifest.xml',
     'shell_apk/res/',
     'shell_apk/src/',
 ]
 
+RES_DIR_LOCAL_PATHS = ['shell_apk/res', 'libs/common/res_splash']
+
 def _DoChangedContentsContain(changed_contents, key):
   for _, line in changed_contents:
     if key in line:
@@ -36,6 +45,19 @@ def _DoChangedContentsContain(changed_contents, key):
   return False
 
 
+def _FindFileNamesInDirectory(input_api, dir_path, search_file_names):
+  """
+  Searches the directory recursively for files with the passed-in file name
+  (not file path) set. Returns the file names of any matches.
+  """
+  matches = []
+  for _, _, file_names in input_api.os_walk(dir_path):
+    for file_name in file_names:
+      if file_name in search_file_names:
+        matches.append(file_name)
+  return matches
+
+
 def _CheckVersionVariableChanged(input_api, version_file_local_path,
                                  variable_name):
   for f in input_api.AffectedFiles():
@@ -92,11 +114,54 @@ def _CheckCurrentVersionIncreaseRule(input_api, output_api):
   return []
 
 
+def _CheckNoOverlappingFileNamesInResourceDirsRule(input_api, output_api):
+  """
+  Checks that if a file has been added to a res/ directory that its file name
+  is unique to the res/ directory.
+    res/values/dimens.xml and res/values-v17/dimens.xml -> OK
+    res/values/dimens.xml and libs/common/res_splash/values/dimens.xml -> BAD
+  """
+  res_dir_file_names_map = {}
+  for f in input_api.AffectedFiles():
+    local_path = input_api.os_path.relpath(f.AbsoluteLocalPath(),
+                                           input_api.PresubmitLocalPath())
+    for res_dir_local_path in RES_DIR_LOCAL_PATHS:
+      if local_path.startswith(res_dir_local_path):
+        file_name = input_api.os_path.basename(local_path)
+        res_dir_file_names_map.setdefault(res_dir_local_path, set()).add(
+            file_name)
+        break
+
+  if len(res_dir_file_names_map) == 0:
+    return []
+
+  overlapping_file_names = set()
+  for res_dir, file_names in res_dir_file_names_map.items():
+    for other_res_dir, other_file_names in res_dir_file_names_map.items():
+      if res_dir == other_res_dir:
+        continue
+
+      # Check for affected files with identical name in |other_res_dir|.
+      overlapping_file_names |= (file_names & other_file_names)
+
+      # Check for existing files with identical name in |other_res_dir|.
+      overlapping_file_names.update(
+          _FindFileNamesInDirectory(input_api, other_res_dir, file_names))
+
+  if len(overlapping_file_names) > 0:
+    error_msg = ('Resources in different top level res/ directories {} should '
+                 'have different names:').format(RES_DIR_LOCAL_PATHS)
+    return [output_api.PresubmitError(error_msg,
+                                      items=list(overlapping_file_names))]
+  return []
+
 def _CommonChecks(input_api, output_api):
   """Checks common to both upload and commit."""
   result = []
   result.extend(_CheckChromeUpdateTriggerRule(input_api, output_api))
   result.extend(_CheckCurrentVersionIncreaseRule(input_api, output_api))
+  result.extend(_CheckNoOverlappingFileNamesInResourceDirsRule(input_api,
+                                                               output_api))
 
   return result
 

chrome/android/webapk/PRESUBMIT_test.py

@@ -14,12 +14,36 @@ sys.path.insert(0, os.path.join(file_dir_path, '..', '..', '..'))
 from PRESUBMIT_test_mocks import MockAffectedFile
 from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi
 
-class ShellApkVersion(unittest.TestCase):
-   UPDATE_CURRENT_VERSION_MESSAGE = (
-       'current_shell_apk_version in '
-       'shell_apk/current_version/current_version.gni needs to updated due to '
-       'changes in:')
+# Mocks os.walk()
+class MockOsWalkFileSystem(object):
+  def __init__(self, file_paths):
+    self.file_paths = file_paths
+
+  def walk(self, top):
+    if not top.endswith('/'):
+      top += '/'
 
+    files = []
+    dirs = []
+    for f in self.file_paths:
+      if f.startswith(top):
+        remaining = f[len(top):]
+        slash_index = remaining.find('/')
+        if slash_index >= 0:
+          dir_name = remaining[:slash_index]
+          if not dir_name in dirs:
+            dirs.append(dir_name)
+        else:
+          files.append(remaining)
+
+      yield top[:-1], dirs, files
+
+    for name in dirs:
+      for result in self.walk(top + name):
+        yield result
+
+
+class CustomMockInputApi(MockInputApi):
    def makeMockAffectedFiles(self, file_names):
      mock_files = []
      for file_name in file_names:
@@ -27,6 +51,13 @@ class ShellApkVersion(unittest.TestCase):
            MockAffectedFile(file_name, ['new_content'], action='A'))
      return mock_files
 
+
+class ShellApkVersion(unittest.TestCase):
+   UPDATE_CURRENT_VERSION_MESSAGE = (
+       'current_shell_apk_version in '
+       'shell_apk/current_version/current_version.gni needs to updated due to '
+       'changes in:')
+
    def testCheckWamMintTriggerRule(self):
      COMMON_SRC_FILE_PATH = (
          'libs/common/src/org/chromium/webapk/lib/common/A.java')
@@ -46,8 +77,8 @@ class ShellApkVersion(unittest.TestCase):
 
      # template_shell_apk_version not updated. There should be a warning about
      # template_shell_apk_version needing to be updated.
-     input_api = MockInputApi()
-     input_api.files = self.makeMockAffectedFiles(
+     input_api = CustomMockInputApi()
+     input_api.files = input_api.makeMockAffectedFiles(
          changed_java_file_paths + [SHELL_APK_RES_FILE_PATH])
      input_api.files += [
          MockAffectedFile(CURRENT_VERSION_FILE_PATH, 'variable=O',
@@ -62,7 +93,7 @@ class ShellApkVersion(unittest.TestCase):
                       warnings[0].items)
 
      # template_shell_apk_version updated. There should be no warnings.
-     input_api.files = self.makeMockAffectedFiles(
+     input_api.files = input_api.makeMockAffectedFiles(
          changed_java_file_paths + [SHELL_APK_RES_FILE_PATH])
      input_api.files += [
          MockAffectedFile(CURRENT_VERSION_FILE_PATH,
@@ -73,5 +104,59 @@ class ShellApkVersion(unittest.TestCase):
                                                            MockOutputApi())
      self.assertEqual([], warnings)
 
+
+class OverlappingResourceFileNames(unittest.TestCase):
+   RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE = (
+       'Resources in different top level res/ directories [\'shell_apk/res\', '
+       '\'libs/common/res_splash\'] should have different names:')
+
+   def testAddFileSameNameWithinResDirectory(self):
+     # Files within a res/ directory can have same file name.
+     MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml',
+                               'libs/common/res_splash/values/dimens.xml']
+     input_api = CustomMockInputApi()
+     input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk
+
+     input_api.files = input_api.makeMockAffectedFiles([
+         'shell_apk/res/values-v22/values.xml'])
+     errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule(
+         input_api, MockOutputApi())
+     self.assertEqual(0, len(errors))
+
+   def testAddFileSameNameAcrossResDirectories(self):
+     # Adding a file to a res/ directory with the same file name as a file in a
+     # different res/ directory is illegal.
+     MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml',
+                               'libs/common/res_splash/values/dimens.xml']
+     input_api = CustomMockInputApi()
+     input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk
+     input_api.files = input_api.makeMockAffectedFiles([
+         'shell_apk/res/values-v17/dimens.xml',
+         'libs/common/res_splash/values-v22/colors.xml'])
+     errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule(
+         input_api, MockOutputApi())
+     self.assertEqual(1, len(errors))
+     self.assertEqual(self.RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE,
+                      errors[0].message)
+     errors[0].items.sort()
+     self.assertEqual(['colors.xml', 'dimens.xml'], errors[0].items)
+
+   def testAddTwoFilesWithSameNameDifferentResDirectories(self):
+     # Adding two files with the same file name but in different res/
+     # directories is illegal.
+     MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml',
+                               'libs/common/res_splash/values/dimens.xml']
+     input_api = CustomMockInputApi()
+     input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk
+     input_api.files = input_api.makeMockAffectedFiles([
+         'shell_apk/res/values/values.xml',
+         'libs/common/res_splash/values-v22/values.xml'])
+     errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule(
+         input_api, MockOutputApi())
+     self.assertEqual(1, len(errors))
+     self.assertEqual(self.RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE,
+                      errors[0].message)
+     self.assertEqual(['values.xml'], errors[0].items)
+
 if __name__ == '__main__':
   unittest.main()

chrome/android/webapk/shell_apk/current_version/current_version.gni

@@ -12,4 +12,4 @@
 # //chrome/android/webapk/shell_apk:webapk is changed. This includes
 # Java files, Android resource files and AndroidManifest.xml. Does not affect
 # Chrome.apk
-current_shell_apk_version = 91
+current_shell_apk_version = 92