Views-implementation of AutocompleteEditView

Views-implementation of AutocompleteEditView.

This is based on GTK implementation. Please see the header file for features that are not implemented yet.

Textfield changes (files under views/controls/textfield) are being reviewed by Ben in separate CL (http://codereview.chromium.org/6314012/), so you don't have to review them (but you're welcome to do so).

* Updated factory method in AutocompleteEditViewGtk to return views-implementation when TextfieldViews is enabled.
* Added new option to FillLayout so that it lays out its child inside border.
* Added missing GD_PK_Delete to keycode conversion.
* Enabled tests in autodomplete_edit_view_browsertests for views-implementation as well. I moved the test body to the class and then called them from each test class to avoid having another test configuration on bots. Let me know if there is better way.

BUG=none
TEST=Enabled autocomplete_edit_view_browsertest for views-implementation.

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

[Peter Kasting, 2011-01-20 19:44:37]

LGTM

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right):

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:123:
set_border(views::Border::CreateEmptyBorder(kAutocompleteVerticalMargin,
Nit: Just put the zeros on the ends of the other two lines.

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:256: // pass the
event through to TextfieldViews.
Nit: pass -> Pass

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:388: // gtk imlp
uses bounds (end) as start and caret (start) as end and
Nit: This comment can go away now, right?

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:395: if (reversed) {
Nit: No {}

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:586: if
(model_->is_keyword_hint() || model_->keyword().empty())
Nit: Could simplify all this to

  if (!model_->is_keyword_hint() && !model_->keyword().empty() &&
      textfield_->GetSelectedText().empty() &&
      !textfield_->GetCursorPosition()) {
    model_->ClearKeyword(GetText());
    return true;
  }

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:622: if (text !=
GetText()) {
Nit: No {}

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.h (right):

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.h:50: // Initialize,
create the underlying views, etc;
Nit: ; -> .

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.h:56: // Called after
key even is handled either by HandleKeyEvent or by Textfield.
Nit: even -> event

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.h:66: // Implements
views::View
Nit: The comments that demarcate the different parent class implementations all
differ in style.  I suggest just giving the parent class name in all cases, e.g.
"// views::View"

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.h:72: 
Nit: Unnecessary blank lines

http://codereview.chromium.org/6245003/diff/58001/ui/base/keycodes/keyboard_c...
File ui/base/keycodes/keyboard_code_conversion_gtk.cc (right):

http://codereview.chromium.org/6245003/diff/58001/ui/base/keycodes/keyboard_c...
ui/base/keycodes/keyboard_code_conversion_gtk.cc:152: case GDK_KP_Delete:
There are lots of other GDK_KP_xxx codes too (I found a random list at
http://www.adacore.com/wp-content/files/auto_update/gtkada-docs/gtkada_rm/gtk...
), it seems like all of them should be added as well?

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:26:
EXPECT_EQ(ASCIIToWide(ascii), UTF16ToWide(utf16))
Nit: Why not just ASCIIToUTF16() instead of converting both to wide?

Also, frankly, I'd probably prefer to just write EXPECT_XX(ASCIIToUTF16("foo"),
...); than use these macros, but whatever.

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/te...
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/te...
views/controls/textfield/textfield.h:41: // selection started.  The range
preserves the direction, and
As we discussed, this change turns out to not be what you want.

http://codereview.chromium.org/6245003/diff/58001/views/fill_layout.cc
File views/fill_layout.cc (right):

http://codereview.chromium.org/6245003/diff/58001/views/fill_layout.cc#newcode30
views/fill_layout.cc:30: if (exclude_insets_) {
Nit: Simpler:

  gfx::Insets insets(exclude_insets_ ? host->GetInsets() : gfx::Insets());
  frame_view->SetBounds(insets.left(), insets.top(),
      host->width() - insets.width(), host->height() - insets.height());

http://codereview.chromium.org/6245003/diff/58001/views/fill_layout.h
File views/fill_layout.h (right):

http://codereview.chromium.org/6245003/diff/58001/views/fill_layout.h#newcode26
views/fill_layout.h:26: explicit FillLayout(bool exclude_insets);
Nit: Having a null constructor makes this arg effectively a default argument,
which is a style no-no.  You should do one of two things:

* If this is a common wish or something all callers should think about, remove
the null constructor and force people to call this one.
* Otherwise, add a setter function instead of a constructor.

http://codereview.chromium.org/6245003/diff/58001/views/fill_layout.h#newcode34
views/fill_layout.h:34: // True to exclude the insets when laying out a child.
Nit: "If true, we exclude our insets when laying out children."

[oshima, 2011-01-20 21:35:48]

Peter, I reverted FillLayout change and add Layout to AutocompleteEditView as
this is only user of that case.

Can you take another look?

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right):

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:123:
set_border(views::Border::CreateEmptyBorder(kAutocompleteVerticalMargin,
On 2011/01/20 19:44:37, Peter Kasting wrote:
> Nit: Just put the zeros on the ends of the other two lines.

Done.

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:256: // pass the
event through to TextfieldViews.
On 2011/01/20 19:44:37, Peter Kasting wrote:
> Nit: pass -> Pass

Done.

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:388: // gtk imlp
uses bounds (end) as start and caret (start) as end and
On 2011/01/20 19:44:37, Peter Kasting wrote:
> Nit: This comment can go away now, right?

Done.

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:395: if (reversed) {
On 2011/01/20 19:44:37, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/6245003/diff/58001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:622: if (text !=
GetText()) {
On 2011/01/20 19:44:37, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/6245003/diff/58001/ui/base/keycodes/keyboard_c...
File ui/base/keycodes/keyboard_code_conversion_gtk.cc (right):

http://codereview.chromium.org/6245003/diff/58001/ui/base/keycodes/keyboard_c...
ui/base/keycodes/keyboard_code_conversion_gtk.cc:152: case GDK_KP_Delete:
On 2011/01/20 19:44:37, Peter Kasting wrote:
> There are lots of other GDK_KP_xxx codes too (I found a random list at
>
http://www.adacore.com/wp-content/files/auto_update/gtkada-docs/gtkada_rm/gtk...
> ), it seems like all of them should be added as well?

OK I'll investigate and address them in separate CL. filed a bug
(crbug.com/70287)

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:26:
EXPECT_EQ(ASCIIToWide(ascii), UTF16ToWide(utf16))
On 2011/01/20 19:44:37, Peter Kasting wrote:
> Nit: Why not just ASCIIToUTF16() instead of converting both to wide?
> 
> Also, frankly, I'd probably prefer to just write
EXPECT_XX(ASCIIToUTF16("foo"),
> ...); than use these macros, but whatever.

Scott suggested to use this in order to be able to print the string as readable
string when check failed because string16 is printed as { 0x80, ... } instead of
"abc" on linux.

I added comment in the original cl.

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/te...
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/te...
views/controls/textfield/textfield.h:41: // selection started.  The range
preserves the direction, and
On 2011/01/20 19:44:37, Peter Kasting wrote:
> As we discussed, this change turns out to not be what you want.

Yes, this is fixed in the original CL.

http://codereview.chromium.org/6245003/diff/58001/views/fill_layout.h
File views/fill_layout.h (right):

http://codereview.chromium.org/6245003/diff/58001/views/fill_layout.h#newcode26
views/fill_layout.h:26: explicit FillLayout(bool exclude_insets);
On 2011/01/20 19:44:37, Peter Kasting wrote:
> Nit: Having a null constructor makes this arg effectively a default argument,
> which is a style no-no.  You should do one of two things:
> 
> * If this is a common wish or something all callers should think about, remove
> the null constructor and force people to call this one.
> * Otherwise, add a setter function instead of a constructor.

I actually did this in AutocompleteEditViewViews, and reverted this change in
FillLayout.

[Peter Kasting, 2011-01-20 21:41:15]

Not going to re-review, I trust you.

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:26:
EXPECT_EQ(ASCIIToWide(ascii), UTF16ToWide(utf16))
On 2011/01/20 21:35:48, oshima wrote:
> On 2011/01/20 19:44:37, Peter Kasting wrote:
> > Nit: Why not just ASCIIToUTF16() instead of converting both to wide?
> > 
> > Also, frankly, I'd probably prefer to just write
> EXPECT_XX(ASCIIToUTF16("foo"),
> > ...); than use these macros, but whatever.
> 
> Scott suggested to use this in order to be able to print the string as
readable
> string when check failed because string16 is printed as { 0x80, ... } instead
of
> "abc" on linux.

Then we should write an appropriate operator<<() for string16.  Otherwise
everyone everywhere suffers from this problem.

Can you either fix this or file a bug on doing so, and note that we should
remove all these kinds of macros when we fix it?

[oshima, 2011-01-21 00:19:25]

On 2011/01/20 21:41:15, Peter Kasting wrote:
> Not going to re-review, I trust you.
> 
>
http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_views_unittest.cc (right):
> 
>
http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views_unittest.cc:26:
> EXPECT_EQ(ASCIIToWide(ascii), UTF16ToWide(utf16))
> On 2011/01/20 21:35:48, oshima wrote:
> > On 2011/01/20 19:44:37, Peter Kasting wrote:
> > > Nit: Why not just ASCIIToUTF16() instead of converting both to wide?
> > > 
> > > Also, frankly, I'd probably prefer to just write
> > EXPECT_XX(ASCIIToUTF16("foo"),
> > > ...); than use these macros, but whatever.
> > 
> > Scott suggested to use this in order to be able to print the string as
> readable
> > string when check failed because string16 is printed as { 0x80, ... }
instead
> of
> > "abc" on linux.
> 
> Then we should write an appropriate operator<<() for string16.  Otherwise
> everyone everywhere suffers from this problem.
> 
> Can you either fix this or file a bug on doing so, and note that we should
> remove all these kinds of macros when we fix it?

I'll file a bug to add operator<<().

[oshima, 2011-01-21 08:12:39]

On 2011/01/21 00:19:25, oshima wrote:
> On 2011/01/20 21:41:15, Peter Kasting wrote:
> > Not going to re-review, I trust you.
> > 
> >
>
http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
> > File views/controls/textfield/native_textfield_views_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/6245003/diff/58001/views/controls/textfield/na...
> > views/controls/textfield/native_textfield_views_unittest.cc:26:
> > EXPECT_EQ(ASCIIToWide(ascii), UTF16ToWide(utf16))
> > On 2011/01/20 21:35:48, oshima wrote:
> > > On 2011/01/20 19:44:37, Peter Kasting wrote:
> > > > Nit: Why not just ASCIIToUTF16() instead of converting both to wide?
> > > > 
> > > > Also, frankly, I'd probably prefer to just write
> > > EXPECT_XX(ASCIIToUTF16("foo"),
> > > > ...); than use these macros, but whatever.
> > > 
> > > Scott suggested to use this in order to be able to print the string as
> > readable
> > > string when check failed because string16 is printed as { 0x80, ... }
> instead
> > of
> > > "abc" on linux.
> > 
> > Then we should write an appropriate operator<<() for string16.  Otherwise
> > everyone everywhere suffers from this problem.
> > 
> > Can you either fix this or file a bug on doing so, and note that we should
> > remove all these kinds of macros when we fix it?
> 
> I'll file a bug to add operator<<().

Looks like what we need is gtest function but not operator. I'll look into more
details tomorrow.