GTK: give the findbox curvy edges....

GTK: give the findbox curvy edges.

Most of the new code concerns shaping the widget to be non-rectangular.

Also, sprinkle some "const" qualifiers on NineBox functions.


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

[Evan Martin, 2009-04-30 21:49:47]

http://codereview.chromium.org/100224/diff/1/2
File chrome/browser/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/100224/diff/1/2#newcode46
Line 46: NineBox* GetDialogBackground() {
comment on this?  (e.g., who owns the pointer, etc)

http://codereview.chromium.org/100224/diff/1/2#newcode272
Line 272: if (widget == findbar->widget()) {
It seems these branches should just be different functions.  They have entirely
different behaviors...

http://codereview.chromium.org/100224/diff/1/2#newcode296
Line 296: static bool first_allocate = true;
I think we generally avoid statics.  Why not put this state in the class? 
(Imagine unit testing, multiple windows with multiple findbars, etc.)

http://codereview.chromium.org/100224/diff/1/3
File chrome/browser/gtk/nine_box.cc (right):

http://codereview.chromium.org/100224/diff/1/3#newcode141
Line 141: // Paint the left and ride sides.
typo: ride

[Evan Stade, 2009-04-30 22:12:42]

http://codereview.chromium.org/100224/diff/1/2
File chrome/browser/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/100224/diff/1/2#newcode46
Line 46: NineBox* GetDialogBackground() {
On 2009/04/30 21:49:47, Evan Martin wrote:
> comment on this?  (e.g., who owns the pointer, etc)

I was just going to let it leak since we don't want to release till browser
shutdown anyway, but I changed it to a scoped_ptr.

http://codereview.chromium.org/100224/diff/1/2#newcode272
Line 272: if (widget == findbar->widget()) {
On 2009/04/30 21:49:47, Evan Martin wrote:
> It seems these branches should just be different functions.  They have
entirely
> different behaviors...

sure, they could be different functions. I think there are other places where
this same thing is done, though. 

changed.

http://codereview.chromium.org/100224/diff/1/2#newcode296
Line 296: static bool first_allocate = true;
On 2009/04/30 21:49:47, Evan Martin wrote:
> I think we generally avoid statics.  Why not put this state in the class? 
> (Imagine unit testing, multiple windows with multiple findbars, etc.)

ok

[Evan Martin, 2009-04-30 22:23:08]

http://codereview.chromium.org/100224/diff/1/2
File chrome/browser/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/100224/diff/1/2#newcode46
Line 46: NineBox* GetDialogBackground() {
On 2009/04/30 22:12:42, Evan Stade wrote:
> On 2009/04/30 21:49:47, Evan Martin wrote:
> > comment on this?  (e.g., who owns the pointer, etc)
> 
> I was just going to let it leak since we don't want to release till browser
> shutdown anyway, but I changed it to a scoped_ptr.

I think leaking was fine... I just meant that the function ought to have
commetns about what kind of pointer it returns, who should be responsible for
it.

[Evan Stade, 2009-04-30 22:53:14]

http://codereview.chromium.org/100224/diff/1/2
File chrome/browser/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/100224/diff/1/2#newcode46
Line 46: NineBox* GetDialogBackground() {
On 2009/04/30 22:23:09, Evan Martin wrote:
> On 2009/04/30 22:12:42, Evan Stade wrote:
> > On 2009/04/30 21:49:47, Evan Martin wrote:
> > > comment on this?  (e.g., who owns the pointer, etc)
> > 
> > I was just going to let it leak since we don't want to release till browser
> > shutdown anyway, but I changed it to a scoped_ptr.
> 
> I think leaking was fine... I just meant that the function ought to have
> commetns about what kind of pointer it returns, who should be responsible for
> it.

Added a comment, and made the return value const in hopes of making it more
intrinsically obvious.

[Evan Martin, 2009-04-30 23:01:36]

Looks good!

http://codereview.chromium.org/100224/diff/23/1010
File chrome/browser/gtk/find_bar_gtk.cc (right):

http://codereview.chromium.org/100224/diff/23/1010#newcode52
Line 52: if (!initialized) {
This can be
if (!dialog_background) {
}

and you can get rid of initialized.

http://codereview.chromium.org/100224/diff/23/1009
File chrome/browser/gtk/find_bar_gtk.h (right):

http://codereview.chromium.org/100224/diff/23/1009#newcode97
Line 97: // We don't want to call ContourWidget() more than once.
Maybe something like "true after ContourWidget is called", just 'cause it's not
obvious how the comment relates to the boolean (the names don't match up, for
example).

[Evan Stade, 2009-04-30 23:12:05]

done )