DevTools window doesn't restore its minimal size if it's shrinked to zero.

DevTools window doesn't restore its minimal size if it's shrinked to zero.

BUG=85185
TEST=none
R=yurys, pfeldman

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88512

[loislo, 2011-06-07 14:08:02]

Old min size check doesn't works because split_offset is the offset from the top
of the contents widget.

[yurys, 2011-06-07 14:08:40]

On 2011/06/07 14:01:38, loislo wrote:

Why other platforms didn't change? Also it looks odd that you use one preference
on Gtk and another on other platforms.

[loislo, 2011-06-07 14:09:11]

I'll check the other platforms

[pfeldman, 2011-06-07 14:54:06]

http://codereview.chromium.org/7121012/diff/1/chrome/browser/ui/gtk/browser_w...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/7121012/diff/1/chrome/browser/ui/gtk/browser_w...
chrome/browser/ui/gtk/browser_window_gtk.cc:1698:
GetInteger(prefs::kDevToolsSplitLocation);
I'd suggest nuking the property and fixing all ports. It will simplify the code
and add one-time-inconvenience to the end user.

[loislo, 2011-06-07 17:24:12]

there were 3 different implementations for win/mac/linux.
linux version had a bug.

0) original patch was reverted;
1) kDefaultDevToolsHeight value was assigned as default for
kDevToolsSplitLocation preference and was removed from the platform specific
code;
2) kMinDevToolsHeight = 100;
3) all the places has identical calculations.

[pfeldman, 2011-06-07 19:59:12]

I think we should default split location to -1 and set the splitter in order to
provide 200px devtools height by default.

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/cocoa/dev_too...
File chrome/browser/ui/cocoa/dev_tools_controller.mm (right):

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/cocoa/dev_too...
chrome/browser/ui/cocoa/dev_tools_controller.mm:24: const int kMinDevToolsHeight
= 100;
Please keep it 50.

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/cocoa/dev_too...
chrome/browser/ui/cocoa/dev_tools_controller.mm:108: splitOffset =
std::max(static_cast<CGFloat>(kMinDevToolsHeight),
One of these should stay kMinWebHeight. I.e. we when whether devtools window
leaves enough room for the actual web content.

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/gtk/browser_w...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/gtk/browser_w...
chrome/browser/ui/gtk/browser_window_gtk.cc:106: const int kMinDevToolsHeight =
100;
Leave it 50

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/gtk/browser_w...
chrome/browser/ui/gtk/browser_window_gtk.cc:1304:
GetInteger(prefs::kDevToolsSplitLocation);
Is default window height 600? Otherwise how do you guarantee that it will be 200
by default? I do like having default value for the height, not for the split
location btw.

[pfeldman, 2011-06-07 20:14:51]

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7121012/diff/8/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:2158: std::max(kMinDevToolsHeight,
split_offset));
While I agree that 50 is a good threshold for the minimum height, I think that
2/3 as the default splitter location was not such a bad idea. I'd rather convert
-1 to 2/3 dynamically on all platforms (if that is possible).

[yurys, 2011-06-08 08:14:27]

http://codereview.chromium.org/7121012/diff/11001/chrome/browser/ui/cocoa/dev...
File chrome/browser/ui/cocoa/dev_tools_controller.mm (right):

http://codereview.chromium.org/7121012/diff/11001/chrome/browser/ui/cocoa/dev...
chrome/browser/ui/cocoa/dev_tools_controller.mm:111: splitOffset =
std::max(static_cast<CGFloat>(kMinDevToolsHeight),
Use of the kMinDevToolsHeight here is confusing, can we replace it with
kMinWebHeight even if it has the same value?

http://codereview.chromium.org/7121012/diff/11001/chrome/browser/ui/cocoa/dev...
chrome/browser/ui/cocoa/dev_tools_controller.mm:113: splitOffset =
std::min(NSHeight([splitView_ frame]) - kMinDevToolsHeight,
What is the expected behavior when splitOffset here is negative?

http://codereview.chromium.org/7121012/diff/11001/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/7121012/diff/11001/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_window_gtk.cc:1312: split_offset =
std::min(contents_rect.height - kMinDevToolsHeight,
Can we handle the case of negative offset explicitly?

http://codereview.chromium.org/7121012/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7121012/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:2160: split_offset =
std::min(contents_split_->height() - kMinDevToolsHeight,
Ditto.

[loislo, 2011-06-08 10:45:24]

comments addressed

[yurys, 2011-06-08 14:00:01]

lgtm