Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)

Review-Url: https://chromiumcodereview.appspot.com/2430773002
Cr-Commit-Position: refs/heads/master@{#426868}

chrome/android/BUILD.gn

@@ -308,7 +308,9 @@ junit_binary("chrome_junit_tests") {
     ":chrome_java_resources",
     "//base:base_java",
     "//base:base_java_test_support",
+    "//chrome/android/webapk/libs/client:client_java",
     "//chrome/android/webapk/libs/common:common_java",
+    "//chrome/android/webapk/test:junit_test_support",
     "//components/bookmarks/common/android:bookmarks_java",
     "//components/invalidation/impl:java",
     "//components/signin/core/browser/android:java",

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

@@ -56,9 +56,10 @@ public class WebApkActivity extends WebappActivity {
                     @Override
                     public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
                         updateStorage(storage);
-                        // Initialize the update related timestamps to the registration time.
+                        // Initialize the time of the last is-update-needed check with the
+                        // registration time. This prevents checking for updates on the first run.
                         storage.updateTimeOfLastCheckForUpdatedWebManifest();
-                        storage.updateTimeOfLastWebApkUpdateRequestCompletion();
+
                         // The downloading of the splash screen image happens before a WebAPK's
                         // package name is available. If we want to use the image in the first
                         // launch, we need to cache the image, register the WebAPK and store the

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,

chrome/android/java_sources.gni

@@ -1471,6 +1471,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java",
   "junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java",
   "junit/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtilsTest.java",
+  "junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java",
 ]
 
 sync_shell_test_java_sources = [

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

@@ -4,9 +4,6 @@
 
 package org.chromium.chrome.browser.webapps;
 
-import android.app.Application;
-import android.content.pm.ApplicationInfo;
-import android.content.pm.PackageInfo;
 import android.os.Bundle;
 
 import org.junit.Assert;
@@ -14,10 +11,7 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.RuntimeEnvironment;
-import org.robolectric.Shadows;
 import org.robolectric.annotation.Config;
-import org.robolectric.res.builder.RobolectricPackageManager;
-import org.robolectric.shadows.ShadowApplication;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.blink_public.platform.WebDisplayMode;
@@ -25,6 +19,7 @@ import org.chromium.content_public.common.ScreenOrientationValues;
 import org.chromium.testing.local.LocalRobolectricTestRunner;
 import org.chromium.webapk.lib.common.WebApkConstants;
 import org.chromium.webapk.lib.common.WebApkMetaDataKeys;
+import org.chromium.webapk.test.WebApkTestHelper;
 
 /**
  * Tests WebApkMetaDataUtils.
@@ -33,8 +28,6 @@ import org.chromium.webapk.lib.common.WebApkMetaDataKeys;
 @Config(manifest = Config.NONE)
 public class WebApkMetaDataUtilsTest {
 
-    private static final String PACKAGE_NAME = "package_name";
-
     // Android Manifest meta data for {@link PACKAGE_NAME}.
     private static final String START_URL = "https://www.google.com/scope/a_is_for_apple";
     private static final String SCOPE = "https://www.google.com/scope";
@@ -45,16 +38,9 @@ public class WebApkMetaDataUtilsTest {
     private static final String THEME_COLOR = "1L";
     private static final String BACKGROUND_COLOR = "2L";
 
-    private ShadowApplication mShadowApplication;
-    private RobolectricPackageManager mPackageManager;
-
     @Before
     public void setUp() {
-        Application application = RuntimeEnvironment.application;
-        ContextUtils.initApplicationContextForTests(application);
-        mShadowApplication = Shadows.shadowOf(application);
-        mShadowApplication.setPackageName(PACKAGE_NAME);
-        mPackageManager = (RobolectricPackageManager) application.getPackageManager();
+        ContextUtils.initApplicationContextForTests(RuntimeEnvironment.application);
     }
 
     @Test
@@ -67,13 +53,13 @@ public class WebApkMetaDataUtilsTest {
         bundle.putString(WebApkMetaDataKeys.ORIENTATION, ORIENTATION);
         bundle.putString(WebApkMetaDataKeys.THEME_COLOR, THEME_COLOR);
         bundle.putString(WebApkMetaDataKeys.BACKGROUND_COLOR, BACKGROUND_COLOR);
-        PackageInfo packageInfo = newPackageInfo(PACKAGE_NAME, bundle);
-        mPackageManager.addPackage(packageInfo);
+        WebApkTestHelper.registerWebApkWithMetaData(bundle);
 
-        WebappInfo webappInfo =
-                WebApkMetaDataUtils.extractWebappInfoFromWebApk(PACKAGE_NAME, START_URL, 0);
+        WebappInfo webappInfo = WebApkMetaDataUtils.extractWebappInfoFromWebApk(
+                WebApkTestHelper.WEBAPK_PACKAGE_NAME, START_URL, 0);
 
-        Assert.assertEquals(WebApkConstants.WEBAPK_ID_PREFIX + PACKAGE_NAME, webappInfo.id());
+        Assert.assertEquals(WebApkConstants.WEBAPK_ID_PREFIX + WebApkTestHelper.WEBAPK_PACKAGE_NAME,
+                webappInfo.id());
         Assert.assertEquals(SCOPE, webappInfo.scopeUri().toString());
         Assert.assertEquals(NAME, webappInfo.name());
         Assert.assertEquals(SHORT_NAME, webappInfo.shortName());
@@ -83,7 +69,7 @@ public class WebApkMetaDataUtilsTest {
         Assert.assertEquals(1L, webappInfo.themeColor());
         Assert.assertTrue(webappInfo.hasValidBackgroundColor());
         Assert.assertEquals(2L, webappInfo.backgroundColor());
-        Assert.assertEquals(PACKAGE_NAME, webappInfo.webApkPackageName());
+        Assert.assertEquals(WebApkTestHelper.WEBAPK_PACKAGE_NAME, webappInfo.webApkPackageName());
     }
 
     /**
@@ -98,20 +84,10 @@ public class WebApkMetaDataUtilsTest {
 
         Bundle bundle = new Bundle();
         bundle.putString(WebApkMetaDataKeys.START_URL, START_URL);
-        PackageInfo packageInfo = newPackageInfo(PACKAGE_NAME, bundle);
-        mPackageManager.addPackage(packageInfo);
+        WebApkTestHelper.registerWebApkWithMetaData(bundle);
 
-        WebappInfo webappInfo =
-                WebApkMetaDataUtils.extractWebappInfoFromWebApk(PACKAGE_NAME, passedInStartUrl, 0);
+        WebappInfo webappInfo = WebApkMetaDataUtils.extractWebappInfoFromWebApk(
+                WebApkTestHelper.WEBAPK_PACKAGE_NAME, passedInStartUrl, 0);
         Assert.assertEquals(passedInStartUrl, webappInfo.uri().toString());
     }
-
-    private static PackageInfo newPackageInfo(String packageName, Bundle metaData) {
-        ApplicationInfo applicationInfo = new ApplicationInfo();
-        applicationInfo.metaData = metaData;
-        PackageInfo packageInfo = new PackageInfo();
-        packageInfo.packageName = packageName;
-        packageInfo.applicationInfo = applicationInfo;
-        return packageInfo;
-    }
 }

chrome/android/webapk/shell_apk/BUILD.gn

@@ -144,5 +144,6 @@ junit_binary("webapk_shell_apk_junit_tests") {
   deps = [
     ":webapk_java",
     "//chrome/android/webapk/libs/common:common_java",
+    "//chrome/android/webapk/test:junit_test_support",
   ]
 }

chrome/android/webapk/shell_apk/junit/DEPS

 include_rules = [
+  "+chrome/android/webapk/test",
   "+testing",
 ]

chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/MainActivityTest.java

@@ -6,14 +6,9 @@ package org.chromium.webapk.shell_apk;
 
 import android.content.Intent;
 import android.content.pm.ActivityInfo;
-import android.content.pm.ApplicationInfo;
-import android.content.pm.PackageInfo;
 import android.content.pm.ResolveInfo;
 import android.os.Bundle;
 
-import org.chromium.testing.local.LocalRobolectricTestRunner;
-import org.chromium.webapk.lib.common.WebApkMetaDataKeys;
-
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
@@ -25,6 +20,10 @@ import org.robolectric.annotation.Config;
 import org.robolectric.res.builder.RobolectricPackageManager;
 import org.robolectric.shadows.ShadowApplication;
 
+import org.chromium.testing.local.LocalRobolectricTestRunner;
+import org.chromium.webapk.lib.common.WebApkMetaDataKeys;
+import org.chromium.webapk.test.WebApkTestHelper;
+
 /**
  * Tests MainActivity.
  */
@@ -32,7 +31,6 @@ import org.robolectric.shadows.ShadowApplication;
 @Config(manifest = Config.NONE)
 public class MainActivityTest {
 
-    private static final String PACKAGE_NAME = "package_name";
     private static final String HOST_BROWSER_PACKAGE_NAME = "truly.random";
 
     private ShadowApplication mShadowApplication;
@@ -41,7 +39,7 @@ public class MainActivityTest {
     @Before
     public void setUp() {
         mShadowApplication = Shadows.shadowOf(RuntimeEnvironment.application);
-        mShadowApplication.setPackageName(PACKAGE_NAME);
+        mShadowApplication.setPackageName(WebApkTestHelper.WEBAPK_PACKAGE_NAME);
         mPackageManager =
                 (RobolectricPackageManager) RuntimeEnvironment.application.getPackageManager();
     }
@@ -59,7 +57,7 @@ public class MainActivityTest {
         Bundle metaData = new Bundle();
         metaData.putString(WebApkMetaDataKeys.RUNTIME_HOST, HOST_BROWSER_PACKAGE_NAME);
         metaData.putString(WebApkMetaDataKeys.START_URL, "http://random.org");
-        mPackageManager.addPackage(newPackageInfo(PACKAGE_NAME, metaData));
+        WebApkTestHelper.registerWebApkWithMetaData(metaData);
 
         // Make intents to Google Play not throw ActivityNotFoundException.
         mPackageManager.addResolveInfoForIntent(
@@ -86,7 +84,7 @@ public class MainActivityTest {
         Bundle metaData = new Bundle();
         metaData.putString(WebApkMetaDataKeys.RUNTIME_HOST, HOST_BROWSER_PACKAGE_NAME);
         metaData.putString(WebApkMetaDataKeys.START_URL, "http://random.org");
-        mPackageManager.addPackage(newPackageInfo(PACKAGE_NAME, metaData));
+        WebApkTestHelper.registerWebApkWithMetaData(metaData);
 
         Robolectric.buildActivity(MainActivity.class).create();
 
@@ -94,15 +92,6 @@ public class MainActivityTest {
         Assert.assertNull(startActivityIntent);
     }
 
-    private static PackageInfo newPackageInfo(String packageName, Bundle metaData) {
-        ApplicationInfo applicationInfo = new ApplicationInfo();
-        applicationInfo.metaData = metaData;
-        PackageInfo packageInfo = new PackageInfo();
-        packageInfo.packageName = packageName;
-        packageInfo.applicationInfo = applicationInfo;
-        return packageInfo;
-    }
-
     private static ResolveInfo newResolveInfo(String packageName) {
         ActivityInfo activityInfo = new ActivityInfo();
         activityInfo.packageName = packageName;

chrome/android/webapk/test/BUILD.gn

+# 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.
+
+import("//build/config/android/rules.gni")
+
+java_library("junit_test_support") {
+  testonly = true
+  java_files = [ "src/org/chromium/webapk/test/WebApkTestHelper.java" ]
+  deps = [
+    "//third_party/robolectric:robolectric_all_java",
+  ]
+}

chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java

+// 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.
+
+package org.chromium.webapk.test;
+
+import android.content.pm.ApplicationInfo;
+import android.content.pm.PackageInfo;
+import android.os.Bundle;
+
+import org.robolectric.RuntimeEnvironment;
+import org.robolectric.res.builder.RobolectricPackageManager;
+
+/**
+ * Helper class for WebAPK JUnit tests.
+ */
+public class WebApkTestHelper {
+    /**
+     * Package name of the WebAPK registered by {@link #registerWebApkWithMetaData}.
+     */
+    public static String WEBAPK_PACKAGE_NAME = "org.chromium.webapk.test_package";
+
+    /**
+     * Registers WebAPK.
+     * @param metaData Bundle with meta data from WebAPK's Android Manifest.
+     */
+    public static void registerWebApkWithMetaData(Bundle metaData) {
+        RobolectricPackageManager packageManager =
+                (RobolectricPackageManager) RuntimeEnvironment.application.getPackageManager();
+        packageManager.addPackage(newPackageInfo(WEBAPK_PACKAGE_NAME, metaData));
+    }
+
+    private static PackageInfo newPackageInfo(String packageName, Bundle metaData) {
+        ApplicationInfo applicationInfo = new ApplicationInfo();
+        applicationInfo.metaData = metaData;
+        PackageInfo packageInfo = new PackageInfo();
+        packageInfo.packageName = packageName;
+        packageInfo.applicationInfo = applicationInfo;
+        return packageInfo;
+    }
+}