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

chrome/browser/debugger/devtools_window.cc

Index: chrome/browser/debugger/devtools_window.cc
diff --git a/chrome/browser/debugger/devtools_window.cc b/chrome/browser/debugger/devtools_window.cc
index 5a128b80ba425e59fc286f1c335c55c34b177ec8..304237c31031ba40309e767332ee84d5d59cea34 100644
--- a/chrome/browser/debugger/devtools_window.cc
+++ b/chrome/browser/debugger/devtools_window.cc
@@ -80,6 +80,12 @@ const char kDockSideBottom[] = "bottom";
 const char kDockSideRight[] = "right";
 const char kDockSideUndocked[] = "undocked";
 
+// Minimal height of devotools pane or content pane when devtools are docked

[Peter Kasting, 2012/10/26 03:22:07]

Nit: devotools

[pfeldman, 2012/10/26 08:56:02]

Done.

+// to the browser window.
+const int kMinDevToolsHeight = 50;
+const int kMinDevToolsWidth = 150;
+const int kMinContentsSize = 50;
+
 // static
 void DevToolsWindow::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(prefs::kDevToolsOpenDocked,
@@ -204,7 +210,9 @@ DevToolsWindow::DevToolsWindow(TabContents* tab_contents,
       browser_(NULL),
       dock_side_(dock_side),
       is_loaded_(false),
-      action_on_load_(DEVTOOLS_TOGGLE_ACTION_SHOW) {
+      action_on_load_(DEVTOOLS_TOGGLE_ACTION_SHOW),
+      width_(-1),
+      height_(-1) {
   frontend_host_ = DevToolsClientHost::CreateDevToolsFrontendHost(
       tab_contents->web_contents(),
       this);
@@ -321,6 +329,56 @@ void DevToolsWindow::Show(DevToolsToggleAction action) {
   ScheduleAction(action);
 }
 
+int DevToolsWindow::GetWidth(int container_width) {
+  if (width_ == -1) {
+    width_ = profile_->GetPrefs()->
+        GetInteger(prefs::kDevToolsVSplitLocation);
+  }
+
+  // By default, size devtools as 1/3 of the browser window.
+  if (width_ == -1)
+    width_ = container_width / 3;
+
+  // Respect the minimum devtools width preset.
+  width_ = std::max(kMinDevToolsWidth, width_);
+
+  // But it should never compromise the content window size.
+  width_ = std::min(container_width - kMinContentsSize, width_);
+  if (width_ < 0)
+    width_ = container_width / 3;

[Peter Kasting, 2012/10/26 03:22:07]

This isn't quite right.  This allows a shrinking window to shrink the devtools
all the way down to zero height, after which point it then suddenly springs up
to nonzero again.

To fix this, change "if (width_ < 0)" to "if (width_ < (kMinContentsSize / 2))"
and add the words "unless the entire window is tiny" to the end of the
"compromise" comment.  This ends up doing what you want (I had to graph this out
to see, though).

[pfeldman, 2012/10/26 03:35:36]

This code is just a fuzzy logic that rescues us in case window is less than 200
pixels. But I can surely do what you are suggesting.

[pfeldman, 2012/10/26 08:56:02]

Done.

+  return width_;
+}
+
+int DevToolsWindow::GetHeight(int container_height) {
+  if (height_ == -1) {
+    height_ = profile_->GetPrefs()->
+        GetInteger(prefs::kDevToolsHSplitLocation);
+  }
+
+  // By default, size devtools as 1/3 of the browser window.
+  if (height_ == -1)
+    height_ = container_height / 3;
+
+  // Respect the minimum devtools width preset.
+  height_ = std::max(kMinDevToolsHeight, height_);
+
+  // But it should never compromise the content window size.
+  height_ = std::min(container_height - kMinContentsSize, height_);
+  if (height_ < 0)
+    height_ = container_height / 3;
+  return height_;
+}
+
+void DevToolsWindow::SetWidth(int width) {
+  width_ = width;

[Peter Kasting, 2012/10/26 03:22:07]

It doesn't seem like these functions actually need to set width_ and height_. 
As long as they write the values to the prefs, GetWidth()/GetHeight() will do
the right thing anyway.

This means these could be eliminated entirely and the caller could just write
the prefs directly.

In fact, you could do even better than this.  If the caller does something like
the following (without loss of generality):

  int previous_width = existing_devtools_window ?
      existing_devtools_window->width : GetPrefs->GetInteger(...);
  int width =
      DevToolsWindow::AdjustWidthForContainer(previous_width, container_width);
  GetPrefs->SetInteger(..., width);
  // ....and set the splitter position

...then you can eliminate the height_ and width_ members entirely and make
GetWidth() and GetHeight() into static methods that simply adjust existing
widths/heights.

Note that with this method you can optionally move the pref reading + writing
around, e.g. the read and write could both move into the static method, or the
write could be moved into a nonstatic method on the DevToolsWindow that gets
auto-called when the size changes (if such a thing is feasible).  These might be
appealing if you don't want the caller to know or care about the existence of
prefs.

[pfeldman, 2012/10/26 03:35:36]

The point here is that width and height should be read / written on per-tab
basis. DevToolsWindow is a container for inspector tab contents and devtools
window properties such as dock side and dimensions. Whenever we switch to the
tab, we look up its DevToolsWindow and apply its properties to the layout.

+  profile_->GetPrefs()->SetInteger(prefs::kDevToolsVSplitLocation, width);
+}
+
+void DevToolsWindow::SetHeight(int height) {
+  height_ = height;
+  profile_->GetPrefs()->SetInteger(prefs::kDevToolsHSplitLocation, height);
+}
+
 RenderViewHost* DevToolsWindow::GetRenderViewHost() {
   return tab_contents_->web_contents()->GetRenderViewHost();
 }