Clean up WebApk logic in ExternalNavigationHandler

This CL does three things:
1. It moves unrelated logic out of launchWebApkIfSoleIntentHandler()
2. It unifies the browser fallback URL handling path.
3. It avoids unnecessarily calling isValidWebApk on a lot of packages

There is a functional change here: we no longer require an intent URL
to specify a package for its fallback URL to be handled by a WebApk.
(The package could have been any package, even unrelated to the WebApk)

Bug: 1006927
Change-Id: I1dd4bf26e40706a37900377e280623aa5ae129fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1837478Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705129}

chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java

@@ -47,13 +47,6 @@ interface ExternalNavigationDelegate {
      */
     int countSpecializedHandlers(List<ResolveInfo> infos);
 
-    /**
-     * Returns the package name of the first valid WebAPK in {@link infos}.
-     * @param infos ResolveInfos to search.
-     * @return The package name of the first valid WebAPK. Null if no valid WebAPK was found.
-     */
-    String findFirstWebApkPackageName(List<ResolveInfo> infos);
-
     /**
      * Start an activity for the intent. Used for intents that must be handled externally.
      * @param intent The intent we want to send.
@@ -163,4 +156,10 @@ interface ExternalNavigationDelegate {
      * @return Whether the Intent points to an app that we trust and that launched Chrome.
      */
     boolean isIntentForTrustedCallingApp(Intent intent);
+
+    /**
+     * @param packageName The package to check.
+     * @return Whether the package is a valid WebAPK package.
+     */
+    boolean isValidWebApk(String packageName);
 }

chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java

@@ -298,11 +298,6 @@ public class ExternalNavigationDelegateImpl implements ExternalNavigationDelegat
         return !getSpecializedHandlersWithFilter(handlers, packageName).isEmpty();
     }
 
-    @Override
-    public String findFirstWebApkPackageName(List<ResolveInfo> infos) {
-        return WebApkValidator.findFirstWebApkPackage(mApplicationContext, infos);
-    }
-
     @Override
     public void startActivity(Intent intent, boolean proxy) {
         try {
@@ -644,4 +639,9 @@ public class ExternalNavigationDelegateImpl implements ExternalNavigationDelegat
     public boolean isIntentForTrustedCallingApp(Intent intent) {
         return false;
     }
+
+    @Override
+    public boolean isValidWebApk(String packageName) {
+        return WebApkValidator.isValidWebApk(ContextUtils.getApplicationContext(), packageName);
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java

@@ -9,6 +9,7 @@ import static org.chromium.chrome.browser.customtabs.CustomTabIntentDataProvider
 import android.content.ActivityNotFoundException;
 import android.content.ComponentName;
 import android.content.Intent;
+import android.content.pm.ActivityInfo;
 import android.content.pm.ResolveInfo;
 import android.net.Uri;
 import android.os.SystemClock;
@@ -44,6 +45,7 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 
@@ -74,21 +76,6 @@ public class ExternalNavigationHandler {
     @VisibleForTesting
     static final String EXTRA_MARKET_REFERRER = "market_referrer";
 
-    @IntDef({WebApkLaunchDecision.LAUNCHED, WebApkLaunchDecision.LAUNCH_FAILED,
-            WebApkLaunchDecision.ALREADY_IN_WEBAPK,
-            WebApkLaunchDecision.WEBAPK_NOT_SOLE_INTENT_HANDLER})
-    public @interface WebApkLaunchDecision {
-        int LAUNCHED = 0;
-        int LAUNCH_FAILED = 1;
-
-        // User is either in target WebAPK or in CCT launched by the target WebAPK.
-        int ALREADY_IN_WEBAPK = 2;
-
-        // The WebAPK either cannot handle intent or there are multiple non-browser apps which
-        // can handle the intent.
-        int WEBAPK_NOT_SOLE_INTENT_HANDLER = 3;
-    }
-
     // These values are persisted in histograms. Please do not renumber. Append only.
     @IntDef({AiaIntent.FALLBACK_USED, AiaIntent.SERP, AiaIntent.OTHER})
     @Retention(RetentionPolicy.SOURCE)
@@ -186,18 +173,34 @@ public class ExternalNavigationHandler {
                 && (params.getRedirectHandler() == null
                         // For instance, if this is a chained fallback URL, we ignore it.
                         || !params.getRedirectHandler().shouldNotOverrideUrlLoading())) {
-            if (InstantAppsHandler.isIntentToInstantApp(targetIntent)) {
-                RecordHistogram.recordEnumeratedHistogram(
-                        "Android.InstantApps.DirectInstantAppsIntent", AiaIntent.FALLBACK_USED,
-                        AiaIntent.NUM_ENTRIES);
-            }
-
-            result = clobberCurrentTabWithFallbackUrl(browserFallbackUrl, params);
+            result = handleFallbackUrl(params, targetIntent, browserFallbackUrl);
         }
         if (DEBUG) printDebugShouldOverrideUrlLoadingResult(result);
         return result;
     }
 
+    private @OverrideUrlLoadingResult int handleFallbackUrl(
+            ExternalNavigationParams params, Intent targetIntent, String browserFallbackUrl) {
+        if (InstantAppsHandler.isIntentToInstantApp(targetIntent)) {
+            RecordHistogram.recordEnumeratedHistogram("Android.InstantApps.DirectInstantAppsIntent",
+                    AiaIntent.FALLBACK_USED, AiaIntent.NUM_ENTRIES);
+        }
+        // Launch WebAPK if it can handle the URL.
+        try {
+            Intent intent = Intent.parseUri(browserFallbackUrl, Intent.URI_INTENT_SCHEME);
+            sanitizeQueryIntentActivitiesIntent(intent);
+            List<ResolveInfo> resolvingInfos = mDelegate.queryIntentActivities(intent);
+            if (!shouldStayInWebApkCCT(params, resolvingInfos)
+                    && !isAlreadyInTargetWebApk(resolvingInfos, params)
+                    && launchWebApkIfSoleIntentHandler(resolvingInfos, intent)) {
+                return OverrideUrlLoadingResult.OVERRIDE_WITH_EXTERNAL_INTENT;
+            }
+        } catch (Exception e) {
+            if (DEBUG) Log.i(TAG, "Could not parse fallback url as intent");
+        }
+        return clobberCurrentTabWithFallbackUrl(browserFallbackUrl, params);
+    }
+
     private void printDebugShouldOverrideUrlLoadingResult(int result) {
         String resultString;
         switch (result) {
@@ -475,46 +478,14 @@ public class ExternalNavigationHandler {
      */
     private int handleUnresolvableIntent(
             ExternalNavigationParams params, Intent targetIntent, String browserFallbackUrl) {
-        if (browserFallbackUrl != null) {
-            return handleFallbackUrl(params, targetIntent, browserFallbackUrl);
-        }
+        // Fallback URL will be handled by the caller of shouldOverrideUrlLoadingInternal.
+        if (browserFallbackUrl != null) return OverrideUrlLoadingResult.NO_OVERRIDE;
         if (targetIntent.getPackage() != null) return handleWithMarketIntent(params, targetIntent);
 
         if (DEBUG) Log.i(TAG, "Could not find an external activity to use");
         return OverrideUrlLoadingResult.NO_OVERRIDE;
     }
 
-    private @OverrideUrlLoadingResult int handleFallbackUrl(
-            ExternalNavigationParams params, Intent intent, String browserFallbackUrl) {
-        // Launch WebAPK if it can handle the URL.
-        if (!TextUtils.isEmpty(intent.getPackage())
-                || (intent.getSelector() != null
-                        && !TextUtils.isEmpty(intent.getSelector().getPackage()))) {
-            try {
-                intent = Intent.parseUri(browserFallbackUrl, Intent.URI_INTENT_SCHEME);
-            } catch (Exception e) {
-                if (DEBUG) Log.i(TAG, "Could not parse fallback url");
-                return OverrideUrlLoadingResult.NO_OVERRIDE;
-            }
-            sanitizeQueryIntentActivitiesIntent(intent);
-            List<ResolveInfo> resolvingInfos = mDelegate.queryIntentActivities(intent);
-            switch (launchWebApkIfSoleIntentHandler(params, resolvingInfos, intent)) {
-                case WebApkLaunchDecision.ALREADY_IN_WEBAPK:
-                    if (DEBUG) Log.i(TAG, "Already in WebAPK");
-                    return OverrideUrlLoadingResult.NO_OVERRIDE;
-                case WebApkLaunchDecision.LAUNCH_FAILED:
-                    if (DEBUG) Log.i(TAG, "WebAPK launch failed");
-                    return OverrideUrlLoadingResult.NO_OVERRIDE;
-                case WebApkLaunchDecision.LAUNCHED:
-                    if (DEBUG) Log.i(TAG, "Launched WebAPK");
-                    return OverrideUrlLoadingResult.OVERRIDE_WITH_EXTERNAL_INTENT;
-                case WebApkLaunchDecision.WEBAPK_NOT_SOLE_INTENT_HANDLER:
-                    break;
-            }
-        }
-        return clobberCurrentTabWithFallbackUrl(browserFallbackUrl, params);
-    }
-
     private @OverrideUrlLoadingResult int handleWithMarketIntent(
             ExternalNavigationParams params, Intent intent) {
         String marketReferrer = IntentUtils.safeGetStringExtra(intent, EXTRA_MARKET_REFERRER);
@@ -698,6 +669,26 @@ public class ExternalNavigationHandler {
         return OverrideUrlLoadingResult.NO_OVERRIDE;
     }
 
+    /**
+     * Returns whether the activity belongs to a WebAPK and the URL is within the scope of the
+     * WebAPK. The WebAPK's main activity is a bouncer that redirects to WebApkActivity in Chrome.
+     * In order to avoid bouncing indefinitely, we should not override the navigation if we are
+     * currently showing the WebAPK (params#nativeClientPackageName()) that we will redirect to.
+     */
+    private boolean isAlreadyInTargetWebApk(
+            List<ResolveInfo> resolveInfos, ExternalNavigationParams params) {
+        String currentName = params.nativeClientPackageName();
+        if (currentName == null) return false;
+        for (ResolveInfo resolveInfo : resolveInfos) {
+            ActivityInfo info = resolveInfo.activityInfo;
+            if (info != null && currentName.equals(info.packageName)) {
+                if (DEBUG) Log.i(TAG, "Already in WebAPK");
+                return true;
+            }
+        }
+        return false;
+    }
+
     private @OverrideUrlLoadingResult int shouldOverrideUrlLoadingInternal(
             ExternalNavigationParams params, Intent targetIntent,
             @Nullable String browserFallbackUrl) {
@@ -828,18 +819,13 @@ public class ExternalNavigationHandler {
             }
         }
 
-        switch (launchWebApkIfSoleIntentHandler(params, resolvingInfos, targetIntent)) {
-            case WebApkLaunchDecision.ALREADY_IN_WEBAPK:
-                if (DEBUG) Log.i(TAG, "Already in WebAPK");
+        if (shouldStayInWebApkCCT(params, resolvingInfos)) {
             return OverrideUrlLoadingResult.NO_OVERRIDE;
-            case WebApkLaunchDecision.LAUNCH_FAILED:
-                if (DEBUG) Log.i(TAG, "WebAPK launch failed");
+        }
+        if (isAlreadyInTargetWebApk(resolvingInfos, params)) {
             return OverrideUrlLoadingResult.NO_OVERRIDE;
-            case WebApkLaunchDecision.LAUNCHED:
-                if (DEBUG) Log.i(TAG, "Launched WebAPK");
+        } else if (launchWebApkIfSoleIntentHandler(resolvingInfos, targetIntent)) {
             return OverrideUrlLoadingResult.OVERRIDE_WITH_EXTERNAL_INTENT;
-            case WebApkLaunchDecision.WEBAPK_NOT_SOLE_INTENT_HANDLER:
-                break;
         }
 
         try {
@@ -1022,50 +1008,36 @@ public class ExternalNavigationHandler {
                 tab.getActivity().getIntent(), Browser.EXTRA_APPLICATION_ID);
         if (appId == null) return false;
 
-        try {
-            Intent.parseUri(params.getUrl(), Intent.URI_INTENT_SCHEME);
-        } catch (URISyntaxException ex) {
-            return false;
-        }
-        return !ExternalNavigationDelegateImpl.getSpecializedHandlersWithFilter(handlers, appId)
+        boolean webApkHasSpecializedHandler =
+                ExternalNavigationDelegateImpl.getSpecializedHandlersWithFilter(handlers, appId)
                         .isEmpty();
+        if (webApkHasSpecializedHandler) return false;
+        if (DEBUG) Log.i(TAG, "Staying in WebApk CCT.");
+        return true;
     }
 
     /**
      * Launches WebAPK if the WebAPK is the sole non-browser handler for the given intent.
-     * Returns whether a WebAPK was launched and if it was not launched returns why.
+     * @return Whether a WebAPK was launched.
      */
-    private @WebApkLaunchDecision int launchWebApkIfSoleIntentHandler(
-            ExternalNavigationParams params, List<ResolveInfo> resolvingInfos, Intent intent) {
-        if (shouldStayInWebApkCCT(params, resolvingInfos)) {
-            return WebApkLaunchDecision.ALREADY_IN_WEBAPK;
-        }
-
-        String targetWebApkPackageName = mDelegate.findFirstWebApkPackageName(resolvingInfos);
-
-        // We can't rely on this falling through to startActivityIfNeeded and behaving
-        // correctly for WebAPKs. This is because the target of the intent is the WebApk's main
-        // activity but that's just a bouncer which will redirect to WebApkActivity in chrome.
-        // To avoid bouncing indefinitely, don't override the navigation if we are currently
-        // showing the WebApk |params.webApkPackageName()| that we will redirect to.
-        if (targetWebApkPackageName != null
-                && targetWebApkPackageName.equals(params.nativeClientPackageName())) {
-            return WebApkLaunchDecision.ALREADY_IN_WEBAPK;
-        }
-
-        if (targetWebApkPackageName == null
-                || mDelegate.countSpecializedHandlers(resolvingInfos) != 1) {
-            return WebApkLaunchDecision.WEBAPK_NOT_SOLE_INTENT_HANDLER;
-        }
-
-        intent.setPackage(targetWebApkPackageName);
+    private boolean launchWebApkIfSoleIntentHandler(
+            List<ResolveInfo> resolvingInfos, Intent targetIntent) {
+        ArrayList<String> packages =
+                ExternalNavigationDelegateImpl.getSpecializedHandlersWithFilter(
+                        resolvingInfos, null);
+        if (packages.size() != 1 || !mDelegate.isValidWebApk(packages.get(0))) return false;
+        Intent webApkIntent = new Intent(targetIntent);
+        webApkIntent.setPackage(packages.get(0));
         try {
-            if (mDelegate.startActivityIfNeeded(intent, false)) {
-                return WebApkLaunchDecision.LAUNCHED;
-            }
+            mDelegate.startActivity(webApkIntent, false);
+            if (DEBUG) Log.i(TAG, "Launched WebAPK");
+            return true;
         } catch (ActivityNotFoundException e) {
+            // The WebApk must have been uninstalled/disabled since we queried for Activities to
+            // handle this intent.
+            if (DEBUG) Log.i(TAG, "WebAPK launch failed");
+            return false;
         }
-        return WebApkLaunchDecision.LAUNCH_FAILED;
     }
 
     /**

chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java

@@ -8,6 +8,7 @@ import android.annotation.SuppressLint;
 import android.content.Context;
 import android.content.ContextWrapper;
 import android.content.Intent;
+import android.content.IntentFilter;
 import android.content.pm.ActivityInfo;
 import android.content.pm.PackageManager;
 import android.content.pm.ResolveInfo;
@@ -1599,6 +1600,14 @@ public class ExternalNavigationHandlerTest {
         return ri;
     }
 
+    private static ResolveInfo newSpecializedResolveInfo(
+            String packageName, IntentActivity activity) {
+        ResolveInfo info = newResolveInfo(packageName);
+        info.filter = new IntentFilter(Intent.ACTION_VIEW);
+        info.filter.addDataAuthority(activity.mUrlPrefix, null);
+        return info;
+    }
+
     private static WebappInfo newWebappInfoFromScope(String scope) {
         Intent webappIntent = WebappTestHelper.createMinimalWebappIntent("" /* id */, "" /* url */);
         webappIntent.putExtra(ShortcutHelper.EXTRA_SCOPE, scope);
@@ -1660,7 +1669,8 @@ public class ExternalNavigationHandlerTest {
             }
             for (IntentActivity intentActivity : mIntentActivities) {
                 if (dataString.startsWith(intentActivity.urlPrefix())) {
-                    list.add(newResolveInfo(intentActivity.packageName()));
+                    list.add(newSpecializedResolveInfo(
+                            intentActivity.packageName(), intentActivity));
                 }
             }
             if (!list.isEmpty()) return list;
@@ -1716,17 +1726,6 @@ public class ExternalNavigationHandlerTest {
             return count;
         }
 
-        @Override
-        public String findFirstWebApkPackageName(List<ResolveInfo> infos) {
-            List<IntentActivity> matchingIntentActivities = findMatchingIntentActivities(infos);
-            for (IntentActivity intentActivity : matchingIntentActivities) {
-                if (intentActivity.isWebApk()) {
-                    return intentActivity.packageName();
-                }
-            }
-            return null;
-        }
-
         private ArrayList<IntentActivity> findMatchingIntentActivities(List<ResolveInfo> infos) {
             ArrayList<IntentActivity> outList = new ArrayList<IntentActivity>();
             for (ResolveInfo info : infos) {
@@ -1823,6 +1822,16 @@ public class ExternalNavigationHandlerTest {
             return mIsCallingAppTrusted;
         }
 
+        @Override
+        public boolean isValidWebApk(String packageName) {
+            for (IntentActivity activity : mIntentActivities) {
+                if (activity.packageName().equals(packageName)) {
+                    return activity.isWebApk();
+                }
+            }
+            return false;
+        }
+
         public void reset() {
             startActivityIntent = null;
             startIncognitoIntentCalled = false;