[Android WebAPK] Add build step checking current_shell_apk_version is updated 2/3

The goal of this "CL set" (1-3) is to:
- Force people to update template_shell_apk_version when code which is
  compiled into WebApk.apk is updated
- Don't require additional reviewers for updating template_shell_apk_version.

This CL:
- Moves template_shell_apk_version into into a new file in a directory where
  it is the only file.
- Adds an OWNERS file to allow anyone to update template_shell_apk_version.
- Renames template_shell_apk_version and expected_shell_apk_version so that
  the GN variable names are more intuitive

BUG=884444

Change-Id: Ic7a15506511f2bbffe2beea04d51e1a6e6d1aed5
Reviewed-on: https://chromium-review.googlesource.com/1243669
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595575}

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java

@@ -229,7 +229,7 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
      * Whether there is a new version of the //chrome/android/webapk/shell_apk code.
      */
     private static boolean isShellApkVersionOutOfDate(WebApkInfo info) {
-        return info.shellApkVersion() < WebApkVersion.CURRENT_SHELL_APK_VERSION;
+        return info.shellApkVersion() < WebApkVersion.REQUEST_UPDATE_FOR_SHELL_APK_VERSION;
     }
 
     /**
@@ -249,7 +249,7 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
         if (!info.apkPackageName().startsWith(WEBAPK_PACKAGE_PREFIX)) return false;
 
         if (isShellApkVersionOutOfDate(info)
-                && WebApkVersion.CURRENT_SHELL_APK_VERSION
+                && WebApkVersion.REQUEST_UPDATE_FOR_SHELL_APK_VERSION
                         > mStorage.getLastRequestedShellApkVersion()) {
             return true;
         }
@@ -267,7 +267,8 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
         mStorage.updateTimeOfLastWebApkUpdateRequestCompletion();
         mStorage.updateDidLastWebApkUpdateRequestSucceed(result == WebApkInstallResult.SUCCESS);
         mStorage.setRelaxedUpdates(relaxUpdates);
-        mStorage.updateLastRequestedShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION);
+        mStorage.updateLastRequestedShellApkVersion(
+                WebApkVersion.REQUEST_UPDATE_FOR_SHELL_APK_VERSION);
     }
 
     /**

chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java

@@ -156,7 +156,8 @@ public class WebApkUpdateManagerTest {
                 WebApkInfo info = WebApkInfo.create(WEBAPK_ID, "", creationData.scope, null, null,
                         null, creationData.name, creationData.shortName, creationData.displayMode,
                         creationData.orientation, 0, creationData.themeColor,
-                        creationData.backgroundColor, "", WebApkVersion.CURRENT_SHELL_APK_VERSION,
+                        creationData.backgroundColor, "",
+                        WebApkVersion.REQUEST_UPDATE_FOR_SHELL_APK_VERSION,
                         creationData.manifestUrl, creationData.startUrl,
                         WebApkInfo.WebApkDistributor.BROWSER, creationData.iconUrlToMurmur2HashMap,
                         null, false /* forceNavigation */);

chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerUnitTest.java

@@ -10,7 +10,7 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
-import static org.chromium.webapk.lib.client.WebApkVersion.CURRENT_SHELL_APK_VERSION;
+import static org.chromium.webapk.lib.client.WebApkVersion.REQUEST_UPDATE_FOR_SHELL_APK_VERSION;
 
 import android.content.Intent;
 import android.graphics.Bitmap;
@@ -347,7 +347,8 @@ public class WebApkUpdateManagerUnitTest {
      */
     private boolean checkUpdateNeededForFetchedManifest(
             ManifestData androidManifestData, ManifestData fetchedManifestData) {
-        registerWebApk(WEBAPK_PACKAGE_NAME, androidManifestData, CURRENT_SHELL_APK_VERSION);
+        registerWebApk(
+                WEBAPK_PACKAGE_NAME, androidManifestData, REQUEST_UPDATE_FOR_SHELL_APK_VERSION);
         mClockRule.advance(WebappDataStorage.UPDATE_INTERVAL);
 
         TestWebApkUpdateManager updateManager =
@@ -364,7 +365,8 @@ public class WebApkUpdateManagerUnitTest {
         PathUtils.setPrivateDataDirectorySuffix("chrome");
         CommandLine.init(null);
 
-        registerWebApk(WEBAPK_PACKAGE_NAME, defaultManifestData(), CURRENT_SHELL_APK_VERSION);
+        registerWebApk(
+                WEBAPK_PACKAGE_NAME, defaultManifestData(), REQUEST_UPDATE_FOR_SHELL_APK_VERSION);
 
         WebappRegistry.getInstance().register(getWebApkId(WEBAPK_PACKAGE_NAME),
                 new WebappRegistry.FetchWebappDataStorageCallback() {
@@ -545,7 +547,8 @@ public class WebApkUpdateManagerUnitTest {
      */
     @Test
     public void testShellApkOutOfDateNoWebManifest() {
-        registerWebApk(WEBAPK_PACKAGE_NAME, defaultManifestData(), CURRENT_SHELL_APK_VERSION - 1);
+        registerWebApk(WEBAPK_PACKAGE_NAME, defaultManifestData(),
+                REQUEST_UPDATE_FOR_SHELL_APK_VERSION - 1);
         mClockRule.advance(WebappDataStorage.UPDATE_INTERVAL);
 
         TestWebApkUpdateManager updateManager =
@@ -568,7 +571,8 @@ public class WebApkUpdateManagerUnitTest {
      */
     @Test
     public void testShellApkOutOfDateStillHasWebManifest() {
-        registerWebApk(WEBAPK_PACKAGE_NAME, defaultManifestData(), CURRENT_SHELL_APK_VERSION - 1);
+        registerWebApk(WEBAPK_PACKAGE_NAME, defaultManifestData(),
+                REQUEST_UPDATE_FOR_SHELL_APK_VERSION - 1);
         mClockRule.advance(WebappDataStorage.UPDATE_INTERVAL);
 
         TestWebApkUpdateManager updateManager =
@@ -658,7 +662,8 @@ public class WebApkUpdateManagerUnitTest {
     public void testUnboundWebApkDoesNotUpgrade() {
         ManifestData androidManifestData = defaultManifestData();
 
-        registerWebApk(UNBOUND_WEBAPK_PACKAGE_NAME, androidManifestData, CURRENT_SHELL_APK_VERSION);
+        registerWebApk(UNBOUND_WEBAPK_PACKAGE_NAME, androidManifestData,
+                REQUEST_UPDATE_FOR_SHELL_APK_VERSION);
         mClockRule.advance(WebappDataStorage.UPDATE_INTERVAL);
 
         TestWebApkUpdateManager updateManager =
@@ -819,7 +824,8 @@ public class WebApkUpdateManagerUnitTest {
      */
     @Test
     public void testShellApkOutOfDate() {
-        registerWebApk(WEBAPK_PACKAGE_NAME, defaultManifestData(), CURRENT_SHELL_APK_VERSION - 1);
+        registerWebApk(WEBAPK_PACKAGE_NAME, defaultManifestData(),
+                REQUEST_UPDATE_FOR_SHELL_APK_VERSION - 1);
         TestWebApkUpdateManager updateManager =
                 new TestWebApkUpdateManager(getStorage(WEBAPK_PACKAGE_NAME));
 

chrome/android/webapk/PRESUBMIT.py

@@ -9,20 +9,20 @@ for more details about the presubmit API built into depot_tools.
 
 This presubmit checks for two rules:
 1. If anything in the webapk/libs/common or the webapk/shell_apk directories
-has changed (excluding test files), $WAM_MINT_TRIGGER_VARIABLE should be
-updated.
-2. If $CHROME_UPDATE_TIRGGER_VARIABLE is changed in
-$SHELL_APK_VERSION_LOCAL_PATH, $SHELL_APK_VERSION_LOCAL_PATH should be the
-only changed file and changing $CHROME_UPDATE_TRIGGER_VARIABLE should be
-the only change.
+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.
 """
 
-WAM_MINT_TRIGGER_VARIABLE = r'template_shell_apk_version'
-CHROME_UPDATE_TRIGGER_VARIABLE = r'expected_shell_apk_version'
+CURRENT_VERSION_VARIABLE = 'current_shell_apk_version'
+CURRENT_VERSION_LOCAL_PATH = 'shell_apk/current_version/current_version.gni'
 
-SHELL_APK_VERSION_LOCAL_PATH = r'shell_apk/shell_apk_version.gni'
+REQUEST_UPDATE_FOR_VERSION_VARIABLE = 'request_update_for_shell_apk_version'
+REQUEST_UPDATE_FOR_VERSION_LOCAL_PATH = (
+    'shell_apk/request_update_for_shell_apk_version.gni')
 
-WAM_MINT_TRIGGER_LOCAL_PATHS = [
+TRIGGER_CURRENT_VERSION_UPDATE_LOCAL_PATHS = [
     'libs/common/src/',
     'shell_apk/AndroidManifest.xml',
     'shell_apk/res/',
@@ -52,20 +52,21 @@ def _CheckChromeUpdateTriggerRule(input_api, output_api):
   Check that if |expected_shell_apk_version| is updated it is the only
   change in the CL.
   """
-  if _CheckVersionVariableChanged(input_api, SHELL_APK_VERSION_LOCAL_PATH,
-                                  CHROME_UPDATE_TRIGGER_VARIABLE):
+  if _CheckVersionVariableChanged(input_api,
+                                  REQUEST_UPDATE_FOR_VERSION_LOCAL_PATH,
+                                  REQUEST_UPDATE_FOR_VERSION_VARIABLE):
     if (len(input_api.AffectedFiles()) != 1 or
         len(input_api.AffectedFiles[0].ChangedContents()) != 1):
       return [
         output_api.PresubmitError(
             '{} in {} must be updated in a standalone CL.'.format(
-                CHROME_UPDATE_TRIGGER_VARIABLE,
-                SHELL_APK_VERSION_LOCAL_PATH))
+                REQUEST_UPDATE_FOR_VERSION_VARIABLE,
+                REQUEST_UPDATE_FOR_VERSION_LOCAL_PATH))
         ]
   return []
 
 
-def _CheckWamMintTriggerRule(input_api, output_api):
+def _CheckCurrentVersionIncreaseRule(input_api, output_api):
   """
   Check that if a file in $WAM_MINT_TRIGGER_LOCAL_PATHS is updated that
   |template_shell_apk_version| is updated as well.
@@ -74,18 +75,18 @@ def _CheckWamMintTriggerRule(input_api, output_api):
   for f in input_api.AffectedFiles():
     local_path = input_api.os_path.relpath(f.AbsoluteLocalPath(),
                                            input_api.PresubmitLocalPath())
-    for wam_mint_trigger_local_path in WAM_MINT_TRIGGER_LOCAL_PATHS:
-      if local_path.startswith(wam_mint_trigger_local_path):
+    for trigger_local_path in TRIGGER_CURRENT_VERSION_UPDATE_LOCAL_PATHS:
+      if local_path.startswith(trigger_local_path):
         files_requiring_version_increase.append(local_path)
 
   if not files_requiring_version_increase:
     return []
 
-  if not _CheckVersionVariableChanged(input_api, SHELL_APK_VERSION_LOCAL_PATH,
-                                      WAM_MINT_TRIGGER_VARIABLE):
+  if not _CheckVersionVariableChanged(input_api, CURRENT_VERSION_LOCAL_PATH,
+                                      CURRENT_VERSION_VARIABLE):
     return [output_api.PresubmitPromptWarning(
         '{} in {} needs to updated due to changes in:'.format(
-            WAM_MINT_TRIGGER_VARIABLE, SHELL_APK_VERSION_LOCAL_PATH),
+            CURRENT_VERSION_VARIABLE, CURRENT_VERSION_LOCAL_PATH),
         items=files_requiring_version_increase)]
 
   return []
@@ -95,7 +96,7 @@ def _CommonChecks(input_api, output_api):
   """Checks common to both upload and commit."""
   result = []
   result.extend(_CheckChromeUpdateTriggerRule(input_api, output_api))
-  result.extend(_CheckWamMintTriggerRule(input_api, output_api))
+  result.extend(_CheckCurrentVersionIncreaseRule(input_api, output_api))
 
   return result
 

chrome/android/webapk/PRESUBMIT_test.py

@@ -15,9 +15,10 @@ from PRESUBMIT_test_mocks import MockAffectedFile
 from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi
 
 class ShellApkVersion(unittest.TestCase):
-   UPDATE_TEMPLATE_SHELL_APK_VERSION_MESSAGE = (
-       'template_shell_apk_version in shell_apk/shell_apk_version.gni needs to '
-       'updated due to changes in:')
+   UPDATE_CURRENT_VERSION_MESSAGE = (
+       'current_shell_apk_version in '
+       'shell_apk/current_version/current_version.gni needs to updated due to '
+       'changes in:')
 
    def makeMockAffectedFiles(self, file_names):
      mock_files = []
@@ -41,7 +42,7 @@ class ShellApkVersion(unittest.TestCase):
      ]
 
      SHELL_APK_RES_FILE_PATH = 'shell_apk/res/mipmap-xxxxxxhdpi/app_icon.png'
-     SHELL_APK_VERSION_FILE_PATH = 'shell_apk/shell_apk_version.gni'
+     CURRENT_VERSION_FILE_PATH = 'shell_apk/current_version/current_version.gni'
 
      # template_shell_apk_version not updated. There should be a warning about
      # template_shell_apk_version needing to be updated.
@@ -49,13 +50,13 @@ class ShellApkVersion(unittest.TestCase):
      input_api.files = self.makeMockAffectedFiles(
          changed_java_file_paths + [SHELL_APK_RES_FILE_PATH])
      input_api.files += [
-         MockAffectedFile(SHELL_APK_VERSION_FILE_PATH, 'variable=O',
+         MockAffectedFile(CURRENT_VERSION_FILE_PATH, 'variable=O',
                           'variable=N', action='M')
      ]
-     warnings = PRESUBMIT._CheckWamMintTriggerRule(input_api, MockOutputApi())
+     warnings = PRESUBMIT._CheckCurrentVersionIncreaseRule(input_api,
+                                                           MockOutputApi())
      self.assertEqual(1, len(warnings))
-     self.assertEqual(self.UPDATE_TEMPLATE_SHELL_APK_VERSION_MESSAGE,
-                      warnings[0].message)
+     self.assertEqual(self.UPDATE_CURRENT_VERSION_MESSAGE, warnings[0].message)
      self.assertEqual([COMMON_SRC_FILE_PATH, SHELL_APK_SRC_FILE_PATH,
                        SHELL_APK_RES_FILE_PATH],
                       warnings[0].items)
@@ -64,11 +65,12 @@ class ShellApkVersion(unittest.TestCase):
      input_api.files = self.makeMockAffectedFiles(
          changed_java_file_paths + [SHELL_APK_RES_FILE_PATH])
      input_api.files += [
-         MockAffectedFile(SHELL_APK_VERSION_FILE_PATH,
-                          ['template_shell_apk_version=1'],
-                          ['template_shell_apk_version=2'], action='M')
+         MockAffectedFile(CURRENT_VERSION_FILE_PATH,
+                          ['current_shell_apk_version=1'],
+                          ['current_shell_apk_version=2'], action='M')
      ]
-     warnings = PRESUBMIT._CheckWamMintTriggerRule(input_api, MockOutputApi())
+     warnings = PRESUBMIT._CheckCurrentVersionIncreaseRule(input_api,
+                                                           MockOutputApi())
      self.assertEqual([], warnings)
 
 if __name__ == '__main__':

chrome/android/webapk/libs/client/BUILD.gn

@@ -4,7 +4,7 @@
 
 import("//build/config/android/rules.gni")
 import("//chrome/android/webapk/libs/runtime_library_version.gni")
-import("//chrome/android/webapk/shell_apk/shell_apk_version.gni")
+import("//chrome/android/webapk/shell_apk/request_update_for_version.gni")
 
 android_library("client_java") {
   java_files = [
@@ -31,7 +31,7 @@ java_cpp_template("runtime_library_version_java") {
   ]
   defines = [
     "CURRENT_RUNTIME_DEX_VERSION_VALUE=$runtime_library_version",
-    "CURRENT_SHELL_APK_VERSION_VALUE=$expected_shell_apk_version",
+    "REQUEST_UPDATE_FOR_SHELL_APK_VERSION_VALUE=$request_update_for_shell_apk_version",
   ]
 }
 

chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVersion.template

@@ -8,6 +8,6 @@ public class WebApkVersion {
 
 public static final int CURRENT_RUNTIME_DEX_VERSION = CURRENT_RUNTIME_DEX_VERSION_VALUE;
 
-public static final int CURRENT_SHELL_APK_VERSION = CURRENT_SHELL_APK_VERSION_VALUE;
+public static final int REQUEST_UPDATE_FOR_SHELL_APK_VERSION = REQUEST_UPDATE_FOR_SHELL_APK_VERSION_VALUE;
 
 }

chrome/android/webapk/shell_apk/BUILD.gn

@@ -3,8 +3,8 @@
 # found in the LICENSE file.
 
 import("//build/config/android/rules.gni")
+import("current_version/current_version.gni")
 import("manifest_processor.gni")
-import("shell_apk_version.gni")
 
 android_resources("new_splash_resources") {
   custom_package = "org.chromium.webapk.shell_apk.h2o"
@@ -84,7 +84,7 @@ template("webapk_tmpl") {
     }
     output = _manifest_output
 
-    extra_variables = [ "shell_apk_version=$template_shell_apk_version" ]
+    extra_variables = [ "shell_apk_version=$current_shell_apk_version" ]
   }
 
   android_resources(_resources_target_name) {

chrome/android/webapk/shell_apk/current_version/OWNERS

+per-file current_version.gni=*

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

+# Copyright 2018 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.
+
+# HOW IS THE VERSION USED?
+# |current_shell_apk_version| is compiled into the WebAPK. Chromium
+# compares the version number with a number compiled into Chromium to determine
+# whether the WebAPK should be updated.
+#
+# WHEN SHOULD THE VERSION BE INCREMENTED?
+# Must be incremented whenever code which is compiled into
+# //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 = 58

chrome/android/webapk/shell_apk/request_update_for_version.gni

+# Copyright 2018 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.
+
+# The ShellAPK version expected by Chrome. Chrome will try to update the WebAPK
+# if the WebAPK's ShellAPK version is less than
+# |request_update_for_shell_apk_version|. The version should be incremented
+# after a new ShellAPK has been uploaded to the WebAPK Minting Server.
+request_update_for_shell_apk_version = 28

chrome/android/webapk/shell_apk/shell_apk_version.gni

-# Copyright 2016 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.
-
-# Must be incremented whenever code in //chrome/android/webapk/shell_apk
-# (including AndroidManifest.xml) is updated. This version should be incremented
-# prior to uploading a new ShellAPK to the WebAPK Minting Server.
-# Does not affect Chrome.apk
-template_shell_apk_version = 57
-
-# The ShellAPK version expected by Chrome. Chrome will try to update the WebAPK
-# if the WebAPK's ShellAPK version is less than |expected_shell_apk_version|.
-# The version should be incremented after a new ShellAPK has been uploaded to
-# the WebAPK Minting Server.
-expected_shell_apk_version = 28