reland r57885 with a fix for the DCHECK failures observed by nsylvain.

Now we only delay shutdown of the constrained window in the escape case (see constrained_window_gtk.cc). Locally this fixes the DCHECKs in both manual testing and the login prompt ui tests.

BUG=53242, 50799
TEST=ui_tests --gtest_filter=LoginPromptTest*

Review URL: http://codereview.chromium.org/3351006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@58570 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 143 insertions, 108 deletions

diff --git a/chrome/browser/gtk/constrained_window_gtk.cc b/chrome/browser/gtk/constrained_window_gtk.cc
index 321aa16..b901083 100644
--- a/chrome/browser/gtk/constrained_window_gtk.cc
+++ b/chrome/browser/gtk/constrained_window_gtk.cc
@@ -16,7 +16,7 @@ ConstrainedWindowGtk::ConstrainedWindowGtk(
     : owner_(owner),
       delegate_(delegate),
       visible_(false),
-      accel_group_(gtk_accel_group_new()) {
+      factory_(this) {
   DCHECK(owner);
   DCHECK(delegate);
   GtkWidget* dialog = delegate->GetWidgetRoot();
@@ -34,20 +34,14 @@ ConstrainedWindowGtk::ConstrainedWindowGtk(
   gtk_container_add(GTK_CONTAINER(frame), alignment);
   gtk_container_add(GTK_CONTAINER(ebox), frame);
   border_.Own(ebox);
-  ConnectAccelerators();
+
+  gtk_widget_add_events(widget(), GDK_KEY_PRESS_MASK);
+  g_signal_connect(widget(), "key-press-event", G_CALLBACK(OnKeyPressThunk),
+                   this);
 }
 
 ConstrainedWindowGtk::~ConstrainedWindowGtk() {
   border_.Destroy();
-
-  gtk_accel_group_disconnect_key(accel_group_, GDK_Escape,
-                                 static_cast<GdkModifierType>(0));
-  if (ContainingView() && ContainingView()->GetTopLevelNativeWindow()) {
-    gtk_window_remove_accel_group(
-        GTK_WINDOW(ContainingView()->GetTopLevelNativeWindow()),
-        accel_group_);
-  }
-  g_object_unref(accel_group_);
 }
 
 void ConstrainedWindowGtk::ShowConstrainedWindow() {
@@ -73,29 +67,18 @@ TabContentsViewGtk* ConstrainedWindowGtk::ContainingView() {
   return static_cast<TabContentsViewGtk*>(owner_->view());
 }
 
-void ConstrainedWindowGtk::ConnectAccelerators() {
-  gtk_accel_group_connect(accel_group_,
-                          GDK_Escape, static_cast<GdkModifierType>(0),
-                          static_cast<GtkAccelFlags>(0),
-                          g_cclosure_new(G_CALLBACK(OnEscapeThunk),
-                                         this, NULL));
-  gtk_window_add_accel_group(
-      GTK_WINDOW(ContainingView()->GetTopLevelNativeWindow()),
-      accel_group_);
-}
-
-
-gboolean ConstrainedWindowGtk::OnEscape(GtkAccelGroup* group,
-                                        GObject* acceleratable,
-                                        guint keyval,
-                                        GdkModifierType modifier) {
-  // Handle this accelerator only if this is on the currently selected tab.
-  Browser* browser = BrowserList::GetLastActive();
-  if (!browser || browser->GetSelectedTabContents() != owner_)
-    return FALSE;
+gboolean ConstrainedWindowGtk::OnKeyPress(GtkWidget* sender,
+                                          GdkEventKey* key) {
+  if (key->keyval == GDK_Escape) {
+    // Let the stack unwind so the event handler can release its ref
+    // on widget().
+    MessageLoop::current()->PostTask(FROM_HERE,
+        factory_.NewRunnableMethod(
+            &ConstrainedWindowGtk::CloseConstrainedWindow));
+    return TRUE;
+  }
 
-  CloseConstrainedWindow();
-  return TRUE;
+  return FALSE;
 }
 
 // static
@@ -104,4 +87,3 @@ ConstrainedWindow* ConstrainedWindow::CreateConstrainedDialog(
     ConstrainedWindowGtkDelegate* delegate) {
   return new ConstrainedWindowGtk(parent, delegate);
 }
-
diff --git a/chrome/browser/gtk/constrained_window_gtk.h b/chrome/browser/gtk/constrained_window_gtk.h
index 2312cb3..117fd33 100644
--- a/chrome/browser/gtk/constrained_window_gtk.h
+++ b/chrome/browser/gtk/constrained_window_gtk.h
@@ -10,12 +10,12 @@
 
 #include "app/gtk_signal.h"
 #include "base/basictypes.h"
+#include "base/task.h"
 #include "chrome/browser/gtk/owned_widget_gtk.h"
 #include "chrome/browser/tab_contents/constrained_window.h"
 
 class TabContents;
 class TabContentsViewGtk;
-typedef struct _GtkWidget GtkWidget;
 
 class ConstrainedWindowGtkDelegate {
  public:
@@ -56,12 +56,9 @@ class ConstrainedWindowGtk : public ConstrainedWindow {
   ConstrainedWindowGtk(TabContents* owner,
                        ConstrainedWindowGtkDelegate* delegate);
 
-  // Connects the ESC accelerator to the window.
-  void ConnectAccelerators();
-
-  // Handles an ESC accelerator being pressed.
-  CHROMEG_CALLBACK_3(ConstrainedWindowGtk, gboolean, OnEscape, GtkAccelGroup*,
-                     GObject*, guint, GdkModifierType);
+  // Handler for Escape.
+  CHROMEGTK_CALLBACK_1(ConstrainedWindowGtk, gboolean, OnKeyPress,
+                       GdkEventKey*);
 
   // The TabContents that owns and constrains this ConstrainedWindow.
   TabContents* owner_;
@@ -75,7 +72,7 @@ class ConstrainedWindowGtk : public ConstrainedWindow {
   // Stores if |ShowConstrainedWindow()| has been called.
   bool visible_;
 
-  GtkAccelGroup* accel_group_;
+  ScopedRunnableMethodFactory<ConstrainedWindowGtk> factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ConstrainedWindowGtk);
 };
diff --git a/chrome/browser/gtk/focus_store_gtk.cc b/chrome/browser/gtk/focus_store_gtk.cc
index dc099fc..50ebfa7 100644
--- a/chrome/browser/gtk/focus_store_gtk.cc
+++ b/chrome/browser/gtk/focus_store_gtk.cc
@@ -19,19 +19,22 @@ FocusStoreGtk::~FocusStoreGtk() {
 }
 
 void FocusStoreGtk::Store(GtkWidget* widget) {
-  DisconnectDestroyHandler();
-  if (!widget) {
-    widget_ = NULL;
-    return;
+  GtkWidget* focus_widget = NULL;
+  if (widget) {
+    GtkWindow* window = platform_util::GetTopLevel(widget);
+    if (window)
+      focus_widget = window->focus_widget;
   }
 
-  GtkWindow* window = platform_util::GetTopLevel(widget);
-  if (!window) {
-    widget_ = NULL;
-    return;
-  }
+  SetWidget(focus_widget);
+}
 
-  widget_ = window->focus_widget;
+void FocusStoreGtk::SetWidget(GtkWidget* widget) {
+  DisconnectDestroyHandler();
+
+  // We don't add a ref. The signal handler below effectively gives us a weak
+  // reference.
+  widget_ = widget;
   if (widget_) {
     // When invoked, |gtk_widget_destroyed| will set |widget_| to NULL.
     destroy_handler_id_ = g_signal_connect(widget_, "destroy",
@@ -41,6 +44,8 @@ void FocusStoreGtk::Store(GtkWidget* widget) {
 }
 
 void FocusStoreGtk::DisconnectDestroyHandler() {
-  if (widget_)
+  if (widget_) {
     g_signal_handler_disconnect(widget_, destroy_handler_id_);
+    widget_ = NULL;
+  }
 }
diff --git a/chrome/browser/gtk/focus_store_gtk.h b/chrome/browser/gtk/focus_store_gtk.h
index ce3b68b..2159f1e 100644
--- a/chrome/browser/gtk/focus_store_gtk.h
+++ b/chrome/browser/gtk/focus_store_gtk.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -19,9 +19,12 @@ class FocusStoreGtk {
   GtkWidget* widget() const { return widget_; }
 
   // Save the widget that is currently focused for |widget|'s toplevel (NOT
-  // |widget|). Call with NULL to clear |widget_|.
+  // |widget|).
   void Store(GtkWidget* widget);
 
+  // Save |widget| as the focus widget. Call with NULL to clear |widget_|.
+  void SetWidget(GtkWidget* widget);
+
  private:
   // Disconnect the previous destroy handler (if any).
   void DisconnectDestroyHandler();
diff --git a/chrome/browser/gtk/gtk_util.cc b/chrome/browser/gtk/gtk_util.cc
index 5a5a815..541d2da 100644
--- a/chrome/browser/gtk/gtk_util.cc
+++ b/chrome/browser/gtk/gtk_util.cc
@@ -1059,4 +1059,11 @@ string16 GetStockPreferencesMenuLabel() {
   return preferences;
 }
 
+bool IsWidgetAncestryVisible(GtkWidget* widget) {
+  GtkWidget* parent = widget;
+  while (parent && GTK_WIDGET_VISIBLE(parent))
+    parent = parent->parent;
+  return !parent;
+}
+
 }  // namespace gtk_util
diff --git a/chrome/browser/gtk/gtk_util.h b/chrome/browser/gtk/gtk_util.h
index 20d3c6a..95200d4 100644
--- a/chrome/browser/gtk/gtk_util.h
+++ b/chrome/browser/gtk/gtk_util.h
@@ -315,6 +315,10 @@ gfx::Rect GetDialogBounds(GtkWidget* dialog);
 // empty string if no stock item found.
 string16 GetStockPreferencesMenuLabel();
 
+// Checks whether a widget is actually visible, i.e. whether it and all its
+// ancestors up to its toplevel are visible.
+bool IsWidgetAncestryVisible(GtkWidget* widget);
+
 }  // namespace gtk_util
 
 #endif  // CHROME_BROWSER_GTK_GTK_UTIL_H_
diff --git a/chrome/browser/login_prompt_gtk.cc b/chrome/browser/login_prompt_gtk.cc
index c3e89d0..a9cfb4b 100644
--- a/chrome/browser/login_prompt_gtk.cc
+++ b/chrome/browser/login_prompt_gtk.cc
@@ -7,6 +7,7 @@
 #include <gtk/gtk.h>
 
 #include "app/l10n_util.h"
+#include "app/gtk_signal.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/gtk/constrained_window_gtk.h"
@@ -16,6 +17,7 @@
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/browser/tab_contents/navigation_controller.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/browser/tab_contents/tab_contents_view_gtk.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/common/notification_service.h"
 #include "grit/generated_resources.h"
@@ -89,15 +91,15 @@ class LoginHandlerGtk : public LoginHandler,
     gtk_button_set_label(
         GTK_BUTTON(ok_),
         l10n_util::GetStringUTF8(IDS_LOGIN_DIALOG_OK_BUTTON_LABEL).c_str());
-    g_signal_connect(ok_, "clicked", G_CALLBACK(OnOKClicked), this);
+    g_signal_connect(ok_, "clicked", G_CALLBACK(OnOKClickedThunk), this);
     gtk_box_pack_end(GTK_BOX(hbox), ok_, FALSE, FALSE, 0);
 
     GtkWidget* cancel = gtk_button_new_from_stock(GTK_STOCK_CANCEL);
-    g_signal_connect(cancel, "clicked", G_CALLBACK(OnCancelClicked), this);
+    g_signal_connect(cancel, "clicked", G_CALLBACK(OnCancelClickedThunk), this);
     gtk_box_pack_end(GTK_BOX(hbox), cancel, FALSE, FALSE, 0);
 
     g_signal_connect(root_.get(), "hierarchy-changed",
-                     G_CALLBACK(OnPromptShown), this);
+                     G_CALLBACK(OnPromptHierarchyChangedThunk), this);
 
     SetModel(manager);
 
@@ -129,34 +131,10 @@ class LoginHandlerGtk : public LoginHandler,
  private:
   friend class LoginPrompt;
 
-  static void OnOKClicked(GtkButton *button, LoginHandlerGtk* handler) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-
-    handler->SetAuth(
-        UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(handler->username_entry_))),
-        UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(handler->password_entry_))));
-  }
-
-  static void OnCancelClicked(GtkButton *button, LoginHandlerGtk* handler) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-
-    handler->CancelAuth();
-  }
-
-  static void OnPromptShown(GtkButton* root,
-                            GtkWidget* previous_toplevel,
-                            LoginHandlerGtk* handler) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-
-    if (!GTK_WIDGET_TOPLEVEL(gtk_widget_get_toplevel(handler->ok_)))
-      return;
-
-    // Now that we have attached ourself to the window, we can make our OK
-    // button the default action and mess with the focus.
-    GTK_WIDGET_SET_FLAGS(handler->ok_, GTK_CAN_DEFAULT);
-    gtk_widget_grab_default(handler->ok_);
-    gtk_widget_grab_focus(handler->username_entry_);
-  }
+  CHROMEGTK_CALLBACK_0(LoginHandlerGtk, void, OnOKClicked);
+  CHROMEGTK_CALLBACK_0(LoginHandlerGtk, void, OnCancelClicked);
+  CHROMEGTK_CALLBACK_1(LoginHandlerGtk, void, OnPromptHierarchyChanged,
+                       GtkWidget*);
 
   // The GtkWidgets that form our visual hierarchy:
   // The root container we pass to our parent.
@@ -170,6 +148,42 @@ class LoginHandlerGtk : public LoginHandler,
   DISALLOW_COPY_AND_ASSIGN(LoginHandlerGtk);
 };
 
+void LoginHandlerGtk::OnOKClicked(GtkWidget* sender) {
+  SetAuth(
+      UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(username_entry_))),
+      UTF8ToWide(gtk_entry_get_text(GTK_ENTRY(password_entry_))));
+}
+
+void LoginHandlerGtk::OnCancelClicked(GtkWidget* sender) {
+  CancelAuth();
+}
+
+void LoginHandlerGtk::OnPromptHierarchyChanged(GtkWidget* sender,
+                                               GtkWidget* previous_toplevel) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
+  if (!GTK_WIDGET_TOPLEVEL(gtk_widget_get_toplevel(ok_)))
+    return;
+
+  // Now that we have attached ourself to the window, we can make our OK
+  // button the default action and mess with the focus.
+  GTK_WIDGET_SET_FLAGS(ok_, GTK_CAN_DEFAULT);
+  gtk_widget_grab_default(ok_);
+
+  // The user may have focused another tab. In this case do not grab focus
+  // until this tab is refocused.
+  if (gtk_util::IsWidgetAncestryVisible(username_entry_)) {
+    gtk_widget_grab_focus(username_entry_);
+  } else {
+  // TODO(estade): this define should not need to be here because this class
+  // should not be used on linux/views.
+#if defined(TOOLKIT_GTK)
+    static_cast<TabContentsViewGtk*>(GetTabContentsForLogin()->view())->
+        SetFocusedWidget(username_entry_);
+#endif
+  }
+}
+
 // static
 LoginHandler* LoginHandler::Create(net::AuthChallengeInfo* auth_info,
                                    URLRequest* request) {
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.cc b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
index 9787050..11a0da0 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.cc
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
@@ -41,25 +41,6 @@ using WebKit::WebDragOperationsMask;
 
 namespace {
 
-// Called when the content view gtk widget is tabbed to, or after the call to
-// gtk_widget_child_focus() in TakeFocus(). We return true
-// and grab focus if we don't have it. The call to
-// FocusThroughTabTraversal(bool) forwards the "move focus forward" effect to
-// webkit.
-gboolean OnFocus(GtkWidget* widget, GtkDirectionType focus,
-                 TabContents* tab_contents) {
-  // If we already have focus, let the next widget have a shot at it. We will
-  // reach this situation after the call to gtk_widget_child_focus() in
-  // TakeFocus().
-  if (gtk_widget_is_focus(widget))
-    return FALSE;
-
-  gtk_widget_grab_focus(widget);
-  bool reverse = focus == GTK_DIR_TAB_BACKWARD;
-  tab_contents->FocusThroughTabTraversal(reverse);
-  return TRUE;
-}
-
 // Called when the mouse leaves the widget. We notify our delegate.
 gboolean OnLeaveNotify(GtkWidget* widget, GdkEventCrossing* event,
                        TabContents* tab_contents) {
@@ -165,8 +146,7 @@ RenderWidgetHostView* TabContentsViewGtk::CreateViewForWidget(
       new RenderWidgetHostViewGtk(render_widget_host);
   view->InitAsChild();
   gfx::NativeView content_view = view->native_view();
-  g_signal_connect(content_view, "focus",
-                   G_CALLBACK(OnFocus), tab_contents());
+  g_signal_connect(content_view, "focus", G_CALLBACK(OnFocusThunk), this);
   g_signal_connect(content_view, "leave-notify-event",
                    G_CALLBACK(OnLeaveNotify), tab_contents());
   g_signal_connect(content_view, "motion-notify-event",
@@ -243,7 +223,7 @@ void TabContentsViewGtk::SizeContents(const gfx::Size& size) {
 void TabContentsViewGtk::Focus() {
   if (tab_contents()->showing_interstitial_page()) {
     tab_contents()->interstitial_page()->Focus();
-  } else {
+  } else if (!constrained_window_) {
     GtkWidget* widget = GetContentNativeView();
     if (widget)
       gtk_widget_grab_focus(widget);
@@ -268,6 +248,10 @@ void TabContentsViewGtk::RestoreFocus() {
     SetInitialFocus();
 }
 
+void TabContentsViewGtk::SetFocusedWidget(GtkWidget* widget) {
+  focus_store_.SetWidget(widget);
+}
+
 void TabContentsViewGtk::UpdateDragCursor(WebDragOperation operation) {
   drag_dest_->UpdateDragStatus(operation);
 }
@@ -332,6 +316,38 @@ void TabContentsViewGtk::InsertIntoContentArea(GtkWidget* widget) {
   gtk_container_add(GTK_CONTAINER(expanded_), widget);
 }
 
+// Called when the content view gtk widget is tabbed to, or after the call to
+// gtk_widget_child_focus() in TakeFocus(). We return true
+// and grab focus if we don't have it. The call to
+// FocusThroughTabTraversal(bool) forwards the "move focus forward" effect to
+// webkit.
+gboolean TabContentsViewGtk::OnFocus(GtkWidget* widget,
+                                     GtkDirectionType focus) {
+  // If we are showing a constrained window, don't allow the native view to take
+  // focus.
+  if (constrained_window_) {
+    // If we return false, it will revert to the default handler, which will
+    // take focus. We don't want that. But if we return true, the event will
+    // stop being propagated, leaving focus wherever it is currently. That is
+    // also bad. So we return false to let the default handler run, but take
+    // focus first so as to trick it into thinking the view was already focused
+    // and allowing the event to propagate.
+    gtk_widget_grab_focus(widget);
+    return FALSE;
+  }
+
+  // If we already have focus, let the next widget have a shot at it. We will
+  // reach this situation after the call to gtk_widget_child_focus() in
+  // TakeFocus().
+  if (gtk_widget_is_focus(widget))
+    return FALSE;
+
+  gtk_widget_grab_focus(widget);
+  bool reverse = focus == GTK_DIR_TAB_BACKWARD;
+  tab_contents()->FocusThroughTabTraversal(reverse);
+  return TRUE;
+}
+
 gboolean TabContentsViewGtk::OnMouseDown(GtkWidget* widget,
                                          GdkEventButton* event) {
   last_mouse_down_ = *event;
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.h b/chrome/browser/tab_contents/tab_contents_view_gtk.h
index e99321d..990ee81 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.h
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.h
@@ -36,6 +36,10 @@ class TabContentsViewGtk : public TabContentsView,
   void AttachConstrainedWindow(ConstrainedWindowGtk* constrained_window);
   void RemoveConstrainedWindow(ConstrainedWindowGtk* constrained_window);
 
+  // Override the stored focus widget. This call only makes sense when the
+  // tab contents is not focused.
+  void SetFocusedWidget(GtkWidget* widget);
+
   // TabContentsView implementation --------------------------------------------
 
   virtual void CreateView(const gfx::Size& initial_size);
@@ -79,6 +83,9 @@ class TabContentsViewGtk : public TabContentsView,
 
   void CancelDragIfAny();
 
+  // Handle focus traversal on the render widget native view.
+  CHROMEGTK_CALLBACK_1(TabContentsViewGtk, gboolean, OnFocus, GtkDirectionType);
+
   // We keep track of the timestamp of the latest mousedown event.
   CHROMEGTK_CALLBACK_1(TabContentsViewGtk, gboolean, OnMouseDown,
                        GdkEventButton*);