DevTools: “Dock to right” broken after turning a tab into a window of its own.

chrome/browser/ui/gtk/browser_window_gtk.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/gtk/browser_window_gtk.h"
 
 #include <gdk/gdkkeysyms.h>
 
 #include <algorithm>
 #include <string>
@@ skipping 101 matching lines @@
 
 using content::NativeWebKeyboardEvent;
 using content::SSLStatus;
 using content::WebContents;
 
 namespace {
 
 // The number of milliseconds between loading animation frames.
 const int kLoadingAnimationFrameTimeMs = 30;
 
-// Minimal height of devotools pane or content pane when devtools are docked
-// to the browser window.
-const int kMinDevToolsHeight = 50;
-const int kMinDevToolsWidth = 150;
-const int kMinContentsSize = 50;
-
 const char* kBrowserWindowKey = "__BROWSER_WINDOW_GTK__";
 
 // The frame border is only visible in restored mode and is hardcoded to 4 px
 // on each side regardless of the system window border size.
 const int kFrameBorderThickness = 4;
 // While resize areas on Windows are normally the same size as the window
 // borders, our top area is shrunk by 1 px to make it easier to move the window
 // around with our thinner top grabbable strip.  (Incidentally, our side and
 // bottom resize areas don't match the frame border thickness either -- they
 // span the whole nonclient area, so there's no "dead zone" for the mouse.)
@@ skipping 93 matching lines @@
     :  window_(NULL),
        window_container_(NULL),
        window_vbox_(NULL),
        render_area_vbox_(NULL),
        render_area_floating_container_(NULL),
        render_area_event_box_(NULL),
        toolbar_border_(NULL),
        browser_(browser),
        state_(GDK_WINDOW_STATE_WITHDRAWN),
        devtools_dock_side_(DEVTOOLS_DOCK_SIDE_BOTTOM),
+       devtools_window_(NULL),
        contents_hsplit_(NULL),
        contents_vsplit_(NULL),
        frame_cursor_(NULL),
        is_active_(false),
        show_state_after_show_(ui::SHOW_STATE_DEFAULT),
        suppress_window_raise_(false),
        accel_group_(NULL),
        fullscreen_exit_bubble_type_(
            FEB_TYPE_BROWSER_FULLSCREEN_EXIT_INSTRUCTION) {
 }
@@ skipping 479 matching lines @@
 
 void BrowserWindowGtk::BookmarkBarStateChanged(
     BookmarkBar::AnimateChangeType change_type) {
   MaybeShowBookmarkBar(change_type == BookmarkBar::ANIMATE_STATE_CHANGE);
 }
 
 void BrowserWindowGtk::UpdateDevTools() {
   UpdateDevToolsForContents(chrome::GetActiveWebContents(browser_.get()));
 }
 
-void BrowserWindowGtk::SetDevToolsDockSide(DevToolsDockSide side) {
-  if (devtools_dock_side_ == side)
-    return;
-
-  if (devtools_container_->tab()) {
-    HideDevToolsContainer();
-    ShowDevToolsContainer(side);
-  } else {
-    devtools_dock_side_ = side;
-  }
-}
-
 void BrowserWindowGtk::UpdateLoadingAnimations(bool should_animate) {
   if (should_animate) {
     if (!loading_animation_timer_.IsRunning()) {
       // Loads are happening, and the timer isn't running, so start it.
       loading_animation_timer_.Start(FROM_HERE,
           base::TimeDelta::FromMilliseconds(kLoadingAnimationFrameTimeMs), this,
           &BrowserWindowGtk::LoadingAnimationCallback);
     }
   } else {
     if (loading_animation_timer_.IsRunning()) {
@@ skipping 546 matching lines @@
 
 extensions::ActiveTabPermissionGranter*
     BrowserWindowGtk::GetActiveTabPermissionGranter() {
   TabContents* tab = GetDisplayedTab();
   if (!tab)
     return NULL;
   return extensions::TabHelper::FromWebContents(tab->web_contents())->
       active_tab_permission_granter();
 }
 
-void BrowserWindowGtk::UpdateDevToolsForContents(WebContents* contents) {
-  TRACE_EVENT0("ui::gtk", "BrowserWindowGtk::UpdateDevToolsForContents");
-  TabContents* old_devtools = devtools_container_->tab();
-  DevToolsWindow* devtools_window = contents ?
-      DevToolsWindow::GetDockedInstanceForInspectedTab(contents) : NULL;
-  TabContents* devtools_contents =
-      devtools_window ? devtools_window->tab_contents() : NULL;
-
-  if (old_devtools == devtools_contents) {
-    if (devtools_contents &&
-        devtools_window->dock_side() != devtools_dock_side_)
-      SetDevToolsDockSide(devtools_window->dock_side());
-    return;
-  }
-
-  if (old_devtools)
-    devtools_container_->DetachTab(old_devtools);
-
-  devtools_container_->SetTab(devtools_contents);
-  if (devtools_contents) {
-    // WebContentsViewGtk::WasShown is not called when tab contents is shown by
-    // anything other than user selecting a Tab.
-    // See TabContentsViewViews::OnWindowPosChanged for reference on how it
-    // should be implemented.
-    devtools_contents->web_contents()->WasShown();
-  }
-
-  bool should_show = old_devtools == NULL && devtools_contents != NULL;
-  bool should_hide = old_devtools != NULL && devtools_contents == NULL;
-
-  if (should_show)
-    ShowDevToolsContainer(devtools_window->dock_side());
-  else if (should_hide)
-    HideDevToolsContainer();
-}
-
-void BrowserWindowGtk::ShowDevToolsContainer(DevToolsDockSide dock_side) {
-  devtools_dock_side_ = dock_side;
-  bool dock_to_right = dock_side == DEVTOOLS_DOCK_SIDE_RIGHT;
-
-  GtkAllocation contents_rect;
-  gtk_widget_get_allocation(contents_vsplit_, &contents_rect);
-  int content_size =
-      dock_to_right ? contents_rect.width : contents_rect.height;
-
-  int split_offset = browser_->profile()->GetPrefs()->
-      GetInteger(dock_to_right ? prefs::kDevToolsVSplitLocation :
-                                 prefs::kDevToolsHSplitLocation);
-  int min_size =
-      dock_to_right ? kMinDevToolsWidth : kMinDevToolsHeight;
-
-  if (split_offset == -1)
-    split_offset = content_size * 1 / 3;
-  // Make sure user can see both panes.
-  split_offset = std::max(min_size, split_offset);
-  split_offset = std::min(content_size - kMinContentsSize, split_offset);
-  if (split_offset < 0)
-    split_offset = content_size * 1 / 3;
-  if (dock_to_right) {
-    gtk_paned_pack2(GTK_PANED(contents_hsplit_), devtools_container_->widget(),
-                    FALSE, TRUE);
-    gtk_paned_set_position(GTK_PANED(contents_hsplit_),
-                           content_size - split_offset);
-  } else {
-    gtk_paned_pack2(GTK_PANED(contents_vsplit_), devtools_container_->widget(),
-                    FALSE, TRUE);
-    gtk_paned_set_position(GTK_PANED(contents_vsplit_),
-                           content_size - split_offset);
-  }
-  gtk_widget_show(devtools_container_->widget());
-}
-
-void BrowserWindowGtk::HideDevToolsContainer() {
-  GtkAllocation contents_rect;
-  gtk_widget_get_allocation(contents_vsplit_, &contents_rect);
-  bool dock_to_right = devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT;
-  gint split_offset;
-  if (dock_to_right) {
-    split_offset = contents_rect.width -
-        gtk_paned_get_position(GTK_PANED(contents_hsplit_));
-    gtk_container_remove(GTK_CONTAINER(contents_hsplit_),
-                         devtools_container_->widget());
-  } else {
-    split_offset = contents_rect.height -
-        gtk_paned_get_position(GTK_PANED(contents_vsplit_));
-    gtk_container_remove(GTK_CONTAINER(contents_vsplit_),
-                         devtools_container_->widget());
-  }
-
-  browser_->profile()->GetPrefs()->
-      SetInteger(dock_to_right ? prefs::kDevToolsVSplitLocation :
-                                 prefs::kDevToolsHSplitLocation,
-                 split_offset);
-}
-
 void BrowserWindowGtk::DestroyBrowser() {
   browser_.reset();
 }
 
 gboolean BrowserWindowGtk::OnConfigure(GtkWidget* widget,
                                        GdkEventConfigure* event) {
   gfx::Rect bounds(event->x, event->y, event->width, event->height);
 
   // When the window moves, we'll get multiple configure-event signals. We can
   // also get events when the bounds haven't changed, but the window's stacking
@@ skipping 348 matching lines @@
 
   status_bubble_.reset(new StatusBubbleGtk(browser_->profile()));
 
   contents_container_.reset(new TabContentsContainerGtk(status_bubble_.get()));
   devtools_container_.reset(new TabContentsContainerGtk(NULL));
   ViewIDUtil::SetID(devtools_container_->widget(), VIEW_ID_DEV_TOOLS_DOCKED);
 
   contents_hsplit_ = gtk_hpaned_new();
   gtk_paned_pack1(GTK_PANED(contents_hsplit_), contents_container_->widget(),
                   TRUE, TRUE);
-
   contents_vsplit_ = gtk_vpaned_new();
   gtk_paned_pack1(GTK_PANED(contents_vsplit_), contents_hsplit_, TRUE, TRUE);
 
   gtk_box_pack_end(GTK_BOX(render_area_vbox_),
                    contents_vsplit_, TRUE, TRUE, 0);
 
   gtk_widget_show_all(render_area_floating_container_);
   render_area_event_box_ = gtk_event_box_new();
   // Set a white background so during startup the user sees white in the
   // content area before we get a WebContents in place.
@@ skipping 615 matching lines @@
 bool BrowserWindowGtk::DrawFrameAsActive() const {
   if (ui::ActiveWindowWatcherX::WMSupportsActivation())
     return is_active_;
 
   // Since we don't get notifications when the active state of the frame
   // changes, we can't consistently repaint the frame at the right time. Instead
   // we always draw the frame as active.
   return true;
 }
 
+void BrowserWindowGtk::UpdateDevToolsForContents(WebContents* contents) {
+  TRACE_EVENT0("ui::gtk", "BrowserWindowGtk::UpdateDevToolsForContents");
+  DevToolsWindow* new_devtools_window = contents ?
+      DevToolsWindow::GetDockedInstanceForInspectedTab(contents) : NULL;
+
+  // Fast return in case of the same window having same orientation.
+  if (devtools_window_ == new_devtools_window) {
+    if (!new_devtools_window ||

[Evan Stade, 2012/10/26 03:56:47]

why nested ifs instead of one big if?

[pfeldman, 2012/10/26 03:59:52]

It is already hard to parse that logic, I thought that explicitly guarding the
equal windows case would ease the pain. I'll do ternary + comment if you insist
though.

[pfeldman, 2012/10/26 08:51:29]

Done.

+        (new_devtools_window->dock_side() == devtools_dock_side_)) {
+      return;
+    }
+  }
+
+  // Replace tab contents.
+  if (devtools_window_ != new_devtools_window) {
+    if (devtools_window_)
+      devtools_container_->DetachTab(devtools_window_->tab_contents());
+    devtools_container_->SetTab(
+        new_devtools_window ? new_devtools_window->tab_contents() : NULL);
+    if (new_devtools_window) {
+      // WebContentsViewGtk::WasShown is not called when tab contents is shown
+      // by anything other than user selecting a Tab.
+      // See TabContentsViewViews::OnWindowPosChanged for reference on how it
+      // should be implemented.
+      new_devtools_window->tab_contents()->web_contents()->WasShown();
+    }
+  }
+
+  // Store last used position.
+  if (devtools_window_) {
+    GtkAllocation contents_rect;
+    gtk_widget_get_allocation(contents_vsplit_, &contents_rect);
+    if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) {
+      devtools_window_->SetWidth(
+          contents_rect.width -
+          gtk_paned_get_position(GTK_PANED(contents_hsplit_)));
+    } else {
+      devtools_window_->SetHeight(
+          contents_rect.height -
+          gtk_paned_get_position(GTK_PANED(contents_vsplit_)));
+    }
+  }
+
+  // Show / hide container if necessary. Changing dock orientation is
+  // hide + show.
+  bool should_hide = devtools_window_ && (!new_devtools_window ||
+      devtools_dock_side_ != new_devtools_window->dock_side());
+  bool should_show = new_devtools_window && (!devtools_window_ || should_hide);
+
+  if (should_hide)
+    HideDevToolsContainer();
+
+  devtools_window_ = new_devtools_window;
+
+  if (should_show) {
+    devtools_dock_side_ = new_devtools_window->dock_side();
+    ShowDevToolsContainer();
+  } else if (new_devtools_window) {
+    UpdateDevToolsSplitPosition();
+  }
+}
+
+void BrowserWindowGtk::ShowDevToolsContainer() {
+  if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) {

[Evan Stade, 2012/10/26 03:56:47]

I might turn this into a ternary operator

[pfeldman, 2012/10/26 03:59:52]

I wanted this to match the other ports where it takes more lines to complete
this. But I can do ternary!

[pfeldman, 2012/10/26 08:51:29]

Done.

+    gtk_paned_pack2(GTK_PANED(contents_hsplit_), devtools_container_->widget(),
+                    FALSE, TRUE);
+  } else {
+    gtk_paned_pack2(GTK_PANED(contents_vsplit_), devtools_container_->widget(),
+                    FALSE, TRUE);
+  }
+  UpdateDevToolsSplitPosition();
+  gtk_widget_show(devtools_container_->widget());
+}
+
+void BrowserWindowGtk::HideDevToolsContainer() {
+  if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) {

[Evan Stade, 2012/10/26 03:56:47]

I might turn this into a ternary operator

[pfeldman, 2012/10/26 03:59:52]

ditto

[pfeldman, 2012/10/26 08:51:29]

Done.

+    gtk_container_remove(GTK_CONTAINER(contents_hsplit_),
+                         devtools_container_->widget());
+  } else {
+    gtk_container_remove(GTK_CONTAINER(contents_vsplit_),
+                         devtools_container_->widget());
+  }
+}
+
+void BrowserWindowGtk::UpdateDevToolsSplitPosition() {
+  GtkAllocation contents_rect;
+  gtk_widget_get_allocation(contents_vsplit_, &contents_rect);
+
+  if (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) {
+    int split_offset = contents_rect.width -
+        devtools_window_->GetWidth(contents_rect.width);
+    gtk_paned_set_position(GTK_PANED(contents_hsplit_), split_offset);
+  } else {
+    int split_offset = contents_rect.height -
+        devtools_window_->GetHeight(contents_rect.height);
+    gtk_paned_set_position(GTK_PANED(contents_vsplit_), split_offset);
+  }
+}
+
 // static
 bool BrowserWindowGtk::GetCustomFramePrefDefault() {
   // Ideally, we'd use the custom frame by default and just fall back on using
   // system decorations for the few (?) tiling window managers where the custom
   // frame doesn't make sense (e.g. awesome, ion3, ratpoison, xmonad, etc.) or
   // other WMs where it has issues (e.g. Fluxbox -- see issue 19130).  The EWMH
   // _NET_SUPPORTING_WM property makes it easy to look up a name for the current
   // WM, but at least some of the WMs in the latter group don't set it.
   // Instead, we default to using system decorations for all WMs and
   // special-case the ones where the custom frame should be used.
   ui::WindowManagerName wm_type = ui::GuessWindowManager();
   return (wm_type == ui::WM_BLACKBOX ||
           wm_type == ui::WM_COMPIZ ||
           wm_type == ui::WM_ENLIGHTENMENT ||
           wm_type == ui::WM_METACITY ||
           wm_type == ui::WM_MUTTER ||
           wm_type == ui::WM_OPENBOX ||
           wm_type == ui::WM_XFWM4);
 }
 
 // static
 BrowserWindow* BrowserWindow::CreateBrowserWindow(Browser* browser) {
   BrowserWindowGtk* browser_window_gtk = new BrowserWindowGtk(browser);
   browser_window_gtk->Init();
   return browser_window_gtk;
 }