Fix issue 11258: Linux: gracefully handle small browser window...

Fix issue 11258: Linux: gracefully handle small browser window

TODO: Make location bar to be freely shrinkable. Will be addressed in another CL.

BUG=11258
TEST=See bug report.


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

[James Su, 2009-12-17 12:35:05]

This CL works for me now. Please help review. I'll address the location bar
issue in another CL, which is a little complicated.

Regards
James Su

[Evan Martin, 2009-12-17 14:22:08]

In general: yay, glad you're looking at this.

http://codereview.chromium.org/507022/diff/2007/3005
File chrome/browser/gtk/browser_window_gtk.cc (left):

http://codereview.chromium.org/507022/diff/2007/3005#oldcode1598
chrome/browser/gtk/browser_window_gtk.cc:1598:
gtk_window_set_geometry_hints(window_, NULL, &geometry, GDK_HINT_MIN_SIZE);
Don't we need this still?  We still want to prevent the window from being too
small to be visible.

http://codereview.chromium.org/507022/diff/2007/3006
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/507022/diff/2007/3006#newcode66
chrome/browser/gtk/download_shelf_gtk.cc:66: // chrome window can be shrinked
freely even with many download items.
For this comment and others, here is a grammar suggestion.
- horizontally shrinkable" rather than "shrinkable in horizontal"
- maybe use "resized" instead of "shrinked"
- s/chrome/Chrome/

http://codereview.chromium.org/507022/diff/2007/3007
File chrome/browser/gtk/gtk_expanded_container.cc (right):

http://codereview.chromium.org/507022/diff/2007/3007#newcode3
chrome/browser/gtk/gtk_expanded_container.cc:3: // found in the LICENSE file.
Is this code copied from a GTK file?  We may need to be careful about licenses.

http://codereview.chromium.org/507022/diff/2007/3008
File chrome/browser/gtk/gtk_expanded_container.h (right):

http://codereview.chromium.org/507022/diff/2007/3008#newcode11
chrome/browser/gtk/gtk_expanded_container.h:11: // A specialized container,
which expands the size of its children to fill the
I don't fully understand what this container does.  It's like a GtkFixed that
also expands its children?  How does it relate to the GtkFloating class we have?

http://codereview.chromium.org/507022/diff/2007/3008#newcode13
chrome/browser/gtk/gtk_expanded_container.h:13: // than GtkFixed.
s/than/to/

http://codereview.chromium.org/507022/diff/2007/3009
File chrome/browser/gtk/gtk_expanded_container_unittest.cc (right):

http://codereview.chromium.org/507022/diff/2007/3009#newcode99
chrome/browser/gtk/gtk_expanded_container_unittest.cc:99:
TEST_F(GtkExpandedContainerTest, HasWindow) {
Could you put comments above these tests indicating what they're trying to test?
 This one seems to verify it still works when has_window is true, I think?  But
I'm not sure why that is something to test...

http://codereview.chromium.org/507022/diff/2007/3015
File chrome/browser/tab_contents/tab_contents_view_gtk.h (right):

http://codereview.chromium.org/507022/diff/2007/3015#newcode113
chrome/browser/tab_contents/tab_contents_view_gtk.h:113: // This container holds
the tab's web page views. It is a GtkFixed so that we
This comment is no longer correct, I think?

[James Su, 2009-12-18 04:21:09]

Hi,
 I reimplemented GtkExpandedContainer to inherit GtkFixed.

Regards
James Su

http://codereview.chromium.org/507022/diff/2007/3005
File chrome/browser/gtk/browser_window_gtk.cc (left):

http://codereview.chromium.org/507022/diff/2007/3005#oldcode1598
chrome/browser/gtk/browser_window_gtk.cc:1598:
gtk_window_set_geometry_hints(window_, NULL, &geometry, GDK_HINT_MIN_SIZE);
On 2009/12/17 14:22:08, Evan Martin wrote:
> Don't we need this still?  We still want to prevent the window from being too
> small to be visible.
We don't need it, as gtk will take the actual size request of the window as the
minimal window geometry. And if we set the geometry hint here, the actual size
request will be ignored, then the window might be resized to a smaller size than
what we want.

http://codereview.chromium.org/507022/diff/2007/3006
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/507022/diff/2007/3006#newcode66
chrome/browser/gtk/download_shelf_gtk.cc:66: // chrome window can be shrinked
freely even with many download items.
On 2009/12/17 14:22:08, Evan Martin wrote:
> For this comment and others, here is a grammar suggestion.
> - horizontally shrinkable" rather than "shrinkable in horizontal"
> - maybe use "resized" instead of "shrinked"
> - s/chrome/Chrome/

Done.

http://codereview.chromium.org/507022/diff/2007/3007
File chrome/browser/gtk/gtk_expanded_container.cc (right):

http://codereview.chromium.org/507022/diff/2007/3007#newcode3
chrome/browser/gtk/gtk_expanded_container.cc:3: // found in the LICENSE file.
On 2009/12/17 14:22:08, Evan Martin wrote:
> Is this code copied from a GTK file?  We may need to be careful about
licenses.

The code is almost same as GtkFixed. I think I can rewrite this class to derive
from GtkFixed, then the license should be no problem.

http://codereview.chromium.org/507022/diff/2007/3008
File chrome/browser/gtk/gtk_expanded_container.h (right):

http://codereview.chromium.org/507022/diff/2007/3008#newcode11
chrome/browser/gtk/gtk_expanded_container.h:11: // A specialized container,
which expands the size of its children to fill the
On 2009/12/17 14:22:08, Evan Martin wrote:
> I don't fully understand what this container does.  It's like a GtkFixed that
> also expands its children?  How does it relate to the GtkFloating class we
have?
This container is similar to GtkFixed. The only difference is, instead of
honoring the actual size request of children, this container always expands its
children to the container's size by default. And a signal is provided to give
the caller a chance to customize the size allocation of its children.
While, GtkFloating is more like a specialized GtkBin with arbitrary floating
children widgets, which can only have one expanded child.

http://codereview.chromium.org/507022/diff/2007/3008#newcode13
chrome/browser/gtk/gtk_expanded_container.h:13: // than GtkFixed.
On 2009/12/17 14:22:08, Evan Martin wrote:
> s/than/to/

Done.

http://codereview.chromium.org/507022/diff/2007/3009
File chrome/browser/gtk/gtk_expanded_container_unittest.cc (right):

http://codereview.chromium.org/507022/diff/2007/3009#newcode99
chrome/browser/gtk/gtk_expanded_container_unittest.cc:99:
TEST_F(GtkExpandedContainerTest, HasWindow) {
On 2009/12/17 14:22:08, Evan Martin wrote:
> Could you put comments above these tests indicating what they're trying to
test?
>  This one seems to verify it still works when has_window is true, I think? 
But
> I'm not sure why that is something to test...

Done.

http://codereview.chromium.org/507022/diff/2007/3015
File chrome/browser/tab_contents/tab_contents_view_gtk.h (right):

http://codereview.chromium.org/507022/diff/2007/3015#newcode113
chrome/browser/tab_contents/tab_contents_view_gtk.h:113: // This container holds
the tab's web page views. It is a GtkFixed so that we
On 2009/12/17 14:22:08, Evan Martin wrote:
> This comment is no longer correct, I think?

Done.

[Evan Martin, 2009-12-18 09:40:31]

Awesome, LGTM!  I am glad you fixed this.

Please double check that nothing goes wrong if you resize the window rapidly,
that you can still drag tabs off, etc.  I expect our existing code might be
fragile in some places.  :)

http://codereview.chromium.org/507022/diff/7007/7010
File chrome/browser/gtk/gtk_expanded_container.cc (right):

http://codereview.chromium.org/507022/diff/7007/7010#newcode8
chrome/browser/gtk/gtk_expanded_container.cc:8: #include <gtk/gtkprivate.h>
Why do you need this include?  Shouldn't it be private?

http://codereview.chromium.org/507022/diff/7007/7010#newcode143
chrome/browser/gtk/gtk_expanded_container.cc:143: static void
gtk_expanded_container_init(GtkExpandedContainer* container) {
Does this need to call the parent init?

http://codereview.chromium.org/507022/diff/7007/7011
File chrome/browser/gtk/gtk_expanded_container.h (right):

http://codereview.chromium.org/507022/diff/7007/7011#newcode17
chrome/browser/gtk/gtk_expanded_container.h:17: // the following signature:
This new comment is perfect, thanks for explaining it.

http://codereview.chromium.org/507022/diff/7007/7012
File chrome/browser/gtk/gtk_expanded_container_unittest.cc (right):

http://codereview.chromium.org/507022/diff/7007/7012#newcode3
chrome/browser/gtk/gtk_expanded_container_unittest.cc:3: // found in the LICENSE
file.
Extra newline here

http://codereview.chromium.org/507022/diff/7007/7012#newcode4
chrome/browser/gtk/gtk_expanded_container_unittest.cc:4: #include
"chrome/browser/gtk/gtk_expanded_container.h"
extra newlinehere

http://codereview.chromium.org/507022/diff/7007/7019
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/507022/diff/7007/7019#newcode367
chrome/browser/tab_contents/tab_contents_view_gtk.cc:367: if
(view->tab_contents()->delegate())
The linter might complain if you don't have curly braces around this if
statemetn.

[James Su, 2009-12-18 16:35:41]

CL submitted. So far, it works well on my machine.

http://codereview.chromium.org/507022/diff/7007/7010
File chrome/browser/gtk/gtk_expanded_container.cc (right):

http://codereview.chromium.org/507022/diff/7007/7010#newcode8
chrome/browser/gtk/gtk_expanded_container.cc:8: #include <gtk/gtkprivate.h>
On 2009/12/18 09:40:31, Evan Martin wrote:
> Why do you need this include?  Shouldn't it be private?

Done.

http://codereview.chromium.org/507022/diff/7007/7012
File chrome/browser/gtk/gtk_expanded_container_unittest.cc (right):

http://codereview.chromium.org/507022/diff/7007/7012#newcode3
chrome/browser/gtk/gtk_expanded_container_unittest.cc:3: // found in the LICENSE
file.
On 2009/12/18 09:40:31, Evan Martin wrote:
> Extra newline here

Done.

http://codereview.chromium.org/507022/diff/7007/7012#newcode4
chrome/browser/gtk/gtk_expanded_container_unittest.cc:4: #include
"chrome/browser/gtk/gtk_expanded_container.h"
On 2009/12/18 09:40:31, Evan Martin wrote:
> extra newlinehere

Done.

http://codereview.chromium.org/507022/diff/7007/7019
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/507022/diff/7007/7019#newcode367
chrome/browser/tab_contents/tab_contents_view_gtk.cc:367: if
(view->tab_contents()->delegate())
On 2009/12/18 09:40:31, Evan Martin wrote:
> The linter might complain if you don't have curly braces around this if
> statemetn.

Done.