Linux: Improve info bubble placement.

Make InfoBubble's constructor take positions relative to the browser
window's origin, rather than the absolute positions used by Views.

Otherwise, we run into issues when the browser window is being moved while
the bubble is created -- this is unlikely to happen for bookmark bubbles,
but when the first run bubble is created, the browser window may or may not
be in the final place that the window manager is going to put it.  There
was previously a 200 ms delay before opening the bubble, but that approach
still fails sometimes when X is being forwarded over a network connection.

This change makes us update the bubble's position as we receive
ConfigureNotify events about its browser window.

TEST=checked that first-run and bookmark bubbles are placed correctly in openbox, KDE, metacity, fluxbox, and ion3

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

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

5 files changed, 125 insertions, 78 deletions

diff --git a/chrome/browser/gtk/info_bubble_gtk.cc b/chrome/browser/gtk/info_bubble_gtk.cc
index 3875d80..2e6adb6 100644
--- a/chrome/browser/gtk/info_bubble_gtk.cc
+++ b/chrome/browser/gtk/info_bubble_gtk.cc
@@ -137,8 +137,7 @@ InfoBubbleGtk::InfoBubbleGtk(GtkThemeProvider* provider)
       window_(NULL),
       theme_provider_(provider),
       accel_group_(gtk_accel_group_new()),
-      screen_x_(0),
-      screen_y_(0),
+      toplevel_window_(NULL),
       mask_region_(NULL) {
 }
 
@@ -148,12 +147,23 @@ InfoBubbleGtk::~InfoBubbleGtk() {
     gdk_region_destroy(mask_region_);
     mask_region_ = NULL;
   }
+
+  g_signal_handlers_disconnect_by_func(
+      toplevel_window_,
+      reinterpret_cast<gpointer>(HandleToplevelConfigureThunk),
+      this);
+  g_signal_handlers_disconnect_by_func(
+      toplevel_window_,
+      reinterpret_cast<gpointer>(HandleToplevelUnmapThunk),
+      this);
+  toplevel_window_ = NULL;
 }
 
 void InfoBubbleGtk::Init(GtkWindow* toplevel_window,
                          const gfx::Rect& rect,
                          GtkWidget* content) {
   DCHECK(!window_);
+  toplevel_window_ = toplevel_window;
   rect_ = rect;
 
   window_ = gtk_window_new(GTK_WINDOW_POPUP);
@@ -180,14 +190,14 @@ void InfoBubbleGtk::Init(GtkWindow* toplevel_window,
   // HandleSizeAllocate, so the mask can be applied to the GdkWindow.
   gtk_widget_realize(window_);
 
-  UpdateScreenX();
-  screen_y_ = rect.y() + rect.height() + kArrowToContentPadding;
   // For RTL, we will have to move the window again when it is allocated, but
   // this should be somewhat close to its final position.
-  gtk_window_move(GTK_WINDOW(window_), screen_x_, screen_y_);
+  MoveWindow();
   GtkRequisition req;
   gtk_widget_size_request(window_, &req);
 
+  StackWindow();
+
   gtk_widget_add_events(window_, GDK_BUTTON_PRESS_MASK |
                                  GDK_BUTTON_RELEASE_MASK);
 
@@ -200,28 +210,12 @@ void InfoBubbleGtk::Init(GtkWindow* toplevel_window,
   g_signal_connect(window_, "destroy",
                    G_CALLBACK(&HandleDestroyThunk), this);
 
-  gtk_widget_show_all(window_);
+  g_signal_connect(toplevel_window, "configure-event",
+                   G_CALLBACK(&HandleToplevelConfigureThunk), this);
+  g_signal_connect(toplevel_window, "unmap-event",
+                   G_CALLBACK(&HandleToplevelUnmapThunk), this);
 
-  // Stack our window directly above the toplevel window.  Our window is a
-  // direct child of the root window, so we need to find a similar ancestor
-  // for the toplevel window (which might have been reparented by a window
-  // manager).
-  XID toplevel_window_base = x11_util::GetHighestAncestorWindow(
-      x11_util::GetX11WindowFromGtkWidget(GTK_WIDGET(toplevel_window)),
-      x11_util::GetX11RootWindow());
-  if (toplevel_window_base) {
-    XID window_xid = x11_util::GetX11WindowFromGtkWidget(GTK_WIDGET(window_));
-    XID window_parent = x11_util::GetParentWindow(window_xid);
-    if (window_parent == x11_util::GetX11RootWindow()) {
-      x11_util::RestackWindow(window_xid, toplevel_window_base, true);
-    } else {
-      // The window manager shouldn't reparent override-redirect windows.
-      DLOG(ERROR) << "override-redirect window " << window_xid
-                  << "'s parent is " << window_parent
-                  << ", rather than root window "
-                  << x11_util::GetX11RootWindow();
-    }
-  }
+  gtk_widget_show_all(window_);
 
   // We add a GTK (application-level) grab.  This means we will get all
   // mouse events for our application, even if they were delivered on another
@@ -246,13 +240,46 @@ void InfoBubbleGtk::Init(GtkWindow* toplevel_window,
   theme_provider_->InitThemesFor(this);
 }
 
-void InfoBubbleGtk::UpdateScreenX() {
+void InfoBubbleGtk::MoveWindow() {
+  gint toplevel_x = 0, toplevel_y = 0;
+  gdk_window_get_position(
+      GTK_WIDGET(toplevel_window_)->window, &toplevel_x, &toplevel_y);
+
+  gint screen_x = 0;
   if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) {
-    screen_x_ = rect_.x() + (rect_.width() / 2) - window_->allocation.width
-                + kArrowX;
+    screen_x = toplevel_x + rect_.x() + (rect_.width() / 2) -
+               window_->allocation.width + kArrowX;
   }
   else {
-    screen_x_ = rect_.x() + (rect_.width() / 2) - kArrowX;
+    screen_x = toplevel_x + rect_.x() + (rect_.width() / 2) - kArrowX;
+  }
+
+  gint screen_y = toplevel_y + rect_.y() + rect_.height() +
+                  kArrowToContentPadding;
+
+  gtk_window_move(GTK_WINDOW(window_), screen_x, screen_y);
+}
+
+void InfoBubbleGtk::StackWindow() {
+  // Stack our window directly above the toplevel window.  Our window is a
+  // direct child of the root window, so we need to find a similar ancestor
+  // for the toplevel window (which might have been reparented by a window
+  // manager).
+  XID toplevel_window_base = x11_util::GetHighestAncestorWindow(
+      x11_util::GetX11WindowFromGtkWidget(GTK_WIDGET(toplevel_window_)),
+      x11_util::GetX11RootWindow());
+  if (toplevel_window_base) {
+    XID window_xid = x11_util::GetX11WindowFromGtkWidget(GTK_WIDGET(window_));
+    XID window_parent = x11_util::GetParentWindow(window_xid);
+    if (window_parent == x11_util::GetX11RootWindow()) {
+      x11_util::RestackWindow(window_xid, toplevel_window_base, true);
+    } else {
+      // The window manager shouldn't reparent override-redirect windows.
+      DLOG(ERROR) << "override-redirect window " << window_xid
+                  << "'s parent is " << window_parent
+                  << ", rather than root window "
+                  << x11_util::GetX11RootWindow();
+    }
   }
 }
 
@@ -320,10 +347,8 @@ gboolean InfoBubbleGtk::HandleEscape() {
 // When our size is initially allocated or changed, we need to recompute
 // and apply our shape mask region.
 void InfoBubbleGtk::HandleSizeAllocate() {
-  if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) {
-    UpdateScreenX();
-    gtk_window_move(GTK_WINDOW(window_), screen_x_, screen_y_);
-  }
+  if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT)
+    MoveWindow();
 
   DCHECK(window_->allocation.x == 0 && window_->allocation.y == 0);
   if (mask_region_) {
@@ -360,3 +385,14 @@ gboolean InfoBubbleGtk::HandleDestroy() {
   delete this;
   return FALSE;  // Propagate.
 }
+
+gboolean InfoBubbleGtk::HandleToplevelConfigure(GdkEventConfigure* event) {
+  MoveWindow();
+  StackWindow();
+  return FALSE;
+}
+
+gboolean InfoBubbleGtk::HandleToplevelUnmap() {
+  Close();
+  return FALSE;
+}
diff --git a/chrome/browser/gtk/info_bubble_gtk.h b/chrome/browser/gtk/info_bubble_gtk.h
index 738e0f1..a92cd3b 100644
--- a/chrome/browser/gtk/info_bubble_gtk.h
+++ b/chrome/browser/gtk/info_bubble_gtk.h
@@ -39,12 +39,12 @@ class InfoBubbleGtkDelegate {
 
 class InfoBubbleGtk : public NotificationObserver {
  public:
-  // Show an InfoBubble, pointing at the area |rect| (in screen coordinates).
-  // An info bubble will try to fit on the screen, so it can point to any edge
-  // of |rect|.  The bubble will host the |content| widget.  The |delegate| will
-  // be notified when the bubble is closed.  The bubble will perform an X grab
-  // of the pointer and keyboard, and will close itself if a click is received
-  // outside of the bubble.
+  // Show an InfoBubble, pointing at the area |rect| (in coordinates relative to
+  // |toplevel_window|'s origin).  An info bubble will try to fit on the screen,
+  // so it can point to any edge of |rect|.  The bubble will host the |content|
+  // widget.  The |delegate| will be notified when the bubble is closed.  The
+  // bubble will perform an X grab of the pointer and keyboard, and will close
+  // itself if a click is received outside of the bubble.
   // TODO(derat): This implementation doesn't try to position itself onscreen.
   static InfoBubbleGtk* Show(GtkWindow* toplevel_window,
                              const gfx::Rect& rect,
@@ -80,8 +80,13 @@ class InfoBubbleGtk : public NotificationObserver {
             const gfx::Rect& rect,
             GtkWidget* content);
 
-  // Sets |screen_x_| according to our allocation and |rect_|.
-  void UpdateScreenX();
+  // Calculate the current screen position for the bubble's window (per
+  // |toplevel_window_|'s position as of its most-recent ConfigureNotify event
+  // and |rect_|) and move it there.
+  void MoveWindow();
+
+  // Restack the bubble's window directly above |toplevel_window_|.
+  void StackWindow();
 
   // Sets the delegate.
   void set_delegate(InfoBubbleGtkDelegate* delegate) { delegate_ = delegate; }
@@ -112,27 +117,41 @@ class InfoBubbleGtk : public NotificationObserver {
 
   static gboolean HandleButtonPressThunk(GtkWidget* widget,
                                          GdkEventButton* event,
-                                         gpointer userdata) {
-    return reinterpret_cast<InfoBubbleGtk*>(userdata)->
+                                         gpointer user_data) {
+    return reinterpret_cast<InfoBubbleGtk*>(user_data)->
         HandleButtonPress(event);
   }
   gboolean HandleButtonPress(GdkEventButton* event);
 
   static gboolean HandleButtonReleaseThunk(GtkWidget* widget,
                                            GdkEventButton* event,
-                                           gpointer userdata) {
-    return reinterpret_cast<InfoBubbleGtk*>(userdata)->
+                                           gpointer user_data) {
+    return reinterpret_cast<InfoBubbleGtk*>(user_data)->
         HandleButtonRelease(event);
   }
   gboolean HandleButtonRelease(GdkEventButton* event);
 
   static gboolean HandleDestroyThunk(GtkWidget* widget,
-                                     gpointer userdata) {
-    return reinterpret_cast<InfoBubbleGtk*>(userdata)->
-        HandleDestroy();
+                                     gpointer user_data) {
+    return reinterpret_cast<InfoBubbleGtk*>(user_data)->HandleDestroy();
   }
   gboolean HandleDestroy();
 
+  static gboolean HandleToplevelConfigureThunk(GtkWidget* widget,
+                                               GdkEventConfigure* event,
+                                               gpointer user_data) {
+    return reinterpret_cast<InfoBubbleGtk*>(user_data)->
+        HandleToplevelConfigure(event);
+  }
+  gboolean HandleToplevelConfigure(GdkEventConfigure* event);
+
+  static gboolean HandleToplevelUnmapThunk(GtkWidget* widget,
+                                           GdkEvent* event,
+                                           gpointer user_data) {
+    return reinterpret_cast<InfoBubbleGtk*>(user_data)->HandleToplevelUnmap();
+  }
+  gboolean HandleToplevelUnmap();
+
   // The caller supplied delegate, can be NULL.
   InfoBubbleGtkDelegate* delegate_;
 
@@ -146,12 +165,12 @@ class InfoBubbleGtk : public NotificationObserver {
   // The accel group attached to |window_|, to handle closing with escape.
   GtkAccelGroup* accel_group_;
 
-  // The rectangle that we use to calculate |screen_x_| and |screen_y_|.
-  gfx::Rect rect_;
+  // The window for which we're being shown (and to which |rect_| is relative).
+  GtkWindow* toplevel_window_;
 
-  // Where we want our window to be positioned on the screen.
-  int screen_x_;
-  int screen_y_;
+  // Provides an offset from |toplevel_window_|'s origin for MoveWindow() to
+  // use.
+  gfx::Rect rect_;
 
   // The current shape of |window_| (used to test whether clicks fall in it or
   // not).
diff --git a/chrome/browser/gtk/location_bar_view_gtk.cc b/chrome/browser/gtk/location_bar_view_gtk.cc
index 3a7544f..d1761e6 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/gtk/location_bar_view_gtk.cc
@@ -48,8 +48,6 @@ const int kBorderThickness = 1;
 const int kFirstRunBubbleLeftMargin = 8;
 // Extra vertical spacing for first run bubble.
 const int kFirstRunBubbleTopMargin = 1;
-// Task delay (in milliseconds) to show first run bubble.
-const int kFirstRunBubbleTaskDelay = 200;
 
 // The padding around the top, bottom, and sides of the location bar hbox.
 // We don't want to edit control's text to be right against the edge,
@@ -363,10 +361,11 @@ std::wstring LocationBarViewGtk::GetTitle() const {
 }
 
 void LocationBarViewGtk::ShowFirstRunBubble(bool use_OEM_bubble) {
+  // We need the browser window to be shown before we can show the bubble, but
+  // we get called before that's happened.
   Task* task = first_run_bubble_.NewRunnableMethod(
       &LocationBarViewGtk::ShowFirstRunBubbleInternal, use_OEM_bubble);
-  MessageLoop::current()->PostDelayedTask(FROM_HERE, task,
-                                          kFirstRunBubbleTaskDelay);
+  MessageLoop::current()->PostTask(FROM_HERE, task);
 }
 
 std::wstring LocationBarViewGtk::GetInputString() const {
@@ -636,22 +635,21 @@ void LocationBarViewGtk::ShowFirstRunBubbleInternal(bool use_OEM_bubble) {
   if (!location_entry_.get() || !widget()->window)
     return;
 
-  gint x, y;
-  gdk_window_get_origin(widget()->window, &x, &y);
+  int x = widget()->allocation.x;
+  int y = widget()->allocation.y;
+
   // The bubble needs to be just below the Omnibox and slightly to the right
   // of star button, so shift x and y co-ordinates.
-  y += widget()->allocation.y + widget()->allocation.height +
-       kFirstRunBubbleTopMargin;
-  if (l10n_util::GetTextDirection() == l10n_util::LEFT_TO_RIGHT) {
-    x += widget()->allocation.x + kFirstRunBubbleLeftMargin;
-  } else {
-    x += widget()->allocation.x + widget()->allocation.width -
-         kFirstRunBubbleLeftMargin;
-  }
+  y += widget()->allocation.height + kFirstRunBubbleTopMargin;
+  if (l10n_util::GetTextDirection() == l10n_util::LEFT_TO_RIGHT)
+    x += kFirstRunBubbleLeftMargin;
+  else
+    x += widget()->allocation.width - kFirstRunBubbleLeftMargin;
 
   FirstRunBubble::Show(profile_,
-      GTK_WINDOW(gtk_widget_get_toplevel(widget())),
-      gfx::Rect(x, y, 0, 0), use_OEM_bubble);
+                       GTK_WINDOW(gtk_widget_get_toplevel(widget())),
+                       gfx::Rect(x, y, 0, 0),
+                       use_OEM_bubble);
 }
 
 // static
diff --git a/chrome/browser/gtk/location_bar_view_gtk.h b/chrome/browser/gtk/location_bar_view_gtk.h
index 21d4d75..bb86a69 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.h
+++ b/chrome/browser/gtk/location_bar_view_gtk.h
@@ -173,6 +173,7 @@ class LocationBarViewGtk : public AutocompleteEditController,
 
   // Set the keyword text for the Search BLAH: keyword box.
   void SetKeywordLabel(const std::wstring& keyword);
+
   // Set the keyword text for the "Press tab to search BLAH" hint box.
   void SetKeywordHintLabel(const std::wstring& keyword);
 
diff --git a/chrome/browser/gtk/toolbar_star_toggle_gtk.cc b/chrome/browser/gtk/toolbar_star_toggle_gtk.cc
index c122a9e..a634618 100644
--- a/chrome/browser/gtk/toolbar_star_toggle_gtk.cc
+++ b/chrome/browser/gtk/toolbar_star_toggle_gtk.cc
@@ -66,15 +66,8 @@ void ToolbarStarToggleGtk::Observe(NotificationType type,
 void ToolbarStarToggleGtk::ShowStarBubble(const GURL& url,
                                           bool newly_bookmarked) {
   GtkWidget* widget = widget_.get();
-  gint x, y;
-  gdk_window_get_origin(widget->window, &x, &y);
-  x += widget->allocation.x;
-  y += widget->allocation.y;
-  gint width = widget->allocation.width;
-  gint height = widget->allocation.height;
-
   BookmarkBubbleGtk::Show(GTK_WINDOW(gtk_widget_get_toplevel(widget)),
-                          gfx::Rect(x, y, width, height),
+                          gfx::Rect(widget->allocation),
                           host_->profile(),
                           url,
                           newly_bookmarked);