Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(636)

Issue 172041: Implements unimplemented methods of AutocompleteEditViewGtk.... (Closed)

Created:
11 years, 4 months ago by James Su
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com, Ben Goodger (Google)
Visibility:
Public.

Description

Implements unimplemented methods of AutocompleteEditViewGtk and fixes a regression issue. This CL implements two unimplemented methods of AutocompleteEditViewGtk: OnRevertTemporaryText() and IsSelectAll(). The implementation are mostly copied from autocomplete_edit_view_mac.mm. Some grammer errors and a valgrind warning introduced by CL 165457 are also fixed. This CL was just updated to fix a regression caused by CL 165457 (issue 19631). BUG=19631 : Tabbing on Omnibox enters in to search UI mode TEST=Input something in omnibox and make sure some text is selected and popup view is opened, change current selection in popup view by pressing up/down then press escape to see if omnibox is reverted to its original content and selection. TEST=Make sure www.google.com is one of the search engine, open www.google.com then move the focus back to omnibox and press tab to see if the focus is moved into web page.

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 7

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 2

Patch Set 5 : '' #

Total comments: 4

Patch Set 6 : '' #

Patch Set 7 : '' #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -89 lines) Patch
M chrome/browser/autocomplete/autocomplete_edit_view_gtk.h View 1 2 3 4 5 6 4 chunks +22 lines, -0 lines 0 comments Download
M chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc View 1 2 3 4 5 6 12 chunks +118 lines, -89 lines 1 comment Download

Messages

Total messages: 16 (0 generated)
James Su
Hi, Please help review this CL. Regards James Su
11 years, 4 months ago (2009-08-17 08:21:41 UTC) #1
Daniel Erat
http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (left): http://codereview.chromium.org/172041/diff/1004/5#oldcode334 Line 334: EmphasizeURLComponents(); it looks like this doesn't get called ...
11 years, 4 months ago (2009-08-17 20:33:50 UTC) #2
James Su
CL updated. Please help review again. http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (left): http://codereview.chromium.org/172041/diff/1004/5#oldcode334 Line 334: EmphasizeURLComponents(); On ...
11 years, 4 months ago (2009-08-18 02:13:54 UTC) #3
James Su
Hi, I just updated this CL to fix issue 19631. Please help review again. I'm ...
11 years, 4 months ago (2009-08-19 03:36:13 UTC) #4
Dean McNamee
http://codereview.chromium.org/172041/diff/2001/2002 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/2001/2002#newcode609 Line 609: char_inserted_ = 0; This would seem to indicate ...
11 years, 4 months ago (2009-08-19 03:41:33 UTC) #5
James Su
Hi, I just updated the CL. Control key support has been added. Regards James Su ...
11 years, 4 months ago (2009-08-19 05:12:23 UTC) #6
James Su
Hi, Let me explain the reason of this CL again: As I said before, a ...
11 years, 4 months ago (2009-08-19 06:44:15 UTC) #7
Dean McNamee
I didn't check all of the details, but overall I understand what you're doing, and ...
11 years, 4 months ago (2009-08-20 18:09:21 UTC) #8
Daniel Erat
http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/1004/5#newcode343 Line 343: gtk_text_buffer_select_range(text_buffer_, &insert, &bound); On 2009/08/18 02:13:54, James Su ...
11 years, 4 months ago (2009-08-21 00:03:33 UTC) #9
use derat at chromium.org
On Thu, Aug 20, 2009 at 5:03 PM, <derat@chromium.org> wrote: > Depends on when it ...
11 years, 4 months ago (2009-08-21 01:39:59 UTC) #10
James Su
Hi, CL has been updated. Please help review. I think it should be ok to ...
11 years, 4 months ago (2009-08-21 02:56:58 UTC) #11
Daniel Erat
Thanks, I think this'll work. I'll give it a try tomorrow morning. On 2009/08/21 02:56:58, ...
11 years, 4 months ago (2009-08-21 03:34:46 UTC) #12
James Su
Just sync with the latest trunk, no other change. Regards James Su
11 years, 4 months ago (2009-08-21 10:28:18 UTC) #13
use derat at chromium.org
LGTM Thanks, the PRIMARY selection works as I'd expect in this latest version. On Fri, ...
11 years, 4 months ago (2009-08-21 18:04:36 UTC) #14
Dean McNamee
LGTM. I still think our ctrl handling is wrong. Even though Windows has similar logic, ...
11 years, 4 months ago (2009-08-21 18:21:34 UTC) #15
James Su
11 years, 4 months ago (2009-08-22 00:29:34 UTC) #16
Ok, I'll look into the Control issue later.

Thanks
James Su

On 2009/08/21 18:21:34, Dean McNamee wrote:
> LGTM.
> 
> I still think our ctrl handling is wrong.  Even though Windows has similar
> logic, since it is getting repeated key presses for ctrl it's less of an
issue. 
> It is currently incorrect on Linux, we will show and do different things.  I
> think this needs to be addressed,  but you can do it in a follow up patch.
> 
> Thanks
> 
> http://codereview.chromium.org/172041/diff/2021/2022
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> 
> http://codereview.chromium.org/172041/diff/2021/2022#newcode666
> Line 666: model_->OnControlKeyChanged(false);
> I am still not happy with this code, but I will let you fix it in a follow up.

Powered by Google App Engine
This is Rietveld 408576698