Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

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

Index: chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
index 6f15044d71949d9599f4704a4c647fbe2793a83d..7aae095dfc8374a4d09b92c8f8b407e2e20a14ee 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
@@ -29,30 +29,26 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
     private static final String TAG = "WebApkUpdateManager";
 
     /** Number of milliseconds between checks for whether the WebAPK's Web Manifest has changed. */
-    private static final long FULL_CHECK_UPDATE_INTERVAL = TimeUnit.DAYS.toMillis(3L);
+    public static final long FULL_CHECK_UPDATE_INTERVAL = TimeUnit.DAYS.toMillis(3L);
 
     /**
      * Number of milliseconds to wait before re-requesting an updated WebAPK from the WebAPK
      * server if the previous update attempt failed.
      */
-    private static final long RETRY_UPDATE_DURATION = TimeUnit.HOURS.toMillis(12L);
+    public static final long RETRY_UPDATE_DURATION = TimeUnit.HOURS.toMillis(12L);
 
     /**
      * Id of WebAPK data in WebappDataStorage
      */
     private String mId;
 
-    /** Version number of //chrome/android/webapk/shell_apk code. */
-    private int mShellApkVersion;
-
     /** Android version code of WebAPK. */
     private int mVersionCode;
 
     /**
-     * Whether a request to upgrade the WebAPK should be sent regardless of whether the Web Manifest
-     * has changed.
+     * Whether the previous WebAPK update succeeded. True if there has not been any update attempts.
      */
-    private boolean mForceUpgrade;
+    private boolean mPreviousUpdateSucceeded;
 
     private ManifestUpgradeDetector mUpgradeDetector;
 
@@ -71,41 +67,57 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
         mId = info.id();
         mVersionCode = packageInfo.versionCode;
         final Bundle metadata = packageInfo.applicationInfo.metaData;
-        mShellApkVersion =
+        int shellApkVersion =
                 IntentUtils.safeGetInt(metadata, WebApkMetaDataKeys.SHELL_APK_VERSION, 0);
 
-        mUpgradeDetector = new ManifestUpgradeDetector(tab, info, metadata, this);
-
-        WebappDataStorage storage = WebappRegistry.getInstance().getWebappDataStorage(info.id());
-        if (forceUpgrade(storage)) mForceUpgrade = true;
+        WebappDataStorage storage = WebappRegistry.getInstance().getWebappDataStorage(mId);
+        mPreviousUpdateSucceeded = didPreviousUpdateSucceed(storage);
 
         // TODO(pkotwicz|hanxi): Request upgrade if ShellAPK version changes if
         // ManifestUpgradeDetector cannot fetch the Web Manifest. For instance, the web developer
         // may have removed the Web Manifest from their site. (http://crbug.com/639536)
 
-        long sinceLastCheckDuration = System.currentTimeMillis()
-                - storage.getLastCheckForWebManifestUpdateTime();
-        if (sinceLastCheckDuration > FULL_CHECK_UPDATE_INTERVAL || mForceUpgrade) {
-            if (mUpgradeDetector.start()) {
-                // crbug.com/636525. The timestamp of the last manifest update check should be
-                // updated after the detector finds the manifest, not when the detector is started.
-                storage.updateTimeOfLastCheckForUpdatedWebManifest();
-            }
+        if (!shouldCheckIfWebManifestUpdated(storage, shellApkVersion, mPreviousUpdateSucceeded)) {
+            return;
         }
+
+        mUpgradeDetector = buildManifestUpgradeDetector(tab, info, metadata);
+        if (!mUpgradeDetector.start()) return;
+
+        // crbug.com/636525. The timestamp of the last manifest update check should be updated after
+        // the detector finds the manifest, not when the detector is started.
+        storage.updateTimeOfLastCheckForUpdatedWebManifest();
     }
 
     @Override
     public void onUpgradeNeededCheckFinished(boolean needsUpgrade,
             ManifestUpgradeDetector.FetchedManifestData data) {
-        needsUpgrade = (needsUpgrade || mForceUpgrade);
-        Log.v(TAG, "WebAPK upgrade needed: " + needsUpgrade);
-        if (needsUpgrade) {
-            updateAsync(data);
-        }
         if (mUpgradeDetector != null) {
             mUpgradeDetector.destroy();
         }
         mUpgradeDetector = null;
+
+        Log.v(TAG, "WebAPK upgrade needed: " + needsUpgrade);
+
+        if (!needsUpgrade) {
+            if (!mPreviousUpdateSucceeded) {
+                recordUpdateInWebappDataStorage(mId, true);
+            }
+            return;
+        }
+
+        // Set WebAPK update as having failed in case that Chrome is killed prior to
+        // {@link onBuiltWebApk} being called.
+        recordUpdateInWebappDataStorage(mId, false);
+        updateAsync(data);
+    }
+
+    /**
+     * Builds {@link ManifestUpgradeDetector}. In a separate function for the sake of tests.
+     */
+    protected ManifestUpgradeDetector buildManifestUpgradeDetector(
+            Tab tab, WebappInfo info, Bundle metaData) {
+        return new ManifestUpgradeDetector(tab, info, metaData, this);
     }
 
     /**
@@ -126,6 +138,11 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
         mUpgradeDetector = null;
     }
 
+    /** Returns the current time. In a separate function for the sake of testing. */
+    protected long currentTimeMillis() {
+        return System.currentTimeMillis();
+    }
+
     /**
      * Reads the WebAPK's PackageInfo from the Android Manifest.
      */
@@ -141,31 +158,50 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
     }
 
     /**
-     * Returns whether an upgrade should be requested regardless of whether the Web Manifest has
-     * changed.
+     * Returns whether the previous WebAPK update attempt succeeded. Returns true if there has not
+     * been any update attempts.
+     */
+    private static boolean didPreviousUpdateSucceed(WebappDataStorage storage) {
+        long lastUpdateCompletionTime = storage.getLastWebApkUpdateRequestCompletionTime();
+        if (lastUpdateCompletionTime == WebappDataStorage.LAST_USED_INVALID
+                || lastUpdateCompletionTime == WebappDataStorage.LAST_USED_UNSET) {
+            return true;
+        }
+        return storage.getDidLastWebApkUpdateRequestSucceed();
+    }
+
+    /**
+     * Returns whether the Web Manifest should be refetched to check whether it has been updated.
+     * TODO: Make this method static once there is a static global clock class.
+     * @param storage WebappDataStorage with the WebAPK's cached data.
+     * @param shellApkVersion Version number of //chrome/android/webapk/shell_apk code.
+     * @param previousUpdateSucceeded Whether the previous update attempt succeeded.
+     * True if there has not been any update attempts.
      */
-    private boolean forceUpgrade(WebappDataStorage storage) {
+    private boolean shouldCheckIfWebManifestUpdated(
+            WebappDataStorage storage, int shellApkVersion, boolean previousUpdateSucceeded) {
         if (CommandLine.getInstance().hasSwitch(
                     ChromeSwitches.CHECK_FOR_WEB_MANIFEST_UPDATE_ON_STARTUP)) {
             return true;
         }
 
-        long sinceLastUpdateRequestDuration =
-                System.currentTimeMillis() - storage.getLastWebApkUpdateRequestCompletionTime();
-        if (sinceLastUpdateRequestDuration <= RETRY_UPDATE_DURATION) {
-            return false;
-        }
+        if (shellApkVersion < WebApkVersion.CURRENT_SHELL_APK_VERSION) return true;
 
-        return mShellApkVersion < WebApkVersion.CURRENT_SHELL_APK_VERSION
-                || !storage.getDidLastWebApkUpdateRequestSucceed();
+        long now = currentTimeMillis();
+        long sinceLastCheckDurationMs = now - storage.getLastCheckForWebManifestUpdateTime();
+        if (sinceLastCheckDurationMs >= FULL_CHECK_UPDATE_INTERVAL) return true;
+
+        long sinceLastUpdateRequestDurationMs =
+                now - storage.getLastWebApkUpdateRequestCompletionTime();
+        return sinceLastUpdateRequestDurationMs >= RETRY_UPDATE_DURATION
+                && !previousUpdateSucceeded;
     }
 
     /**
-     * Called after either a request to update the WebAPK has been sent or the update process
-     * fails.
+     * Updates {@link WebappDataStorage} with the time of the latest WebAPK update and whether the
+     * WebAPK update succeeded.
      */
-    @CalledByNative
-    private static void onBuiltWebApk(String id, final boolean success) {
+    private static void recordUpdateInWebappDataStorage(String id, boolean success) {
         WebappDataStorage storage = WebappRegistry.getInstance().getWebappDataStorage(id);
         // Update the request time and result together. It prevents getting a correct request time
         // but a result from the previous request.
@@ -173,6 +209,15 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
         storage.updateDidLastWebApkUpdateRequestSucceed(success);
     }
 
+    /**
+     * Called after either a request to update the WebAPK has been sent or the update process
+     * fails.
+     */
+    @CalledByNative
+    private static void onBuiltWebApk(String id, boolean success) {
+        recordUpdateInWebappDataStorage(id, success);
+    }
+
     private static native void nativeUpdateAsync(String id, String startUrl, String scope,
             String name, String shortName, String iconUrl, String iconMurmur2Hash, Bitmap icon,
             int displayMode, int orientation, long themeColor, long backgroundColor,