Implement a GTK LocationBarView and Autocomplete.

Implement a GTK LocationBarView and Autocomplete{Edit,Popup}View.

This implements some beginning functionality of "omnibox".  It uses GtkTextView for a rich text edit for the location bar.  Color emphasis, inline autocomplete, and the results popup work.  You can select one of the omnibox results using the keyboard.  Mouse selection doesn't work.  The results popup code will have to be scraped and reimplemented with cairo / pango.

BUG=8236

[Evan Martin, 2009-03-04 02:54:07]

I think this would be fine to check in, if it doesn't break our existing stuff. 
It looks like there are a couple too many notimpls, tho.

http://codereview.chromium.org/40013/diff/1014/1020
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/1014/1020#newcode185
Line 185: // location_bar_->Update(should_restore_state ? contents : NULL);
why not call this?

http://codereview.chromium.org/40013/diff/1014/1022
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/1014/1022#newcode1
Line 1: #include "chrome/browser/gtk/location_bar_view_gtk.h"
needs file copyright etc

http://codereview.chromium.org/40013/diff/1014/1022#newcode67
Line 67: // what's going on with the LaternateNavURLFetcher better, and hook it
up.
typo

http://codereview.chromium.org/40013/diff/1014/1022#newcode89
Line 89: }
newlines between functions here and below

http://codereview.chromium.org/40013/diff/1014/1023
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/1014/1023#newcode70
Line 70: Profile* profile_;
comments on these

[Dean McNamee, 2009-03-04 15:14:38]

Here we goes.  I still need to do a bunch of cleanup, I'm not
gtk_widget_destroying() or freeing any of my GtkTextBuffer / etc objects. 
Besides that, I suggest you patch this into your client and give it a spin. 
There are a whole bunch of spacing / positioning issues.  It looks good on my
machine, but maybe your fonts are different.

[Evan Martin, 2009-03-04 20:45:29]

ack, i thought i sent this earlier :(

http://codereview.chromium.org/40013/diff/1026/20
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/1026/20#newcode52
Line 52: gtk_text_view_set_left_margin(GTK_TEXT_VIEW(text_view_), 4);
constants => named static const.

kMarginPixels, kTopSpacing

http://codereview.chromium.org/40013/diff/1026/20#newcode62
Line 62: gtk_text_view_set_accepts_tab(GTK_TEXT_VIEW(text_view_), false);
false -> FALSE

http://codereview.chromium.org/40013/diff/1026/20#newcode65
Line 65: NULL, "foreground", "#009614", NULL);
should htese strings be pulled out and named somewhere?

http://codereview.chromium.org/40013/diff/1026/20#newcode74
Line 74: GCallback(&HandleBufferChangedThunk), this);
G_CALLBACK

http://codereview.chromium.org/40013/diff/1026/20#newcode144
Line 144: // TODO(deanm): Is this right, caret_pos is in characters, not bytes?
It better be!  I'd remove this comment.

http://codereview.chromium.org/40013/diff/1026/20#newcode224
Line 224: if (had_newline) {
I wonder if you could just catch enter being pressed for triggering navigation.

The difference is that when someone pastes in multiline text, we *don't* want to
commit the navigation, we just want to make it one line.

This can be done in a different change if you want.

http://codereview.chromium.org/40013/diff/1026/20#newcode248
Line 248: // This is a bit ugly.  We shim in to get the click before the
GtkTextView,
This is a good comment.

http://codereview.chromium.org/40013/diff/1026/20#newcode270
Line 270: gtk_text_view_move_visually(GTK_TEXT_VIEW(text_view_), &start, -1);
http://library.gnome.org/devel/gtk/stable/GtkTextBuffer.html#gtk-text-buffer-...
?

Or you want the view to scroll as well so you use this one.  Hrm, that makes
sense I think.

http://codereview.chromium.org/40013/diff/1026/20#newcode321
Line 321: gdk_color_parse("#fff5c3", &background);
more string constants could be pulled out to top-level

http://codereview.chromium.org/40013/diff/1026/21
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/1026/21#newcode38
Line 38: void FocusLocation();
comments on all non-virtual funcs

http://codereview.chromium.org/40013/diff/1026/21#newcode87
Line 87: void HandleBufferChanged();
I think we stabilized on "OnFooBar" for handling the FooBar signal.

http://codereview.chromium.org/40013/diff/1026/21#newcode108
Line 108: GtkWidget* text_view_;
comments on all member variables

http://codereview.chromium.org/40013/diff/1026/25
File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/1026/25#newcode92
Line 92: // Get the location bar to fit nicely in the toolbar, kinda ugly.
This seems ok to me.  Might want to pull out the padding pixels as constants. 
(E.g., I know that the "1" below means "100%" but the "2" means "pixels" but
someone else might not.)

http://codereview.chromium.org/40013/diff/1026/25#newcode119
Line 119: location_bar_->FocusLocation();
so many layers of indirection!

http://codereview.chromium.org/40013/diff/1026/26
File chrome/browser/gtk/browser_toolbar_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/1026/26#newcode104
Line 104: // The location bar view.
This is implicit from the class name used.  Maybe better would be "the location
bar, including the edit box and go button and blah" or whatever the location bar
consists of.

http://codereview.chromium.org/40013/diff/1026/28
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/1026/28#newcode36
Line 36: // Return the native vbox widget.  You must call Init() first, or the
result
If this returns a vbox, perhaps we could just set the padding on it directly
instead of using the GtkAlignment?

[Dean McNamee, 2009-03-05 14:03:21]

I realize this is missing a lot of comments, it's super early and there is a ton
of surface area here.  Commenting it isn't really helping anything right now,
because I can't honestly write half of the comments until I'm further along and
know how things are actually supposed to work.

Also, I didn't spend time pulling the "#ffffff" style string literals out,
because I will eventually make them linker initialized GdkColors, and it seems
like a waste of time to worry about this now.

There are so many TODOs here, I'm not sure writing it as if it was finished code
(kBlahBlah) when that is all going to change seems like a super waste of time.

Do we really want to use the gboolean macros of TRUE and FALSE? :\

On 2009/03/04 20:45:29, Evan Martin wrote:
> ack, i thought i sent this earlier :(
> 
> http://codereview.chromium.org/40013/diff/1026/20
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode52
> Line 52: gtk_text_view_set_left_margin(GTK_TEXT_VIEW(text_view_), 4);
> constants => named static const.
> 
> kMarginPixels, kTopSpacing
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode62
> Line 62: gtk_text_view_set_accepts_tab(GTK_TEXT_VIEW(text_view_), false);
> false -> FALSE
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode65
> Line 65: NULL, "foreground", "#009614", NULL);
> should htese strings be pulled out and named somewhere?
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode74
> Line 74: GCallback(&HandleBufferChangedThunk), this);
> G_CALLBACK
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode144
> Line 144: // TODO(deanm): Is this right, caret_pos is in characters, not
bytes?
> It better be!  I'd remove this comment.
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode224
> Line 224: if (had_newline) {
> I wonder if you could just catch enter being pressed for triggering
navigation.
> 
> The difference is that when someone pastes in multiline text, we *don't* want
to
> commit the navigation, we just want to make it one line.
> 
> This can be done in a different change if you want.
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode248
> Line 248: // This is a bit ugly.  We shim in to get the click before the
> GtkTextView,
> This is a good comment.
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode270
> Line 270: gtk_text_view_move_visually(GTK_TEXT_VIEW(text_view_), &start, -1);
>
http://library.gnome.org/devel/gtk/stable/GtkTextBuffer.html#gtk-text-buffer-...
> ?
> 
> Or you want the view to scroll as well so you use this one.  Hrm, that makes
> sense I think.
> 
> http://codereview.chromium.org/40013/diff/1026/20#newcode321
> Line 321: gdk_color_parse("#fff5c3", &background);
> more string constants could be pulled out to top-level
> 
> http://codereview.chromium.org/40013/diff/1026/21
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):
> 
> http://codereview.chromium.org/40013/diff/1026/21#newcode38
> Line 38: void FocusLocation();
> comments on all non-virtual funcs
> 
> http://codereview.chromium.org/40013/diff/1026/21#newcode87
> Line 87: void HandleBufferChanged();
> I think we stabilized on "OnFooBar" for handling the FooBar signal.
> 
> http://codereview.chromium.org/40013/diff/1026/21#newcode108
> Line 108: GtkWidget* text_view_;
> comments on all member variables
> 
> http://codereview.chromium.org/40013/diff/1026/25
> File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):
> 
> http://codereview.chromium.org/40013/diff/1026/25#newcode92
> Line 92: // Get the location bar to fit nicely in the toolbar, kinda ugly.
> This seems ok to me.  Might want to pull out the padding pixels as constants. 
> (E.g., I know that the "1" below means "100%" but the "2" means "pixels" but
> someone else might not.)
> 
> http://codereview.chromium.org/40013/diff/1026/25#newcode119
> Line 119: location_bar_->FocusLocation();
> so many layers of indirection!
> 
> http://codereview.chromium.org/40013/diff/1026/26
> File chrome/browser/gtk/browser_toolbar_view_gtk.h (right):
> 
> http://codereview.chromium.org/40013/diff/1026/26#newcode104
> Line 104: // The location bar view.
> This is implicit from the class name used.  Maybe better would be "the
location
> bar, including the edit box and go button and blah" or whatever the location
bar
> consists of.
> 
> http://codereview.chromium.org/40013/diff/1026/28
> File chrome/browser/gtk/location_bar_view_gtk.h (right):
> 
> http://codereview.chromium.org/40013/diff/1026/28#newcode36
> Line 36: // Return the native vbox widget.  You must call Init() first, or the
> result
> If this returns a vbox, perhaps we could just set the padding on it directly
> instead of using the GtkAlignment?

[Dean McNamee, 2009-03-06 16:05:33]

On 2009/03/05 14:03:21, Dean McNamee wrote:
> I realize this is missing a lot of comments, it's super early and there is a
ton
> of surface area here.  Commenting it isn't really helping anything right now,
> because I can't honestly write half of the comments until I'm further along
and
> know how things are actually supposed to work.
> 
> Also, I didn't spend time pulling the "#ffffff" style string literals out,
> because I will eventually make them linker initialized GdkColors, and it seems
> like a waste of time to worry about this now.
> 
> There are so many TODOs here, I'm not sure writing it as if it was finished
code
> (kBlahBlah) when that is all going to change seems like a super waste of time.
> 
> Do we really want to use the gboolean macros of TRUE and FALSE? :\
> 
> On 2009/03/04 20:45:29, Evan Martin wrote:
> > ack, i thought i sent this earlier :(
> > 
> > http://codereview.chromium.org/40013/diff/1026/20
> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode52
> > Line 52: gtk_text_view_set_left_margin(GTK_TEXT_VIEW(text_view_), 4);
> > constants => named static const.
> > 
> > kMarginPixels, kTopSpacing
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode62
> > Line 62: gtk_text_view_set_accepts_tab(GTK_TEXT_VIEW(text_view_), false);
> > false -> FALSE
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode65
> > Line 65: NULL, "foreground", "#009614", NULL);
> > should htese strings be pulled out and named somewhere?
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode74
> > Line 74: GCallback(&HandleBufferChangedThunk), this);
> > G_CALLBACK
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode144
> > Line 144: // TODO(deanm): Is this right, caret_pos is in characters, not
> bytes?
> > It better be!  I'd remove this comment.
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode224
> > Line 224: if (had_newline) {
> > I wonder if you could just catch enter being pressed for triggering
> navigation.
> > 
> > The difference is that when someone pastes in multiline text, we *don't*
want
> to
> > commit the navigation, we just want to make it one line.
> > 
> > This can be done in a different change if you want.


I just implemented this, and now I'm pretty scared it's not right.  The problem
is looking through the gtktextview code, they have special handling of the
Return key to reset the IME.  By eating this key, I'm worried it would break
IME.  I realize this isn't important right now, but it just made me think about
it.  The easier thing to do here to catch pasting, which is a very small amount
of code in gtktextview.  When we paste, I will eat newline characters in the
pasting, so they aren't treated as a navigation.

> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode248
> > Line 248: // This is a bit ugly.  We shim in to get the click before the
> > GtkTextView,
> > This is a good comment.
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode270
> > Line 270: gtk_text_view_move_visually(GTK_TEXT_VIEW(text_view_), &start,
-1);
> >
>
http://library.gnome.org/devel/gtk/stable/GtkTextBuffer.html#gtk-text-buffer-...
> > ?
> > 
> > Or you want the view to scroll as well so you use this one.  Hrm, that makes
> > sense I think.
> > 
> > http://codereview.chromium.org/40013/diff/1026/20#newcode321
> > Line 321: gdk_color_parse("#fff5c3", &background);
> > more string constants could be pulled out to top-level
> > 
> > http://codereview.chromium.org/40013/diff/1026/21
> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):
> > 
> > http://codereview.chromium.org/40013/diff/1026/21#newcode38
> > Line 38: void FocusLocation();
> > comments on all non-virtual funcs
> > 
> > http://codereview.chromium.org/40013/diff/1026/21#newcode87
> > Line 87: void HandleBufferChanged();
> > I think we stabilized on "OnFooBar" for handling the FooBar signal.
> > 
> > http://codereview.chromium.org/40013/diff/1026/21#newcode108
> > Line 108: GtkWidget* text_view_;
> > comments on all member variables
> > 
> > http://codereview.chromium.org/40013/diff/1026/25
> > File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):
> > 
> > http://codereview.chromium.org/40013/diff/1026/25#newcode92
> > Line 92: // Get the location bar to fit nicely in the toolbar, kinda ugly.
> > This seems ok to me.  Might want to pull out the padding pixels as
constants. 
> > (E.g., I know that the "1" below means "100%" but the "2" means "pixels" but
> > someone else might not.)
> > 
> > http://codereview.chromium.org/40013/diff/1026/25#newcode119
> > Line 119: location_bar_->FocusLocation();
> > so many layers of indirection!
> > 
> > http://codereview.chromium.org/40013/diff/1026/26
> > File chrome/browser/gtk/browser_toolbar_view_gtk.h (right):
> > 
> > http://codereview.chromium.org/40013/diff/1026/26#newcode104
> > Line 104: // The location bar view.
> > This is implicit from the class name used.  Maybe better would be "the
> location
> > bar, including the edit box and go button and blah" or whatever the location
> bar
> > consists of.
> > 
> > http://codereview.chromium.org/40013/diff/1026/28
> > File chrome/browser/gtk/location_bar_view_gtk.h (right):
> > 
> > http://codereview.chromium.org/40013/diff/1026/28#newcode36
> > Line 36: // Return the native vbox widget.  You must call Init() first, or
the
> > result
> > If this returns a vbox, perhaps we could just set the padding on it directly
> > instead of using the GtkAlignment?

[Dean McNamee, 2009-03-06 17:54:54]

Bunch of new code, comments welcome.  Will polish things once I figure out
what's going on.  The Gtk model basically doesn't fit the Windows model at all,
so everything is a mess.

On 2009/03/06 16:05:33, Dean McNamee wrote:
> On 2009/03/05 14:03:21, Dean McNamee wrote:
> > I realize this is missing a lot of comments, it's super early and there is a
> ton
> > of surface area here.  Commenting it isn't really helping anything right
now,
> > because I can't honestly write half of the comments until I'm further along
> and
> > know how things are actually supposed to work.
> > 
> > Also, I didn't spend time pulling the "#ffffff" style string literals out,
> > because I will eventually make them linker initialized GdkColors, and it
seems
> > like a waste of time to worry about this now.
> > 
> > There are so many TODOs here, I'm not sure writing it as if it was finished
> code
> > (kBlahBlah) when that is all going to change seems like a super waste of
time.
> > 
> > Do we really want to use the gboolean macros of TRUE and FALSE? :\
> > 
> > On 2009/03/04 20:45:29, Evan Martin wrote:
> > > ack, i thought i sent this earlier :(
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20
> > > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode52
> > > Line 52: gtk_text_view_set_left_margin(GTK_TEXT_VIEW(text_view_), 4);
> > > constants => named static const.
> > > 
> > > kMarginPixels, kTopSpacing
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode62
> > > Line 62: gtk_text_view_set_accepts_tab(GTK_TEXT_VIEW(text_view_), false);
> > > false -> FALSE
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode65
> > > Line 65: NULL, "foreground", "#009614", NULL);
> > > should htese strings be pulled out and named somewhere?
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode74
> > > Line 74: GCallback(&HandleBufferChangedThunk), this);
> > > G_CALLBACK
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode144
> > > Line 144: // TODO(deanm): Is this right, caret_pos is in characters, not
> > bytes?
> > > It better be!  I'd remove this comment.
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode224
> > > Line 224: if (had_newline) {
> > > I wonder if you could just catch enter being pressed for triggering
> > navigation.
> > > 
> > > The difference is that when someone pastes in multiline text, we *don't*
> want
> > to
> > > commit the navigation, we just want to make it one line.
> > > 
> > > This can be done in a different change if you want.
> 
> 
> I just implemented this, and now I'm pretty scared it's not right.  The
problem
> is looking through the gtktextview code, they have special handling of the
> Return key to reset the IME.  By eating this key, I'm worried it would break
> IME.  I realize this isn't important right now, but it just made me think
about
> it.  The easier thing to do here to catch pasting, which is a very small
amount
> of code in gtktextview.  When we paste, I will eat newline characters in the
> pasting, so they aren't treated as a navigation.
> 
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode248
> > > Line 248: // This is a bit ugly.  We shim in to get the click before the
> > > GtkTextView,
> > > This is a good comment.
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode270
> > > Line 270: gtk_text_view_move_visually(GTK_TEXT_VIEW(text_view_), &start,
> -1);
> > >
> >
>
http://library.gnome.org/devel/gtk/stable/GtkTextBuffer.html#gtk-text-buffer-...
> > > ?
> > > 
> > > Or you want the view to scroll as well so you use this one.  Hrm, that
makes
> > > sense I think.
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/20#newcode321
> > > Line 321: gdk_color_parse("#fff5c3", &background);
> > > more string constants could be pulled out to top-level
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/21
> > > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/21#newcode38
> > > Line 38: void FocusLocation();
> > > comments on all non-virtual funcs
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/21#newcode87
> > > Line 87: void HandleBufferChanged();
> > > I think we stabilized on "OnFooBar" for handling the FooBar signal.
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/21#newcode108
> > > Line 108: GtkWidget* text_view_;
> > > comments on all member variables
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/25
> > > File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right):
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/25#newcode92
> > > Line 92: // Get the location bar to fit nicely in the toolbar, kinda ugly.
> > > This seems ok to me.  Might want to pull out the padding pixels as
> constants. 
> > > (E.g., I know that the "1" below means "100%" but the "2" means "pixels"
but
> > > someone else might not.)
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/25#newcode119
> > > Line 119: location_bar_->FocusLocation();
> > > so many layers of indirection!
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/26
> > > File chrome/browser/gtk/browser_toolbar_view_gtk.h (right):
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/26#newcode104
> > > Line 104: // The location bar view.
> > > This is implicit from the class name used.  Maybe better would be "the
> > location
> > > bar, including the edit box and go button and blah" or whatever the
location
> > bar
> > > consists of.
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/28
> > > File chrome/browser/gtk/location_bar_view_gtk.h (right):
> > > 
> > > http://codereview.chromium.org/40013/diff/1026/28#newcode36
> > > Line 36: // Return the native vbox widget.  You must call Init() first, or
> the
> > > result
> > > If this returns a vbox, perhaps we could just set the padding on it
directly
> > > instead of using the GtkAlignment?

[Evan Martin, 2009-03-06 17:58:23]

http://codereview.chromium.org/40013/diff/51/53
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/51/53#newcode38
Line 38: void FocusLocation();
what's the point of commenting if you don't respond to my earlier ones. 
:~~~~~~~~(

[Evan Stade, 2009-03-06 23:28:32]

There are different senses of the word View, but I think in the case of
autocomplete, View means "depends on chrome/views". So, for example, the common
class should be called AutoCompletePopup, and then there should be an
AutoCompletePopupView (or AutoCompletePopupWin) and an AutoCompletePopupGtk.

At least that's how I see it. In any case it would be easy to go back and change
at a later date.

http://codereview.chromium.org/40013/diff/51/52
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/51/52#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009

[Peter Kasting, 2009-03-06 23:32:01]

On 2009/03/06 23:28:32, estade wrote:
> There are different senses of the word View, but I think in the case of
> autocomplete, View means "depends on chrome/views".

No, it doesn't.  In fact I have a bug on file about how "maybe I should actually
make the autocomplete views be views".

[Dean McNamee, 2009-03-08 20:23:50]

Evan: Sorry, I looked through all your feedback, but I just didn't have time to
clean up the style yet.  I was hoping to still get some feedback on my approach
and GTK code, before I finish all of the polishing requested.

Other Evan (estade):  It's view as in Model View Controller.

More code and cleanup tomorrow.

On 2009/03/06 23:32:01, pkasting wrote:
> On 2009/03/06 23:28:32, estade wrote:
> > There are different senses of the word View, but I think in the case of
> > autocomplete, View means "depends on chrome/views".
> 
> No, it doesn't.  In fact I have a bug on file about how "maybe I should
actually
> make the autocomplete views be views".

[Dean McNamee, 2009-03-09 17:40:06]

Ok, I think this is good enough for review now.  I think this would be a good
place to get this committed, before it becomes a super monster patch.  I think
it is usable (although definitely has bugs).

On 2009/03/08 20:23:50, Dean McNamee wrote:
> Evan: Sorry, I looked through all your feedback, but I just didn't have time
to
> clean up the style yet.  I was hoping to still get some feedback on my
approach
> and GTK code, before I finish all of the polishing requested.
> 
> Other Evan (estade):  It's view as in Model View Controller.
> 
> More code and cleanup tomorrow.
> 
> On 2009/03/06 23:32:01, pkasting wrote:
> > On 2009/03/06 23:28:32, estade wrote:
> > > There are different senses of the word View, but I think in the case of
> > > autocomplete, View means "depends on chrome/views".
> > 
> > No, it doesn't.  In fact I have a bug on file about how "maybe I should
> actually
> > make the autocomplete views be views".

[Peter Kasting, 2009-03-09 18:04:12]

Looks pretty good for a rough cut.

http://codereview.chromium.org/40013/diff/98/2001
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2001#newcode105
Line 105: // Select all.
Maybe you should just call SelectAll() and put this code there, since right now
it's NOTIMPLEMENTED()?

http://codereview.chromium.org/40013/diff/98/2001#newcode175
Line 175: // TODO(deanm): Is this right, caret_pos is in characters, not bytes?
Right, caret_pos is in characters, so your code is right iff get_iter_at_offset
expects a character position.

http://codereview.chromium.org/40013/diff/98/2001#newcode201
Line 201: // Don't inline autocomplete when:
Nit: Should probably update this comment.  Dunno if you need any IME handling at
all or if you just don't get called back in that case.  Feel free to also update
the Windows comment, both to the effect of "the model handles the case where the
user is deleteing text or has just pasted something blah blah".

http://codereview.chromium.org/40013/diff/98/2001#newcode226
Line 226: if (display_text == GetText()) {
Nit: no {}

http://codereview.chromium.org/40013/diff/98/2001#newcode281
Line 281: // TODO(deanm): This OnBefore/After business doesn't fit our model.
Really?

http://codereview.chromium.org/40013/diff/98/2001#newcode300
Line 300: // TODO(deanm): obviously super inefficient.
Yeah, this is pretty worrisome if you paste in a string of 10k characters
followed by 10k newlines or something.  Maybe  you can just loop through the
string once copying non-newlines into a buffer?

http://codereview.chromium.org/40013/diff/98/2002
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/98/2002#newcode39
Line 39: // Grab keyboard input focus to the location widget.
Nit: grammar

http://codereview.chromium.org/40013/diff/98/2002#newcode96
Line 96: // Work in integers since to match the gint GTK APIs.
Nit: grammar

http://codereview.chromium.org/40013/diff/98/2009
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/98/2009#newcode68
Line 68: scoped_ptr<AutocompleteEditViewGtk> ace_view_;
Nit: Not a big fan of this name since "ace" looks like a word, not an
abbreviation

[Evan Martin, 2009-03-09 18:27:53]

I have one overall comment that I'll write here instead of writing below -- you
should open each header file and verify there are comments on all non-overridden
functions and member variables.  Throw anything in there that isn't obvious from
the variable name -- e.g. "base_tag_" is light gray, not dark, I think?

http://codereview.chromium.org/40013/diff/98/2001
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2001#newcode289
Line 289: *y += 25;  // TODO(deanm): Scientific huh.
This is a guess at the height of the widget?  You could just do
text_view_->requisition.height

[Evan Stade, 2009-03-09 21:07:44]

http://codereview.chromium.org/40013/diff/98/2001
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2001#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
2009

http://codereview.chromium.org/40013/diff/98/2003
File chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2003#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
2009

http://codereview.chromium.org/40013/diff/98/2003#newcode21
Line 21: const GdkColor kPopupBackground = {0, 61680, 61680, 61680};  //
#f0f0f0
I think this color isn't actually being shown. You need to add

gtk_container_set_border_width(GTK_CONTAINER(window_), 1);

in the constructor.

http://codereview.chromium.org/40013/diff/98/2003#newcode78
Line 78: }
else {
 /* set bg to white */
}

http://codereview.chromium.org/40013/diff/98/2003#newcode98
Line 98: gtk_widget_set_size_request(GTK_WIDGET(window_), width, -1);
it's a mystery to me why this doesn't work (the popup refuses to shrink
with the number of suggestions). Evan?

http://codereview.chromium.org/40013/diff/98/2008
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2008#newcode40
Line 40: // Get the location bar to fit nicely in the toolbar, kinda
ugly.
Can't you pretty much get the same thing (just 1 pixel off on the bottom
alignment) by doing:

gtk_box_pack_start(GTK_BOX(vbox_), ace_view_->widget(), TRUE, TRUE, 2);

or, if there is some problem with that, does it work to get rid of the
vbox and just use the alignment as the base widget?

http://codereview.chromium.org/40013/diff/98/2008#newcode73
Line 73: command_updater_->ExecuteCommand(IDC_OPEN_CURRENT_URL);
I don't think you want this here (?)

[Evan Stade, 2009-03-10 06:58:43]

oh yeah, forgot to mention: I know that you are going to refactor this, so feel
free to ignore whichever comments would be made irrelevant by those plans.

[Dean McNamee, 2009-03-10 12:34:39]

Thanks for all the feedback, estade had a bunch of great comments, and I now
have a good working border and the popup window resizes correctly when there are
fewer results.

Committing!

http://codereview.chromium.org/40013/diff/98/2001
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2001#newcode105
Line 105: // Select all.
Done.

On 2009/03/09 18:04:12, pkasting wrote:
> Maybe you should just call SelectAll() and put this code there, since right
now
> it's NOTIMPLEMENTED()?

http://codereview.chromium.org/40013/diff/98/2001#newcode201
Line 201: // Don't inline autocomplete when:
Yeah, I am going to try to do a diff / cleanup phase to get the windows and
Linux code to match better.  I left the comment unchanged on Windows for now.

On 2009/03/09 18:04:12, pkasting wrote:
> Nit: Should probably update this comment.  Dunno if you need any IME handling
at
> all or if you just don't get called back in that case.  Feel free to also
update
> the Windows comment, both to the effect of "the model handles the case where
the
> user is deleteing text or has just pasted something blah blah".

http://codereview.chromium.org/40013/diff/98/2001#newcode289
Line 289: *y += 25;  // TODO(deanm): Scientific huh.
On 2009/03/09 18:27:53, Evan Martin wrote:
> This is a guess at the height of the widget?  You could just do
> text_view_->requisition.height
You mean allocation?

http://codereview.chromium.org/40013/diff/98/2001#newcode300
Line 300: // TODO(deanm): obviously super inefficient.
On 2009/03/09 18:04:13, pkasting wrote:
> Yeah, this is pretty worrisome if you paste in a string of 10k characters
> followed by 10k newlines or something.  Maybe  you can just loop through the
> string once copying non-newlines into a buffer?

I definitely agree, I can save the character position and get an iterator for it
the next time around.  I will have to follow up on it, it's not super important
right now, and I'm afraid I'll break something subtle.

http://codereview.chromium.org/40013/diff/98/2003
File chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2003#newcode21
Line 21: const GdkColor kPopupBackground = {0, 61680, 61680, 61680};  // #f0f0f0
On 2009/03/09 21:06:06, estade wrote:
> I think this color isn't actually being shown. You need to add
> 
> gtk_container_set_border_width(GTK_CONTAINER(window_), 1);

Smart, I was originally trying to do this differently.  Also, I never know when
to use _base or _bg.  I think this is a case it seems I should be using _bg.

> 
> in the constructor.

http://codereview.chromium.org/40013/diff/98/2003#newcode78
Line 78: }
Yeah, it's always a new widget, so I'm just letting the background be the
default.  I see what you're saying now though, which is different than what I
was trying to achieve.  This was all before I was using an event box, I get what
you're saying now.  Hopefully fixed.

On 2009/03/09 21:06:06, estade wrote:
> else {
>   /* set bg to white */
> }

http://codereview.chromium.org/40013/diff/98/2003#newcode98
Line 98: gtk_widget_set_size_request(GTK_WIDGET(window_), width, -1);
On 2009/03/09 21:06:06, estade wrote:
> it's a mystery to me why this doesn't work (the popup refuses to shrink with
the
> number of suggestions). Evan?

Yeah, I thought the same thing.  Evan?

Ah, fixed with   gtk_window_set_resizable(GTK_WINDOW(window_), FALSE);

http://codereview.chromium.org/40013/diff/98/2008
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/40013/diff/98/2008#newcode40
Line 40: // Get the location bar to fit nicely in the toolbar, kinda ugly.
You are probably right, I am hoping this code will be temporary, the layout will
have to completely change when there are more members to the location bar
(keyword search icon, ssl icon, etc).

On 2009/03/09 21:06:06, estade wrote:
> Can't you pretty much get the same thing (just 1 pixel off on the bottom
> alignment) by doing:
> 
> gtk_box_pack_start(GTK_BOX(vbox_), ace_view_->widget(), TRUE, TRUE, 2);
> 
> or, if there is some problem with that, does it work to get rid of the vbox
and
> just use the alignment as the base widget?

http://codereview.chromium.org/40013/diff/98/2008#newcode73
Line 73: command_updater_->ExecuteCommand(IDC_OPEN_CURRENT_URL);
Super good catch, thanks.

On 2009/03/09 21:06:06, estade wrote:
> I don't think you want this here (?)

http://codereview.chromium.org/40013/diff/98/2009
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/40013/diff/98/2009#newcode68
Line 68: scoped_ptr<AutocompleteEditViewGtk> ace_view_;
On 2009/03/09 18:04:13, pkasting wrote:
> Nit: Not a big fan of this name since "ace" looks like a word, not an
> abbreviation

changed to edit_view_