Revert "WebLayer: Close tabs on launching external intents when instructed"

This reverts commit 0c7bb0a0.

Reason for revert: Broke the build

Original change's description:
> WebLayer: Close tabs on launching external intents when instructed
> 
> This CL augments WebLayer with closing of tabs when the external
> intent launching code asks to do so (namely, when an initial navigation
> in a Tab results in the launch of an external intent). To do so we
> invoke BrowserImpl#destroyTab(). As clients < 84 are not necessarily
> expecting the last tab to be closed by the WebLayer implementation, we
> do this only when the client is 84+.
> 
> As part of this CL we looked at hardening WebLayer against the case
> where the client calls Browser#destroyTab() from within
> TabListCallback#onTabRemoved() (which itself is received after the Tab
> destruction process has started in the implementation). It turns out
> that WebLayer will already fail fast in this case:
> Browser#onTabRemoved() sets the removed Tab's Browser to null, and
> Browser#destroyTab() raises an exception if the passed-in Tab's Browser
> is not equal to |this|.
> 
> Bug: 1031465
> Change-Id: I87e1464d3be4b0fe138ab7aa14fe700b8b930722
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2144120
> Commit-Queue: Colin Blundell <blundell@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Bo <boliu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#759969}

TBR=sky@chromium.org,boliu@chromium.org,blundell@chromium.org

Change-Id: Icf90ed217d0586b22fe3f55452c0ab6878d57118
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1031465
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152809Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759976}

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/ExternalNavigationTest.java

@@ -21,7 +21,6 @@ import org.chromium.base.test.util.CallbackHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.weblayer.Browser;
 import org.chromium.weblayer.Tab;
-import org.chromium.weblayer.TabListCallback;
 import org.chromium.weblayer.shell.InstrumentationActivity;
 
 /**
@@ -203,8 +202,7 @@ public class ExternalNavigationTest {
 
     /**
      * Tests that clicking on a link that goes to an external intent in a new tab results in
-     * a new tab being opened whose URL is that of the intent and the intent being launched,
-     * followed by the new tab being closed.
+     * a new tab being opened whose URL is that of the intent and the intent being launched.
      */
     @Test
     @SmallTest
@@ -217,26 +215,8 @@ public class ExternalNavigationTest {
 
         mActivityTestRule.navigateAndWait(url);
 
-        // Set up listening for the tab addition and removal that we expect to happen.
-        CallbackHelper onTabAddedCallbackHelper = new CallbackHelper();
-        CallbackHelper onTabRemovedCallbackHelper = new CallbackHelper();
-        TabListCallback tabListCallback = new TabListCallback() {
-            @Override
-            public void onTabAdded(Tab tab) {
-                onTabAddedCallbackHelper.notifyCalled();
-            }
-
-            @Override
-            public void onTabRemoved(Tab tab) {
-                onTabRemovedCallbackHelper.notifyCalled();
-            }
-        };
-        Browser browser = mActivityTestRule.getActivity().getBrowser();
-        TestThreadUtils.runOnUiThreadBlocking(
-                () -> { browser.registerTabListCallback(tabListCallback); });
-
-        // Grab the original tab before it changes.
-        Tab originalTab = mActivityTestRule.getActivity().getTab();
+        // Grab the existing tab before causing a new one to be opened.
+        Tab tab = mActivityTestRule.getActivity().getTab();
 
         mActivityTestRule.executeScriptSync(
                 "document.onclick = function() {document.getElementById('link').click()}",
@@ -244,30 +224,23 @@ public class ExternalNavigationTest {
         EventUtils.simulateTouchCenterOfView(
                 mActivityTestRule.getActivity().getWindow().getDecorView());
 
-        // (1) A new tab should be created...
-        onTabAddedCallbackHelper.waitForFirst();
-
-        // (2) The intent should be launched in that tab...
         intentInterceptor.waitForIntent();
+
+        // The current URL should not have changed in the existing tab, and the intent should have
+        // been launched.
+        Assert.assertEquals(url, mActivityTestRule.getLastCommittedUrlInTab(tab));
         Intent intent = intentInterceptor.mLastIntent;
         Assert.assertNotNull(intent);
         Assert.assertEquals(INTENT_TO_CHROME_PACKAGE, intent.getPackage());
         Assert.assertEquals(INTENT_TO_CHROME_ACTION, intent.getAction());
         Assert.assertEquals(INTENT_TO_CHROME_DATA_STRING, intent.getDataString());
 
-        // (3) And finally the new tab should be closed.
-        onTabRemovedCallbackHelper.waitForFirst();
-
-        // Now the original tab should be all that's left in the browser, with the display URL being
-        // the original URL.
+        // A new tab should have been created whose URL is that of the intent.
+        Browser browser = mActivityTestRule.getActivity().getBrowser();
         int numTabs =
                 TestThreadUtils.runOnUiThreadBlocking(() -> { return browser.getTabs().size(); });
-        Assert.assertEquals(1, numTabs);
-        Assert.assertEquals(mActivityTestRule.getActivity().getTab(), originalTab);
-        Assert.assertEquals(url, mActivityTestRule.getCurrentDisplayUrl());
-
-        TestThreadUtils.runOnUiThreadBlocking(
-                () -> { browser.unregisterTabListCallback(tabListCallback); });
+        Assert.assertEquals(2, numTabs);
+        Assert.assertEquals(INTENT_TO_CHROME_URL, mActivityTestRule.getCurrentDisplayUrl());
     }
 
     /**

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/InstrumentationActivityTestRule.java

@@ -31,6 +31,7 @@ import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.net.test.EmbeddedTestServerRule;
 import org.chromium.weblayer.CookieManager;
+import org.chromium.weblayer.NavigationController;
 import org.chromium.weblayer.Tab;
 import org.chromium.weblayer.WebLayer;
 import org.chromium.weblayer.shell.InstrumentationActivity;
@@ -262,6 +263,17 @@ public class InstrumentationActivityTestRule extends ActivityTestRule<Instrument
         return getTestServer().getURL("/weblayer/test/data/" + path);
     }
 
+    // Returns the display URL of the last committed navigation entry in |tab|. Note that this will
+    // return an empty URL if there have been no committed navigations in |tab|.
+    public String getLastCommittedUrlInTab(Tab tab) {
+        return TestThreadUtils.runOnUiThreadBlockingNoException(() -> {
+            NavigationController navController = tab.getNavigationController();
+            return navController
+                    .getNavigationEntryDisplayUri(navController.getNavigationListCurrentIndex())
+                    .toString();
+        });
+    }
+
     // Returns the URL that is currently being displayed to the user.
     public String getCurrentDisplayUrl() {
         return getActivity().getCurrentDisplayUrl();

weblayer/browser/java/org/chromium/weblayer_private/InterceptNavigationDelegateClientImpl.java

@@ -5,7 +5,6 @@
 package org.chromium.weblayer_private;
 
 import android.app.Activity;
-import android.os.RemoteException;
 import android.os.SystemClock;
 
 import org.chromium.base.ContextUtils;
@@ -119,15 +118,7 @@ public class InterceptNavigationDelegateClientImpl implements InterceptNavigatio
 
     @Override
     public void closeTab() {
-        // Prior to 84 the client was not equipped to handle the case of WebLayer initiating the
-        // last tab being closed, so we simply short-circuit out here in that case.
-        if (WebLayerFactoryImpl.getClientMajorVersion() < 84) return;
-
-        try {
-            mTab.getBrowser().destroyTab(mTab);
-        } catch (RemoteException e) {
-            throw new RuntimeException(e);
-        }
+        // TODO(crbug.com/1031465): Bring up tab closing.
     }
 
     @Override

weblayer/shell/android/shell_apk/src/org/chromium/weblayer/shell/InstrumentationActivity.java

@@ -27,7 +27,6 @@ import androidx.fragment.app.FragmentTransaction;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.weblayer.Browser;
-import org.chromium.weblayer.NavigationController;
 import org.chromium.weblayer.NewTabCallback;
 import org.chromium.weblayer.NewTabType;
 import org.chromium.weblayer.Profile;
@@ -37,7 +36,6 @@ import org.chromium.weblayer.TabListCallback;
 import org.chromium.weblayer.UnsupportedVersionException;
 import org.chromium.weblayer.WebLayer;
 
-import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -65,8 +63,6 @@ public class InstrumentationActivity extends FragmentActivity {
     private IntentInterceptor mIntentInterceptor;
     private Bundle mSavedInstanceState;
     private TabCallback mTabCallback;
-    private TabListCallback mTabListCallback;
-    private List<Tab> mPreviousTabList = new ArrayList<>();
 
     private static boolean isJaCoCoEnabled() {
         // Nothing is set at runtime indicating jacoco is being used. This looks for the existence
@@ -198,9 +194,6 @@ public class InstrumentationActivity extends FragmentActivity {
             mTab.unregisterTabCallback(mTabCallback);
             mTabCallback = null;
         }
-        if (mTabListCallback != null) {
-            mBrowser.unregisterTabListCallback(mTabListCallback);
-        }
     }
 
     private void createWebLayerAsync() {
@@ -231,58 +224,26 @@ public class InstrumentationActivity extends FragmentActivity {
 
         mBrowser.setTopView(mTopContentsContainer);
 
-        mTabListCallback = new TabListCallback() {
-            @Override
-            public void onTabAdded(Tab tab) {
-                // The first tab can be added asynchronously with session restore enabled.
-                if (mTab == null) {
-                    setTab(tab);
-                }
-            }
-
-            @Override
-            public void onTabRemoved(Tab tab) {
-                mPreviousTabList.remove(tab);
-
-                if (mTab == tab) {
-                    Tab prevTab = null;
-                    if (!mPreviousTabList.isEmpty()) {
-                        prevTab = mPreviousTabList.remove(mPreviousTabList.size() - 1);
-                    }
-
-                    setTab(prevTab);
-                }
-            }
-        };
-
-        mBrowser.registerTabListCallback(mTabListCallback);
-
         if (mBrowser.getActiveTab() == null) {
-            // This happens with session restore enabled.
             assert mBrowser.getTabs().size() == 0;
+            // This happens with session restore enabled.
+            mBrowser.registerTabListCallback(new TabListCallback() {
+                @Override
+                public void onTabAdded(Tab tab) {
+                    if (mTab == null) {
+                        mBrowser.unregisterTabListCallback(this);
+                        setTab(tab);
+                    }
+                }
+            });
         } else {
             setTab(mBrowser.getActiveTab());
         }
     }
 
-    // Clears the state associated with |mTab| and sets |tab|, if non-null, as |mTab| and the
-    // active tab in the browser.
     private void setTab(Tab tab) {
-        if (mTab != null) {
-            mTab.unregisterTabCallback(mTabCallback);
-            mTabCallback = null;
-            mTab = null;
-        }
-
+        assert mTab == null;
         mTab = tab;
-
-        if (mTab == null) return;
-
-        // TODO(crbug.com/1066382): This will not be correct in the case where the initial
-        // navigation in |tab| was a failed navigation and there have been no more navigations since
-        // then.
-        mUrlView.setText(getLastCommittedUrlInTab(mTab));
-
         mTabCallback = new TabCallback() {
             @Override
             public void onVisibleUriChanged(Uri uri) {
@@ -294,17 +255,22 @@ public class InstrumentationActivity extends FragmentActivity {
         mTab.setNewTabCallback(new NewTabCallback() {
             @Override
             public void onNewTab(Tab newTab, @NewTabType int type) {
-                mPreviousTabList.add(mTab);
+                // NOTE: At this time there isn't a need to hang on to the previous tab as this
+                // activity doesn't support closing tabs. If needed that could be added following
+                // the implementation in WebLayerShellActivity.java.
+                mTab.unregisterTabCallback(mTabCallback);
+                mTabCallback = null;
+                mTab = null;
+
                 setTab(newTab);
+                mBrowser.setActiveTab(newTab);
             }
+
             @Override
             public void onCloseTab() {
                 assert false;
             }
         });
-
-        // Will be a no-op if this tab is already the active tab.
-        mBrowser.setActiveTab(mTab);
     }
 
     private Fragment getOrCreateBrowserFragment() {
@@ -341,16 +307,6 @@ public class InstrumentationActivity extends FragmentActivity {
         return fragment;
     }
 
-    // Returns the display URL of the last committed navigation entry in |tab|. This will
-    // return an empty URL if there have been no committed navigations in |tab|.
-    public String getLastCommittedUrlInTab(Tab tab) {
-        NavigationController navController = tab.getNavigationController();
-        int currentIndex = navController.getNavigationListCurrentIndex();
-        return currentIndex == -1
-                ? ""
-                : navController.getNavigationEntryDisplayUri(currentIndex).toString();
-    }
-
     public String getCurrentDisplayUrl() {
         return mUrlView.getText().toString();
     }