Refactor InterceptNavigationDelegate#handleSubframeExternalProtocol

InterceptNavigationDelegate#handleSubframeExternalProtocol needs a
separate return type from shouldIgnoreNavigation so that subframe
navigations can return a GURL to redirect to.

A followup change will cause the GURL to be returned in place of
clobbering the tab for subframe navigations, which will then be used to
redirect the subframe instead of clobbering the top level frame.

No functional changes intended. Some ContextualSearchManager tests
weren't written correctly and needed updating.

Bug: 1365100, 1316518
Change-Id: Ia5e2a09d97078780476a72cf2c510212fde0e99c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4053000
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1075824}

chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayContentDelegate.java

@@ -5,7 +5,7 @@
 package org.chromium.chrome.browser.compositor.bottombar;
 
 import org.chromium.components.external_intents.ExternalNavigationHandler;
-import org.chromium.content_public.browser.NavigationHandle;
+import org.chromium.ui.base.PageTransition;
 import org.chromium.url.GURL;
 
 /**
@@ -53,7 +53,9 @@ public void onNavigationEntryCommitted() {}
      * @return True if the navigation should be intercepted.
      */
     public boolean shouldInterceptNavigation(ExternalNavigationHandler externalNavHandler,
-            NavigationHandle navigationHandle, GURL escapedUrl) {
+            GURL escapedUrl, @PageTransition int pageTransition, boolean isRedirect,
+            boolean hasUserGesture, boolean isRendererInitiated, GURL referrerUrl,
+            boolean isInPrimaryMainFrame, boolean isExternalProtocol) {
         return true;
     }
 

chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java

@@ -35,9 +35,11 @@
 import org.chromium.content_public.browser.RenderCoordinates;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.WebContentsObserver;
+import org.chromium.ui.base.PageTransition;
 import org.chromium.ui.base.ViewAndroidDelegate;
 import org.chromium.ui.base.WindowAndroid;
 import org.chromium.url.GURL;
+import org.chromium.url.Origin;
 
 /**
  * Content container for an OverlayPanel. This class is responsible for the management of the
@@ -165,10 +167,21 @@ public boolean shouldIgnoreNavigation(NavigationHandle navigationHandle, GURL es
             // If either of the required params for the delegate are null, do not call the
             // delegate and ignore the navigation.
             if (mExternalNavHandler == null || navigationHandle == null) return true;
-            // TODO(mdjones): Rather than passing the two navigation params, instead consider
-            // passing a boolean to make this API simpler.
-            return !mContentDelegate.shouldInterceptNavigation(
-                    mExternalNavHandler, navigationHandle, escapedUrl);
+            return !mContentDelegate.shouldInterceptNavigation(mExternalNavHandler, escapedUrl,
+                    navigationHandle.pageTransition(), navigationHandle.isRedirect(),
+                    navigationHandle.hasUserGesture(), navigationHandle.isRendererInitiated(),
+                    navigationHandle.getReferrerUrl(), navigationHandle.isInPrimaryMainFrame(),
+                    navigationHandle.isExternalProtocol());
+        }
+
+        @Override
+        public GURL handleSubframeExternalProtocol(GURL escapedUrl, @PageTransition int transition,
+                boolean hasUserGesture, Origin initiatorOrigin) {
+            mContentDelegate.shouldInterceptNavigation(mExternalNavHandler, escapedUrl, transition,
+                    false /* isRedirect */, hasUserGesture, true /* isRendererInitiated */,
+                    GURL.emptyGURL() /* referrerUrl */, false /* isInPrimaryMainFrame */,
+                    true /* isExternalProtocol */);
+            return null;
         }
     }
 
@@ -592,6 +605,10 @@ public void destroy() {
         }
     }
 
+    public InterceptNavigationDelegate getInterceptNavigationDelegateForTesting() {
+        return mInterceptNavigationDelegate;
+    }
+
     @NativeMethods
     interface Natives {
         // Native calls.

chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java

@@ -1229,4 +1229,10 @@ public void updatePanelToStateForTest(@PanelState int panelState) {
     public boolean getCanHideAndroidBrowserControls() {
         return super.getCanHideAndroidBrowserControls();
     }
+
+    @Override
+    @VisibleForTesting
+    public OverlayPanelContent getOverlayPanelContent() {
+        return super.getOverlayPanelContent();
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java

@@ -66,14 +66,14 @@
 import org.chromium.content_public.browser.GestureStateListener;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.browser.NavigationEntry;
-import org.chromium.content_public.browser.NavigationHandle;
 import org.chromium.content_public.browser.SelectAroundCaretResult;
 import org.chromium.content_public.browser.SelectionClient;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.common.ContentUrlConstants;
 import org.chromium.net.NetworkChangeNotifier;
 import org.chromium.ui.base.IntentRequestTracker;
 import org.chromium.ui.base.LocalizationUtils;
+import org.chromium.ui.base.PageTransition;
 import org.chromium.ui.base.WindowAndroid;
 import org.chromium.ui.touch_selection.SelectionEventType;
 import org.chromium.url.GURL;
@@ -1132,27 +1132,26 @@ public void onContentViewSeen() {
 
         @Override
         public boolean shouldInterceptNavigation(ExternalNavigationHandler externalNavHandler,
-                NavigationHandle navigationHandle, GURL escapedUrl) {
+                GURL escapedUrl, @PageTransition int pageTransition, boolean isRedirect,
+                boolean hasUserGesture, boolean isRendererInitiated, GURL referrerUrl,
+                boolean isInPrimaryMainFrame, boolean isExternalProtocol) {
             assert mSearchPanel != null;
-            mRedirectHandler.updateNewUrlLoading(navigationHandle.pageTransition(),
-                    navigationHandle.isRedirect(), navigationHandle.hasUserGesture(),
+            mRedirectHandler.updateNewUrlLoading(pageTransition, isRedirect, hasUserGesture,
                     mLastUserInteractionTimeSupplier.get(),
                     RedirectHandler.NO_COMMITTED_ENTRY_INDEX, true /* isInitialNavigation */,
-                    navigationHandle.isRendererInitiated());
+                    isRendererInitiated);
             ExternalNavigationParams params =
                     new ExternalNavigationParams
-                            .Builder(escapedUrl, false, navigationHandle.getReferrerUrl(),
-                                    navigationHandle.pageTransition(),
-                                    navigationHandle.isRedirect())
+                            .Builder(escapedUrl, false, referrerUrl, pageTransition, isRedirect)
                             .setApplicationMustBeInForeground(true)
                             .setRedirectHandler(mRedirectHandler)
-                            .setIsMainFrame(navigationHandle.isInPrimaryMainFrame())
+                            .setIsMainFrame(isInPrimaryMainFrame)
                             .build();
             if (externalNavHandler.shouldOverrideUrlLoading(params).getResultType()
                     != OverrideUrlLoadingResultType.NO_OVERRIDE) {
                 return false;
             }
-            return !navigationHandle.isExternalProtocol();
+            return !isExternalProtocol;
         }
 
         @Override

chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInstrumentationBase.java

@@ -320,6 +320,9 @@ public Iterable<ParameterSet> getParameters() {
     private static final String LOW_PRIORITY_INVALID_SEARCH_ENDPOINT = "/s/invalid";
     private static final String CONTEXTUAL_SEARCH_PREFETCH_PARAM = "&pf=c";
 
+    protected static final String EXTERNAL_APP_URL =
+            "intent://test/#Intent;scheme=externalappscheme;end";
+
     //--------------------------------------------------------------------------------------------
     // Feature maps that we use for parameterized tests.
     // NOTE: We want to test all Features under development both on and off, regardless of whether
@@ -452,7 +455,7 @@ public boolean needToCheckForSearchEnginePromo() {
 
         IntentFilter filter = new IntentFilter(Intent.ACTION_VIEW);
         filter.addCategory(Intent.CATEGORY_BROWSABLE);
-        filter.addDataScheme("market");
+        filter.addDataScheme("externalappscheme");
         mActivityMonitor = InstrumentationRegistry.getInstrumentation().addMonitor(
                 filter, new Instrumentation.ActivityResult(Activity.RESULT_OK, null), true);
 

chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

@@ -49,15 +49,13 @@
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeTabbedActivity;
 import org.chromium.chrome.browser.app.ChromeActivity;
-import org.chromium.chrome.browser.compositor.bottombar.OverlayContentDelegate;
 import org.chromium.chrome.browser.compositor.bottombar.OverlayPanel.PanelState;
 import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchBarControl;
 import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchImageControl;
 import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel;
 import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchQuickActionControl;
 import org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.InternalState;
 import org.chromium.chrome.browser.contextualsearch.ResolvedSearchTerm.CardTag;
-import org.chromium.chrome.browser.externalnav.ExternalNavigationDelegateImpl;
 import org.chromium.chrome.browser.findinpage.FindToolbar;
 import org.chromium.chrome.browser.firstrun.FirstRunStatus;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
@@ -73,7 +71,6 @@
 import org.chromium.chrome.test.util.FullscreenTestUtils;
 import org.chromium.chrome.test.util.MenuUtils;
 import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
-import org.chromium.components.external_intents.ExternalNavigationHandler;
 import org.chromium.content_public.browser.NavigationHandle;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
@@ -325,94 +322,52 @@ public void testAcceptedPrivacy(@EnabledFeature int enabledFeature) throws Excep
         simulateResolvableSearchAndAssertResolveAndPreload("states", true);
     }
 
-    /**
-     * Tests ContextualSearchManager#shouldInterceptNavigation for a case that an external
-     * navigation has a user gesture.
-     */
-    @Test
-    @SmallTest
-    @Feature({"ContextualSearch"})
-    @DisabledTest(message = "crbug.com/1037667 crbug.com/1316518")
-    @ParameterAnnotations.UseMethodParameter(FeatureParamProvider.class)
-    public void testExternalNavigationWithUserGesture(@EnabledFeature int enabledFeature) {
-        final ExternalNavigationDelegateImpl delegate =
-                TestThreadUtils.runOnUiThreadBlockingNoException(
-                        ()
-                                -> new ExternalNavigationDelegateImpl(
-                                        sActivityTestRule.getActivity().getActivityTab()));
-        final ExternalNavigationHandler externalNavHandler =
-                new ExternalNavigationHandler(delegate);
-        GURL url = new GURL("intent://test/#Intent;scheme=test;package=com.chrome.test;end");
-        final NavigationHandle navigationHandle = new NavigationHandle(
-                0 /* nativeNavigationHandleProxy*/,
-                new GURL("intent://test/#Intent;scheme=test;package=com.chrome.test;end"),
-                GURL.emptyGURL() /* referrerUrl */, GURL.emptyGURL() /* baseUrlForDataUrl */,
-                true /* isInPrimaryMainFrame */, false /* isSameDocument*/,
-                true /* isRendererInitiated */, null /* initiatorOrigin */, PageTransition.LINK,
-                false /* isPost */, true /* hasUserGesture */, false /* isRedirect */,
-                true /* isExternalProtocol */, 0 /* navigationId */, false /* isPageActivation */,
-                false /* isReload */);
-        InstrumentationRegistry.getInstrumentation().runOnMainSync(new Runnable() {
-            @Override
-            public void run() {
-                sActivityTestRule.getActivity().onUserInteraction();
-                Assert.assertFalse(mManager.getOverlayContentDelegate().shouldInterceptNavigation(
-                        externalNavHandler, navigationHandle, url));
-            }
-        });
-        Assert.assertEquals(1, mActivityMonitor.getHits());
-    }
-
     /**
      * Tests ContextualSearchManager#shouldInterceptNavigation for a case that an initial
      * navigation has a user gesture but the redirected external navigation doesn't.
      */
     @Test
     @SmallTest
     @Feature({"ContextualSearch"})
-    @DisabledTest(message = "crbug.com/1037667 crbug.com/1316518")
-    @ParameterAnnotations.UseMethodParameter(FeatureParamProvider.class)
-    public void testRedirectedExternalNavigationWithUserGesture(
-            @EnabledFeature int enabledFeature) {
-        final ExternalNavigationDelegateImpl delegate =
-                TestThreadUtils.runOnUiThreadBlockingNoException(
-                        ()
-                                -> new ExternalNavigationDelegateImpl(
-                                        sActivityTestRule.getActivity().getActivityTab()));
-        final ExternalNavigationHandler externalNavHandler =
-                new ExternalNavigationHandler(delegate);
-
+    public void testRedirectedExternalNavigationWithUserGesture() throws Exception {
+        simulateResolveSearch("intelligence");
         GURL initialUrl = new GURL("http://test.com");
-        final NavigationHandle initialNavigationHandle = new NavigationHandle(
+        final NavigationHandle navigationHandle = new NavigationHandle(
                 0 /* nativeNavigationHandleProxy*/, initialUrl, GURL.emptyGURL() /* referrerUrl */,
                 GURL.emptyGURL() /* baseUrlForDataUrl */, true /* isInPrimaryMainFrame */,
                 false /* isSameDocument*/, true /* isRendererInitiated */,
                 null /* initiatorOrigin */, PageTransition.LINK, false /* isPost */,
                 true /* hasUserGesture */, false /* isRedirect */, false /* isExternalProtocol */,
                 0 /* navigationId */, false /* isPageActivation */, false /* isReload */);
 
-        GURL redirectUrl =
-                new GURL("intent://test/#Intent;scheme=test;package=com.chrome.test;end");
-        final NavigationHandle redirectedNavigationHandle = new NavigationHandle(
-                0 /* nativeNavigationHandleProxy*/, redirectUrl, GURL.emptyGURL() /* referrerUrl */,
-                GURL.emptyGURL() /* baseUrlForDataUrl */, true /* isInPrimaryMainFrame */,
-                false /* isSameDocument*/, true /* isRendererInitiated */,
-                null /* initiatorOrigin */, PageTransition.LINK, false /* isPost */,
-                false /* hasUserGesture */, true /* isRedirect */, true /* isExternalProtocol */,
-                0 /* navigationId */, false /* isPageActivation */, false /* isReload */);
+        GURL redirectUrl = new GURL(EXTERNAL_APP_URL);
 
         InstrumentationRegistry.getInstrumentation().runOnMainSync(new Runnable() {
             @Override
             public void run() {
-                sActivityTestRule.getActivity().onUserInteraction();
-                OverlayContentDelegate delegate = mManager.getOverlayContentDelegate();
-                Assert.assertTrue(delegate.shouldInterceptNavigation(
-                        externalNavHandler, initialNavigationHandle, initialUrl));
-                Assert.assertFalse(delegate.shouldInterceptNavigation(
-                        externalNavHandler, redirectedNavigationHandle, redirectUrl));
+                Assert.assertFalse(mPanel.getOverlayPanelContent()
+                                           .getInterceptNavigationDelegateForTesting()
+                                           .shouldIgnoreNavigation(navigationHandle, initialUrl));
+                Assert.assertEquals(0, mActivityMonitor.getHits());
+
+                navigationHandle.didRedirect(redirectUrl, true);
+                Assert.assertTrue(mPanel.getOverlayPanelContent()
+                                          .getInterceptNavigationDelegateForTesting()
+                                          .shouldIgnoreNavigation(navigationHandle, redirectUrl));
+                Assert.assertEquals(1, mActivityMonitor.getHits());
             }
         });
-        Assert.assertEquals(1, mActivityMonitor.getHits());
+    }
+
+    /**
+     * Tests ContextualSearchManager#shouldInterceptNavigation for a case that an external
+     * navigation has a user gesture.
+     */
+    @Test
+    @SmallTest
+    @Feature({"ContextualSearch"})
+    public void testExternalNavigationWithUserGesture() throws Exception {
+        testExternalNavigationImpl(true);
     }
 
     /**
@@ -422,33 +377,31 @@ public void run() {
     @Test
     @SmallTest
     @Feature({"ContextualSearch"})
-    @ParameterAnnotations.UseMethodParameter(FeatureParamProvider.class)
-    public void testExternalNavigationWithoutUserGesture(@EnabledFeature int enabledFeature) {
-        final ExternalNavigationDelegateImpl delegate =
-                TestThreadUtils.runOnUiThreadBlockingNoException(
-                        ()
-                                -> new ExternalNavigationDelegateImpl(
-                                        sActivityTestRule.getActivity().getActivityTab()));
-        final ExternalNavigationHandler externalNavHandler =
-                new ExternalNavigationHandler(delegate);
-        GURL url = new GURL("intent://test/#Intent;scheme=test;package=com.chrome.test;end");
+    public void testExternalNavigationWithoutUserGesture() throws Exception {
+        testExternalNavigationImpl(false);
+    }
+
+    private void testExternalNavigationImpl(boolean hasGesture) throws Exception {
+        simulateResolveSearch("intelligence");
+        GURL url = new GURL(EXTERNAL_APP_URL);
         final NavigationHandle navigationHandle = new NavigationHandle(
                 0 /* nativeNavigationHandleProxy*/, url, GURL.emptyGURL() /* referrerUrl */,
                 GURL.emptyGURL() /* baseUrlForDataUrl */, true /* isInPrimaryMainFrame */,
                 false /* isSameDocument*/, true /* isRendererInitiated */,
                 null /* initiatorOrigin */, PageTransition.LINK, false /* isPost */,
-                false /* hasUserGesture */, false /* isRedirect */, true /* isExternalProtocol */,
-                0 /* navigationId */, false /* isPageActivation */, false /* isReload */);
+                hasGesture /* hasUserGesture */, false /* isRedirect */,
+                true /* isExternalProtocol */, 0 /* navigationId */, false /* isPageActivation */,
+                false /* isReload */);
 
         InstrumentationRegistry.getInstrumentation().runOnMainSync(new Runnable() {
             @Override
             public void run() {
-                sActivityTestRule.getActivity().onUserInteraction();
-                Assert.assertFalse(mManager.getOverlayContentDelegate().shouldInterceptNavigation(
-                        externalNavHandler, navigationHandle, url));
+                Assert.assertTrue(mPanel.getOverlayPanelContent()
+                                          .getInterceptNavigationDelegateForTesting()
+                                          .shouldIgnoreNavigation(navigationHandle, url));
             }
         });
-        Assert.assertEquals(0, mActivityMonitor.getHits());
+        Assert.assertEquals(hasGesture ? 1 : 0, mActivityMonitor.getHits());
     }
 
     //============================================================================================