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

chrome/browser/debugger/devtools_window.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 <algorithm>
 
 #include "base/command_line.h"
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/string_number_conversions.h"
@@ skipping 62 matching lines @@
 const char kOldPrefRight[] = "right";
 
 const char kPrefBottom[] = "dock_bottom";
 const char kPrefRight[] = "dock_right";
 const char kPrefUndocked[] = "undocked";
 
 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,
                              true,
                              PrefService::UNSYNCABLE_PREF);
   prefs->RegisterStringPref(prefs::kDevToolsDockSide,
                             kDockSideBottom,
                             PrefService::UNSYNCABLE_PREF);
   prefs->RegisterDictionaryPref(prefs::kDevToolsEditedFiles,
                                 PrefService::UNSYNCABLE_PREF);
@@ skipping 104 matching lines @@
 DevToolsWindow::DevToolsWindow(TabContents* tab_contents,
                                Profile* profile,
                                RenderViewHost* inspected_rvh,
                                DevToolsDockSide dock_side)
     : profile_(profile),
       inspected_tab_(NULL),
       tab_contents_(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);
   file_helper_.reset(new DevToolsFileHelper(profile, this));
 
   g_instances.Get().push_back(this);
   // Wipe out page icon so that the default application icon is used.
   NavigationEntry* entry =
       tab_contents_->web_contents()->GetController().GetActiveEntry();
   entry->GetFavicon().image = gfx::Image();
@@ skipping 96 matching lines @@
     CreateDevToolsBrowser();
 
   if (should_show_window) {
     browser_->window()->Show();
     tab_contents_->web_contents()->GetView()->SetInitialFocus();
   }
 
   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();
 }
 
 void DevToolsWindow::CreateDevToolsBrowser() {
   // TODO(pfeldman): Make browser's getter for this key static.
   std::string wp_key;
   wp_key.append(prefs::kBrowserWindowPlacement);
   wp_key.append("_");
   wp_key.append(kDevToolsApp);
@@ skipping 501 matching lines @@
 
 // static
 DevToolsDockSide DevToolsWindow::SideFromString(
     const std::string& dock_side) {
   if (dock_side == kDockSideRight)
     return DEVTOOLS_DOCK_SIDE_RIGHT;
   if (dock_side == kDockSideBottom)
     return DEVTOOLS_DOCK_SIDE_BOTTOM;
   return DEVTOOLS_DOCK_SIDE_UNDOCKED;
 }