Restore custom dismissal method for extension popups.

Using the default method as in
https://chromium-review.googlesource.com/c/chromium/src/+/1367129 prevents
extension popups from opening their own popup windows, e.g. JS alerts.  Since
there's no plan to deprecate that functionality and we can't come up with a good
alternate route, this restores part of the previous code.

This does not re-introduce the problem with long clicks re-showing the popup;
that was sufficiently addressed by the rest of the previous CL.

Bug: 941994
Change-Id: I59bb7d695b51e250a096b8ca8331876593507ead
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1529753
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642391}

chrome/browser/ui/views/extensions/extension_popup.cc

@@ -21,7 +21,9 @@
 
 #if defined(USE_AURA)
 #include "chrome/browser/ui/browser_dialogs.h"
+#include "ui/aura/window.h"
 #include "ui/wm/core/window_animations.h"
+#include "ui/wm/public/activation_client.h"
 #endif
 
 // static
@@ -40,6 +42,12 @@ void ExtensionPopup::ShowPopup(
       native_view, wm::WINDOW_VISIBILITY_ANIMATION_TYPE_VERTICAL);
   wm::SetWindowVisibilityAnimationVerticalPosition(native_view, -3.0f);
 
+  // This is removed in ExtensionPopup::OnWidgetDestroying(), which is
+  // guaranteed to be called before the Widget goes away.  It's not safe to use
+  // a ScopedObserver for this, since the activation client may be deleted
+  // without a call back to this class.
+  wm::GetActivationClient(native_view->GetRootWindow())->AddObserver(popup);
+
   chrome::RecordDialogCreation(chrome::DialogIdentifier::EXTENSION_POPUP_AURA);
 #endif
 }
@@ -75,11 +83,17 @@ void ExtensionPopup::OnWidgetActivationChanged(views::Widget* widget,
                                                bool active) {
   BubbleDialogDelegateView::OnWidgetActivationChanged(widget, active);
 
-  if (active && observe_next_widget_activation_) {
-    observe_next_widget_activation_ = false;
-    views::Widget* const my_widget = GetWidget();
-    if (widget != my_widget)
-      my_widget->CloseWithReason(views::Widget::ClosedReason::kLostFocus);
+  // The widget is shown asynchronously and may take a long time to appear, so
+  // only close if it's actually been shown.
+  if (GetWidget()->IsVisible()) {
+    // Extension popups need to open child windows sometimes (e.g. for JS
+    // alerts), which take activation; so ExtensionPopup can't close on
+    // deactivation.  Instead, close when the parent widget is activated; this
+    // leaves the popup open when e.g. a non-Chrome window is activated, which
+    // doesn't feel very menu-like, but is better than any alternative.  See
+    // https://crbug.com/941994 for more discussion.
+    if (widget == anchor_widget() && active)
+      CloseUnlessUnderInspection();
   }
 }
 
@@ -87,6 +101,34 @@ bool ExtensionPopup::ShouldHaveRoundCorners() const {
   return false;
 }
 
+#if defined(USE_AURA)
+void ExtensionPopup::OnWidgetDestroying(views::Widget* widget) {
+  BubbleDialogDelegateView::OnWidgetDestroying(widget);
+
+  if (widget == GetWidget()) {
+    auto* activation_client =
+        wm::GetActivationClient(widget->GetNativeWindow()->GetRootWindow());
+    // If the popup was being inspected with devtools and the browser window
+    // was closed, then the root window and activation client are already
+    // destroyed.
+    if (activation_client)
+      activation_client->RemoveObserver(this);
+  }
+}
+
+void ExtensionPopup::OnWindowActivated(
+    wm::ActivationChangeObserver::ActivationReason reason,
+    aura::Window* gained_active,
+    aura::Window* lost_active) {
+  // Close on anchor window activation (i.e. user clicked the browser window).
+  // DesktopNativeWidgetAura does not trigger the expected browser widget
+  // [de]activation events when activating widgets in its own root window.
+  // This additional check handles those cases. See https://crbug.com/320889 .
+  if (gained_active == anchor_widget()->GetNativeWindow())
+    CloseUnlessUnderInspection();
+}
+#endif  // defined(USE_AURA)
+
 void ExtensionPopup::OnExtensionSizeChanged(ExtensionViewViews* view) {
   SizeToContents();
 }
@@ -119,15 +161,13 @@ void ExtensionPopup::OnTabStripModelChanged(
 void ExtensionPopup::DevToolsAgentHostAttached(
     content::DevToolsAgentHost* agent_host) {
   if (host()->host_contents() == agent_host->GetWebContents())
-    UpdateShowAction(SHOW_AND_INSPECT);
+    show_action_ = SHOW_AND_INSPECT;
 }
 
 void ExtensionPopup::DevToolsAgentHostDetached(
     content::DevToolsAgentHost* agent_host) {
-  if (host()->host_contents() == agent_host->GetWebContents()) {
-    UpdateShowAction(SHOW);
-    observe_next_widget_activation_ = true;
-  }
+  if (host()->host_contents() == agent_host->GetWebContents())
+    show_action_ = SHOW;
 }
 
 ExtensionPopup::ExtensionPopup(extensions::ExtensionViewHost* host,
@@ -137,12 +177,15 @@ ExtensionPopup::ExtensionPopup(extensions::ExtensionViewHost* host,
     : BubbleDialogDelegateView(anchor_view,
                                arrow,
                                views::BubbleBorder::SMALL_SHADOW),
-      host_(host) {
+      host_(host),
+      show_action_(show_action) {
   set_margins(gfx::Insets());
   SetLayoutManager(std::make_unique<views::FillLayout>());
   AddChildView(GetExtensionView());
   GetExtensionView()->set_container(this);
-  UpdateShowAction(show_action);
+
+  // See comments in OnWidgetActivationChanged().
+  set_close_on_deactivate(false);
 
   // Listen for the containing view calling window.close();
   registrar_.Add(
@@ -163,11 +206,6 @@ ExtensionPopup::ExtensionPopup(extensions::ExtensionViewHost* host,
   }
 }
 
-void ExtensionPopup::UpdateShowAction(ShowAction show_action) {
-  show_action_ = show_action;
-  set_close_on_deactivate(show_action == SHOW);
-}
-
 void ExtensionPopup::ShowBubble() {
   GetWidget()->Show();
 
@@ -180,6 +218,11 @@ void ExtensionPopup::ShowBubble() {
   }
 }
 
+void ExtensionPopup::CloseUnlessUnderInspection() {
+  if (show_action_ != SHOW_AND_INSPECT)
+    GetWidget()->CloseWithReason(views::Widget::ClosedReason::kLostFocus);
+}
+
 ExtensionViewViews* ExtensionPopup::GetExtensionView() {
   return static_cast<ExtensionViewViews*>(host_.get()->view());
 }

chrome/browser/ui/views/extensions/extension_popup.h

@@ -18,6 +18,10 @@
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
 #include "url/gurl.h"
 
+#if defined(USE_AURA)
+#include "ui/wm/public/activation_change_observer.h"
+#endif
+
 class ExtensionViewViews;
 
 namespace content {
@@ -30,6 +34,9 @@ class ExtensionViewHost;
 
 // The bubble used for hosting a browser-action popup provided by an extension.
 class ExtensionPopup : public views::BubbleDialogDelegateView,
+#if defined(USE_AURA)
+                       public wm::ActivationChangeObserver,
+#endif
                        public ExtensionViewViews::Container,
                        public content::NotificationObserver,
                        public TabStripModelObserver,
@@ -71,6 +78,14 @@ class ExtensionPopup : public views::BubbleDialogDelegateView,
   int GetDialogButtons() const override;
   void OnWidgetActivationChanged(views::Widget* widget, bool active) override;
   bool ShouldHaveRoundCorners() const override;
+#if defined(USE_AURA)
+  void OnWidgetDestroying(views::Widget* widget) override;
+
+  // wm::ActivationChangeObserver:
+  void OnWindowActivated(wm::ActivationChangeObserver::ActivationReason reason,
+                         aura::Window* gained_active,
+                         aura::Window* lost_active) override;
+#endif
 
   // ExtensionViewViews::Container:
   void OnExtensionSizeChanged(ExtensionViewViews* view) override;
@@ -98,11 +113,12 @@ class ExtensionPopup : public views::BubbleDialogDelegateView,
                  views::BubbleBorder::Arrow arrow,
                  ShowAction show_action);
 
-  // Changes internal state to follow the supplied |show_action|.
-  void UpdateShowAction(ShowAction show_action);
   // Shows the bubble, focuses its content, and registers listeners.
   void ShowBubble();
 
+  // Closes the bubble if the devtools window is not attached.
+  void CloseUnlessUnderInspection();
+
   ExtensionViewViews* GetExtensionView();
 
   // The contained host for the view.
@@ -110,12 +126,6 @@ class ExtensionPopup : public views::BubbleDialogDelegateView,
 
   ShowAction show_action_;
 
-  // When dev tools closes, we should close the popup iff activation isn't going
-  // back to it.  There's no way to know which widget will be activated when dev
-  // tools closes, so this flag is set instead, which causes
-  // OnWidgetActivationChanged() to optionally close the current widget.
-  bool observe_next_widget_activation_ = false;
-
   content::NotificationRegistrar registrar_;
   ScopedObserver<TabStripModel, TabStripModelObserver> observer_{this};