Add support for webapk without runtimeHost

chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java

+// Copyright 2017 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.webapk.shell_apk;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+
+import android.content.Context;
+import android.content.Intent;
+import android.content.SharedPreferences;
+import android.content.pm.ActivityInfo;
+import android.content.pm.ResolveInfo;
+import android.os.Bundle;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mockito;
+import org.robolectric.RuntimeEnvironment;
+import org.robolectric.annotation.Config;
+import org.robolectric.res.builder.RobolectricPackageManager;
+
+import org.chromium.testing.local.LocalRobolectricTestRunner;
+import org.chromium.webapk.lib.common.WebApkConstants;
+import org.chromium.webapk.lib.common.WebApkMetaDataKeys;
+import org.chromium.webapk.test.WebApkTestHelper;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * Tests for WebApkUtils.
+ */
+
+@RunWith(LocalRobolectricTestRunner.class)
+@Config(manifest = Config.NONE, packageName = WebApkUtilsTest.WEBAPK_PACKAGE_NAME)
+public class WebApkUtilsTest {
+    protected static final String WEBAPK_PACKAGE_NAME = "org.chromium.webapk.test_package";
+    private static final String BROWSER_INSTALLED_SUPPORTING_WEBAPKS =
+            "browser.installed.supporting.webapks";

[pkotwicz, 2017/05/26 22:38:38]

Can this be com.chrome.canary?

[Xi Han, 2017/05/29 21:18:55]

I would prefer this naming, since it reflects what the browser is supposed to
be.

[pkotwicz, 2017/05/31 01:06:21]

I am suggesting this naming because it enables getting rid of
WebApkUtils#setBrowsersSupportingWebApkForTesting()

[Xi Han, 2017/05/31 15:50:15]

Got it. Updated the naming.

+    private static final String BROWSER_UNINSTALLED_SUPPORTING_WEBAPK =
+            "browser.uninstalled.supporting.webapks";

[pkotwicz, 2017/05/26 22:38:38]

Can this be com.chrome.dev?

+    private static final String BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK =
+            "browser.installed.not.supporting.webapks";
+    private static final String ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS =
+            "another.browser.installed.supporting.webapks";

[pkotwicz, 2017/05/26 22:38:38]

Can this be com.chrome.beta?

+
+    private static final List<String> sInstalledBrowsers = new ArrayList<String>(Arrays.asList(
+            BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK, BROWSER_INSTALLED_SUPPORTING_WEBAPKS,
+            ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS));
+
+    private static final List<String> sBrowsersSupportingWebApks =
+            new ArrayList<String>(Arrays.asList(BROWSER_UNINSTALLED_SUPPORTING_WEBAPK,
+                    BROWSER_INSTALLED_SUPPORTING_WEBAPKS,
+                    ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS));
+
+    private Context mContext;
+    private RobolectricPackageManager mPackageManager;
+
+    @Before
+    public void setUp() {
+        mContext = RuntimeEnvironment.application;
+        mPackageManager =
+                Mockito.spy((RobolectricPackageManager) RuntimeEnvironment.getPackageManager());
+        RuntimeEnvironment.setRobolectricPackageManager(mPackageManager);
+
+        WebApkUtils.resetCachedHostPackageForTesting();
+        WebApkUtils.setBrowsersSupportingWebApkForTesting(sBrowsersSupportingWebApks);
+    }
+
+    /**
+     * Tests that null will be returned if there isn't any browser installed on the device.
+     */
+    @Test
+    public void testReturnsNullWhenNoBrowserInstalled() {
+        String hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertNull(hostBrowser);
+    }
+
+    /**
+     * Tests that the package name of the host browser in the SharedPreference will be returned if
+     * it is installed, no matter whether a host browser is specified in the AndroidManifest.xml or
+     * not.

[pkotwicz, 2017/05/26 22:38:38]

How about: "no matter whether a host browser is specified in the
AndroidManifest.xml or not" -> "even if a host browser is specified in the
AndroidManifest.xml."

You don't test of the "host browser" not being specified in AndroidManifest.xml
(I think that it is unnecessary)

[Xi Han, 2017/05/29 21:18:55]

Sound good, thanks!

+     */
+    @Test
+    public void testReturnsHostBrowserInSharedPreferenceIfInstalled() {
+        String expectedHostBrowser = BROWSER_INSTALLED_SUPPORTING_WEBAPKS;
+        mockInstallBrowsers();

[pkotwicz, 2017/05/26 22:38:38]

The default browser should be a parameter to mockInstallBrowsers().

If mockDefaultBrowser()
is not called the default browser is BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK
because PackageManager#addResolveInfoForIntent() is called for
BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK
first.

[Xi Han, 2017/05/29 21:18:55]

Yes, adding the default browser as a parameter is a good idea. It guarantees
that the default browser is in the installed browser list. Right, the first one
in the |sInstalledBrowser| is the default one when nothing is set. Since both of
them are used as default browser once, putting
ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS as the first one in the
|sInstalledBrowser|.

+        mockWebApkPackageMetadata(ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS);
+        mockSharedPreferences(expectedHostBrowser);
+
+        String hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertEquals(hostBrowser, expectedHostBrowser);
+    }
+
+    /**
+     * Tests that the package name of the host browser specified in the AndroidManifest.xml will be
+     * returned if:
+     * 1. there isn't a host browser specified in the SharedPreference or the specified one is
+     *    uninstalled;

[pkotwicz, 2017/05/26 22:38:38]

Nit: Make the comment reflect the test. You don't test the scenario of the
SharedPreference containing the package name of an uninstalled browser. I don't
think you need to test the scenario. If an "uninstalled browser which supports
WebAPKs" in SharedPreferences did anything
WebApkUtils#getHostBrowserPackageName() would not return null in
testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled()

[Xi Han, 2017/05/29 21:18:55]

Good catch! I add the missing part to the test to make the test including all
the possible cases. Even though it might be covered by other tests, but not very
obvious to me.

+     * And
+     * 2. there is a host browser stored in the AndroidManifest and it is still installed.
+     */
+    @Test
+    public void testReturnsHostBrowserInManifestIfInstalled() {
+        String expectedHostBrowser = BROWSER_INSTALLED_SUPPORTING_WEBAPKS;
+        mockInstallBrowsers();
+        mockWebApkPackageMetadata(expectedHostBrowser);
+        mockSharedPreferences(null);
+
+        String hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertEquals(hostBrowser, expectedHostBrowser);
+    }
+
+    /**
+     * Tests that null will be returned if:
+     * 1) there isn't a host browser stored in the SharedPreference or it isn't installed.
+     * And
+     * 2) the host browser specified in the AndroidManifest.xml isn't installed;
+     */
+    @Test
+    public void testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled() {

[pkotwicz, 2017/05/26 22:38:38]

This test is identical to
testReturnsNullWhenDefaultBrowserDoesNotSupportWebApks()

The default browser for this test is BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK

[Xi Han, 2017/05/29 21:18:55]

Sorry for the confusion. They are different tests, this one is for
browser-WebAPK (has specified runtime host); the other is for play-WebAPK
(without runtime host). I added a comment to specify the situation.

+        mockInstallBrowsers();
+        mockWebApkPackageMetadata(BROWSER_UNINSTALLED_SUPPORTING_WEBAPK);
+        // Simulates that there isn't any host browser stored in the SharedPreference.
+        mockSharedPreferences(null);
+
+        String hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertNull(hostBrowser);
+
+        // Simulates that the host browser stored in the SharedPreference has been uninstalled.

[pkotwicz, 2017/05/26 22:38:38]

Perhaps this should be its own test. We can simplify the test to not specify a
host browser in the WebAPK meta data.

[Xi Han, 2017/05/29 21:18:55]

This is a test for browser-WebAPK, which has a host browser in the meta data.

+        mockSharedPreferences(BROWSER_UNINSTALLED_SUPPORTING_WEBAPK);
+        hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertNull(hostBrowser);
+    }
+
+    /**
+     * Tests that the default browser package name will be returned if:
+     * 1. there isn't any host browser stored in the SharedPreference, or the specified one has
+     *    been uninstalled;
+     * And
+     * 2. there isn't a host browser specified in the AndroidManifest.xml;
+     * And
+     * 3. the default browser supports WebAPKs.
+     */
+    @Test
+    public void testReturnsDefaultBrowser() {
+        String defaultBrowser = BROWSER_INSTALLED_SUPPORTING_WEBAPKS;
+        mockInstallBrowsers();
+        mockWebApkPackageMetadata(null);
+        // Simulates that there isn't any host browser stored in the SharedPreference.
+        mockSharedPreferences(null);
+        mockDefaultBrowser(defaultBrowser);
+
+        String hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertEquals(hostBrowser, defaultBrowser);
+
+        // Simulates that the host browser stored in the SharedPreference isn't installed.

[pkotwicz, 2017/05/26 22:38:38]

I don't think that you need to test this case. If an "uninstalled browser which
supports WebAPKs" in SharedPreferences did anything
WebApkUtils#getHostBrowserPackageName() would not return null in
testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled()

[Xi Han, 2017/05/29 21:18:55]

That test is for browser-webapk, while this is for play-webapk. Only in the case
of no host browser in the meta data will lead to a default browser check.

[pkotwicz, 2017/05/31 01:06:21]

I think of WebApkUtils#getHostBrowserPackageName() mostly as

String getHostBrowserPackageName() {
    if (...) {
        return not-null;
    }
    if (...) {
        return not-null;
    }
    if (...) {
        return not-null;
    }
    return null;
}

with the exception that the default browser is ignored if the Android Manifest
contains a runtime host.

For this reason, I think that you only need a single test case to check the
behavior when the browser listed in the SharedPreference has been uninstalled

The test cases to care about are:
S=Shared Preferences
M=Manifest
D=Default Browser

I=Installed
U=Uninstalled
N=Null
Q=Not WebAPK Compatible

SI
SN MN DQ
SN MN DI
SN MU DQ --> we do not need this test because the test case for "SN MU DI"
returns null
SN MU DI
SN MI
SU MN DQ
SU MN DI --> we do not need this test
SU MU DQ --> we do not need this test
SU MU DI --> we do not need this test
SU MI --> we do not need this test

+        WebApkUtils.resetCachedHostPackageForTesting();
+        mockSharedPreferences(BROWSER_UNINSTALLED_SUPPORTING_WEBAPK);
+        hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertEquals(hostBrowser, defaultBrowser);
+    }
+
+    /**
+     * Tests that null will be returned if:
+     * 1. there isn't any host browser stored in the SharedPreference, or the specified one has
+     *    been uninstalled;
+     * And
+     * 2. there isn't a host browser specified in the AndroidManifest.xml;
+     * And
+     * 3. the default browser doesn't support WebAPKs.
+     */
+    @Test
+    public void testReturnsNullWhenDefaultBrowserDoesNotSupportWebApks() {
+        mockInstallBrowsers();
+        mockWebApkPackageMetadata(null);
+        mockSharedPreferences(null);
+        mockDefaultBrowser(BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK);
+
+        String hostBrowser = WebApkUtils.getHostBrowserPackageName(mContext);
+        Assert.assertNull(hostBrowser);
+    }
+
+    private static ResolveInfo newResolveInfo(String packageName) {
+        ActivityInfo activityInfo = new ActivityInfo();
+        activityInfo.packageName = packageName;
+        ResolveInfo resolveInfo = new ResolveInfo();
+        resolveInfo.activityInfo = activityInfo;
+        return resolveInfo;
+    }
+
+    private void mockInstallBrowsers() {
+        Intent intent = null;
+        try {
+            intent = Intent.parseUri("http://", Intent.URI_INTENT_SCHEME);
+        } catch (Exception e) {
+            e.printStackTrace();
+            return;
+        }
+
+        for (String name : sInstalledBrowsers) {
+            mPackageManager.addResolveInfoForIntent(intent, newResolveInfo(name));
+        }
+    }
+
+    private void mockSharedPreferences(String hostBrowserPackage) {

[pkotwicz, 2017/05/26 22:38:38]

Maybe rename this function to setHostBrowserInSharedPreferences()

I think that my suggestion better describes what the function does and the role
of |hostBrowserPackage|

[Xi Han, 2017/05/29 21:18:55]

SG

+        SharedPreferences sharedPref =
+                mContext.getSharedPreferences(WebApkConstants.PREF_PACKAGE, Context.MODE_PRIVATE);
+        SharedPreferences.Editor editor = sharedPref.edit();
+        editor.putString(WebApkUtils.SHARED_PREF_RUNTIME_HOST, hostBrowserPackage);
+        editor.apply();
+    }
+
+    private void mockWebApkPackageMetadata(String hostBrowserPackage) {

[pkotwicz, 2017/05/26 22:38:38]

Maybe rename this function to setHostBrowserInMetadata()

[Xi Han, 2017/05/29 21:18:55]

Done.

+        Bundle bundle = new Bundle();
+        bundle.putString(WebApkMetaDataKeys.RUNTIME_HOST, hostBrowserPackage);
+        WebApkTestHelper.registerWebApkWithMetaData(WEBAPK_PACKAGE_NAME, bundle);
+    }
+
+    private void mockDefaultBrowser(String defaultBrowser) {

[pkotwicz, 2017/05/26 22:38:38]

Maybe rename this function to setDefaultBrowser()

[Xi Han, 2017/05/29 21:18:55]

Move the logic to |mockInstalledBrowsers()|.

+        ResolveInfo defaultBrowserInfo = newResolveInfo(defaultBrowser);
+        Mockito.when(mPackageManager.resolveActivity(any(Intent.class), anyInt()))
+                .thenReturn(defaultBrowserInfo);
+    }
+}