Enable some pinned tab tests for multi-instance

Bug: b:448448098
Change-Id: I6a6a6964ba88487440ff49ec49c4b0644dba66b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7050702
Reviewed-by: Bryant Chandler <[email protected]>
Commit-Queue: Dan Harrington <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1532368}

Author: harringtond

chrome/browser/glic/host/glic_api_browsertest.cc

@@ -1906,24 +1906,24 @@ IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
   ExecuteJsTest();
 }
 
-IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
+IN_PROC_BROWSER_TEST_P(GlicApiTest,
                        testPinTabsStatePersistWhenClosePanelAndReopen) {
   TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
+  NavigateTabAndOpenGlicFloating();
   const int tab_id =
       GetTabId(browser()->tab_strip_model()->GetActiveWebContents());
   RunTestSequence(AddInstrumentedTab(kSecondTab, page_url()));
 
   ExecuteJsTest({.params = base::Value(base::Value::Dict().Set(
                      "tabId", base::NumberToString(tab_id)))});
 
-  RunTestSequence(OpenGlicWindow(GlicWindowMode::kDetached,
-                                 GlicInstrumentMode::kHostAndContents));
+  RunTestSequence(OpenGlicFloatingWindow());
   ContinueJsTest();
 }
 
-IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
-                       testPinTabsStatePersistWhenClientRestarts) {
+IN_PROC_BROWSER_TEST_P(GlicApiTest, testPinTabsStatePersistWhenClientRestarts) {
   TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
+  NavigateTabAndOpenGlicFloating();
   const int tab_id =
       GetTabId(browser()->tab_strip_model()->GetActiveWebContents());
   RunTestSequence(AddInstrumentedTab(kSecondTab, page_url()));
@@ -1944,8 +1944,7 @@ IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
 IN_PROC_BROWSER_TEST_P(GlicApiTest, testPinTabsFailsWhenIncognitoWindow) {
   TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
   browser_activator().SetMode(BrowserActivator::Mode::kFirst);
-  RunTestSequence(OpenGlicWindow(GlicWindowMode::kDetached,
-                                 GlicInstrumentMode::kHostAndContents));
+  NavigateTabAndOpenGlicFloating();
 
   // Open a new incognito window.
   auto* incognito = CreateIncognitoBrowser();
@@ -1961,8 +1960,10 @@ IN_PROC_BROWSER_TEST_P(GlicApiTest, testPinTabsFailsWhenIncognitoWindow) {
            "incognitoTabId", base::NumberToString(incognito_tab_id)))});
 }
 
-IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab, testUnpinTabsFailsWhenNotPinned) {
-  TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
+IN_PROC_BROWSER_TEST_P(GlicApiTest, testUnpinTabsFailsWhenNotPinned) {
+  // TODO(bryantchandler): This segfauts on multi-instance. Fix and re-enable.
+  SKIP_TEST_FOR_MULTI_INSTANCE();
+  NavigateTabAndOpenGlicFloating();
   // Unpinning a tab that is not pinned should fail.
   const int tab_id =
       GetTabId(browser()->tab_strip_model()->GetActiveWebContents());
@@ -1972,8 +1973,10 @@ IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab, testUnpinTabsFailsWhenNotPinned) {
                      "tabId", base::NumberToString(tab_id)))});
 }
 
-IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab, testUnpinAllTabs) {
-  TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
+IN_PROC_BROWSER_TEST_P(GlicApiTest, testUnpinAllTabs) {
+  // TODO(bryantchandler): This has a UAF on multi-instance. Fix and re-enable.
+  SKIP_TEST_FOR_MULTI_INSTANCE();
+  NavigateTabAndOpenGlicFloating();
   const int tab_id =
       GetTabId(browser()->tab_strip_model()->GetActiveWebContents());
   RunTestSequence(AddInstrumentedTab(kSecondTab, page_url()));
@@ -1984,7 +1987,8 @@ IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab, testUnpinAllTabs) {
 
 IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
                        testPinTabsHaveNoEffectOnFocusedTab) {
-  TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
+  // In multi-instance, pinned tabs do have an effect on the focused tab.
+  SKIP_TEST_FOR_MULTI_INSTANCE();
   const int tab_id =
       GetTabId(browser()->tab_strip_model()->GetActiveWebContents());
   RunTestSequence(AddInstrumentedTab(kSecondTab, page_url()));
@@ -2028,9 +2032,8 @@ IN_PROC_BROWSER_TEST_P(GlicApiTest, testUnpinTabsThatNavigateInBackground) {
   ContinueJsTest();
 }
 
-IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
-                       testTabDataUpdateOnUrlChangeForPinnedTab) {
-  TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
+IN_PROC_BROWSER_TEST_P(GlicApiTest, testTabDataUpdateOnUrlChangeForPinnedTab) {
+  NavigateTabAndOpenGlicFloating();
   const int tab_id =
       GetTabId(browser()->tab_strip_model()->GetActiveWebContents());
   RunTestSequence(AddInstrumentedTab(kSecondTab, page_url()));
@@ -2046,9 +2049,9 @@ IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
   ContinueJsTest();
 }
 
-IN_PROC_BROWSER_TEST_P(GlicApiTestWithOneTab,
+IN_PROC_BROWSER_TEST_P(GlicApiTest,
                        testTabDataUpdateOnFaviconChangeForPinnedTab) {
-  TODO_SKIP_BROKEN_MULTI_INSTANCE_TEST();
+  NavigateTabAndOpenGlicFloating();
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(web_contents);

chrome/browser/glic/test_support/interactive_glic_test.h

@@ -77,8 +77,16 @@ template <typename T>
 class InteractiveGlicTestMixin : public T {
  public:
   // Determines whether this is an attached or detached Glic window.
+  // WARNING: This is no longer very meaningful, and should be replaced. These
+  // do not provide the ability to open glic as a floating window when in
+  // multi-instance mode. See the comments just below.
   enum GlicWindowMode {
+    // Opens glic by pressing the Glic button on the browser.
+    // In multi-instance, this means it will open glic as a side panel.
+    // Otherwise, glic is opened as a floating window.
     kAttached,
+    // Opens glic by calling ShowDetachedForTesting() on the window controller.
+    // There may not be a good reason for using this.
     kDetached,
   };
 

chrome/test/data/webui/glic/browser_tests/glic_api_browsertest.ts

@@ -1167,9 +1167,9 @@ class ApiTests extends ApiTestFixtureBase {
     assertDefined(this.host.getPinnedTabs);
 
     const tabId = this.testParams.tabId;
-    const focusedTabId = this.getFocusedTabId();
+    const activeTabId = this.getActiveTabId();
 
-    assertTrue(await this.host.pinTabs([focusedTabId, tabId]));
+    assertTrue(await this.host.pinTabs([activeTabId, tabId]));
     const pinnedTabsUpdates = observeSequence(this.host.getPinnedTabs());
     await pinnedTabsUpdates.waitFor((tabs) => tabs.length === 2);
 
@@ -1189,9 +1189,9 @@ class ApiTests extends ApiTestFixtureBase {
       assertDefined(this.host.getPinnedTabs);
 
       const tabId = this.testParams.tabId;
-      const focusedTabId = this.getFocusedTabId();
+      const activeTabId = this.getActiveTabId();
 
-      assertTrue(await this.host.pinTabs([focusedTabId, tabId]));
+      assertTrue(await this.host.pinTabs([activeTabId, tabId]));
       const pinnedTabsUpdates = observeSequence(this.host.getPinnedTabs());
       await pinnedTabsUpdates.waitFor((tabs) => tabs.length === 2);
     } else {
@@ -1215,7 +1215,7 @@ class ApiTests extends ApiTestFixtureBase {
     assertDefined(this.host.unpinTabs);
 
     const tabId = this.testParams.tabId;
-    const tabId2 = this.getFocusedTabId();
+    const tabId2 = this.getActiveTabId();
     // Pin both tabs.
     assertTrue(await this.host.pinTabs([tabId2, tabId]));
 
@@ -1237,7 +1237,7 @@ class ApiTests extends ApiTestFixtureBase {
     assertDefined(this.host.unpinAllTabs);
 
     const tabId = this.testParams.tabId;
-    const tabId2 = this.getFocusedTabId();
+    const tabId2 = this.getActiveTabId();
 
     // Pin both tabs.
     assertTrue(await this.host.pinTabs([tabId2, tabId]));
@@ -1313,7 +1313,7 @@ class ApiTests extends ApiTestFixtureBase {
     assertDefined(this.host.pinTabs);
 
     const tabId = this.testParams.tabId;
-    assertNotEquals(tabId, this.getFocusedTabId());
+    assertNotEquals(tabId, this.getActiveTabId());
 
     await this.host.pinTabs([tabId]);
     const pinnedTabsUpdates = observeSequence(this.host.getPinnedTabs());
@@ -1324,7 +1324,7 @@ class ApiTests extends ApiTestFixtureBase {
     await this.advanceToNextStep();
 
     // Make sure that the pinned tab is not focused.
-    assertNotEquals(tabId, this.getFocusedTabId());
+    assertNotEquals(tabId, this.getActiveTabId());
     await pinnedTabsUpdates.waitFor(
         (tabs) =>
             tabs.some(t => t.tabId === tabId && t.url.includes('changed')));
@@ -1335,7 +1335,7 @@ class ApiTests extends ApiTestFixtureBase {
     assertDefined(this.host.pinTabs);
 
     const tabId = this.testParams.tabId;
-    assertNotEquals(tabId, this.getFocusedTabId());
+    assertNotEquals(tabId, this.getActiveTabId());
 
     await this.host.pinTabs([tabId]);
     const pinnedTabsUpdates = observeSequence(this.host.getPinnedTabs());