Refactor Textfield and AutocompleteEditViewViews.

Refactor Textfield and AutocompleteEditViewViews.

Changes made by this CL:
1. Adds OnBeforeUserAction() and OnAfterUserAction() in TextfieldController.
2. Adds Textfield::HasSelection().
3. Changes NativeTextfieldViews to use KeyEvent::GetCharacter().
3. Refactors AutocompleteEditViewViews to use new TextfieldController methods.

BUG=75003
TEST=views_unittests --gtest_filter=*Textfield* and interactive_ui_tests --gtest_filter=AutocompleteEditViewViews*

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

[James Su, 2011-03-18 05:10:29]

Hi,
  It's the third CL of the new Views input method API. This CL depends on
http://codereview.chromium.org/6713027/.
  With this CL, XKB keyboard layouts can be used in textfield to input non-Latin
characters.

Regards
James Su

[oshima, 2011-03-21 21:11:43]

ouse

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

http://codereview.chromium.org/6685082/diff/3001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:478:
(ime_composing_before_change_ != textfield_->IsIMEComposing());
Does this mean we expect text is changed when IME is turned on/off?

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:767: bool
NativeTextfieldViews::IsValidChar(char16 ch, int flags) {
Isn't there better, more intuitive name? IsValidChar is a bit hard to understand
what's "valid".

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:772: // flag that we don't
care about.
looks to me that this returns true for ctrl, and i guess its because it's
already handled in caller. If that's the case, the comment is a bit misleading.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield.h:92: bool HasSelection() const;
Comment

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield_controller.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
Isn't it feasible to make this pure virtual?

[oshima, 2011-03-21 21:12:33]

On 2011/03/21 21:11:43, oshima wrote:
> ouse
Please ignore this garbage.

[James Su, 2011-03-21 22:05:23]

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

http://codereview.chromium.org/6685082/diff/3001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:478:
(ime_composing_before_change_ != textfield_->IsIMEComposing());
On 2011/03/21 21:11:43, oshima wrote:
> Does this mean we expect text is changed when IME is turned on/off?

We treat the text being composed and final text differently. Because
autocomplete will be prevented when IsIMEComposing() is true. We need to ask the
model to update autocomplete suggest when it turns to false.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:767: bool
NativeTextfieldViews::IsValidChar(char16 ch, int flags) {
On 2011/03/21 21:11:43, oshima wrote:
> Isn't there better, more intuitive name? IsValidChar is a bit hard to
understand
> what's "valid".

How about ShouldInsertChar()?

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:772: // flag that we don't
care about.
On 2011/03/21 21:11:43, oshima wrote:
> looks to me that this returns true for ctrl, and i guess its because it's
> already handled in caller. If that's the case, the comment is a bit
misleading.

keys with ctrl key pressed should generate control characters, which will be
filtered by ((ch >= 0x20 && ch < 0x7F) || ch > 0x9F). If it's not the case, then
we need to insert the character.
Eg. an input method may generate normal characters with such key events.

Filtering out all chars with Alt key is because they are so called system key
events on Windows, which are usually bound to special actions, eg. menu
accelerators, etc.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield.h:92: bool HasSelection() const;
On 2011/03/21 21:11:43, oshima wrote:
> Comment

Done.

[oshima, 2011-03-22 03:02:28]

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

http://codereview.chromium.org/6685082/diff/3001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:478:
(ime_composing_before_change_ != textfield_->IsIMEComposing());
On 2011/03/21 22:05:23, James Su wrote:
> On 2011/03/21 21:11:43, oshima wrote:
> > Does this mean we expect text is changed when IME is turned on/off?
> 
> We treat the text being composed and final text differently. Because
> autocomplete will be prevented when IsIMEComposing() is true. We need to ask
the
> model to update autocomplete suggest when it turns to false.

this becomes true for both "false -> true" and "true -> false". Is this
intentional?

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:767: bool
NativeTextfieldViews::IsValidChar(char16 ch, int flags) {
On 2011/03/21 22:05:23, James Su wrote:
> On 2011/03/21 21:11:43, oshima wrote:
> > Isn't there better, more intuitive name? IsValidChar is a bit hard to
> understand
> > what's "valid".
> 
> How about ShouldInsertChar()?

SGTM

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:772: // flag that we don't
care about.
On 2011/03/21 22:05:23, James Su wrote:
> On 2011/03/21 21:11:43, oshima wrote:
> > looks to me that this returns true for ctrl, and i guess its because it's
> > already handled in caller. If that's the case, the comment is a bit
> misleading.

Ok, this is because ch is GetCharacter bu not GetUnmodified right? If control is
taken care of, do you want to remove !control on caller side?


> 
> keys with ctrl key pressed should generate control characters, which will be
> filtered by ((ch >= 0x20 && ch < 0x7F) || ch > 0x9F). If it's not the case,
then
> we need to insert the character.
> Eg. an input method may generate normal characters with such key events.
> 
> Filtering out all chars with Alt key is because they are so called system key
> events on Windows, which are usually bound to special actions, eg. menu
> accelerators, etc.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield_controller.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
On 2011/03/21 21:11:43, oshima wrote:
> Isn't it feasible to make this pure virtual?

how about this?

And related question. I think supporting composition text in findbar is useful.
What do you think?

[James Su, 2011-03-22 04:27:23]

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

http://codereview.chromium.org/6685082/diff/3001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:478:
(ime_composing_before_change_ != textfield_->IsIMEComposing());
On 2011/03/22 03:02:28, oshima wrote:
> On 2011/03/21 22:05:23, James Su wrote:
> > On 2011/03/21 21:11:43, oshima wrote:
> > > Does this mean we expect text is changed when IME is turned on/off?
> > 
> > We treat the text being composed and final text differently. Because
> > autocomplete will be prevented when IsIMEComposing() is true. We need to ask
> the
> > model to update autocomplete suggest when it turns to false.
> 
> this becomes true for both "false -> true" and "true -> false". Is this
> intentional?

Yes. Though false -> true is not useful for now, in the future we may support
reconversion, that converts a final text back into composition text.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:772: // flag that we don't
care about.
On 2011/03/22 03:02:28, oshima wrote:
> On 2011/03/21 22:05:23, James Su wrote:
> > On 2011/03/21 21:11:43, oshima wrote:
> > > looks to me that this returns true for ctrl, and i guess its because it's
> > > already handled in caller. If that's the case, the comment is a bit
> > misleading.
> 
> Ok, this is because ch is GetCharacter bu not GetUnmodified right? If control
is
> taken care of, do you want to remove !control on caller side?
The caller side does not check control flag at all. It's all up to the text
input client. For example, RenderWidgetHostViewViews should send all characters
to the webpage, including control characters and characters with alt down.

> 
> 
> > 
> > keys with ctrl key pressed should generate control characters, which will be
> > filtered by ((ch >= 0x20 && ch < 0x7F) || ch > 0x9F). If it's not the case,
> then
> > we need to insert the character.
> > Eg. an input method may generate normal characters with such key events.
> > 
> > Filtering out all chars with Alt key is because they are so called system
key
> > events on Windows, which are usually bound to special actions, eg. menu
> > accelerators, etc.
>

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield_controller.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
On 2011/03/21 21:11:43, oshima wrote:
> Isn't it feasible to make this pure virtual?

Sorry I missed it. I'd like to make these two methods pure virtual, but I need
to touch lots of files to do it. So I'd rather keep them non-pure virtual for
now. And maybe keeping them non-pure virtual is better, because most of those
TextfieldController implementations don't need them.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
On 2011/03/22 03:02:28, oshima wrote:
> On 2011/03/21 21:11:43, oshima wrote:
> > Isn't it feasible to make this pure virtual?
> 
> how about this?
> 
> And related question. I think supporting composition text in findbar is
useful.
> What do you think?

I personally like it, and it's already supported in my CLs. I remembered that
it's disabled in NativeTextfieldWin because of a bug report, but I didn't
remember the details.

[oshima, 2011-03-22 23:22:23]

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:772: // flag that we don't
care about.
On 2011/03/22 04:27:24, James Su wrote:
> On 2011/03/22 03:02:28, oshima wrote:
> > On 2011/03/21 22:05:23, James Su wrote:
> > > On 2011/03/21 21:11:43, oshima wrote:
> > > > looks to me that this returns true for ctrl, and i guess its because
it's
> > > > already handled in caller. If that's the case, the comment is a bit
> > > misleading.
> > 
> > Ok, this is because ch is GetCharacter bu not GetUnmodified right? If
control
> is
> > taken care of, do you want to remove !control on caller side?
> The caller side does not check control flag at all. It's all up to the text
> input client. For example, RenderWidgetHostViewViews should send all
characters
> to the webpage, including control characters and characters with alt down.
> 
> > 
> > 
> > > 
> > > keys with ctrl key pressed should generate control characters, which will
be
> > > filtered by ((ch >= 0x20 && ch < 0x7F) || ch > 0x9F). If it's not the
case,
> > then
> > > we need to insert the character.
> > > Eg. an input method may generate normal characters with such key events.
> > > 
> > > Filtering out all chars with Alt key is because they are so called system
> key
> > > events on Windows, which are usually bound to special actions, eg. menu
> > > accelerators, etc.
> > 
> 
I was talking about !control in

if (!control && editable && ShouldInsertChar(ch, key_event.flags())) 

I'm wondering if we can clean up because the code / comment with no real effect
may confuse people later.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield_controller.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
On 2011/03/22 04:27:24, James Su wrote:
> On 2011/03/21 21:11:43, oshima wrote:
> > Isn't it feasible to make this pure virtual?
> 
> Sorry I missed it. I'd like to make these two methods pure virtual, but I need
> to touch lots of files to do it. So I'd rather keep them non-pure virtual for
> now. And maybe keeping them non-pure virtual is better, because most of those
> TextfieldController implementations don't need them.

In fact, many use of this class has empty impl. It was useful to avoid issue of
renaming method, but since we now has OVERRIDE, may be we can just use {}

It's out of this CL's scope of course.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
On 2011/03/22 04:27:24, James Su wrote:
> On 2011/03/22 03:02:28, oshima wrote:
> > On 2011/03/21 21:11:43, oshima wrote:
> > > Isn't it feasible to make this pure virtual?
> > 
> > how about this?
> > 
> > And related question. I think supporting composition text in findbar is
> useful.
> > What do you think?
> 
> I personally like it, and it's already supported in my CLs. I remembered that
> it's disabled in NativeTextfieldWin because of a bug report, but I didn't
> remember the details.
What I meant is to enable search with composition text. I think FindBar needs to
implement OnAfterUserAction, doesn't it? (This is out of scope too.)

http://codereview.chromium.org/6685082/diff/1009/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6685082/diff/1009/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.h:199: // Checks if a char is ok
to be inserted into the textfield.
Can you add comment that ch is modified char?

[James Su, 2011-03-22 23:49:57]

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:772: // flag that we don't
care about.
On 2011/03/22 23:22:23, oshima wrote:
> On 2011/03/22 04:27:24, James Su wrote:
> > On 2011/03/22 03:02:28, oshima wrote:
> > > On 2011/03/21 22:05:23, James Su wrote:
> > > > On 2011/03/21 21:11:43, oshima wrote:
> > > > > looks to me that this returns true for ctrl, and i guess its because
> it's
> > > > > already handled in caller. If that's the case, the comment is a bit
> > > > misleading.
> > > 
> > > Ok, this is because ch is GetCharacter bu not GetUnmodified right? If
> control
> > is
> > > taken care of, do you want to remove !control on caller side?
> > The caller side does not check control flag at all. It's all up to the text
> > input client. For example, RenderWidgetHostViewViews should send all
> characters
> > to the webpage, including control characters and characters with alt down.
> > 
> > > 
> > > 
> > > > 
> > > > keys with ctrl key pressed should generate control characters, which
will
> be
> > > > filtered by ((ch >= 0x20 && ch < 0x7F) || ch > 0x9F). If it's not the
> case,
> > > then
> > > > we need to insert the character.
> > > > Eg. an input method may generate normal characters with such key events.
> > > > 
> > > > Filtering out all chars with Alt key is because they are so called
system
> > key
> > > > events on Windows, which are usually bound to special actions, eg. menu
> > > > accelerators, etc.
> > > 
> > 
> I was talking about !control in
> 
> if (!control && editable && ShouldInsertChar(ch, key_event.flags())) 
> 
> I'm wondering if we can clean up because the code / comment with no real
effect
> may confuse people later.

Ahh, good catch. We can just remove that !control.

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield_controller.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
On 2011/03/22 23:22:23, oshima wrote:
> On 2011/03/22 04:27:24, James Su wrote:
> > On 2011/03/22 03:02:28, oshima wrote:
> > > On 2011/03/21 21:11:43, oshima wrote:
> > > > Isn't it feasible to make this pure virtual?
> > > 
> > > how about this?
> > > 
> > > And related question. I think supporting composition text in findbar is
> > useful.
> > > What do you think?
> > 
> > I personally like it, and it's already supported in my CLs. I remembered
that
> > it's disabled in NativeTextfieldWin because of a bug report, but I didn't
> > remember the details.
> What I meant is to enable search with composition text. I think FindBar needs
to
> implement OnAfterUserAction, doesn't it? (This is out of scope too.)
> 

FindBar implements ContentsChanged(), it should be enough. OnAfterUserAction is
for cases that something other than content was changed, eg. selection range,
composition state, etc.

http://codereview.chromium.org/6685082/diff/1009/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6685082/diff/1009/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.h:199: // Checks if a char is ok
to be inserted into the textfield.
On 2011/03/22 23:22:23, oshima wrote:
> Can you add comment that ch is modified char?

Done.

[oshima, 2011-03-23 18:53:04]

LGTM

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
File views/controls/textfield/textfield_controller.h (right):

http://codereview.chromium.org/6685082/diff/3001/views/controls/textfield/tex...
views/controls/textfield/textfield_controller.h:36: virtual void
OnAfterUserAction(Textfield* sender) {}
On 2011/03/22 23:49:57, James Su wrote:
> On 2011/03/22 23:22:23, oshima wrote:
> > On 2011/03/22 04:27:24, James Su wrote:
> > > On 2011/03/22 03:02:28, oshima wrote:
> > > > On 2011/03/21 21:11:43, oshima wrote:
> > > > > Isn't it feasible to make this pure virtual?
> > > > 
> > > > how about this?
> > > > 
> > > > And related question. I think supporting composition text in findbar is
> > > useful.
> > > > What do you think?
> > > 
> > > I personally like it, and it's already supported in my CLs. I remembered
> that
> > > it's disabled in NativeTextfieldWin because of a bug report, but I didn't
> > > remember the details.
> > What I meant is to enable search with composition text. I think FindBar
needs
> to
> > implement OnAfterUserAction, doesn't it? (This is out of scope too.)
> > 
> 
> FindBar implements ContentsChanged(), it should be enough. OnAfterUserAction
is
> for cases that something other than content was changed, eg. selection range,
> composition state, etc.
This wasn't working for views/gtk. bit I now see your another change calls
ContentsChagned when composition text is changed. Sweet!