Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes.

chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java

Index: chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
new file mode 100644
index 0000000000000000000000000000000000000000..cdb0e9f29b709ca8960391df3b906e3ad4d13b7b
--- /dev/null
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
@@ -0,0 +1,149 @@
+// 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.Context;
+import android.content.Intent;
+import android.net.Uri;
+import android.os.Environment;
+import android.test.suitebuilder.annotation.MediumTest;
+
+import org.chromium.base.ThreadUtils;
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.blink_public.platform.WebDisplayMode;
+import org.chromium.chrome.browser.ShortcutHelper;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.test.ChromeTabbedActivityTestBase;
+import org.chromium.chrome.test.util.browser.TabLoadObserver;
+import org.chromium.content.browser.test.util.Criteria;
+import org.chromium.content.browser.test.util.CriteriaHelper;
+import org.chromium.content_public.common.ScreenOrientationValues;
+import org.chromium.net.test.EmbeddedTestServer;
+import org.chromium.webapk.lib.common.WebApkConstants;
+
+/**
+ * Tests the ManifestUpgradeDetector.
+ */
+public class ManifestUpgradeDetectorTest extends ChromeTabbedActivityTestBase {

[pkotwicz, 2016/07/25 17:35:53]

Please add a comment where that the field values come from {@link
WEBAPK_WEB_MANIFEST_URL}

[Xi Han, 2016/07/25 20:01:37]

Done.

+    private static final String WEBAPK_ID = WebApkConstants.WEBAPK_ID_PREFIX + "webapp_id";
+    private static final String WEBAPK_NAME = "Manifest test app";
+    private static final String WEBAPK_SHORT_NAME = "App";
+    private static final int WEBAPK_ORIENTATION = ScreenOrientationValues.LANDSCAPE;
+    private static final String WEBAPK_START_URL_PATH =
+            "/chrome/test/data/webapps/manifest_test_page.html";
+    private static final String WEBAPK_SCOPE_PATH = "/chrome/test/data/webapps/";
+    private static final String WEBAPK_WEB_MANIFEST_URL =
+            "/chrome/test/data/webapps/manifest.json";
+
+    private String mStartUrl;
+    private ManifestUpgradeDetector mDetector;

[pkotwicz, 2016/07/25 17:35:53]

Nit: Change the type to TestManifestUpgradeDetector. This should enable removing
several casts

[Xi Han, 2016/07/25 20:01:38]

Done.

+    private EmbeddedTestServer mTestServer;
+

[pkotwicz, 2016/07/25 17:35:53]

Nit: Please add JavaDoc for the test class.

[Xi Han, 2016/07/25 20:01:37]

Done.

+    private static class TestManifestUpgradeDetector extends ManifestUpgradeDetector {
+        private boolean mIsUpgraded;

[pkotwicz, 2016/07/25 17:35:53]

Please make these variables public to reflect how they are being used

I looked it up. Here is an explanation of why an outer class can access an inner
class' privates
http://stackoverflow.com/questions/19747812/why-can-the-private-member-of-an-...

[Xi Han, 2016/07/25 20:01:38]

Done.

+        private WebappInfo mInfo;
+        private String mStartUrl;
+        private boolean mIsDataAvaliable = false;

[pkotwicz, 2016/07/25 17:35:53]

Nit: new line

[Xi Han, 2016/07/25 20:01:37]

Done.

+        public TestManifestUpgradeDetector(Tab tab, WebappInfo info) {
+            super(tab, info);
+        }
+
+        public boolean isDataAvaliable() {
+            return mIsDataAvaliable;
+        }
+
+        @Override
+        protected boolean requireUpgrade(WebappInfo newInfo, String startUrl) {
+            mIsUpgraded = super.requireUpgrade(newInfo, startUrl);
+            mStartUrl = startUrl;
+            mIsDataAvaliable = true;

[pkotwicz, 2016/07/25 17:35:53]

mIsDataAvaliable -> mIsDataAvailable

[Xi Han, 2016/07/25 20:01:37]

Done.

+            mInfo = newInfo;

[pkotwicz, 2016/07/25 17:35:53]

This does not seem to be used

[Xi Han, 2016/07/25 20:01:37]

Removed, since most of the tests that used it had been removed.

+            return mIsUpgraded;
+        }
+    }
+
+    @Override
+    protected void setUp() throws Exception {
+        super.setUp();
+        final Context context = getInstrumentation().getTargetContext();
+        mTestServer = EmbeddedTestServer.createAndStartFileServer(
+                context, Environment.getExternalStorageDirectory());
+        mStartUrl = mTestServer.getURL(WEBAPK_START_URL_PATH);
+    }
+
+    @Override
+    protected void tearDown() throws Exception {
+        mTestServer.stopAndDestroyServer();
+        super.tearDown();
+    }
+
+    @Override
+    public void startMainActivity() throws InterruptedException {
+        startMainActivityOnBlankPage();
+    }
+

[pkotwicz, 2016/07/25 17:35:53]

Please add a comment say that this creates an intent with the properties in
chrome/test/data/webapps/manifest.json

[Xi Han, 2016/07/25 20:01:37]

Done.

+    private Intent createIntent() {
+        Intent intent = new Intent();
+        intent.setData(Uri.parse(WebappActivity.WEBAPP_SCHEME + "://" + WEBAPK_ID));

[pkotwicz, 2016/07/25 17:35:53]

Is setting the data necessary?

[Xi Han, 2016/07/25 20:01:37]

It also works without it. removed.

+        intent.putExtra(ShortcutHelper.EXTRA_ID, WEBAPK_ID)
+                .putExtra(ShortcutHelper.EXTRA_URL, WEBAPK_START_URL_PATH)
+                .putExtra(ShortcutHelper.EXTRA_SCOPE, mTestServer.getURL(WEBAPK_SCOPE_PATH))
+                .putExtra(ShortcutHelper.EXTRA_NAME, WEBAPK_NAME)
+                .putExtra(ShortcutHelper.EXTRA_SHORT_NAME, WEBAPK_SHORT_NAME)
+                .putExtra(ShortcutHelper.EXTRA_BACKGROUND_COLOR,
+                        ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING)
+                .putExtra(ShortcutHelper.EXTRA_THEME_COLOR,
+                        ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING)
+                .putExtra(ShortcutHelper.EXTRA_ORIENTATION, WEBAPK_ORIENTATION)
+                .putExtra(ShortcutHelper.EXTRA_DISPLAY_MODE, WebDisplayMode.Standalone)
+                .putExtra(ShortcutHelper.EXTRA_WEB_MANIFEST_URL,
+                      mTestServer.getURL(WEBAPK_WEB_MANIFEST_URL));
+        return intent;
+    }
+
+    private void waitUntilManifestDataAvailable(final Intent intent) throws Exception {
+        loadUrlInNewTab("about:blank");
+        final Tab tab = getActivity().getActivityTab();

[pkotwicz, 2016/07/25 17:35:53]

Can you please add a comment why ManifestUpgradeDetector#start() must be called
on the UI thread? (I actually do not know why it is necessary)

The ManifestUpgradeDetector constructor and
ManifestUpgradeDetector#setMetadataForTesting() do not need to be called on the
UI thread. Can you move those outside of the ThreadUtils#runOnUiThreadBlocking()
block?

[Xi Han, 2016/07/25 20:01:37]

Move the rest and add a comment for detector.start(). I think it is because it
makes JNI calls to initialize pointer in c++.

+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                mDetector = new TestManifestUpgradeDetector(tab, WebappInfo.create(createIntent()));

[pkotwicz, 2016/07/25 17:35:53]

This function is not using the intent parameter at all.

I think that you should:
- Use the passed in WebappInfo (not an intent)
- Pass in the startUrl as a parameter as well

Document that the "passed in WebappInfo" and the "passed in start URL" are "the
old WebAPK data"

[Xi Han, 2016/07/25 20:01:38]

This refactoring is due to the removing of other tests. I am ok with it for now.

+                mDetector.setMetadataForTesting(mStartUrl);
+                mDetector.start();
+            }
+        });
+
+        new TabLoadObserver(tab).fullyLoadUrl(mStartUrl);
+        CriteriaHelper.pollUiThread(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return ((TestManifestUpgradeDetector) mDetector).mIsDataAvaliable;
+            }
+        });
+    }
+
+    @MediumTest
+    @CommandLineFlags.Add(
+            ManifestUpgradeDetector.SWITCH_DO_NOT_USE_META_DATA_FROM_WEBAPK_FOR_TESTING)
+    public void testManifestDoesNotUpgrade() throws Exception {
+        waitUntilManifestDataAvailable(createIntent());
+
+        assertFalse(((TestManifestUpgradeDetector) mDetector).mIsUpgraded);
+    }
+
+    @MediumTest
+    @CommandLineFlags.Add(
+            ManifestUpgradeDetector.SWITCH_DO_NOT_USE_META_DATA_FROM_WEBAPK_FOR_TESTING)
+    public void testStartUrlChangeShouldReturnUpgradeTrue() throws Exception {
+        String currentStartUrl =
+                "/chrome/test/data/webapps/manifest_test_page_test_start_url_change.html";
+        mStartUrl = mTestServer.getURL(currentStartUrl);
+        waitUntilManifestDataAvailable(createIntent());
+
+        assertTrue(((TestManifestUpgradeDetector) mDetector).mIsUpgraded);
+        assertEquals(mTestServer.getURL(WEBAPK_START_URL_PATH),
+                ((TestManifestUpgradeDetector) mDetector).mStartUrl);
+    }
+}