Linux findbar improvements:...

Linux findbar improvements:

* clean up toolbar/infobar/findbar borders
* move findbar to BrowserWindowGtk's vbox (so it stacks on top of infobar, as on windows)
* properly show findbar when switching between tabs

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

[Evan Stade, 2009-04-29 00:55:47]

So I ran into a couple problems with this GtkFixed approach. The first problem
is that the GtkFixed has to be at least 1 pixel tall, so currently we are
drawing one grey pixel. This patch at least changes the pixel to the border
color, but it still would be nice if there were no pixel there. (See comments in
constructor)

The second problem is switching between two tabs that have the findbar open.
Since you have to paint the tab contents, then paint the findbar back over it,
there is flicker that is not present on windows.

[Evan Martin, 2009-04-29 01:17:03]

Couldn't we just hide the GtkFixed when it's not needed?

[Evan Stade, 2009-04-29 01:18:46]

yes, but then showing the findbar makes the border grow by 1 pixel

[Evan Stade, 2009-04-29 01:53:44]

alright, solved the flickering. Just had to make sure the hide()/show() came
before any painting (see tab_contents_container_gtk.cc::51-54)

I think it's worth reviewing now.

[tony, 2009-04-29 17:23:54]

What did you mean by properly show findbar when switching tabs?  It seems to
already remember which tabs have a find bar and show/hide appropriately for me.

http://codereview.chromium.org/99166/diff/13/1015
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/99166/diff/13/1015#newcode645
Line 645: gtk_box_pack_start(GTK_BOX(content_vbox_), findbar->widget(), FALSE,
FALSE, 0);
Nit: 80 cols.

http://codereview.chromium.org/99166/diff/13/1013
File chrome/browser/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/99166/diff/13/1013#newcode67
Line 67: // alignment and a gtk event box to get the padding and light blue
Can you update this comment with the new hierarchy?

http://codereview.chromium.org/99166/diff/13/1013#newcode80
Line 80: gtk_widget_set_size_request(border_, 1, 1);
Out of curiosity, what happens if you try to size |fixed_| to 0 pixels tall?

http://codereview.chromium.org/99166/diff/13/1013#newcode168
Line 168: // TODO(port): Logic for the positioning of the find bar should be
factored
I would rather this have an actual name associated with it since it's in GTK
code and we're likely the only ones who will see it.

http://codereview.chromium.org/99166/diff/13/1013#newcode177
Line 177: gtk_fixed_move(GTK_FIXED(widget()), container_, new_pos.x(), -1);
Nit: Maybe move the -1 into a const?

http://codereview.chromium.org/99166/diff/13/1013#newcode197
Line 197: void FindBarGtk::AssertOnTop() {
Assert makes it sound like this only checks a condition (no side effects). 
Maybe call it PlaceFindBarOnTop?

http://codereview.chromium.org/99166/diff/13/1013#newcode221
Line 221: void FindBarGtk::ShowContainerWidget() {
Isn't this the same as AssertOnTop?  Do we need both methods?

http://codereview.chromium.org/99166/diff/13/1012
File chrome/browser/gtk/tab_contents_container_gtk.h (right):

http://codereview.chromium.org/99166/diff/13/1012#newcode34
Line 34: void SetFindBar(FindBarGtk* findbar) { findbar_ = findbar; }
Nit: set_find_bar for fast setters?

http://codereview.chromium.org/99166/diff/13/1011
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/99166/diff/13/1011#newcode185
Line 185: NOTIMPLEMENTED() << " Need to restore the focus position on this
page.";
This change seems unrelated?

[Evan Stade, 2009-04-29 18:07:01]

I think there are currently problems with switching between tabs that both have
the find bar open, although I can't go back and test at the moment since my
build is borked.

http://codereview.chromium.org/99166/diff/13/1013
File chrome/browser/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/99166/diff/13/1013#newcode67
Line 67: // alignment and a gtk event box to get the padding and light blue
On 2009/04/29 17:23:54, tony wrote:
> Can you update this comment with the new hierarchy?

Done.

http://codereview.chromium.org/99166/diff/13/1013#newcode80
Line 80: gtk_widget_set_size_request(border_, 1, 1);
On 2009/04/29 17:23:54, tony wrote:
> Out of curiosity, what happens if you try to size |fixed_| to 0 pixels tall?

From the docs:
Widgets can't actually be allocated a size less than 1 by 1, but you can pass
0,0 to this function to mean "as small as possible."

As you can see on line 87, I do actually set the height request to 0.

http://codereview.chromium.org/99166/diff/13/1013#newcode168
Line 168: // TODO(port): Logic for the positioning of the find bar should be
factored
On 2009/04/29 17:23:54, tony wrote:
> I would rather this have an actual name associated with it since it's in GTK
> code and we're likely the only ones who will see it.

Done.

http://codereview.chromium.org/99166/diff/13/1013#newcode177
Line 177: gtk_fixed_move(GTK_FIXED(widget()), container_, new_pos.x(), -1);
On 2009/04/29 17:23:54, tony wrote:
> Nit: Maybe move the -1 into a const?

Done.

http://codereview.chromium.org/99166/diff/13/1013#newcode197
Line 197: void FindBarGtk::AssertOnTop() {
On 2009/04/29 17:23:54, tony wrote:
> Assert makes it sound like this only checks a condition (no side effects). 
> Maybe call it PlaceFindBarOnTop?

Assert->Assure

http://codereview.chromium.org/99166/diff/13/1013#newcode221
Line 221: void FindBarGtk::ShowContainerWidget() {
On 2009/04/29 17:23:54, tony wrote:
> Isn't this the same as AssertOnTop?  Do we need both methods?

oh woops, this one's vestigial from patch set 1

http://codereview.chromium.org/99166/diff/13/1012
File chrome/browser/gtk/tab_contents_container_gtk.h (right):

http://codereview.chromium.org/99166/diff/13/1012#newcode34
Line 34: void SetFindBar(FindBarGtk* findbar) { findbar_ = findbar; }
On 2009/04/29 17:23:54, tony wrote:
> Nit: set_find_bar for fast setters?

Done.

http://codereview.chromium.org/99166/diff/13/1011
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/99166/diff/13/1011#newcode185
Line 185: NOTIMPLEMENTED() << " Need to restore the focus position on this
page.";
On 2009/04/29 17:23:54, tony wrote:
> This change seems unrelated?

It is related to improving find bar focus, but I can take this file out of the
CL if you want.

[tony, 2009-04-29 19:36:46]

LGTM!

If there are problems with switching tabs (I seem to get one when I edit one
that's open), it's probably code we haven't implemented or it's in the cross
platform code since that's what controls the show/hide behavior.