Chromium Code Reviews
Help | Chromium Project | Sign in
(6)

Issue 40013: Implement a GTK LocationBarView and Autocomplete. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 12 months ago by Dean McNamee
Modified:
5 years, 9 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : More. #

Total comments: 5

Patch Set 3 : Way more. #

Total comments: 16

Patch Set 4 : Merge to trunk. #

Patch Set 5 : Working towards a popup. #

Total comments: 2

Patch Set 6 : Merge to trunk #

Patch Set 7 : More more more. #

Patch Set 8 : Okkk #

Patch Set 9 : 2009 #

Patch Set 10 : Cleanup #

Total comments: 27

Patch Set 11 : Feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1114 lines, -89 lines) Patch
A chrome/browser/autocomplete/autocomplete_edit_view_gtk.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +201 lines, -0 lines 0 comments Download
A chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +483 lines, -0 lines 0 comments Download
A chrome/browser/autocomplete/autocomplete_popup_view_gtk.h View 1 2 3 4 5 6 7 8 1 chunk +55 lines, -0 lines 0 comments Download
A chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +115 lines, -0 lines 0 comments Download
M chrome/browser/browser.scons View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/gtk/browser_toolbar_gtk.h View 4 5 3 chunks +3 lines, -21 lines 0 comments Download
M chrome/browser/gtk/browser_toolbar_gtk.cc View 4 5 6 8 chunks +23 lines, -68 lines 0 comments Download
A chrome/browser/gtk/location_bar_view_gtk.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +88 lines, -0 lines 0 comments Download
chrome/browser/gtk/location_bar_view_gtk.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +143 lines, -0 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 16 (0 generated)
Evan Martin
I think this would be fine to check in, if it doesn't break our existing ...
7 years, 12 months ago (2009-03-04 02:54:07 UTC) #1
Dean McNamee
Here we goes. I still need to do a bunch of cleanup, I'm not gtk_widget_destroying() ...
7 years, 12 months ago (2009-03-04 15:14:38 UTC) #2
Evan Martin
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: ...
7 years, 12 months ago (2009-03-04 20:45:29 UTC) #3
Dean McNamee
I realize this is missing a lot of comments, it's super early and there is ...
7 years, 12 months ago (2009-03-05 14:03:21 UTC) #4
Dean McNamee
On 2009/03/05 14:03:21, Dean McNamee wrote: > I realize this is missing a lot of ...
7 years, 12 months ago (2009-03-06 16:05:33 UTC) #5
Dean McNamee
Bunch of new code, comments welcome. Will polish things once I figure out what's going ...
7 years, 12 months ago (2009-03-06 17:54:54 UTC) #6
Evan Martin
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 ...
7 years, 12 months ago (2009-03-06 17:58:23 UTC) #7
Evan Stade
There are different senses of the word View, but I think in the case of ...
7 years, 11 months ago (2009-03-06 23:28:32 UTC) #8
Peter Kasting
On 2009/03/06 23:28:32, estade wrote: > There are different senses of the word View, but ...
7 years, 11 months ago (2009-03-06 23:32:01 UTC) #9
Dean McNamee
Evan: Sorry, I looked through all your feedback, but I just didn't have time to ...
7 years, 11 months ago (2009-03-08 20:23:50 UTC) #10
Dean McNamee
Ok, I think this is good enough for review now. I think this would be ...
7 years, 11 months ago (2009-03-09 17:40:06 UTC) #11
Peter Kasting
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: // ...
7 years, 11 months ago (2009-03-09 18:04:12 UTC) #12
Evan Martin
I have one overall comment that I'll write here instead of writing below -- you ...
7 years, 11 months ago (2009-03-09 18:27:53 UTC) #13
Evan Stade
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 ...
7 years, 11 months ago (2009-03-09 21:07:44 UTC) #14
Evan Stade
oh yeah, forgot to mention: I know that you are going to refactor this, so ...
7 years, 11 months ago (2009-03-10 06:58:43 UTC) #15
Dean McNamee
7 years, 11 months ago (2009-03-10 12:34:39 UTC) #16
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_
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd