[WebLayer] Make tab handling consistent between native and Java

There were a few inconsistencies between native and Java handling of
Tabs which made writing browsertests that work on all platforms
difficult if any tabs had to be created. This change fixes a few things:

 - Previously native code allowed tabs not attached to a browser, while
   Java required tabs to be attached to a browser. Now all tabs must be
   attached to a browser whether created in C++ or Java.
 - Tabs created with Tab::Create() in C++ would not work correctly if a
   test was running on Android. This is now fixed by forcing all tab
   creation to go through Browser::CreateTab().
 - Adds a Browser.createTab() API in both Java and C++.
 - Removes Browser::RemoveTab() in favor of Browser::DestroyTab() to
   match the Java API.

Bug: 1093899
Change-Id: I98de4715b58eb8cf601610d4eb676bb2bd9875af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2242211
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777593}

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

@@ -15,7 +15,9 @@
 
 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;
 
 import java.util.HashMap;
@@ -212,4 +214,30 @@ public void testSetDataMaxSize() {
             Assert.fail("Expected IllegalArgumentException.");
         });
     }
+
+    @Test
+    @SmallTest
+    @MinWebLayerVersion(85)
+    public void testCreateTab() throws Exception {
+        mActivity = mActivityTestRule.launchShellWithUrl("about:blank");
+        CallbackHelper helper = new CallbackHelper();
+        Tab tab = TestThreadUtils.runOnUiThreadBlocking(() -> {
+            Browser browser = mActivity.getBrowser();
+            browser.registerTabListCallback(new TabListCallback() {
+                @Override
+                public void onTabAdded(Tab tab) {
+                    helper.notifyCalled();
+                }
+            });
+            Tab newTab = mActivity.getBrowser().createTab();
+            Assert.assertEquals(mActivity.getBrowser().getTabs().size(), 2);
+            Assert.assertNotEquals(newTab, mActivity.getTab());
+            return newTab;
+        });
+        helper.waitForFirst();
+
+        // Make sure the new tab can navigate correctly.
+        mActivityTestRule.navigateAndWait(
+                tab, mActivityTestRule.getTestDataURL("simple_page.html"), false);
+    }
 }

weblayer/browser/browser_impl.cc

@@ -72,7 +72,7 @@ BrowserImpl::~BrowserImpl() {
   DCHECK(tabs_.empty());
 #else
   while (!tabs_.empty())
-    RemoveTab(tabs_.back().get());
+    DestroyTab(tabs_.back().get());
 #endif
   base::Erase(GetBrowsers(), this);
 
@@ -93,12 +93,15 @@ TabImpl* BrowserImpl::CreateTabForSessionRestore(
   std::unique_ptr<TabImpl> tab =
       std::make_unique<TabImpl>(profile_, std::move(web_contents), guid);
 #if defined(OS_ANDROID)
-  Java_BrowserImpl_createTabForSessionRestore(
+  Java_BrowserImpl_createJavaTabForNativeTab(
       AttachCurrentThread(), java_impl_, reinterpret_cast<jlong>(tab.get()));
 #endif
-  TabImpl* tab_ptr = tab.get();
-  AddTab(std::move(tab));
-  return tab_ptr;
+  return AddTab(std::move(tab));
+}
+
+TabImpl* BrowserImpl::CreateTab(
+    std::unique_ptr<content::WebContents> web_contents) {
+  return CreateTabForSessionRestore(std::move(web_contents), std::string());
 }
 
 #if defined(OS_ANDROID)
@@ -109,13 +112,7 @@ bool BrowserImpl::CompositorHasSurface() {
 
 void BrowserImpl::AddTab(JNIEnv* env,
                          long native_tab) {
-  TabImpl* tab = reinterpret_cast<TabImpl*>(native_tab);
-  std::unique_ptr<Tab> owned_tab;
-  if (tab->browser())
-    owned_tab = tab->browser()->RemoveTab(tab);
-  else
-    owned_tab.reset(tab);
-  AddTab(std::move(owned_tab));
+  AddTab(reinterpret_cast<TabImpl*>(native_tab));
 }
 
 void BrowserImpl::RemoveTab(JNIEnv* env,
@@ -236,41 +233,19 @@ void BrowserImpl::SetWebPreferences(content::WebPreferences* prefs) {
 #endif
 }
 
-Tab* BrowserImpl::AddTab(std::unique_ptr<Tab> tab) {
+void BrowserImpl::AddTab(Tab* tab) {
   DCHECK(tab);
-  TabImpl* tab_impl = static_cast<TabImpl*>(tab.get());
-  DCHECK(!tab_impl->browser());
-  tabs_.push_back(std::move(tab));
-  tab_impl->set_browser(this);
-#if defined(OS_ANDROID)
-  Java_BrowserImpl_onTabAdded(AttachCurrentThread(), java_impl_,
-                              tab_impl->GetJavaTab());
-#endif
-  for (BrowserObserver& obs : browser_observers_)
-    obs.OnTabAdded(tab_impl);
-  return tab_impl;
-}
-
-std::unique_ptr<Tab> BrowserImpl::RemoveTab(Tab* tab) {
   TabImpl* tab_impl = static_cast<TabImpl*>(tab);
-  DCHECK_EQ(this, tab_impl->browser());
-  static_cast<TabImpl*>(tab)->set_browser(nullptr);
-  auto iter =
-      std::find_if(tabs_.begin(), tabs_.end(), base::MatchesUniquePtr(tab));
-  DCHECK(iter != tabs_.end());
-  std::unique_ptr<Tab> owned_tab = std::move(*iter);
-  tabs_.erase(iter);
-  const bool active_tab_changed = active_tab_ == tab;
-  if (active_tab_changed)
-    SetActiveTab(nullptr);
+  std::unique_ptr<Tab> owned_tab;
+  if (tab_impl->browser())
+    owned_tab = tab_impl->browser()->RemoveTab(tab_impl);
+  else
+    owned_tab.reset(tab_impl);
+  AddTab(std::move(owned_tab));
+}
 
-#if defined(OS_ANDROID)
-  Java_BrowserImpl_onTabRemoved(AttachCurrentThread(), java_impl_,
-                                tab ? tab_impl->GetJavaTab() : nullptr);
-#endif
-  for (BrowserObserver& obs : browser_observers_)
-    obs.OnTabRemoved(tab, active_tab_changed);
-  return owned_tab;
+void BrowserImpl::DestroyTab(Tab* tab) {
+  RemoveTab(tab);
 }
 
 void BrowserImpl::SetActiveTab(Tab* tab) {
@@ -304,6 +279,10 @@ std::vector<Tab*> BrowserImpl::GetTabs() {
   return tabs;
 }
 
+Tab* BrowserImpl::CreateTab() {
+  return CreateTab(nullptr);
+}
+
 void BrowserImpl::PrepareForShutdown() {
   browser_persister_.reset();
 }
@@ -325,6 +304,16 @@ void BrowserImpl::RemoveObserver(BrowserObserver* observer) {
   browser_observers_.RemoveObserver(observer);
 }
 
+void BrowserImpl::VisibleSecurityStateOfActiveTabChanged() {
+  if (visible_security_state_changed_callback_for_tests_)
+    std::move(visible_security_state_changed_callback_for_tests_).Run();
+
+#if defined(OS_ANDROID)
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_BrowserImpl_onVisibleSecurityStateOfActiveTabChanged(env, java_impl_);
+#endif
+}
+
 BrowserImpl::BrowserImpl(ProfileImpl* profile) : profile_(profile) {
   GetBrowsers().push_back(this);
 }
@@ -340,14 +329,40 @@ void BrowserImpl::RestoreStateIfNecessary(
   }
 }
 
-void BrowserImpl::VisibleSecurityStateOfActiveTabChanged() {
-  if (visible_security_state_changed_callback_for_tests_)
-    std::move(visible_security_state_changed_callback_for_tests_).Run();
+TabImpl* BrowserImpl::AddTab(std::unique_ptr<Tab> tab) {
+  TabImpl* tab_impl = static_cast<TabImpl*>(tab.get());
+  DCHECK(!tab_impl->browser());
+  tabs_.push_back(std::move(tab));
+  tab_impl->set_browser(this);
+#if defined(OS_ANDROID)
+  Java_BrowserImpl_onTabAdded(AttachCurrentThread(), java_impl_,
+                              tab_impl->GetJavaTab());
+#endif
+  for (BrowserObserver& obs : browser_observers_)
+    obs.OnTabAdded(tab_impl);
+  return tab_impl;
+}
+
+std::unique_ptr<Tab> BrowserImpl::RemoveTab(Tab* tab) {
+  TabImpl* tab_impl = static_cast<TabImpl*>(tab);
+  DCHECK_EQ(this, tab_impl->browser());
+  static_cast<TabImpl*>(tab)->set_browser(nullptr);
+  auto iter =
+      std::find_if(tabs_.begin(), tabs_.end(), base::MatchesUniquePtr(tab));
+  DCHECK(iter != tabs_.end());
+  std::unique_ptr<Tab> owned_tab = std::move(*iter);
+  tabs_.erase(iter);
+  const bool active_tab_changed = active_tab_ == tab;
+  if (active_tab_changed)
+    SetActiveTab(nullptr);
 
 #if defined(OS_ANDROID)
-  JNIEnv* env = base::android::AttachCurrentThread();
-  Java_BrowserImpl_onVisibleSecurityStateOfActiveTabChanged(env, java_impl_);
+  Java_BrowserImpl_onTabRemoved(AttachCurrentThread(), java_impl_,
+                                tab ? tab_impl->GetJavaTab() : nullptr);
 #endif
+  for (BrowserObserver& obs : browser_observers_)
+    obs.OnTabRemoved(tab, active_tab_changed);
+  return owned_tab;
 }
 
 base::FilePath BrowserImpl::GetBrowserPersisterDataPath() {

weblayer/browser/browser_impl.h

@@ -52,6 +52,7 @@ class BrowserImpl : public Browser {
   TabImpl* CreateTabForSessionRestore(
       std::unique_ptr<content::WebContents> web_contents,
       const std::string& guid);
+  TabImpl* CreateTab(std::unique_ptr<content::WebContents> web_contents);
 
 #if defined(OS_ANDROID)
   bool CompositorHasSurface();
@@ -95,11 +96,12 @@ class BrowserImpl : public Browser {
   void SetWebPreferences(content::WebPreferences* prefs);
 
   // Browser:
-  Tab* AddTab(std::unique_ptr<Tab> tab) override;
-  std::unique_ptr<Tab> RemoveTab(Tab* tab) override;
+  void AddTab(Tab* tab) override;
+  void DestroyTab(Tab* tab) override;
   void SetActiveTab(Tab* tab) override;
   Tab* GetActiveTab() override;
   std::vector<Tab*> GetTabs() override;
+  Tab* CreateTab() override;
   void PrepareForShutdown() override;
   std::string GetPersistenceId() override;
   std::vector<uint8_t> GetMinimalPersistenceState() override;
@@ -121,6 +123,9 @@ class BrowserImpl : public Browser {
 
   void RestoreStateIfNecessary(const PersistenceInfo& persistence_info);
 
+  TabImpl* AddTab(std::unique_ptr<Tab> tab);
+  std::unique_ptr<Tab> RemoveTab(Tab* tab);
+
   // Returns the path used by |browser_persister_|.
   base::FilePath GetBrowserPersisterDataPath();
 

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

@@ -194,6 +194,13 @@ public void setBottomView(IObjectWrapper viewWrapper) {
         getViewController().setBottomView(ObjectWrapper.unwrap(viewWrapper, View.class));
     }
 
+    @Override
+    public TabImpl createTab() {
+        TabImpl tab = new TabImpl(mProfile, mWindowAndroid);
+        addTab(tab);
+        return tab;
+    }
+
     @Override
     public void setSupportsEmbedding(boolean enable, IObjectWrapper valueCallback) {
         StrictModeWorkaround.apply();
@@ -237,7 +244,7 @@ public void addTab(ITab iTab) {
     }
 
     @CalledByNative
-    private void createTabForSessionRestore(long nativeTab) {
+    private void createJavaTabForNativeTab(long nativeTab) {
         new TabImpl(mProfile, mWindowAndroid, nativeTab);
     }
 
@@ -395,10 +402,8 @@ public void setClient(IBrowserClient client) {
             updateAllTabsAndSetActive();
         } else if (persistenceInfo.mPersistenceId == null
                 || persistenceInfo.mPersistenceId.isEmpty()) {
-            TabImpl tab = new TabImpl(mProfile, mWindowAndroid);
-            addTab(tab);
-            boolean set_active_result = setActiveTab(tab);
-            assert set_active_result;
+            boolean setActiveResult = setActiveTab(createTab());
+            assert setActiveResult;
         } // else case is session restore, which will asynchronously create tabs.
     }
 
@@ -411,7 +416,6 @@ public void destroyTab(ITab iTab) {
     }
 
     private void destroyTabImpl(TabImpl tab) {
-        BrowserImplJni.get().removeTab(mNativeBrowser, tab.getNativeTab());
         tab.destroy();
     }
 

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

@@ -48,12 +48,10 @@ private static int implTypeToJavaType(@ImplNewTabType int type) {
     }
 
     @CalledByNative
-    public void onNewTab(long nativeTab, @ImplNewTabType int mode) throws RemoteException {
+    public void onNewTab(TabImpl tab, @ImplNewTabType int mode) throws RemoteException {
         // This class should only be created while the tab is attached to a fragment.
         assert mTab.getBrowser() != null;
-        TabImpl tab =
-                new TabImpl(mTab.getProfile(), mTab.getBrowser().getWindowAndroid(), nativeTab);
-        mTab.getBrowser().addTab(tab);
+        assert mTab.getBrowser().equals(tab.getBrowser());
         mTab.getClient().onNewTab(tab.getId(), mode);
     }
 

weblayer/browser/java/org/chromium/weblayer_private/interfaces/IBrowser.aidl

@@ -34,4 +34,6 @@ interface IBrowser {
   IUrlBarController getUrlBarController() = 9;
 
   void setBottomView(in IObjectWrapper view) = 10;
+
+  ITab createTab() = 11;
 }

weblayer/browser/new_tab_callback_proxy.cc

@@ -23,12 +23,12 @@ NewTabCallbackProxy::~NewTabCallbackProxy() {
   tab_->SetNewTabDelegate(nullptr);
 }
 
-void NewTabCallbackProxy::OnNewTab(std::unique_ptr<Tab> tab, NewTabType type) {
+void NewTabCallbackProxy::OnNewTab(Tab* tab, NewTabType type) {
   JNIEnv* env = AttachCurrentThread();
   // The Java side takes ownership of Tab.
   TRACE_EVENT0("weblayer", "Java_NewTabCallbackProxy_onNewTab");
   Java_NewTabCallbackProxy_onNewTab(env, java_impl_,
-                                    reinterpret_cast<jlong>(tab.release()),
+                                    static_cast<TabImpl*>(tab)->GetJavaTab(),
                                     static_cast<int>(type));
 }
 

weblayer/browser/new_tab_callback_proxy.h

@@ -23,7 +23,7 @@ class NewTabCallbackProxy : public NewTabDelegate {
   ~NewTabCallbackProxy() override;
 
   // NewTabDelegate:
-  void OnNewTab(std::unique_ptr<Tab> tab, NewTabType type) override;
+  void OnNewTab(Tab* tab, NewTabType type) override;
   void CloseTab() override;
 
  private: