Linux: Make the findbar hang over the render view.

Linux: Make the findbar hang over the render view.

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

[Evan Stade, 2009-04-27 22:46:07]

still needs work (animations, curvy edges, getting borders right), but that can
be a later patch

[Evan Martin, 2009-04-27 22:55:25]

+findbar man

[Evan Martin, 2009-04-27 22:57:53]

http://codereview.chromium.org/100077/diff/10/1004
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/100077/diff/10/1004#newcode127
Line 127: void TabContentsContainerGtk::OnSizeAllocate(GtkWidget* fixed,
The name of this function makes it look like it's the size-allocate for the
entire TabContentsContainer.  Is that not the case?  Why do we have vbox_?

[Evan Stade, 2009-04-27 23:09:04]

http://codereview.chromium.org/100077/diff/10/1004
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/100077/diff/10/1004#newcode127
Line 127: void TabContentsContainerGtk::OnSizeAllocate(GtkWidget* fixed,
On 2009/04/27 22:57:54, Evan Martin wrote:
> The name of this function makes it look like it's the size-allocate for the
> entire TabContentsContainer.  Is that not the case? 

It's not the case.

> Why do we have vbox_?

We need vbox_ to hold the (0 pixel tall) GtkFixed right above the content area.

[tony, 2009-04-27 23:20:27]

http://codereview.chromium.org/100077/diff/10/1004
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/100077/diff/10/1004#newcode131
Line 131: // TODO(port): Take into account whether the vertical scrollbar is
showing.
The logic for the horizontal position of the find bar should be refactored out
of FindBarWin and into FindBarController.  That said, it doesn't have to be in
this change, but please update the comment.

http://codereview.chromium.org/100077/diff/10/1004#newcode132
Line 132: int xposition = allocation->width -
What happens if xposition is negative?

http://codereview.chromium.org/100077/diff/10/1004#newcode137
Line 137: contents_container->findbar_x_pos_ = xposition;
Can't we query the widget to get it's position?  This seems unnecessary.

[Evan Stade, 2009-04-28 18:42:38]

http://codereview.chromium.org/100077/diff/10/1004
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/100077/diff/10/1004#newcode131
Line 131: // TODO(port): Take into account whether the vertical scrollbar is
showing.
On 2009/04/27 23:20:28, tony wrote:
> The logic for the horizontal position of the find bar should be refactored out
> of FindBarWin and into FindBarController.  That said, it doesn't have to be in
> this change, but please update the comment.

Done.

http://codereview.chromium.org/100077/diff/10/1004#newcode132
Line 132: int xposition = allocation->width -
On 2009/04/27 23:20:28, tony wrote:
> What happens if xposition is negative?

You might not see the left side of the find bar, but it won't crash or anything.
On Windows, you always see the left side, but might not see the right, so we
don't quite match that behavior, but it will be fixed when the TODO is fixed.

However, I did notice that the browser crashes when it gets too narrow (with or
without this patch), so I'm going to look into that next.

http://codereview.chromium.org/100077/diff/10/1004#newcode137
Line 137: contents_container->findbar_x_pos_ = xposition;
On 2009/04/27 23:20:28, tony wrote:
> Can't we query the widget to get it's position?  This seems unnecessary.

Done.

[Evan Stade, 2009-04-28 18:45:14]

> However, I did notice that the browser crashes when it gets too narrow (with
or
> without this patch), so I'm going to look into that next.

(I think it's the location bar)

[tony, 2009-04-28 19:09:54]

On 2009/04/28 18:42:38, Evan Stade wrote:
> On 2009/04/27 23:20:28, tony wrote:
> > Can't we query the widget to get it's position?  This seems unnecessary.
> 
> Done.

What I meant was, do we need to keep findbar_x_pos_ at all?  Can't we compare
|xposition| to widget->allocation.x?

Other than that, LGTM.

[Evan Stade, 2009-04-28 21:28:45]

On 2009/04/28 19:09:54, tony wrote:
> On 2009/04/28 18:42:38, Evan Stade wrote:
> > On 2009/04/27 23:20:28, tony wrote:
> > > Can't we query the widget to get it's position?  This seems unnecessary.
> > 
> > Done.
> 
> What I meant was, do we need to keep findbar_x_pos_ at all?  Can't we compare
> |xposition| to widget->allocation.x?

right, by Done I meant "o yea, good point, I'll do that instead"

[tony, 2009-04-28 21:36:12]

LGTM