Revert "Tab Search: Update TabSearchButton tests"

This reverts commit 0d88901a.

Reason for revert: Introduced a heap user after free in TabSearchButtonBrowserTest.TestBubbleVisible.

[my hunch is, it's just missing a call to tab_search_button()->bubble_for_testing()->Close()]

Original change's description:
> Tab Search: Update TabSearchButton tests
> 
> This CL creates a new file for TabSearchButton browser tests. These
> tests run for both the fixed and regular entrypoints.
> 
> Existing tests for the TabSearchButton have been moved from the tab
> strip into the TabSearchButton browser tests.
> 
> Bug: 1099917
> Change-Id: I0b1d57826768705316b9bed814dac7af96ce52e8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422730
> Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#809376}

TBR=sky@chromium.org,tluk@chromium.org

Change-Id: I7b90eff0efbd7a807c0f085496f75139b1961909
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1099917,1131399
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424288Reviewed-by: Tim Schumann <tschumann@chromium.org>
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809723}

chrome/browser/ui/views/frame/browser_view.cc

@@ -699,19 +699,6 @@ int BrowserView::GetTabStripHeight() const {
   return IsTabStripVisible() ? tabstrip_->GetPreferredSize().height() : 0;
 }
 
-TabSearchButton* BrowserView::GetTabSearchButton() {
-  if (!base::FeatureList::IsEnabled(features::kTabSearch))
-    return nullptr;
-
-  // If kTabSearchFixedEntrypoint is enabled then the tab search button is
-  // defined in the tab strip region view.
-  // TODO(tluk): Consolidate these once Tab Scrolling successfully moves the
-  // tab controls container to the tab strip region view.
-  return base::FeatureList::IsEnabled(features::kTabSearchFixedEntrypoint)
-             ? tab_strip_region_view_->tab_search_button()
-             : tabstrip_->tab_search_button();
-}
-
 bool BrowserView::IsTabStripVisible() const {
   // Return false if this window does not normally display a tabstrip.
   if (!browser_->SupportsWindowFeature(Browser::FEATURE_TABSTRIP))
@@ -2624,8 +2611,13 @@ void BrowserView::CreateTabSearchBubble() {
     return;
 #endif  // BUILDFLAG(ENABLE_WEBUI_TAB_STRIP)
 
-  DCHECK(GetTabSearchButton());
-  if (GetTabSearchButton()->ShowTabSearchBubble()) {
+  // If kTabSearchFixedEntrypoint is enabled then the tab search button is
+  // defined in the tab strip region view.
+  // TODO(tluk): Consolidate these once Tab Scrolling successfully moves the
+  // tab controls container to the tab strip region view.
+  if ((base::FeatureList::IsEnabled(features::kTabSearchFixedEntrypoint) &&
+       tab_strip_region_view_->tab_search_button()->ShowTabSearchBubble()) ||
+      tabstrip_->tab_search_button()->ShowTabSearchBubble()) {
     // Only log the open action if it resulted in creating a new instance of the
     // Tab Search bubble.
     base::UmaHistogramEnumeration("Tabs.TabSearch.OpenAction",

chrome/browser/ui/views/frame/browser_view.h

@@ -66,7 +66,6 @@ class FullscreenControlHost;
 class InfoBarContainerView;
 class LocationBarView;
 class StatusBubbleViews;
-class TabSearchButton;
 class TabStrip;
 class TabStripRegionView;
 class ToolbarButtonProvider;
@@ -220,9 +219,6 @@ class BrowserView : public BrowserWindow,
   // Accessor for the contents WebView.
   views::WebView* contents_web_view() { return contents_web_view_; }
 
-  // Accessor for the BrowserView's TabSearchButton instance.
-  TabSearchButton* GetTabSearchButton();
-
   // Returns true if various window components are visible.
   bool IsTabStripVisible() const;
 

chrome/browser/ui/views/tabs/tab_search_button_browsertest.cc

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/ui/views/tabs/tab_search_button.h"
-
-#include <vector>
-
-#include "base/feature_list.h"
-#include "base/test/scoped_feature_list.h"
-#include "build/build_config.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/ui_features.h"
-#include "chrome/browser/ui/views/frame/browser_view.h"
-#include "chrome/browser/ui/views/tabs/tab_search_button.h"
-#include "chrome/test/base/in_process_browser_test.h"
-#include "content/public/test/browser_test.h"
-#include "ui/base/accelerators/accelerator.h"
-
-namespace {
-ui::MouseEvent GetDummyEvent() {
-  return ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::PointF(), gfx::PointF(),
-                        base::TimeTicks::Now(), 0, 0);
-}
-}  // namespace
-
-class TabSearchButtonBrowserTest : public InProcessBrowserTest,
-                                   public ::testing::WithParamInterface<bool> {
- public:
-  // InProcessBrowserTest:
-  void SetUp() override {
-    // Run the test with both kTabSearchFixedEntrypoint enabled and disabled.
-    if (GetParam()) {
-      scoped_feature_list_.InitWithFeatures(
-          /*enabled_features=*/{features::kTabSearch,
-                                features::kTabSearchFixedEntrypoint},
-          /*disabled_features=*/{});
-    } else {
-      scoped_feature_list_.InitWithFeatures(
-          /*enabled_features=*/{features::kTabSearch},
-          /*disabled_features=*/{features::kTabSearchFixedEntrypoint});
-    }
-    InProcessBrowserTest::SetUp();
-  }
-
-  BrowserView* browser_view() {
-    return BrowserView::GetBrowserViewForBrowser(browser());
-  }
-
-  TabSearchButton* tab_search_button() {
-    return browser_view()->GetTabSearchButton();
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-};
-
-IN_PROC_BROWSER_TEST_P(TabSearchButtonBrowserTest, CreateAndClose) {
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
-  tab_search_button()->ButtonPressed(tab_search_button(), GetDummyEvent());
-  ASSERT_NE(nullptr, tab_search_button()->bubble_for_testing());
-
-  // Close the tab search bubble widget, the bubble should be cleared from the
-  // TabSearchButton.
-  tab_search_button()->bubble_for_testing()->CloseWithReason(
-      views::Widget::ClosedReason::kUnspecified);
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
-}
-
-IN_PROC_BROWSER_TEST_P(TabSearchButtonBrowserTest, TestBubbleVisible) {
-  EXPECT_FALSE(tab_search_button()->IsBubbleVisible());
-
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
-  tab_search_button()->ButtonPressed(tab_search_button(), GetDummyEvent());
-  ASSERT_NE(nullptr, tab_search_button()->bubble_for_testing());
-
-  // The bubble should not be visible initially since the UI must notify the
-  // bubble it is ready before the bubble is shown.
-  EXPECT_FALSE(tab_search_button()->IsBubbleVisible());
-
-  // Trigger showing the bubble.
-  tab_search_button()->bubble_for_testing()->Show();
-
-  // The bubble should be visible after being shown.
-  EXPECT_TRUE(tab_search_button()->IsBubbleVisible());
-}
-
-// On macOS, most accelerators are handled by CommandDispatcher.
-#if !defined(OS_MAC)
-IN_PROC_BROWSER_TEST_P(TabSearchButtonBrowserTest, TestBubbleKeyboardShortcut) {
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
-
-  auto accelerator = ui::Accelerator(
-      ui::VKEY_A, ui::EF_SHIFT_DOWN | ui::EF_PLATFORM_ACCELERATOR);
-  browser_view()->AcceleratorPressed(accelerator);
-
-  // Accelerator keys should have created the tab search bubble.
-  ASSERT_NE(nullptr, tab_search_button()->bubble_for_testing());
-
-  // Close the tab search bubble widget, the bubble should be cleared from the
-  // TabSearchButton.
-  tab_search_button()->bubble_for_testing()->CloseWithReason(
-      views::Widget::ClosedReason::kUnspecified);
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
-}
-#endif
-
-INSTANTIATE_TEST_SUITE_P(All,
-                         TabSearchButtonBrowserTest,
-                         ::testing::Values(true, false));

chrome/browser/ui/views/tabs/tab_strip_browsertest.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/ui_features.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/tabs/tab_search_button.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/tab_groups/tab_group_id.h"
@@ -812,3 +813,28 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
   tab_strip()->SelectTab(tab_strip()->tab_at(0), GetDummyEvent());
   EXPECT_FALSE(tab_strip()->controller()->IsGroupCollapsed(group));
 }
+
+class TabSearchButtonTest : public TabStripBrowsertest {
+ public:
+  void SetUp() override {
+    scoped_feature_list_.InitWithFeatureState(features::kTabSearch, true);
+    TabStripBrowsertest::SetUp();
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(TabSearchButtonTest, TabSearchBubble_CreateAndClose) {
+  TabSearchButton* tab_search_button = tab_strip()->tab_search_button();
+
+  DCHECK_EQ(nullptr, tab_search_button->bubble_for_testing());
+  tab_search_button->ButtonPressed(tab_search_button, GetDummyEvent());
+  DCHECK_NE(nullptr, tab_search_button->bubble_for_testing());
+
+  // Close the tab search bubble widget, the bubble should be cleared from the
+  // TabSearchButton.
+  tab_search_button->bubble_for_testing()->CloseWithReason(
+      views::Widget::ClosedReason::kUnspecified);
+  DCHECK_EQ(nullptr, tab_search_button->bubble_for_testing());
+}

chrome/test/BUILD.gn

@@ -1454,7 +1454,6 @@ if (!is_android) {
       "../browser/ui/views/sharing/shared_clipboard_browsertest.cc",
       "../browser/ui/views/sharing/sharing_browsertest.cc",
       "../browser/ui/views/sharing/sharing_browsertest.h",
-      "../browser/ui/views/tabs/tab_search_button_browsertest.cc",
       "../browser/ui/views/tabs/tab_strip_browsertest.cc",
       "../browser/ui/views/try_chrome_dialog_win/try_chrome_dialog_browsertest.cc",
       "../browser/ui/views/web_apps/web_app_frame_toolbar_browsertest.cc",