Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java b/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
new file mode 100644
index 0000000000000000000000000000000000000000..049d436c8c9eaa13b5aa9c826082a6ac0d167ada
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
@@ -0,0 +1,213 @@
+// 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.ApplicationInfo;
+import android.content.pm.PackageManager;
+import android.text.TextUtils;
+
+import org.chromium.base.CommandLine;
+import org.chromium.base.ContextUtils;
+import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.chrome.browser.tab.EmptyTabObserver;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.content_public.browser.WebContents;
+
+/**
+ * This class interacts with C++ to detect whether resources in web manifest of a WebAPK has been
+ * updated.
+ */
+public class ManifestUpgradeDetector extends EmptyTabObserver {
+    @VisibleForTesting
+    public static final String SWITCH_USE_START_URL_FOR_TESTING =
+            "switch-use-start-url-for-testing";

[pkotwicz, 2016/07/19 18:02:38]

Please add a comment for META_DATA_START_URL

[Xi Han, 2016/07/20 18:54:59]

Done.

+    private static final String META_DATA_START_URL = "startUrl";
+    /** Pointer to the native side ManifestUpgradeDetector. */
+    private long mNativePointer;
+
+    /** The tab that is being observed. */
+    private final Tab mTab;
+
+    private WebappInfo mWebappInfo;
+    private String mStartUrl;
+    private boolean mWasStarted;

[Yaron, 2016/07/20 02:27:58]

I don't think you need a separate variable for this - it's just a proxy for
mNativePointer != 0

[Xi Han, 2016/07/20 18:54:59]

It makes sense to me. Removed.

+
+    private static final String TAG = "cr_UpgradeDetector";
+
+    public ManifestUpgradeDetector(Tab tab, WebappInfo info) {
+        mTab = tab;
+        mWebappInfo = info;
+    }
+
+    @VisibleForTesting
+    public void setStartUrl(String startUrl) {
+        mStartUrl = startUrl;
+    }
+
+    /**
+     * Starts fetching the web manifest resources.
+     */
+    public void start() {
+        if (mWebappInfo.webManifestUri() == null
+                || TextUtils.isEmpty(mWebappInfo.webManifestUri().toString())) {

[pkotwicz, 2016/07/19 18:02:38]

How about:
|| mWebappInfo.webManifestUri().equals(Uri.EMPTY)) {

According the Uri#equals() my suggestion seems to do the right thing

[Xi Han, 2016/07/20 18:54:59]

Done.

+            return;
+        }
+
+        if (!mWasStarted) {

[pkotwicz, 2016/07/19 18:02:38]

Why not early return if {@link mWasStarted} is true?

[pkotwicz, 2016/07/20 21:47:52]

Bump

[Xi Han, 2016/07/22 17:13:11]

I have changed this to a check "if (mNativePointer == null)". This check is only
for if the native pointer is initialized, not if the detection pipeline is
starting.

[pkotwicz, 2016/07/22 20:13:05]

You should call Tab#addObserver() only if (mNativePointer == null)

(Technically your code is correct due to the implementation of
ObserverList#addObserver() but I think that it is clearer if you call
Tab#addObserver() only if (mNativePointer == null).

[Xi Han, 2016/07/22 21:59:55]

Done.

+            getMetaDataFromAndroidManifest();
+            mNativePointer = nativeInitialize(mTab.getWebContents(),
+                    mWebappInfo.scopeUri().toString(), mStartUrl,
+                    mWebappInfo.webManifestUri().toString());
+            mWasStarted = true;
+        }
+        mTab.addObserver(this);
+        nativeStart(mNativePointer);
+    }
+
+    private void getMetaDataFromAndroidManifest() {
+        if (CommandLine.getInstance().hasSwitch(SWITCH_USE_START_URL_FOR_TESTING)) {
+            // The start URL is set via {@link setStartUrl()} for testing.
+            return;
+        }
+        try {
+            ApplicationInfo appinfo =
+                    ContextUtils.getApplicationContext().getPackageManager().getApplicationInfo(
+                            mWebappInfo.webApkPackageName(), PackageManager.GET_META_DATA);
+            mStartUrl = appinfo.metaData.getString(META_DATA_START_URL);
+        } catch (PackageManager.NameNotFoundException e) {
+            return;

[pkotwicz, 2016/07/19 18:02:38]

Nit: The return is kind of useless 😀

[Xi Han, 2016/07/20 18:54:59]

Copy and paste, but forget to remove:(

+        }
+    }
+
+    /**
+     * Puts the object in a state where it is safe to be destroyed.
+     */
+    public void destroy() {
+        if (mWasStarted) {
+            nativeDestroy(mNativePointer);
+        }
+        mNativePointer = 0;
+    }
+
+    @Override
+    public void onWebContentsSwapped(Tab tab, boolean didStartLoad,
+            boolean didFinishLoad) {
+        updatePointers();
+    }
+
+    @Override
+    public void onContentChanged(Tab tab) {
+        updatePointers();
+    }
+
+    /**
+     * Updates which WebContents the native ManifestUpgradeDetector is monitoring.
+     */
+    private void updatePointers() {
+        nativeReplaceWebContents(mNativePointer, mTab.getWebContents());
+    }
+
+    /**
+     * Checks whether the new fetched resources match the ones of the current WebAppInfo.
+     * The changes of start Url, name, background color and so on will lead to request
+     * updating the installed WebAPK.
+     */
+    @CalledByNative
+    private void onDataAvailable(String startUrl, String scope, String name, String shortName,
+            int displayMode, int orientation, long themeColor, long backgroundColor) {
+        mTab.removeObserver(this);

[pkotwicz, 2016/07/19 18:02:38]

Should we call destroy() instead?
This would prevent memory leaks in the case of:
- ManifestUpgradeDetector#start() being called
- ManifestUpgradeDetector#onDataAvailable() callback is called by native
- ManifestUpgradeDetector#start() is called again

ManifestUpgradeDetector#destroy() can also call mTab#removeObserver()

[Xi Han, 2016/07/20 18:54:59]

Done.

+        // TODO(hanxi): crubug.com/627824. Validate whether the new WebappInfo is
+        // WebAPK-compatible.

[pkotwicz, 2016/07/19 18:02:38]

crubug.com -> crbug.com

[Xi Han, 2016/07/20 18:54:59]

Sorry for the typo.

+        final WebappInfo newInfo = WebappInfo.create(mWebappInfo.id(), startUrl,
+                scope, mWebappInfo.encodedIcon(), name, shortName, displayMode, orientation,
+                mWebappInfo.source(), themeColor, backgroundColor, false,
+                mWebappInfo.webApkPackageName(), mWebappInfo.webManifestUri().toString());
+        if (requireUpgrade(newInfo, startUrl)) {
+            upgrade();
+        }
+        updateWebappDataStorage(newInfo);

[Yaron, 2016/07/20 02:27:58]

i'm a little surprised that we do this unconditionally. isn't there an existing
mechanism to keep this up to date? I (perhaps) naively thought this component
should only look for upgrades to webapks

[Xi Han, 2016/07/20 18:54:59]

We also update the sharedPreference, since there are properties like orientation
etc. that don't require to re-mint but keep the updated values in
WebappDataStorage. Initially I checked every property to see if any of them
changed. Peter suggests to simplify the logic by always updating the
WebappDataStorage. 

+    }
+
+    /**
+     * Checks whether properties, like start URL, short name, background color, are changed. The
+     * change of any of them requires an upgrade of the WebAPK. {@link mWebappInfo} keeps the
+     * latest version of these properties, so no metadata query is needed.
+     */

[pkotwicz, 2016/07/19 18:02:38]

How about: "Checks whether an upgraded WebAPK needs to be downloaded from the
WebAPK server."

[Xi Han, 2016/07/20 18:54:59]

Done.

[pkotwicz, 2016/07/20 21:47:52]

Bump

[Xi Han, 2016/07/22 17:13:11]

? The description has been updated.

+    protected boolean requireUpgrade(WebappInfo newInfo, String startUrl) {
+        // Scope is used by WebAPK's intent filter. It determines which URLs are opened by
+        // the WebAPK instead of Chrome.
+        boolean scopeMatch = mWebappInfo.scopeUri().compareTo(newInfo.scopeUri()) == 0;

[pkotwicz, 2016/07/19 18:02:38]

Can you use Uri#equals() instead?

[Xi Han, 2016/07/20 18:54:59]

Done.

+        if (!scopeMatch) {
+            // Sometimes the scope doesn't match due to a missing "/" at the end of the scope URL.
+            // Print log to find such cases.
+            Log.d(TAG, "Needs to request update since the scope from WebappInfo (%s) doesn't match"
+                    + "the one fetched from Web Manifest(%s).", mWebappInfo.scopeUri().toString(),
+                    newInfo.scopeUri().toString());
+            return true;
+        }
+
+        // Start URL is the initial URL when the WebAPK is launched. A change in the
+        // start URL requires the WebAPK to be re-installed because the initial URL starts
+        // loading prior to the WebAPK's WebappDataStorage being fetched.
+        if (!mStartUrl.equals(startUrl)) {
+            return true;
+        }
+
+        // Short name is used as the WebAPK's name in the app list.
+        if (!mWebappInfo.shortName().equals(newInfo.shortName())) {
+            return true;
+        }
+
+        // Background color is the splash screen's background color. A change in the
+        // background color requires the WebAPK to be re-installed because a
+        // background-color-only splash screen is displayed prior to the WebAPK's
+        // WebappDataStorage being fetched. There can be a several second delay between the
+        // background-color-only splash screen being displayed and the WebAPK's
+        // WebappDataStorage being fetched.
+        if (mWebappInfo.backgroundColor() != newInfo.backgroundColor()) {
+            return true;
+        }

[Yaron, 2016/07/20 02:27:58]

todo: add icon comparison (or at a minimum, icon url so at least developers have
a way of forcing update)

[Xi Han, 2016/07/20 18:54:59]

Done.

+
+        return false;
+    }
+
+    /**
+     * Updates the SharedPreference.

[pkotwicz, 2016/07/19 18:02:38]

How about: "Updates WebappDataStorage."

[Xi Han, 2016/07/20 18:54:59]

Done.

+     */
+    private void updateWebappDataStorage(WebappInfo newInfo) {
+        mWebappInfo = newInfo;
+
+        WebappRegistry.getWebappDataStorage(ContextUtils.getApplicationContext(), mWebappInfo.id(),
+                new WebappRegistry.FetchWebappDataStorageCallback() {
+                    @Override
+                    public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
+                        if (storage == null) {
+                            WebappRegistry.registerWebapp(ContextUtils.getApplicationContext(),
+                                    mWebappInfo.id(),
+                                    new WebappRegistry.FetchWebappDataStorageCallback() {
+                                        @Override
+                                        public void onWebappDataStorageRetrieved(
+                                                WebappDataStorage storage) {
+                                            storage.updateFromWebappInfo(mWebappInfo);
+                                        }
+                                    });
+                            return;
+                        }
+                        storage.updateFromWebappInfo(mWebappInfo);
+                    }
+                });
+    }
+
+    private void upgrade() {}
+
+    private native long nativeInitialize(WebContents webContents, String scope, String startUrl,
+            String webManifestUrl);
+    private native void nativeReplaceWebContents(long nativeManifestUpgradeDetector,
+            WebContents webContents);
+    private native void nativeDestroy(long nativeManifestUpgradeDetector);
+    private native void nativeStart(long nativeManifestUpgradeDetector);
+}