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..bb704a3d9aa97f79290739e49dde6f823f7112fa 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
+// 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,46 @@ void DevToolsWindow::Show(DevToolsToggleAction action) {
   ScheduleAction(action);
 }
 
+int DevToolsWindow::GetWidth(int container_width) {
+  if (width_ == -1) {
+    width_ = profile_->GetPrefs()->
+        GetInteger(prefs::kDevToolsVSplitLocation);
+  }
+
+  if (width_ == -1)
+    width_ = container_width * 1 / 3;

[Peter Kasting, 2012/10/24 21:18:09]

Nit: Remove "* 1"

[pfeldman, 2012/10/25 15:17:40]

Done.

+
+  width_ = std::max(kMinDevToolsWidth, width_);
+  width_ = std::min(container_width - kMinContentsSize, width_);

[Peter Kasting, 2012/10/24 21:18:09]

This means that the minimum constant isn't a true minimum.

Your code and constants need better documentation -- the constants to say which
rules win when there's conflict, the code to document assumptions like "if
GetWidth() is being called, the tools are assumed to be docked to the side".

[pfeldman, 2012/10/25 15:17:40]

Done.

+  if (width_ < 0)
+    width_ = width_ * 1 / 3;

[Peter Kasting, 2012/10/24 21:18:09]

This makes no sense.  If width is negative, leave it negative but make it a
third as big?

[pfeldman, 2012/10/25 15:17:40]

Done.

Thanks for spotting it, it should have been container_width / 3.

+  return width_;
+}
+
+int DevToolsWindow::GetHeight(int container_height) {

[Peter Kasting, 2012/10/24 21:18:09]

All the same comments apply.

[pfeldman, 2012/10/25 15:17:40]

Done.

+  if (height_ == -1) {
+    height_ = profile_->GetPrefs()->
+        GetInteger(prefs::kDevToolsHSplitLocation);
+  }
+  if (height_ == -1)
+    height_ = container_height * 1 / 3;
+  height_ = std::max(kMinDevToolsHeight, height_);
+  height_ = std::min(container_height - kMinContentsSize, height_);
+  if (height_ < 0)
+    height_ = height_ * 1 / 3;
+  return height_;
+}
+
+void DevToolsWindow::SetWidth(int width) {
+  width_ = width;

[Peter Kasting, 2012/10/24 21:18:09]

These setters need to clamp to the minima.

[pfeldman, 2012/10/25 15:17:40]

Then scaling devtools window to say 100px will restore it upon the tab switch
back and forth. I was thinking that validation on read would be sufficient.

+  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();
 }