views: centralize LocationBarBubble close logic

This change:
1) Has LocationBarBubbleDelegateView close itself when the WebContents is
   hidden or closed;
2) Removes matching logic that was already in two of its subclasses;
3) Fixes the browser tests for one of those subclasses to allow for async
   window closure.

Bug: 780131
Change-Id: I2362c82c89be1356356663789e97b51b3b2362f8
Reviewed-on: https://chromium-review.googlesource.com/919308
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541454}

chrome/browser/ui/views/intent_picker_bubble_view.cc

@@ -306,7 +306,6 @@ IntentPickerBubbleView::IntentPickerBubbleView(
     : LocationBarBubbleDelegateView(nullptr /* anchor_view */,
                                     gfx::Point(),
                                     web_contents),
-      content::WebContentsObserver(web_contents),
       intent_picker_cb_(intent_picker_cb),
       selected_app_tag_(0),
       scroll_view_(nullptr),
@@ -338,12 +337,6 @@ void IntentPickerBubbleView::ArrowButtonPressed(int index) {
   AdjustScrollViewVisibleRegion();
 }
 
-// If the actual web_contents gets destroyed in the middle of the process we
-// should inform the caller about this error.
-void IntentPickerBubbleView::WebContentsDestroyed() {
-  GetWidget()->Close();
-}
-
 void IntentPickerBubbleView::OnKeyEvent(ui::KeyEvent* event) {
   if (!IsKeyboardCodeArrow(event->key_code()) ||
       event->type() != ui::ET_KEY_RELEASED)

chrome/browser/ui/views/intent_picker_bubble_view.h

@@ -57,8 +57,7 @@ class IntentPickerLabelButton;
 //   +--------------------------------+
 
 class IntentPickerBubbleView : public LocationBarBubbleDelegateView,
-                               public views::ButtonListener,
-                               public content::WebContentsObserver {
+                               public views::ButtonListener {
  public:
   using AppInfo = arc::ArcNavigationThrottle::AppInfo;
 
@@ -119,9 +118,6 @@ class IntentPickerBubbleView : public LocationBarBubbleDelegateView,
   // while focusing on the |scroll_view_|.
   void ArrowButtonPressed(int index);
 
-  // content::WebContentsObserver overrides:
-  void WebContentsDestroyed() override;
-
   // ui::EventHandler overrides:
   void OnKeyEvent(ui::KeyEvent* event) override;
 

chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc

@@ -57,7 +57,8 @@ LocationBarBubbleDelegateView::LocationBarBubbleDelegateView(
     content::WebContents* web_contents)
     : BubbleDialogDelegateView(anchor_view,
                                anchor_view ? views::BubbleBorder::TOP_RIGHT
-                                           : views::BubbleBorder::NONE) {
+                                           : views::BubbleBorder::NONE),
+      WebContentsObserver(web_contents) {
   // Add observer to close the bubble if the fullscreen state changes.
   if (web_contents) {
     Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
@@ -115,6 +116,16 @@ void LocationBarBubbleDelegateView::Observe(
   CloseBubble();
 }
 
+void LocationBarBubbleDelegateView::OnVisibilityChanged(
+    content::Visibility visibility) {
+  if (visibility == content::Visibility::HIDDEN)
+    CloseBubble();
+}
+
+void LocationBarBubbleDelegateView::WebContentsDestroyed() {
+  CloseBubble();
+}
+
 void LocationBarBubbleDelegateView::AdjustForFullscreen(
     const gfx::Rect& screen_bounds) {
   if (GetAnchorView())

chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "ui/views/bubble/bubble_dialog_delegate.h"
 #include "ui/views/event_monitor.h"
 
@@ -21,7 +22,8 @@ class WebContents;
 // will automatically close when the browser transitions in or out of fullscreen
 // mode.
 class LocationBarBubbleDelegateView : public views::BubbleDialogDelegateView,
-                                      public content::NotificationObserver {
+                                      public content::NotificationObserver,
+                                      public content::WebContentsObserver {
  public:
   enum DisplayReason {
     // The bubble appears as a direct result of a user action (clicking on the
@@ -52,6 +54,10 @@ class LocationBarBubbleDelegateView : public views::BubbleDialogDelegateView,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
+  // content::WebContentsObserver:
+  void OnVisibilityChanged(content::Visibility visibility) override;
+  void WebContentsDestroyed() override;
+
   // If the bubble is not anchored to a view, places the bubble in the top right
   // (left in RTL) of the |screen_bounds| that contain web contents's browser
   // window. Because the positioning is based on the size of the bubble, this

chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc

@@ -88,6 +88,29 @@ IN_PROC_BROWSER_TEST_F(LocationBarViewBrowserTest, LocationBarDecoration) {
   EXPECT_FALSE(ZoomBubbleView::GetZoomBubble());
 }
 
+// Ensure that location bar bubbles close when the webcontents hides.
+IN_PROC_BROWSER_TEST_F(LocationBarViewBrowserTest, BubblesCloseOnHide) {
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  zoom::ZoomController* zoom_controller =
+      zoom::ZoomController::FromWebContents(web_contents);
+  ZoomView* zoom_view = GetLocationBarView()->zoom_view();
+
+  ASSERT_TRUE(zoom_view);
+  EXPECT_FALSE(zoom_view->visible());
+
+  zoom_controller->SetZoomLevel(content::ZoomFactorToZoomLevel(1.5));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(zoom_view->visible());
+  EXPECT_TRUE(ZoomBubbleView::GetZoomBubble());
+
+  chrome::NewTab(browser());
+  chrome::SelectNextTab(browser());
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(ZoomBubbleView::GetZoomBubble());
+}
+
 // After AddUrlHandler() is called, requests to this hostname will be mocked
 // and use specified certificate validation results. This allows tests to mock
 // Extended Validation (EV) certificate connections.

chrome/browser/ui/views/location_bar/zoom_bubble_view.cc

@@ -224,7 +224,6 @@ ZoomBubbleView::ZoomBubbleView(
     DisplayReason reason,
     ImmersiveModeController* immersive_mode_controller)
     : LocationBarBubbleDelegateView(anchor_view, anchor_point, web_contents),
-      WebContentsObserver(web_contents),
       auto_close_duration_(kBubbleCloseDelayDefault),
       image_button_(nullptr),
       label_(nullptr),
@@ -417,15 +416,6 @@ void ZoomBubbleView::OnExtensionIconImageChanged(
   image_button_->SchedulePaint();
 }
 
-void ZoomBubbleView::OnVisibilityChanged(content::Visibility visibility) {
-  if (visibility == content::Visibility::HIDDEN)
-    CloseBubble();
-}
-
-void ZoomBubbleView::WebContentsDestroyed() {
-  CloseBubble();
-}
-
 void ZoomBubbleView::SetExtensionInfo(const extensions::Extension* extension) {
   DCHECK(extension);
   extension_info_.id = extension->id();

chrome/browser/ui/views/location_bar/zoom_bubble_view.h

@@ -30,8 +30,7 @@ class ImageButton;
 class ZoomBubbleView : public LocationBarBubbleDelegateView,
                        public views::ButtonListener,
                        public ImmersiveModeController::Observer,
-                       public extensions::IconImage::Observer,
-                       public content::WebContentsObserver {
+                       public extensions::IconImage::Observer {
  public:
   // Shows the bubble and automatically closes it after a short time period if
   // |reason| is AUTOMATIC.
@@ -102,10 +101,6 @@ class ZoomBubbleView : public LocationBarBubbleDelegateView,
   // extensions::IconImage::Observer:
   void OnExtensionIconImageChanged(extensions::IconImage* /* image */) override;
 
-  // content::WebContentsObserver:
-  void OnVisibilityChanged(content::Visibility visibility) override;
-  void WebContentsDestroyed() override;
-
   // Sets information about the extension that initiated the zoom change.
   // Calling this method asserts that the extension |extension| did initiate
   // the zoom change.

chrome/browser/ui/views/translate/translate_bubble_view.cc

@@ -426,10 +426,6 @@ void TranslateBubbleView::StyledLabelLinkClicked(views::StyledLabel* label,
   translate::ReportUiAction(translate::ADVANCED_LINK_CLICKED);
 }
 
-void TranslateBubbleView::WebContentsDestroyed() {
-  GetWidget()->CloseNow();
-}
-
 void TranslateBubbleView::OnWidgetClosing(views::Widget* widget) {
   if (GetBubbleFrameView()->close_button_clicked()) {
     model_->DeclineTranslation();
@@ -448,7 +444,6 @@ TranslateBubbleView::TranslateBubbleView(
     translate::TranslateErrors::Type error_type,
     content::WebContents* web_contents)
     : LocationBarBubbleDelegateView(anchor_view, anchor_point, web_contents),
-      WebContentsObserver(web_contents),
       before_translate_view_(NULL),
       translating_view_(NULL),
       after_translate_view_(NULL),

chrome/browser/ui/views/translate/translate_bubble_view.h

@@ -41,8 +41,7 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
                             public views::ComboboxListener,
                             public views::LinkListener,
                             public ui::SimpleMenuModel::Delegate,
-                            public views::StyledLabelListener,
-                            public content::WebContentsObserver {
+                            public views::StyledLabelListener {
  public:
   // Item IDs for the option button's menu.
   enum OptionsMenuItem {
@@ -106,9 +105,6 @@ class TranslateBubbleView : public LocationBarBubbleDelegateView,
                               const gfx::Range& range,
                               int event_flags) override;
 
-  // content::WebContentsObserver method.
-  void WebContentsDestroyed() override;
-
   // Overridden from views::WidgetObserver:
   void OnWidgetClosing(views::Widget* widget) override;
 

chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc

@@ -83,8 +83,10 @@ IN_PROC_BROWSER_TEST_F(TranslateBubbleViewBrowserTest,
   NavigateAndWaitForLanguageDetection(french_url, "fr");
   EXPECT_TRUE(TranslateBubbleView::GetCurrentBubble());
 
-  // Close the window without translating.
+  // Close the window without translating. Spin the runloop to allow
+  // asynchronous window closure to happen.
   chrome::CloseWindow(browser());
+  base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(TranslateBubbleView::GetCurrentBubble());
 }
 
@@ -100,9 +102,11 @@ IN_PROC_BROWSER_TEST_F(TranslateBubbleViewBrowserTest,
   NavigateAndWaitForLanguageDetection(french_url, "fr");
   EXPECT_TRUE(TranslateBubbleView::GetCurrentBubble());
 
-  // Close the tab without translating.
+  // Close the tab without translating. Spin the runloop to allow asynchronous
+  // window closure to happen.
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
   chrome::CloseWebContents(browser(), current_web_contents, false);
+  base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(TranslateBubbleView::GetCurrentBubble());
 }