Auto-size the views version of the options dialog.

Auto-size the views version of the options dialog.

This adds support for auto-sizing tab controls, adjusts the options dialog to
use it and takes care of any fallout due to the new layout handling. Also fixes
a couple of small bugs in the views Layout() machinery  and sanitizes layouting
of options pages.

BUG=36497
TEST=unit tests in tabbed_pane_unittest.cc and grid_layout_unittest.cc, as well as checking the options dialog in any supported language.

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

[Peter Kasting, 2010-06-25 18:29:15]

You should have sky review this.  He'd be a better reviewer.

http://codereview.chromium.org/2812026/diff/1/3
File chrome/browser/views/options/advanced_contents_view.cc (left):

http://codereview.chromium.org/2812026/diff/1/3#oldcode290
chrome/browser/views/options/advanced_contents_view.cc:290: contents_->Layout();
I'm not sure I understand the removal of various Layout() calls in this file.

http://codereview.chromium.org/2812026/diff/1/4
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/2812026/diff/1/4#newcode44
chrome/browser/views/options/content_page_view.cc:44: const int
kPasswordSavingRadioGroup = 201;
Nit: Did these need changing?  I don't think radio group numbers need to be
exclusive with other kinds of group numbers.

http://codereview.chromium.org/2812026/diff/1/5
File chrome/browser/views/options/general_page_view.cc (right):

http://codereview.chromium.org/2812026/diff/1/5#newcode38
chrome/browser/views/options/general_page_view.cc:38: static const int
kStartupRadioGroup = 101;
Nit: Did these need changing?  I don't think radio group numbers need to be
exclusive with other kinds of group numbers.

http://codereview.chromium.org/2812026/diff/1/8
File views/controls/button/native_button_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/8#newcode97
views/controls/button/native_button_win.cc:97: SendMessage(native_view(),
BCM_GETIDEALSIZE, 0,
Nit: Generally we prefer function-call style to SendMessage() where possible. 
In this case, try using Button_GetIdealSize() (see
http://msdn.microsoft.com/en-us/library/bb761851(v=VS.85).aspx ).

http://codereview.chromium.org/2812026/diff/1/10
File views/controls/native_control.cc (right):

http://codereview.chromium.org/2812026/diff/1/10#newcode204
views/controls/native_control.cc:204: if (is_add && parent != this &&
!container_ && GetWidget()) {
I confess, I don't understand this change at all.

http://codereview.chromium.org/2812026/diff/1/11
File views/controls/scrollbar/native_scroll_bar_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/11#newcode1
views/controls/scrollbar/native_scroll_bar_win.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2812026/diff/1/12
File views/controls/scrollbar/native_scroll_bar_win.h (right):

http://codereview.chromium.org/2812026/diff/1/12#newcode1
views/controls/scrollbar/native_scroll_bar_win.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2812026/diff/1/14
File views/controls/tabbed_pane/native_tabbed_pane_gtk.h (right):

http://codereview.chromium.org/2812026/diff/1/14#newcode1
views/controls/tabbed_pane/native_tabbed_pane_gtk.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2812026/diff/1/15
File views/controls/tabbed_pane/native_tabbed_pane_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/15#newcode52
views/controls/tabbed_pane/native_tabbed_pane_win.cc:52: TabLayout() { }
Nit: { } -> {}

http://codereview.chromium.org/2812026/diff/1/15#newcode60
views/controls/tabbed_pane/native_tabbed_pane_win.cc:60: // The child might not
have been layed out yet.
Nit: layed -> laid

http://codereview.chromium.org/2812026/diff/1/15#newcode143
views/controls/tabbed_pane/native_tabbed_pane_win.cc:143: // If this is the only
tab displayed, make sure the contents is set.
Nit: This comment is out of date.

http://codereview.chromium.org/2812026/diff/1/15#newcode361
views/controls/tabbed_pane/native_tabbed_pane_win.cc:361: for (size_t i = 0; i <
tab_titles_.size(); ++i) {
Nit: No {}

http://codereview.chromium.org/2812026/diff/1/15#newcode367
views/controls/tabbed_pane/native_tabbed_pane_win.cc:367: tab !=
tab_views_.end(); ++tab) {
Nit: Indent one more space to line up with above line.  No {}.

http://codereview.chromium.org/2812026/diff/1/17
File views/controls/tabbed_pane/native_tabbed_pane_wrapper.h (right):

http://codereview.chromium.org/2812026/diff/1/17#newcode1
views/controls/tabbed_pane/native_tabbed_pane_wrapper.h:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2812026/diff/1/18
File views/controls/tabbed_pane/tabbed_pane.cc (right):

http://codereview.chromium.org/2812026/diff/1/18#newcode133
views/controls/tabbed_pane/tabbed_pane.cc:133: if (native_tabbed_pane_)
Nit: No else after return; I'd just do

return native_tabbed_pane_ ?
    native_tabbed_pane_->GetPreferredSize() : gfx::Size();

http://codereview.chromium.org/2812026/diff/1/21
File views/controls/table/table_view.h (right):

http://codereview.chromium.org/2812026/diff/1/21#newcode410
views/controls/table/table_view.h:410: // Overriden in order to update the
column sizes, which only gets the sizing
Nit: gets -> get

http://codereview.chromium.org/2812026/diff/1/22
File views/examples/tabbed_pane_example.h (right):

http://codereview.chromium.org/2812026/diff/1/22#newcode1
views/examples/tabbed_pane_example.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2812026/diff/1/23
File views/grid_layout.cc (right):

http://codereview.chromium.org/2812026/diff/1/23#newcode847
views/grid_layout.cc:847: // Calculate the preferred width of each of the
columns. Some views preferred
Nit: views -> views'

http://codereview.chromium.org/2812026/diff/1/25
File views/view.cc (right):

http://codereview.chromium.org/2812026/diff/1/25#newcode225
views/view.cc:225: return;
Make sure interactive_ui_tests don't hang.

[Mattias Nissler (ping if slow), 2010-06-28 09:58:28]

Thanks for your feedback, Peter! I'll get this to sky after fixing up the loose
ends.

http://codereview.chromium.org/2812026/diff/1/3
File chrome/browser/views/options/advanced_contents_view.cc (left):

http://codereview.chromium.org/2812026/diff/1/3#oldcode290
chrome/browser/views/options/advanced_contents_view.cc:290: contents_->Layout();
On 2010/06/25 18:29:15, Peter Kasting wrote:
> I'm not sure I understand the removal of various Layout() calls in this file.

They're all redundant. I generally found GridLayout::Layout to be the biggest
performance problem (it generates an exponential number of GetPreferredSize()
calls to its transitive children w.r.t. depth if they also use GridLayout as
their layout manager).

http://codereview.chromium.org/2812026/diff/1/4
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/2812026/diff/1/4#newcode44
chrome/browser/views/options/content_page_view.cc:44: const int
kPasswordSavingRadioGroup = 201;
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: Did these need changing?  I don't think radio group numbers need to be
> exclusive with other kinds of group numbers.

Radio groups must be unique within a view hierarchy. Since I changed TabbedPane
to always add the pages as children (needed for sizing them), the options pages
are now in the same collision domain.

http://codereview.chromium.org/2812026/diff/1/5
File chrome/browser/views/options/general_page_view.cc (right):

http://codereview.chromium.org/2812026/diff/1/5#newcode38
chrome/browser/views/options/general_page_view.cc:38: static const int
kStartupRadioGroup = 101;
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: Did these need changing?  I don't think radio group numbers need to be
> exclusive with other kinds of group numbers.

See my other comment.

http://codereview.chromium.org/2812026/diff/1/8
File views/controls/button/native_button_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/8#newcode97
views/controls/button/native_button_win.cc:97: SendMessage(native_view(),
BCM_GETIDEALSIZE, 0,
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: Generally we prefer function-call style to SendMessage() where possible. 
> In this case, try using Button_GetIdealSize() (see
> http://msdn.microsoft.com/en-us/library/bb761851%28v=VS.85%29.aspx ).

Done.

http://codereview.chromium.org/2812026/diff/1/10
File views/controls/native_control.cc (right):

http://codereview.chromium.org/2812026/diff/1/10#newcode204
views/controls/native_control.cc:204: if (is_add && parent != this &&
!container_ && GetWidget()) {
On 2010/06/25 18:29:15, Peter Kasting wrote:
> I confess, I don't understand this change at all.

This makes sure the native control is created as soon as we have a widget.
Otherwise, the native control will never be created if there is no widget
available when adding the control. This is the case for the list view on the
general page, which disappears without the change.

http://codereview.chromium.org/2812026/diff/1/11
File views/controls/scrollbar/native_scroll_bar_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/11#newcode1
views/controls/scrollbar/native_scroll_bar_win.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2812026/diff/1/14
File views/controls/tabbed_pane/native_tabbed_pane_gtk.h (right):

http://codereview.chromium.org/2812026/diff/1/14#newcode1
views/controls/tabbed_pane/native_tabbed_pane_gtk.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2812026/diff/1/15
File views/controls/tabbed_pane/native_tabbed_pane_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/15#newcode52
views/controls/tabbed_pane/native_tabbed_pane_win.cc:52: TabLayout() { }
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: { } -> {}

Done.

http://codereview.chromium.org/2812026/diff/1/15#newcode60
views/controls/tabbed_pane/native_tabbed_pane_win.cc:60: // The child might not
have been layed out yet.
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: layed -> laid

Done.

http://codereview.chromium.org/2812026/diff/1/15#newcode143
views/controls/tabbed_pane/native_tabbed_pane_win.cc:143: // If this is the only
tab displayed, make sure the contents is set.
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: This comment is out of date.

Done.

http://codereview.chromium.org/2812026/diff/1/15#newcode361
views/controls/tabbed_pane/native_tabbed_pane_win.cc:361: for (size_t i = 0; i <
tab_titles_.size(); ++i) {
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/2812026/diff/1/15#newcode367
views/controls/tabbed_pane/native_tabbed_pane_win.cc:367: tab !=
tab_views_.end(); ++tab) {
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: Indent one more space to line up with above line.  No {}.

Done.

http://codereview.chromium.org/2812026/diff/1/17
File views/controls/tabbed_pane/native_tabbed_pane_wrapper.h (right):

http://codereview.chromium.org/2812026/diff/1/17#newcode1
views/controls/tabbed_pane/native_tabbed_pane_wrapper.h:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2812026/diff/1/18
File views/controls/tabbed_pane/tabbed_pane.cc (right):

http://codereview.chromium.org/2812026/diff/1/18#newcode133
views/controls/tabbed_pane/tabbed_pane.cc:133: if (native_tabbed_pane_)
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: No else after return; I'd just do
> 
> return native_tabbed_pane_ ?
>     native_tabbed_pane_->GetPreferredSize() : gfx::Size();

Done.

http://codereview.chromium.org/2812026/diff/1/21
File views/controls/table/table_view.h (right):

http://codereview.chromium.org/2812026/diff/1/21#newcode410
views/controls/table/table_view.h:410: // Overriden in order to update the
column sizes, which only gets the sizing
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: gets -> get

Rephrased the comment for accuracy.
Done.

http://codereview.chromium.org/2812026/diff/1/22
File views/examples/tabbed_pane_example.h (right):

http://codereview.chromium.org/2812026/diff/1/22#newcode1
views/examples/tabbed_pane_example.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2812026/diff/1/23
File views/grid_layout.cc (right):

http://codereview.chromium.org/2812026/diff/1/23#newcode847
views/grid_layout.cc:847: // Calculate the preferred width of each of the
columns. Some views preferred
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Nit: views -> views'

Done.

http://codereview.chromium.org/2812026/diff/1/25
File views/view.cc (right):

http://codereview.chromium.org/2812026/diff/1/25#newcode225
views/view.cc:225: return;
On 2010/06/25 18:29:15, Peter Kasting wrote:
> Make sure interactive_ui_tests don't hang.

Will do, need to fix up some tests anyway.

[tfarina, 2010-06-28 13:36:49]

http://codereview.chromium.org/2812026/diff/1/2
File chrome/browser/chromeos/options/options_window_view.cc (right):

http://codereview.chromium.org/2812026/diff/1/2#newcode52
chrome/browser/chromeos/options/options_window_view.cc:52: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/2812026/diff/1/15
File views/controls/tabbed_pane/native_tabbed_pane_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/15#newcode72
views/controls/tabbed_pane/native_tabbed_pane_win.cc:72: for (int i = 0; i <
host->GetChildViewCount(); i++) {
++i

http://codereview.chromium.org/2812026/diff/1/15#newcode83
views/controls/tabbed_pane/native_tabbed_pane_win.cc:83: for (int i = 0; i <
host->GetChildViewCount(); i++) {
++i

http://codereview.chromium.org/2812026/diff/1/15#newcode93
views/controls/tabbed_pane/native_tabbed_pane_win.cc:93: for (int i = 0; i <
host->GetChildViewCount(); i++) {
++i

[Mattias Nissler (ping if slow), 2010-06-28 16:14:00]

Thanks for your comments, Thiago!

http://codereview.chromium.org/2812026/diff/1/2
File chrome/browser/chromeos/options/options_window_view.cc (right):

http://codereview.chromium.org/2812026/diff/1/2#newcode52
chrome/browser/chromeos/options/options_window_view.cc:52: };
On 2010/06/28 13:36:49, tfarina wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/2812026/diff/1/15
File views/controls/tabbed_pane/native_tabbed_pane_win.cc (right):

http://codereview.chromium.org/2812026/diff/1/15#newcode72
views/controls/tabbed_pane/native_tabbed_pane_win.cc:72: for (int i = 0; i <
host->GetChildViewCount(); i++) {
On 2010/06/28 13:36:49, tfarina wrote:
> ++i

Done.

http://codereview.chromium.org/2812026/diff/1/15#newcode83
views/controls/tabbed_pane/native_tabbed_pane_win.cc:83: for (int i = 0; i <
host->GetChildViewCount(); i++) {
On 2010/06/28 13:36:49, tfarina wrote:
> ++i

Done.

http://codereview.chromium.org/2812026/diff/1/15#newcode93
views/controls/tabbed_pane/native_tabbed_pane_win.cc:93: for (int i = 0; i <
host->GetChildViewCount(); i++) {
On 2010/06/28 13:36:49, tfarina wrote:
> ++i

Done.

[Mattias Nissler (ping if slow), 2010-06-28 16:20:28]

Hi, Peter suggested you as a better reviewer for this CL, so would you mind?

[Peter Kasting, 2010-06-28 17:40:00]

BTW, while I personally prefer ++i to i++, when |i| is an integral type there is
no style rule about which way you should do things (despite tfarina's drive-by).

http://codereview.chromium.org/2812026/diff/1/10
File views/controls/native_control.cc (right):

http://codereview.chromium.org/2812026/diff/1/10#newcode204
views/controls/native_control.cc:204: if (is_add && parent != this &&
!container_ && GetWidget()) {
On 2010/06/28 09:58:28, mnissler wrote:
> On 2010/06/25 18:29:15, Peter Kasting wrote:
> > I confess, I don't understand this change at all.
> 
> This makes sure the native control is created as soon as we have a widget.
> Otherwise, the native control will never be created if there is no widget
> available when adding the control. This is the case for the list view on the
> general page, which disappears without the change.

I suggest adding comments to this effect.

[sky, 2010-06-28 17:40:07]

http://codereview.chromium.org/2812026/diff/19001/20053
File chrome/browser/chromeos/options/options_window_view.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20053#newcode57
chrome/browser/chromeos/options/options_window_view.cc:57: : widget_(widget) {
indent by 4.

http://codereview.chromium.org/2812026/diff/19001/20053#newcode70
chrome/browser/chromeos/options/options_window_view.cc:70: GtkRequisition
requisition = { 0, 0 };
This feels like it should be in NativeViewHost.

http://codereview.chromium.org/2812026/diff/19001/20066
File views/controls/tabbed_pane/native_tabbed_pane_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20066#newcode79
views/controls/tabbed_pane/native_tabbed_pane_gtk.cc:79:
GetRootView()->Layout();
Make sure GetRootView is non-null.

http://codereview.chromium.org/2812026/diff/19001/20066#newcode186
views/controls/tabbed_pane/native_tabbed_pane_gtk.cc:186: // Removing a tab
might change the size of the tabbed pane.
Removing -> Adding.

http://codereview.chromium.org/2812026/diff/19001/20066#newcode187
views/controls/tabbed_pane/native_tabbed_pane_gtk.cc:187:
GetRootView()->Layout();
Make sure GetRootView is non-null.

http://codereview.chromium.org/2812026/diff/19001/20068
File views/controls/tabbed_pane/native_tabbed_pane_win.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20068#newcode61
views/controls/tabbed_pane/native_tabbed_pane_win.cc:61: if (child->bounds() !=
bounds)
view::SetBounds already does this check for you.

http://codereview.chromium.org/2812026/diff/19001/20068#newcode198
views/controls/tabbed_pane/native_tabbed_pane_win.cc:198:
content_window_->GetRootView()->RemoveChildView(removed_tab);
Do you need to make sure content_window_ is non-null?

http://codereview.chromium.org/2812026/diff/19001/20068#newcode296
views/controls/tabbed_pane/native_tabbed_pane_win.cc:296: tab_layout_manager_ =
new TabLayout();
initialize this to null in the member initializer list.

http://codereview.chromium.org/2812026/diff/19001/20073
File views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20073#newcode16
views/controls/tabbed_pane/tabbed_pane_unittest.cc:16: FixedSizeView(const
gfx::Size size)
const&

http://codereview.chromium.org/2812026/diff/19001/20074
File views/controls/table/table_view.cc (left):

http://codereview.chromium.org/2812026/diff/19001/20074#oldcode136
views/controls/table/table_view.cc:136: if ((!sized_columns_ ||
autosize_columns_) && width() > 0) {
Did you want to nuke the field too?

http://codereview.chromium.org/2812026/diff/19001/20077
File views/grid_layout.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20077#newcode843
views/grid_layout.cc:843: pref->SetSize(std::max(0, width - left_inset_ -
right_inset_), 0);
Why should the preferred width be the current width if emtpy?

http://codereview.chromium.org/2812026/diff/19001/20079
File views/grid_layout_unittest.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20079#newcode38
views/grid_layout_unittest.cc:38: explicit FlexibleView(const int circumference)
{
no const

http://codereview.chromium.org/2812026/diff/19001/20079#newcode607
views/grid_layout_unittest.cc:607: EXPECT_EQ(gfx::Size(20, 50),
layout->GetPreferredSize(&host));
Can you also do a layout and make sure the views are sized as you expect them.

http://codereview.chromium.org/2812026/diff/19001/20080
File views/view.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20080#newcode225
views/view.cc:225: return;
Did you make sure there are no more hangs in the interactive ui tests?

[Mattias Nissler (ping if slow), 2010-06-29 11:26:03]

Thanks for your comments, here is an updated version.

http://codereview.chromium.org/2812026/diff/19001/20053
File chrome/browser/chromeos/options/options_window_view.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20053#newcode57
chrome/browser/chromeos/options/options_window_view.cc:57: : widget_(widget) {
On 2010/06/28 17:40:07, sky wrote:
> indent by 4.

Done.

http://codereview.chromium.org/2812026/diff/19001/20053#newcode70
chrome/browser/chromeos/options/options_window_view.cc:70: GtkRequisition
requisition = { 0, 0 };
On 2010/06/28 17:40:07, sky wrote:
> This feels like it should be in NativeViewHost.

There are two reasons I didn't add this to NativeViewHost: First, there is no
way to implement GetPreferredSize() properly on Windows. Second, NativeViewHost
provides facilities to set the preferred size in advance, which we need to keep.
In order to play nice with that, we could use dynamic sizing only when there's
not explicit preferred size set, but that's just overkill IMO.

http://codereview.chromium.org/2812026/diff/19001/20066
File views/controls/tabbed_pane/native_tabbed_pane_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20066#newcode79
views/controls/tabbed_pane/native_tabbed_pane_gtk.cc:79:
GetRootView()->Layout();
On 2010/06/28 17:40:07, sky wrote:
> Make sure GetRootView is non-null.

Done.

http://codereview.chromium.org/2812026/diff/19001/20066#newcode186
views/controls/tabbed_pane/native_tabbed_pane_gtk.cc:186: // Removing a tab
might change the size of the tabbed pane.
On 2010/06/28 17:40:07, sky wrote:
> Removing -> Adding.

Done.

http://codereview.chromium.org/2812026/diff/19001/20066#newcode187
views/controls/tabbed_pane/native_tabbed_pane_gtk.cc:187:
GetRootView()->Layout();
On 2010/06/28 17:40:07, sky wrote:
> Make sure GetRootView is non-null.

Done.

http://codereview.chromium.org/2812026/diff/19001/20068
File views/controls/tabbed_pane/native_tabbed_pane_win.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20068#newcode61
views/controls/tabbed_pane/native_tabbed_pane_win.cc:61: if (child->bounds() !=
bounds)
On 2010/06/28 17:40:07, sky wrote:
> view::SetBounds already does this check for you.

Done.

http://codereview.chromium.org/2812026/diff/19001/20068#newcode198
views/controls/tabbed_pane/native_tabbed_pane_win.cc:198:
content_window_->GetRootView()->RemoveChildView(removed_tab);
On 2010/06/28 17:40:07, sky wrote:
> Do you need to make sure content_window_ is non-null?

Yes. Done.

http://codereview.chromium.org/2812026/diff/19001/20068#newcode296
views/controls/tabbed_pane/native_tabbed_pane_win.cc:296: tab_layout_manager_ =
new TabLayout();
On 2010/06/28 17:40:07, sky wrote:
> initialize this to null in the member initializer list.

Done.

http://codereview.chromium.org/2812026/diff/19001/20073
File views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20073#newcode16
views/controls/tabbed_pane/tabbed_pane_unittest.cc:16: FixedSizeView(const
gfx::Size size)
On 2010/06/28 17:40:07, sky wrote:
> const&

Done.

http://codereview.chromium.org/2812026/diff/19001/20074
File views/controls/table/table_view.cc (left):

http://codereview.chromium.org/2812026/diff/19001/20074#oldcode136
views/controls/table/table_view.cc:136: if ((!sized_columns_ ||
autosize_columns_) && width() > 0) {
On 2010/06/28 17:40:07, sky wrote:
> Did you want to nuke the field too?

Yes. Done.

http://codereview.chromium.org/2812026/diff/19001/20077
File views/grid_layout.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20077#newcode843
views/grid_layout.cc:843: pref->SetSize(std::max(0, width - left_inset_ -
right_inset_), 0);
On 2010/06/28 17:40:07, sky wrote:
> Why should the preferred width be the current width if emtpy?

I'm not sure whether I understand you. By empty you mean no rows? I put that
statement to keep the behavior of respecting the width as a lower bound when
calculating pref->width(), but now as I think about it, we really don't need
this.

http://codereview.chromium.org/2812026/diff/19001/20079
File views/grid_layout_unittest.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20079#newcode38
views/grid_layout_unittest.cc:38: explicit FlexibleView(const int circumference)
{
On 2010/06/28 17:40:07, sky wrote:
> no const

Done.

http://codereview.chromium.org/2812026/diff/19001/20079#newcode607
views/grid_layout_unittest.cc:607: EXPECT_EQ(gfx::Size(20, 50),
layout->GetPreferredSize(&host));
On 2010/06/28 17:40:07, sky wrote:
> Can you also do a layout and make sure the views are sized as you expect them.

I could, but that's not the point of the test case. Layout() didn't show any
problems, only GetPreferredSize(). So I'd rather keep this test focused.

http://codereview.chromium.org/2812026/diff/19001/20080
File views/view.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20080#newcode225
views/view.cc:225: return;
On 2010/06/28 17:40:07, sky wrote:
> Did you make sure there are no more hangs in the interactive ui tests?

Yes, I did :) Actually, I still do, but everything looks good locally now and
I'm confident the next trybot runs will be green.

[sky, 2010-06-29 15:40:38]

http://codereview.chromium.org/2812026/diff/19001/20077
File views/grid_layout.cc (right):

http://codereview.chromium.org/2812026/diff/19001/20077#newcode843
views/grid_layout.cc:843: pref->SetSize(std::max(0, width - left_inset_ -
right_inset_), 0);
On 2010/06/29 11:26:03, mnissler wrote:
> On 2010/06/28 17:40:07, sky wrote:
> > Why should the preferred width be the current width if emtpy?
> 
> I'm not sure whether I understand you. By empty you mean no rows? 

I was thinking no columns.

> I put that
> statement to keep the behavior of respecting the width as a lower bound when
> calculating pref->width(), but now as I think about it, we really don't need
> this.

Glad to hear it.

http://codereview.chromium.org/2812026/diff/24002/32081
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/24002/32081#newcode583
views/widget/widget_gtk.cc:583: gtk_widget_set_size_request(widget_,
bounds.width(), bounds.height());
Doesn't this give different behavior than resizing the window? Same comment on
653 new.

http://codereview.chromium.org/2812026/diff/24002/32081#newcode890
views/widget/widget_gtk.cc:890: gtk_widget_get_size_request(widget,
&requisition->width,
Please add a comment as to why you need to get the size request first.

[Mattias Nissler (ping if slow), 2010-06-29 16:35:48]

http://codereview.chromium.org/2812026/diff/24002/32081
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/24002/32081#newcode583
views/widget/widget_gtk.cc:583: gtk_widget_set_size_request(widget_,
bounds.width(), bounds.height());
On 2010/06/29 15:40:38, sky wrote:
> Doesn't this give different behavior than resizing the window? Same comment on
> 653 new.

The problem with gtk_window_resize() is that it won't allow you to resize the
window to a smaller value than the requested size. This actually broke the menu
code, which depends on sizing the window to a smaller value for menus larger
than the screen height. It wasn't a problem before since size_request wasn't
implemented.

http://codereview.chromium.org/2812026/diff/24002/32081#newcode890
views/widget/widget_gtk.cc:890: gtk_widget_get_size_request(widget,
&requisition->width,
On 2010/06/29 15:40:38, sky wrote:
> Please add a comment as to why you need to get the size request first.

Probably I shouldn't, I just realized that this defeats the purpose of the
function in the light of the gtk_widget_set_size_request() calls I introduced.
There's tab dragging test failing for linux_views which fails if I remove this.
I'll look into this tomorrow to see whether there's a better solution.

[Mattias Nissler (ping if slow), 2010-06-30 11:54:21]

I just uploaded an updated version. I think I've resolved all failing unit tests
now, but let's see what the trybots say. Other than that, are you ok with what I
have now?

http://codereview.chromium.org/2812026/diff/24002/32081
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/24002/32081#newcode890
views/widget/widget_gtk.cc:890: gtk_widget_get_size_request(widget,
&requisition->width,
On 2010/06/29 16:35:48, mnissler wrote:
> On 2010/06/29 15:40:38, sky wrote:
> > Please add a comment as to why you need to get the size request first.
> 
> Probably I shouldn't, I just realized that this defeats the purpose of the
> function in the light of the gtk_widget_set_size_request() calls I introduced.
> There's tab dragging test failing for linux_views which fails if I remove
this.
> I'll look into this tomorrow to see whether there's a better solution.

After thinking a bit more about this and reading GTK source code, this block is
wrong here. Forced size requests are the responsibility of the caller. I fixed
the problem in TabContentsViewGtk now and removed this block.

[sky, 2010-06-30 15:17:17]

http://codereview.chromium.org/2812026/diff/16002/39082
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/16002/39082#newcode654
views/widget/widget_gtk.cc:654: bounds.width(), bounds.height());
I'm still worried about this part of the change. Does setting the size request
of a GtkWindow always update the size as gtk_window_resize would do?

> The problem with gtk_window_resize() is that it won't allow you
> to resize the window to a smaller value than the requested size.
> This actually broke the menu code, which depends on sizing the
> window to a smaller value for menus larger than the screen
> height. It wasn't a problem before since size_request wasn't
> implemented.

It should be up to the caller to make sure the size passed to SetBounds makes
sense. If it's smaller than the requested size, so be it.

[Mattias Nissler (ping if slow), 2010-06-30 17:21:44]

http://codereview.chromium.org/2812026/diff/16002/39082
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/16002/39082#newcode654
views/widget/widget_gtk.cc:654: bounds.width(), bounds.height());
On 2010/06/30 15:17:17, sky wrote:
> I'm still worried about this part of the change. Does setting the size request
> of a GtkWindow always update the size as gtk_window_resize would do?

Well, I double-checked the GTK code and it doesn't. We could just make both
calls, first set_size_request and then resize. I'm not so sure about that, WDYT?

> 
> > The problem with gtk_window_resize() is that it won't allow you
> > to resize the window to a smaller value than the requested size.
> > This actually broke the menu code, which depends on sizing the
> > window to a smaller value for menus larger than the screen
> > height. It wasn't a problem before since size_request wasn't
> > implemented.
> 
> It should be up to the caller to make sure the size passed to SetBounds makes
> sense. If it's smaller than the requested size, so be it.

Yes, that's exactly what I'm trying to implement. Sizes set through
gtk_window_resize() are actually constrained by the size request (be it set
explicitly through gtk_widget_set_size_request() or computed through
size_request). That's what the documentation at
http://library.gnome.org/devel/gtk/unstable/GtkWindow.html#gtk-window-resize
says and it mentions to use gtk_widget_set_size_request() if you want
unconstrained control.

[sky, 2010-06-30 17:40:15]

http://codereview.chromium.org/2812026/diff/16002/39082
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2812026/diff/16002/39082#newcode654
views/widget/widget_gtk.cc:654: bounds.width(), bounds.height());
On 2010/06/30 17:21:44, mnissler wrote:
> On 2010/06/30 15:17:17, sky wrote:
> > I'm still worried about this part of the change. Does setting the size
request
> > of a GtkWindow always update the size as gtk_window_resize would do?
> 
> Well, I double-checked the GTK code and it doesn't. We could just make both
> calls, first set_size_request and then resize. I'm not so sure about that,
WDYT?

Ugh. I guess both calls sound good. Alternatively we could set the requested
size to 0x0 (or 1x1 in case gtk enforces > 0) in the constructor and be done
with it. Be sure and add documentation as to why we need to do this.

  -Scott

> > 
> > > The problem with gtk_window_resize() is that it won't allow you
> > > to resize the window to a smaller value than the requested size.
> > > This actually broke the menu code, which depends on sizing the
> > > window to a smaller value for menus larger than the screen
> > > height. It wasn't a problem before since size_request wasn't
> > > implemented.
> > 
> > It should be up to the caller to make sure the size passed to SetBounds
makes
> > sense. If it's smaller than the requested size, so be it.
> 
> Yes, that's exactly what I'm trying to implement. Sizes set through
> gtk_window_resize() are actually constrained by the size request (be it set
> explicitly through gtk_widget_set_size_request() or computed through
> size_request). That's what the documentation at
> http://library.gnome.org/devel/gtk/unstable/GtkWindow.html#gtk-window-resize
> says and it mentions to use gtk_widget_set_size_request() if you want
> unconstrained control.
> 
>

[Mattias Nissler (ping if slow), 2010-06-30 21:10:54]

On 2010/06/30 17:40:15, sky wrote:
> http://codereview.chromium.org/2812026/diff/16002/39082
> File views/widget/widget_gtk.cc (right):
> 
> http://codereview.chromium.org/2812026/diff/16002/39082#newcode654
> views/widget/widget_gtk.cc:654: bounds.width(), bounds.height());
> On 2010/06/30 17:21:44, mnissler wrote:
> > On 2010/06/30 15:17:17, sky wrote:
> > > I'm still worried about this part of the change. Does setting the size
> request
> > > of a GtkWindow always update the size as gtk_window_resize would do?
> > 
> > Well, I double-checked the GTK code and it doesn't. We could just make both
> > calls, first set_size_request and then resize. I'm not so sure about that,
> WDYT?
> 
> Ugh. I guess both calls sound good. Alternatively we could set the requested
> size to 0x0 (or 1x1 in case gtk enforces > 0) in the constructor and be done
> with it. Be sure and add documentation as to why we need to do this.

Yes, that should work. I missed your answer until now and came up with a
different solution in the mean time: Making size_request only return the
preferred size for child widgets. CL is updated accordingly.

The reason why I implemented size_request is only for bridging the size_request
call from NativeTabbedPaneGtk back into the views world. So handling only child
widgets is perfectly fine for this CL and AFAIK, we handle toplevel window
sizing completely different anyway. Are you ok with that?

[sky, 2010-06-30 21:13:30]

What you've done is fine. LGTM