status-bubble: delays destruction for 10 seconds

Perf data indicates deleting the bubble can be expensive. This is because
the deletion of a top-level widget triggers synchronous operations with viz.
During startup there may be lots of things happening in viz, so, delay deletion
for 10 seconds in hopes of things having settled down.

BUG=943268
TEST=covered by tests

Change-Id: Id1d56997553b01f9cf64a3866c013f20d0b75c86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1532583
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642715}
diff --git a/chrome/browser/ui/views/status_bubble_views.cc b/chrome/browser/ui/views/status_bubble_views.cc
index 45d2c50..e29783f 100644
--- a/chrome/browser/ui/views/status_bubble_views.cc
+++ b/chrome/browser/ui/views/status_bubble_views.cc
@@ -14,6 +14,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "cc/paint/paint_flags.h"
 #include "chrome/browser/themes/theme_properties.h"
@@ -76,6 +77,11 @@
 constexpr auto kMaxExpansionStepDuration =
     base::TimeDelta::FromMilliseconds(150);
 
+#if !defined(OS_MACOSX)
+// How long to delay before destroying an unused status bubble widget.
+constexpr auto kDestroyPopupDelay = base::TimeDelta::FromSeconds(10);
+#endif
+
 const gfx::FontList& GetFont() {
   return views::style::GetFont(views::style::CONTEXT_LABEL,
                                views::style::STYLE_PRIMARY);
@@ -164,6 +170,12 @@
 
   void SetWidth(int new_width);
 
+  gfx::Animation* animation() { return animation_.get(); }
+
+  bool IsDestroyPopupTimerRunning() const {
+    return destroy_popup_timer_.IsRunning();
+  }
+
  private:
   class InitialTimer;
 
@@ -195,6 +207,11 @@
 
   gfx::Size popup_size_;
 
+  // A timer used to delay destruction of the popup widget. This is meant to
+  // balance the performance tradeoffs of rapid creation/destruction and the
+  // memory savings of closing the widget when it's hidden and unused.
+  base::OneShotTimer destroy_popup_timer_;
+
   base::WeakPtrFactory<StatusBubbleViews::StatusView> timer_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(StatusView);
@@ -237,6 +254,7 @@
     GetWidget()->ShowInactive();
 #else
   GetWidget()->ShowInactive();
+  destroy_popup_timer_.Stop();
 #endif
 }
 
@@ -250,7 +268,13 @@
   // Don't orderOut: the window on macOS. Doing so for a child window requires
   // it to be detached/reattached, which may trigger a space switch. Instead,
   // just leave the window fully transparent and unclickable.
-  status_bubble_->DestroyPopup();
+  GetWidget()->Hide();
+  destroy_popup_timer_.Stop();
+  // This isn't done in the constructor as tests may change the task runner
+  // after the fact.
+  destroy_popup_timer_.SetTaskRunner(status_bubble_->task_runner_);
+  destroy_popup_timer_.Start(FROM_HERE, kDestroyPopupDelay, status_bubble_,
+                             &StatusBubbleViews::DestroyPopup);
 #endif
 }
 
@@ -258,7 +282,7 @@
   if (timer_factory_.HasWeakPtrs())
     timer_factory_.InvalidateWeakPtrs();
 
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+  status_bubble_->task_runner_->PostDelayedTask(
       FROM_HERE,
       base::BindOnce(&StatusBubbleViews::StatusView::OnTimer,
                      timer_factory_.GetWeakPtr()),
@@ -320,6 +344,10 @@
 }
 
 void StatusBubbleViews::StatusView::StartShowing() {
+#if !defined(OS_MACOSX)
+  destroy_popup_timer_.Stop();
+#endif
+
   if (state_ == BUBBLE_HIDDEN) {
     GetWidget()->ShowInactive();
     state_ = BUBBLE_SHOWING_TIMER;
@@ -597,7 +625,8 @@
 const int StatusBubbleViews::kShadowThickness = 1;
 
 StatusBubbleViews::StatusBubbleViews(views::View* base_view)
-    : base_view_(base_view) {}
+    : base_view_(base_view),
+      task_runner_(base::ThreadTaskRunnerHandle::Get().get()) {}
 
 StatusBubbleViews::~StatusBubbleViews() {
   DestroyPopup();
@@ -746,7 +775,7 @@
       ExpandBubble();
     } else if (url_formatter::FormatUrl(url).length() >
                url_text_.length()) {
-      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      task_runner_->PostDelayedTask(
           FROM_HERE,
           base::BindOnce(&StatusBubbleViews::ExpandBubble,
                          expand_timer_factory_.GetWeakPtr()),
@@ -792,6 +821,7 @@
 }
 
 void StatusBubbleViews::AvoidMouse(const gfx::Point& location) {
+  DCHECK(view_);
   // Get the position of the frame.
   gfx::Point top_left;
   views::View::ConvertPointToScreen(base_view_, &top_left);
@@ -930,3 +960,11 @@
   if (expand_timer_factory_.HasWeakPtrs())
     expand_timer_factory_.InvalidateWeakPtrs();
 }
+
+gfx::Animation* StatusBubbleViews::GetShowHideAnimationForTest() {
+  return view_ ? view_->animation() : nullptr;
+}
+
+bool StatusBubbleViews::IsDestroyPopupTimerRunningForTest() {
+  return view_ && view_->IsDestroyPopupTimerRunning();
+}
diff --git a/chrome/browser/ui/views/status_bubble_views.h b/chrome/browser/ui/views/status_bubble_views.h
index a5df98a8..60e3dc2 100644
--- a/chrome/browser/ui/views/status_bubble_views.h
+++ b/chrome/browser/ui/views/status_bubble_views.h
@@ -15,7 +15,11 @@
 #include "ui/gfx/geometry/rect.h"
 #include "url/gurl.h"
 
+namespace base {
+class SequencedTaskRunner;
+}
 namespace gfx {
+class Animation;
 class Point;
 }
 namespace views {
@@ -108,6 +112,9 @@
   // Set the bounds of the bubble relative to |base_view_|.
   void SetBounds(int x, int y, int w, int h);
 
+  gfx::Animation* GetShowHideAnimationForTest();
+  bool IsDestroyPopupTimerRunningForTest();
+
   // The status text we want to display when there are no URLs to display.
   base::string16 status_text_;
 
@@ -132,8 +139,8 @@
   // How vertically offset the bubble is from its root position_.
   int offset_ = 0;
 
-  // We use a HWND for the popup so that it may float above any HWNDs in our
-  // UI (the location bar, for example).
+  // Use a Widget for the popup so that it floats above all content as well as
+  // going outside the bounds of the hosting widget.
   std::unique_ptr<views::Widget> popup_;
 
   views::View* base_view_;
@@ -149,6 +156,11 @@
   // change size immediately, with no hover.
   bool is_expanded_ = false;
 
+  // Used for posting tasks. This is typically
+  // base::ThreadTaskRunnerHandle::Get(), but may be set to something else for
+  // tests.
+  base::SequencedTaskRunner* task_runner_;
+
   // Times expansion of status bubble when URL is too long for standard width.
   base::WeakPtrFactory<StatusBubbleViews> expand_timer_factory_{this};
 
diff --git a/chrome/browser/ui/views/status_bubble_views_browsertest.cc b/chrome/browser/ui/views/status_bubble_views_browsertest.cc
index 1c97efd..2972d10 100644
--- a/chrome/browser/ui/views/status_bubble_views_browsertest.cc
+++ b/chrome/browser/ui/views/status_bubble_views_browsertest.cc
@@ -6,23 +6,39 @@
 
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/views/status_bubble_views_browsertest_mac.h"
 #include "chrome/test/base/in_process_browser_test.h"
-#include "ui/views/test/widget_test.h"
+#include "ui/gfx/animation/animation.h"
 #include "ui/views/widget/widget.h"
 
 class StatusBubbleViewsTest : public InProcessBrowserTest {
  public:
-  StatusBubble* GetBubble() { return browser()->window()->GetStatusBubble(); }
-  views::Widget* GetWidget() {
-    return static_cast<StatusBubbleViews*>(GetBubble())->popup();
+  StatusBubbleViews* GetBubble() {
+    return static_cast<StatusBubbleViews*>(
+        browser()->window()->GetStatusBubble());
+  }
+  views::Widget* GetWidget() { return GetBubble()->popup(); }
+  bool IsDestroyPopupTimerRunning() {
+    return GetBubble()->IsDestroyPopupTimerRunningForTest();
+  }
+  gfx::Animation* GetShowHideAnimationForTesting() {
+    return GetBubble()->GetShowHideAnimationForTest();
+  }
+  void SetTaskRunner(base::SequencedTaskRunner* task_runner) {
+    GetBubble()->task_runner_ = task_runner;
   }
 };
 
 IN_PROC_BROWSER_TEST_F(StatusBubbleViewsTest, WidgetLifetime) {
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner =
+      base::MakeRefCounted<base::TestSimpleTaskRunner>();
+  SetTaskRunner(task_runner.get());
+
   // The widget does not exist until it needs to be shown.
   StatusBubble* bubble = GetBubble();
   ASSERT_TRUE(bubble);
@@ -44,14 +60,77 @@
 
 #if !defined(OS_MACOSX)
   // Clearing the URL and status closes the widget on platforms other than Mac.
-  views::test::WidgetClosingObserver widget_closing_observer(widget);
+  EXPECT_FALSE(IsDestroyPopupTimerRunning());
   bubble->SetStatus(base::string16());
   bubble->SetURL(GURL());
-  widget_closing_observer.Wait();
-  EXPECT_TRUE(widget_closing_observer.widget_closed());
+  // The widget is not hidden immediately, instead a task is scheduled. Run that
+  // now.
+  task_runner->RunPendingTasks();
+  // After the task, a timer is created that animates hidden. Advance that.
+  ASSERT_TRUE(GetShowHideAnimationForTesting());
+  // Advance well past the time for the animation to ensure it completes.
+  static_cast<gfx::AnimationContainerElement*>(GetShowHideAnimationForTesting())
+      ->Step(base::TimeTicks::Now() + base::TimeDelta::FromMinutes(1));
+  // Widget should still exist.
+  ASSERT_TRUE(GetWidget());
+  EXPECT_FALSE(widget->IsVisible());
+  EXPECT_TRUE(IsDestroyPopupTimerRunning());
+
+  // Run until idle, which should trigger deleting the widget.
+  task_runner->RunUntilIdle();
+  EXPECT_FALSE(IsDestroyPopupTimerRunning());
+  EXPECT_FALSE(GetWidget());
 #endif
+  SetTaskRunner(base::ThreadTaskRunnerHandle::Get().get());
 }
 
+// Mac does not delete the widget after a delay, so this test only runs on
+// non-mac platforms.
+#if !defined(OS_MACOSX)
+IN_PROC_BROWSER_TEST_F(StatusBubbleViewsTest, ShowHideDestroyShow) {
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner =
+      base::MakeRefCounted<base::TestSimpleTaskRunner>();
+  SetTaskRunner(task_runner.get());
+
+  // The widget does not exist until it needs to be shown.
+  StatusBubble* bubble = GetBubble();
+  ASSERT_TRUE(bubble);
+
+  // Setting status text shows the widget.
+  bubble->SetStatus(base::ASCIIToUTF16("test"));
+  views::Widget* widget = GetWidget();
+  ASSERT_TRUE(widget);
+  EXPECT_TRUE(widget->IsVisible());
+
+  bubble->SetStatus(base::string16());
+  // The widget is not hidden immediately, instead a task is scheduled. Run that
+  // now.
+  task_runner->RunPendingTasks();
+  // After the task, a timer is created that animates hidden. Advance that.
+  ASSERT_TRUE(GetShowHideAnimationForTesting());
+  // Advance well past the time for the animation to ensure it completes.
+  static_cast<gfx::AnimationContainerElement*>(GetShowHideAnimationForTesting())
+      ->Step(base::TimeTicks::Now() + base::TimeDelta::FromMinutes(1));
+  // Widget should still exist.
+  ASSERT_TRUE(GetWidget());
+  EXPECT_FALSE(widget->IsVisible());
+  EXPECT_TRUE(IsDestroyPopupTimerRunning());
+
+  // Run until idle, which should trigger deleting the widget.
+  task_runner->RunUntilIdle();
+  EXPECT_FALSE(IsDestroyPopupTimerRunning());
+  EXPECT_FALSE(GetWidget());
+
+  // Setting status text shows the widget.
+  bubble->SetStatus(base::ASCIIToUTF16("test"));
+  widget = GetWidget();
+  ASSERT_TRUE(widget);
+  EXPECT_TRUE(widget->IsVisible());
+
+  SetTaskRunner(base::ThreadTaskRunnerHandle::Get().get());
+}
+#endif
+
 // Ensure the status bubble does not close itself on Mac. Doing so can trigger
 // unwanted space switches due to rdar://9037452. See https://crbug.com/866760.
 IN_PROC_BROWSER_TEST_F(StatusBubbleViewsTest, NeverHideOrCloseOnMac) {
@@ -68,10 +147,11 @@
   // However, it is fully transparent.
   EXPECT_TRUE(widget->IsVisible());
   EXPECT_EQ(0.f, test::GetNativeWindowAlphaValue(widget->GetNativeWindow()));
+  EXPECT_FALSE(IsDestroyPopupTimerRunning());
 #else
-  views::test::WidgetClosingObserver widget_closing_observer(widget);
+  EXPECT_FALSE(IsDestroyPopupTimerRunning());
   bubble->Hide();
-  widget_closing_observer.Wait();
-  EXPECT_TRUE(widget_closing_observer.widget_closed());
+  EXPECT_FALSE(widget->IsVisible());
+  EXPECT_TRUE(IsDestroyPopupTimerRunning());
 #endif
 }