Make FindBar obey dialog bounds for GTK. Will fix other platforms in separate patches.

Test to verify that FindBar fits within narrow browser window.

BUG=None
TEST=FindInPageControllerTest.FitWindow

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

[jennb, 2011-05-23 18:56:13]

Evan, do you mind reviewing this for me? 

Thanks,
Jenn

[Evan Stade, 2011-05-23 19:29:15]

http://codereview.chromium.org/7063003/diff/1/chrome/browser/ui/gtk/find_bar_...
File chrome/browser/ui/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/7063003/diff/1/chrome/browser/ui/gtk/find_bar_...
chrome/browser/ui/gtk/find_bar_gtk.cc:593: gtk_widget_size_request(container_,
&req);
perhaps a note here about why you are returning the width request and not the
actual width (i.e. container_->allocation.width)

http://codereview.chromium.org/7063003/diff/1/chrome/browser/ui/gtk/find_bar_...
chrome/browser/ui/gtk/find_bar_gtk.cc:765:
gtk_widget_set_size_request(content_hbox, new_width, -1);
technically you aren't supposed to change your request in response to an
allocate signal because it can cause an infinite loop (size request causes
another allocate causes another size request...)

What about connecting to the allocation signal on the content area (in
browser_window_gtk.cc) and passing that through to the findbar (e.g.
SetAvailableWidth or something like that)

[jennb, 2011-05-24 01:05:37]

Ready for another look.

http://codereview.chromium.org/7063003/diff/1/chrome/browser/ui/gtk/find_bar_...
File chrome/browser/ui/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/7063003/diff/1/chrome/browser/ui/gtk/find_bar_...
chrome/browser/ui/gtk/find_bar_gtk.cc:593: gtk_widget_size_request(container_,
&req);
On 2011/05/23 19:29:15, Evan Stade wrote:
> perhaps a note here about why you are returning the width request and not the
> actual width (i.e. container_->allocation.width)

Changed to use allocation. I didn't know about it before.

http://codereview.chromium.org/7063003/diff/1/chrome/browser/ui/gtk/find_bar_...
chrome/browser/ui/gtk/find_bar_gtk.cc:765:
gtk_widget_set_size_request(content_hbox, new_width, -1);
On 2011/05/23 19:29:15, Evan Stade wrote:
> technically you aren't supposed to change your request in response to an
> allocate signal because it can cause an infinite loop (size request causes
> another allocate causes another size request...)

Good tip. Thanks!

> 
> What about connecting to the allocation signal on the content area (in
> browser_window_gtk.cc) and passing that through to the findbar (e.g.
> SetAvailableWidth or something like that)

I think I managed to do what you're suggesting...

[jennb, 2011-05-24 17:00:27]

Converted to test only patch.  Will re-run try jobs after Evan's patch is
committed.  See:

http://codereview.chromium.org/6992017/

[Evan Stade, 2011-05-25 18:18:10]

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right):

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:1092:
IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, FitWindow) {
don't you need to disable this on mac?

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/gtk/find_b...
File chrome/browser/ui/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/gtk/find_b...
chrome/browser/ui/gtk/find_bar_gtk.cc:583: gtk_widget_get_allocation(container_,
&allocation);
I'd just use container_->allocation.width, we use it in a million other places
and we might as well be consistent

[jennb, 2011-05-25 19:02:41]

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right):

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:1092:
IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, FitWindow) {
On 2011/05/25 18:18:10, Evan Stade wrote:
> don't you need to disable this on mac?

I've set things up so that this test passes on the other platforms even though I
haven't fixed FindBar there by making GetWidth return 0 on them.

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/gtk/find_b...
File chrome/browser/ui/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/gtk/find_b...
chrome/browser/ui/gtk/find_bar_gtk.cc:583: gtk_widget_get_allocation(container_,
&allocation);
On 2011/05/25 18:18:10, Evan Stade wrote:
> I'd just use container_->allocation.width, we use it in a million other places
> and we might as well be consistent

Done.

[Evan Stade, 2011-05-25 19:14:01]

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right):

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:1092:
IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, FitWindow) {
On 2011/05/25 19:02:41, jennb wrote:
> On 2011/05/25 18:18:10, Evan Stade wrote:
> > don't you need to disable this on mac?
> 
> I've set things up so that this test passes on the other platforms even though
I
> haven't fixed FindBar there by making GetWidth return 0 on them.

ok, but that seems deceptive. If I looked at this test I'd assume it's
meaningful on all platforms.

[jennb, 2011-05-25 19:33:41]

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right):

http://codereview.chromium.org/7063003/diff/5001/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:1092:
IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, FitWindow) {
On 2011/05/25 19:14:01, Evan Stade wrote:
> On 2011/05/25 19:02:41, jennb wrote:
> > On 2011/05/25 18:18:10, Evan Stade wrote:
> > > don't you need to disable this on mac?
> > 
> > I've set things up so that this test passes on the other platforms even
though
> I
> > haven't fixed FindBar there by making GetWidth return 0 on them.
> 
> ok, but that seems deceptive. If I looked at this test I'd assume it's
> meaningful on all platforms.

I'm actively working on the FindBars for the other platforms.  This is just a
way to break up my patches.  I have NOTIMPLEMENTED in the GetWIdth() impls as a
TODO indicator.

[Evan Stade, 2011-05-25 19:39:21]

lgtm