Implement server-suggested update check backoff

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 3c4bd649acc5159c4475ebd082fb6d99abd44b54..1d718ef7033e6bd91ba7db11121d8ddc49f35a9b 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
@@ -46,6 +46,12 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
      */
     private static final int MAX_UPDATE_ATTEMPTS = 3;
 
+    /**
+    * crbug.com/680128. Number of milliseconds between checks of updates for a WebAPK that is

[dominickn, 2017/02/14 04:13:33]

Nit: misaligned * at the start of the comment

Nit: I would put the crbug link after the comment explaining what it is.

Nit: move this below UPDATE_INTERVAL

[Xi Han, 2017/02/14 22:25:46]

Done.

+     * expected to check updates less frequently.
+     */
+    public static final long INFREQUENT_UPDATE_INTERVAL = TimeUnit.DAYS.toMillis(30L);
+
     /** Data extracted from the WebAPK's launch intent and from the WebAPK's Android Manifest. */
     private WebApkInfo mInfo;
 
@@ -158,14 +164,14 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
 
         if (!needsUpgrade) {
             if (!mPreviousUpdateSucceeded) {
-                recordUpdate(storage, true);
+                recordUpdate(storage, true, null);
             }
             return;
         }
 
         // Set WebAPK update as having failed in case that Chrome is killed prior to
         // {@link onBuiltWebApk} being called.
-        recordUpdate(storage, false);
+        recordUpdate(storage, false, null);
 
         if (fetchedInfo != null) {
             scheduleUpdate(fetchedInfo, bestIconUrl, false /* isManifestStale */);
@@ -318,8 +324,11 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
         if (isShellApkVersionOutOfDate(info)) return true;
 
         long now = currentTimeMillis();
+        boolean isInfrequentUpdates = storage.getInfrequentUpdates();

[dominickn, 2017/02/14 04:13:34]

I feel like this computation should be pushed into the WebappDataStorage. That
way, the update manager can just do:

return storage.shouldUpdate();

This would mean moving all of the intervals into WebappDataStorage as constants.
WDYT?

[Xi Han, 2017/02/14 22:25:46]

Personally I would prefer to leave all these logic in WebApkUpdateManager, and
use WebAppDataStorage as a place to cache values. Especially all existing tests
are written for WebApkUpdateManager, we have to move tests to test these logic.

[dominickn, 2017/02/15 05:58:32]

I'm not sure why the tests would need to be rewritten? The only thing to change
is that you'd have WebappDataStorage.UPDATE_INTERVAL instead of
WebApkManager.UPDATE_INTERVAL, RETRY_UPDATE_DURATION, etc.

In fact, the mock clock used in tests is from WebappDataStorage, which further
suggests that all the time based calculations should be in there.

+        long checkUpdatesInterval =
+                isInfrequentUpdates ? INFREQUENT_UPDATE_INTERVAL : FULL_CHECK_UPDATE_INTERVAL;
         long sinceLastCheckDurationMs = now - storage.getLastCheckForWebManifestUpdateTime();
-        if (sinceLastCheckDurationMs >= FULL_CHECK_UPDATE_INTERVAL) return true;
+        if (sinceLastCheckDurationMs >= checkUpdatesInterval) return true;
 
         long sinceLastUpdateRequestDurationMs =
                 now - storage.getLastWebApkUpdateRequestCompletionTime();
@@ -331,11 +340,15 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
      * Updates {@link WebappDataStorage} with the time of the latest WebAPK update and whether the
      * WebAPK update succeeded.
      */
-    private static void recordUpdate(WebappDataStorage storage, boolean success) {
+    private static void recordUpdate(
+            WebappDataStorage storage, boolean success, Boolean isInfrequentUpdates) {

[dominickn, 2017/02/14 04:13:33]

Why does this need to be a Boolean object that's nullable as opposed to a
boolean value? Don't we assume a regular update schedule unless explicitly
instructed otherwise?

[Xi Han, 2017/02/14 22:25:46]

Hmmm, I guess a boolean works too. Updated.

         // Update the request time and result together. It prevents getting a correct request time
         // but a result from the previous request.
         storage.updateTimeOfLastWebApkUpdateRequestCompletion();
         storage.updateDidLastWebApkUpdateRequestSucceed(success);
+        if (isInfrequentUpdates != null) {
+            storage.updateInfrequentUpdates(isInfrequentUpdates);

[dominickn, 2017/02/14 04:13:33]

storage.shouldRelaxUpdates(shouldRelaxUpdates). This would work better with the
WebappDataStorage just having a shouldUpdate() method so all of the update
interval calculations happen inside it.

[Xi Han, 2017/02/14 22:25:46]

I would prefer to keep the logic in here.

[dominickn, 2017/02/15 05:58:32]

I think it makes much more sense for this logic to be in WebappDataStorage. For
instance, whether or not a webapp was launched recently has its computation done
in there.

+        }
     }
 
     /**
@@ -392,9 +405,9 @@ public class WebApkUpdateManager implements WebApkUpdateDataFetcher.Observer {
      * fails.
      */
     @CalledByNative
-    private static void onBuiltWebApk(String id, boolean success) {
+    private static void onBuiltWebApk(String id, boolean success, boolean isInfrequentUpdates) {

[pkotwicz, 2017/02/14 00:15:12]

Nit: isInfrequentUpdates -> lessUpdates

[Xi Han, 2017/02/14 22:25:46]

Done.

         WebappDataStorage storage = WebappRegistry.getInstance().getWebappDataStorage(id);
-        recordUpdate(storage, success);
+        recordUpdate(storage, success, isInfrequentUpdates);
     }
 
     private static native void nativeUpdateAsync(String id, String startUrl, String scope,