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

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 7aae095dfc8374a4d09b92c8f8b407e2e20a14ee..54384571e31f6690cf7ad49799419c77663d9db9 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
@@ -7,7 +7,6 @@ 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;
@@ -15,9 +14,7 @@ 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;
 
@@ -37,13 +34,17 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
      */
     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.
@@ -59,29 +60,21 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
      * @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
@@ -89,16 +82,40 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
         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();
-        }
-        mUpgradeDetector = null;
+    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();
+        }
+
         if (!needsUpgrade) {
             if (!mPreviousUpdateSucceeded) {
                 recordUpdateInWebappDataStorage(mId, true);
@@ -109,32 +126,54 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
         // 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;
     }
 
@@ -144,17 +183,18 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
     }
 
     /**
-     * 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;
     }
 
     /**
@@ -171,21 +211,28 @@ public class WebApkUpdateManager implements ManifestUpgradeDetector.Callback {
     }
 
     /**
+     * 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();