Update WebAPKs even if the WebAPK start URL has no Web Manifest part 1/3

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.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.chrome.browser.webapps;
 
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager;
 import android.graphics.Bitmap;
-import android.os.Bundle;
 
 import org.chromium.base.CommandLine;
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.tab.Tab;
-import org.chromium.chrome.browser.util.IntentUtils;
 import org.chromium.webapk.lib.client.WebApkVersion;
-import org.chromium.webapk.lib.common.WebApkMetaDataKeys;
 
 import java.util.concurrent.TimeUnit;
 
 /**
  * WebApkUpdateManager manages when to check for updates to the WebAPK's Web Manifest, and sends
  * an update request to the WebAPK Server when an update is needed.
  */
 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. */
     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.
      */
     public static final long RETRY_UPDATE_DURATION = TimeUnit.HOURS.toMillis(12L);
 
-    /**
-     * Id of WebAPK data in WebappDataStorage
-     */
+    /** Id of WebAPK data in WebappDataStorage */
     private String mId;
 
-    /** Android version code of WebAPK. */
-    private int mVersionCode;
+    /** WebAPK package name. */
+    private String mWebApkPackageName;
+
+    /** Meta data from the WebAPK's Android Manifest */
+    private WebApkMetaData mMetaData;
+
+    /** WebAPK's icon. */
+    private Bitmap mIcon;
 
     /**
      * Whether the previous WebAPK update succeeded. True if there has not been any update attempts.
      */
     private boolean mPreviousUpdateSucceeded;
 
     private ManifestUpgradeDetector mUpgradeDetector;
 
     /**
      * Checks whether the WebAPK's Web Manifest has changed. Requests an updated WebAPK if the
      * Web Manifest has changed. Skips the check if the check was done recently.
      * @param tab  The tab of the WebAPK.
      * @param info The WebappInfo of the WebAPK.
      */
     public void updateIfNeeded(Tab tab, WebappInfo info) {
-        PackageInfo packageInfo = readPackageInfoFromAndroidManifest(info.webApkPackageName());
-        if (packageInfo == null) {
+        mMetaData = WebApkMetaDataUtils.extractMetaDataFromWebApk(info.webApkPackageName());
+        if (mMetaData == null) {

[dominickn, 2016/10/24 11:34:59]

Nit: inline the return.

             return;
         }
 
+        mWebApkPackageName = info.webApkPackageName();
         mId = info.id();
-        mVersionCode = packageInfo.versionCode;
-        final Bundle metadata = packageInfo.applicationInfo.metaData;
-        int shellApkVersion =
-                IntentUtils.safeGetInt(metadata, WebApkMetaDataKeys.SHELL_APK_VERSION, 0);
+        mIcon = info.icon();
 
         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)
+        if (!shouldCheckIfWebManifestUpdated(storage, mMetaData, mPreviousUpdateSucceeded)) return;
 
-        if (!shouldCheckIfWebManifestUpdated(storage, shellApkVersion, mPreviousUpdateSucceeded)) {
-            return;
-        }
-
-        mUpgradeDetector = buildManifestUpgradeDetector(tab, info, metadata);
+        mUpgradeDetector = buildManifestUpgradeDetector(tab, mMetaData);
         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();
     }
 
+    public void destroy() {

[dominickn, 2016/10/24 11:34:59]

Can you just override this method in tests, and call destroy() everywhere rather
than having the separate destroyUpgradeDetector() method? Just looking at it,
destroy() is only called in WebApkActivity, and that shouldn't happen in
WebApkUpdateManagerTest so it would be safe just to merge it with
destroyUpgradeDetector(). Let me know if I'm being blind though.

[pkotwicz, 2016/10/24 16:03:43]

I made destroyUpgradeDetector() its own function to make the code more
intuitive.

When destroyUpgradeDetector() is called from
onFinishedFetchingWebManifestForInitialUrl() the entire class is not being
destroyed. updateAsync() and onBuiltWebApk() can be called after
destroyUpgradeDetector() is invoked.

Although destroy() and destroyUpgradeDetector() do the same thing, their
semantic meaning is different:
destroy() destroys the entire class
destroyUpgradeDetector() destroys {@link mUpgradeDetector}

[dominickn, 2016/10/25 07:39:16]

The semantic difference doesn't really make sense to me (WebApkActivity never
sets mUpdateManager to null, so it holds a reference to the WebApkUpdateManager
for the whole time, and destroy() is only called when the activity is stopping).
I don't really want to stop the CL on this though.

[pkotwicz, 2016/10/26 01:53:29]

I think that the semantic difference makes sense if you think of the user of
WebApkUpdateManager as a black box and treat WebApkUpdateManager#destroy() as a
public API

+        destroyUpgradeDetector();
+    }
+
     @Override
-    public void onUpgradeNeededCheckFinished(boolean needsUpgrade,
-            ManifestUpgradeDetector.FetchedManifestData data) {
-        if (mUpgradeDetector != null) {
-            mUpgradeDetector.destroy();
+    public void onFinishedFetchingWebManifestForInitialUrl(
+            boolean needsUpgrade, ManifestUpgradeDetector.FetchedManifestData data) {
+        onGotManifestData(needsUpgrade, data);
+    }
+
+    @Override
+    public void onGotManifestData(
+            boolean needsUpgrade, ManifestUpgradeDetector.FetchedManifestData data) {
+        boolean gotManifest = (data != null);
+        needsUpgrade |= isShellApkVersionOutOfDate(mMetaData);
+        Log.v(TAG, "Got Manifest: " + gotManifest);
+        Log.v(TAG, "WebAPK upgrade needed: " + needsUpgrade);
+
+        // If the Web Manifest was not found and an upgrade is requested, stop fetching Web
+        // Manifests as the user navigates to avoid sending multiple WebAPK update requests. In
+        // particular:
+        // - A WebAPK update request on the initial load because the Shell APK version is out of
+        //   date.
+        // - A second WebAPK update request once the user navigates to a page which points to the
+        //   correct Web Manifest URL because the Web Manifest has been updated by the Web
+        //   developer.
+        //
+        // If the Web Manifest was not found and an upgrade is not requested, keep on fetching
+        // Web Manifests as the user navigates. For instance, the WebAPK's start_url might not
+        // point to a Web Manifest because start_url redirects to the WebAPK's main page.
+        if (gotManifest || needsUpgrade) {
+            destroyUpgradeDetector();
         }
-        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);
+
+        if (data != null) {
+            updateAsync(data.startUrl, data.scopeUrl, data.name, data.shortName, data.iconUrl,
+                    data.iconMurmur2Hash, data.icon, data.displayMode, data.orientation,
+                    data.themeColor, data.backgroundColor);
+            return;
+        }
+
+        updateAsyncUsingAndroidManifestMetaData();
     }
 
     /**
      * 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);
+            Tab tab, WebApkMetaData metaData) {
+        return new ManifestUpgradeDetector(tab, metaData, this);
+    }
+
+    /**
+     * Sends a request to WebAPK Server to update WebAPK using the meta data from the WebAPK's
+     * Android Manifest.
+     */
+    private void updateAsyncUsingAndroidManifestMetaData() {
+        updateAsync(mMetaData.startUrl, mMetaData.scope, mMetaData.name, mMetaData.shortName,
+                mMetaData.iconUrl, mMetaData.iconMurmur2Hash, mIcon, mMetaData.displayMode,
+                mMetaData.orientation, mMetaData.themeColor, mMetaData.backgroundColor);
     }
 
     /**
      * Sends request to WebAPK Server to update WebAPK.
      */
-    public void updateAsync(ManifestUpgradeDetector.FetchedManifestData data) {
-        String packageName = mUpgradeDetector.getWebApkPackageName();
-        nativeUpdateAsync(mId, data.startUrl, data.scopeUrl, data.name, data.shortName,
-                data.iconUrl, data.iconMurmur2Hash, data.icon, data.displayMode, data.orientation,
-                data.themeColor, data.backgroundColor, mUpgradeDetector.getManifestUrl(),
-                packageName, mVersionCode);
+    protected void updateAsync(String startUrl, String scopeUrl, String name, String shortName,
+            String iconUrl, String iconMurmur2Hash, Bitmap icon, int displayMode, int orientation,
+            long themeColor, long backgroundColor) {
+        int versionCode = readVersionCodeFromAndroidManifest(mWebApkPackageName);
+        nativeUpdateAsync(mId, startUrl, scopeUrl, name, shortName, iconUrl, iconMurmur2Hash, icon,
+                displayMode, orientation, themeColor, backgroundColor, mMetaData.manifestUrl,
+                mWebApkPackageName, versionCode);
     }
 
-    public void destroy() {
-        if (mUpgradeDetector != null) {
-            mUpgradeDetector.destroy();
-        }
+    /**
+     * Destroys {@link mUpgradeDetector}. In a separate function for the sake of tests.
+     */
+    protected void destroyUpgradeDetector() {
+        if (mUpgradeDetector == null) return;
+
+        mUpgradeDetector.destroy();
         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.
+     * Reads the WebAPK's version code. Returns 0 on failure.
      */
-    private PackageInfo readPackageInfoFromAndroidManifest(String webApkPackage) {
+    private int readVersionCodeFromAndroidManifest(String webApkPackage) {
         try {
             PackageManager packageManager =
                     ContextUtils.getApplicationContext().getPackageManager();
-            return packageManager.getPackageInfo(webApkPackage, PackageManager.GET_META_DATA);
+            PackageInfo packageInfo = packageManager.getPackageInfo(webApkPackage, 0);
+            return packageInfo.versionCode;
         } catch (PackageManager.NameNotFoundException e) {
             e.printStackTrace();
         }
-        return null;
+        return 0;
     }
 
     /**
      * 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();
     }
 
     /**
+     * Whether there is a new version of the //chrome/android/webapk/shell_apk code.
+     */
+    private static boolean isShellApkVersionOutOfDate(WebApkMetaData metaData) {
+        return metaData.shellApkVersion < WebApkVersion.CURRENT_SHELL_APK_VERSION;
+    }
+
+    /**
      * 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 metaData Meta data from WebAPK's Android Manifest.
      * @param previousUpdateSucceeded Whether the previous update attempt succeeded.
      * True if there has not been any update attempts.
      */
     private boolean shouldCheckIfWebManifestUpdated(
-            WebappDataStorage storage, int shellApkVersion, boolean previousUpdateSucceeded) {
+            WebappDataStorage storage, WebApkMetaData metaData, boolean previousUpdateSucceeded) {
         if (CommandLine.getInstance().hasSwitch(
                     ChromeSwitches.CHECK_FOR_WEB_MANIFEST_UPDATE_ON_STARTUP)) {
             return true;
         }
 
-        if (shellApkVersion < WebApkVersion.CURRENT_SHELL_APK_VERSION) return true;
+        if (isShellApkVersionOutOfDate(metaData)) return true;
 
         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;
     }
@@ skipping 17 matching lines @@
     @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,
             String manifestUrl, String webApkPackage, int webApkVersion);
 }