New InputMethod api for Views.

New InputMethod api for Views.

This CL adds:
1. Interfaces: InputMethod, InputMethodDelegate, TextInputClient.
2. InputMethodGtk: an InputMethod implementation based on GtkIMContext.
3. MockInputMethod: a mock InputMethod implementation for unit tests.

BUG=75003
TEST=none

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

[James Su, 2011-03-19 01:18:09]

Hi,
  It's the fifth CL of the new Views input method API. It depends on following
CLs:

http://codereview.chromium.org/6711008/
http://codereview.chromium.org/6713027/
http://codereview.chromium.org/6709023/

I'll integrate this new API with Views code in the next CL.

Regards
James Su

[oshima, 2011-03-21 20:30:56]

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.c...
views/ime/input_method_gtk.cc:67: delegate_ = delegate;
can delegate be NULL? DCHECK if not.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.c...
views/ime/input_method_gtk.cc:258: // To prevent any text from being committed
when resetting the |context_|;
Does this mean  
filtered = gtk_im_context_filter_keypress,
gtk_im_context_reset, gtk_im_context_focus_out/in functions may send 
commit signal?

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.c...
views/ime/input_method_gtk.cc:268: gtk_im_context_focus_in(context_);
don't we have to do this for contet_simple_? And if this  has to be done anyway,
do  we need gtk_im_context_reset?

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.h
File views/ime/input_method_gtk.h (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.h...
views/ime/input_method_gtk.h:47: // Asks the client to confirm current
composition text.
What does this really do? Just curious because this is called when focus is
lost.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.h...
views/ime/input_method_gtk.h:83: // Sends pending input method result to the
focused text input client.
Is this comment correct?

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.c...
views/ime/input_method_win.cc:129: std::string InputMethodWin::GetInputLocale()
{
std::string&

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.c...
views/ime/input_method_win.cc:166: direction_ = ime_input_.GetTextDirection();
look like these two (locale_, direction_) is just a copy of imp_input_. Is there
any reason to copy these two variables in InputMethodWin class?

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.c...
views/ime/input_method_win.cc:314: }
Looks like these are can be shared Win and Gtk. Would it make sense to have
a common super class?

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:70: virtual bool HasComposition() = 0;
"CompositionText" may be better than "Composition" because there is another
"composition" (graphics) work for views.

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:72: 
remove extra empty line

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:75: // Retrieves the UTF-16 based character range
containing all text in the View.
This is still UCS2, correct?

[James Su, 2011-03-21 21:21:16]

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.c...
views/ime/input_method_gtk.cc:67: delegate_ = delegate;
On 2011/03/21 20:30:56, oshima wrote:
> can delegate be NULL? DCHECK if not.

Done.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.c...
views/ime/input_method_gtk.cc:258: // To prevent any text from being committed
when resetting the |context_|;
On 2011/03/21 20:30:56, oshima wrote:
> Does this mean  
> filtered = gtk_im_context_filter_keypress,
> gtk_im_context_reset, gtk_im_context_focus_out/in functions may send 
> commit signal?

yes.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.c...
views/ime/input_method_gtk.cc:268: gtk_im_context_focus_in(context_);
On 2011/03/21 20:30:56, oshima wrote:
> don't we have to do this for contet_simple_? And if this  has to be done
anyway,
> do  we need gtk_im_context_reset?

context_simple_ doesn't have this problem, reset is enough.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.h
File views/ime/input_method_gtk.h (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.h...
views/ime/input_method_gtk.h:47: // Asks the client to confirm current
composition text.
On 2011/03/21 20:30:56, oshima wrote:
> What does this really do? Just curious because this is called when focus is
> lost.

It'll convert ongoing composition text into final content.
It's called when a view is about to lose focus.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_gtk.h...
views/ime/input_method_gtk.h:83: // Sends pending input method result to the
focused text input client.
On 2011/03/21 20:30:56, oshima wrote:
> Is this comment correct?

Done.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.c...
views/ime/input_method_win.cc:129: std::string InputMethodWin::GetInputLocale()
{
On 2011/03/21 20:30:56, oshima wrote:
> std::string&

Different implementation may use different approach here, eg. a temporary
variable may be returned.

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.c...
views/ime/input_method_win.cc:166: direction_ = ime_input_.GetTextDirection();
On 2011/03/21 20:30:56, oshima wrote:
> look like these two (locale_, direction_) is just a copy of imp_input_. Is
there
> any reason to copy these two variables in InputMethodWin class?
ime_input_ doesn't cache the result. And how these values are generated inside
ime_input_ is opaque to InputMethodWin. Eg. the locale name is recalculated each
time when calling ime_input_.GetInputLanguageName();

http://codereview.chromium.org/6688049/diff/5012/views/ime/input_method_win.c...
views/ime/input_method_win.cc:314: }
On 2011/03/21 20:30:56, oshima wrote:
> Looks like these are can be shared Win and Gtk. Would it make sense to have
> a common super class?
Make sense. I'll do it asap.

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:70: virtual bool HasComposition() = 0;
On 2011/03/21 20:30:56, oshima wrote:
> "CompositionText" may be better than "Composition" because there is another
> "composition" (graphics) work for views.

I'll do it asap.

If it's necessary to rename ui::Composition to ui::CompositionText as well?

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:72: 
On 2011/03/21 20:30:56, oshima wrote:
> remove extra empty line

Done.

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:75: // Retrieves the UTF-16 based character range
containing all text in the View.
On 2011/03/21 20:30:56, oshima wrote:
> This is still UCS2, correct?

The text encoding should be UTF-16.

[oshima, 2011-03-22 03:35:00]

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:70: virtual bool HasComposition() = 0;
On 2011/03/21 21:21:16, James Su wrote:
> On 2011/03/21 20:30:56, oshima wrote:
> > "CompositionText" may be better than "Composition" because there is another
> > "composition" (graphics) work for views.
> 
> I'll do it asap.
> 
> If it's necessary to rename ui::Composition to ui::CompositionText as well?
> 

I think it's better to use CompositionText to be more clear.

http://codereview.chromium.org/6688049/diff/5012/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/6688049/diff/5012/views/view.cc#newcode846
views/view.cc:846: // TODO(suzhe): Implement it.
nit: NOTIMPLEMENTED()

http://codereview.chromium.org/6688049/diff/10001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/6688049/diff/10001/views/ime/input_method_win....
views/ime/input_method_win.cc:115: rect = win_rect;
looks like this method is very similar to one in gtk.

is it possible to define something like

widget_->GetPointIn(view->GetWidget(), &rect);

to make this code work for both win and gtk?

http://codereview.chromium.org/6688049/diff/10001/views/ime/input_method_win....
views/ime/input_method_win.cc:121: ime_input_.UpdateCaretRect(hwnd(), rect);
and define template method to implement this for each platform. (design
pattern's template method, not C++ template function)

[James Su, 2011-03-22 08:39:35]

I added InputMethodBase and renamed Composition in TextInputClient to
CompositionText.

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/5012/views/ime/text_input_client....
views/ime/text_input_client.h:70: virtual bool HasComposition() = 0;
On 2011/03/22 03:35:00, oshima wrote:
> On 2011/03/21 21:21:16, James Su wrote:
> > On 2011/03/21 20:30:56, oshima wrote:
> > > "CompositionText" may be better than "Composition" because there is
another
> > > "composition" (graphics) work for views.
> > 
> > I'll do it asap.
> > 
> > If it's necessary to rename ui::Composition to ui::CompositionText as well?
> > 
> 
> I think it's better to use CompositionText to be more clear.
> 

Done.

http://codereview.chromium.org/6688049/diff/5012/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/6688049/diff/5012/views/view.cc#newcode846
views/view.cc:846: // TODO(suzhe): Implement it.
On 2011/03/22 03:35:00, oshima wrote:
> nit: NOTIMPLEMENTED()

Done.

http://codereview.chromium.org/6688049/diff/10001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/6688049/diff/10001/views/ime/input_method_win....
views/ime/input_method_win.cc:115: rect = win_rect;
On 2011/03/22 03:35:00, oshima wrote:
> looks like this method is very similar to one in gtk.
> 
> is it possible to define something like
> 
> widget_->GetPointIn(view->GetWidget(), &rect);
> 
> to make this code work for both win and gtk?

Done.

http://codereview.chromium.org/6688049/diff/10001/views/ime/input_method_win....
views/ime/input_method_win.cc:121: ime_input_.UpdateCaretRect(hwnd(), rect);
On 2011/03/22 03:35:00, oshima wrote:
> and define template method to implement this for each platform. (design
> pattern's template method, not C++ template function)

Done.

[Ben Goodger (Google), 2011-03-22 20:45:14]

http://codereview.chromium.org/6688049/diff/12001/views/widget/widget.h
File views/widget/widget.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/widget/widget.h#newco...
views/widget/widget.h:98: static bool ConvertRect(const Widget* from,
source/target

http://codereview.chromium.org/6688049/diff/12001/views/widget/widget.h#newco...
views/widget/widget.h:271: NativeWidget* native_widget() const { return
native_widget_; }
This method is deliberately non-const.

If you need const, add a 

const NativeWidget* native_widget() const { .. }

[oshima, 2011-03-22 22:35:51]

LGTM on my bits. Consider renaming IsClientSupportTextInput, but not mandatory. 
 Make sure you get OK from ben and other IME folks.

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method.h#ne...
views/ime/input_method.h:32: virtual void
set_delegate(internal::InputMethodDelegate* delegate) = 0;
may be we can move this to protected if we know who should access this?

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_base.h
File views/ime/input_method_base.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_base...
views/ime/input_method_base.h:64: // Check is the focused text input client
supports text input, i.e. if its
Check if

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_base...
views/ime/input_method_base.h:66: bool IsClientSupportTextInput() const;
IfClientSupports..,DoesClientSupports.. ? don't have better suggestion, but
IsClientSupport sounds a bit weird.

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_gtk.h
File views/ime/input_method_gtk.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_gtk....
views/ime/input_method_gtk.h:99: 
// Signal ids for corresponding gtk signals.

[James Su, 2011-03-22 23:08:47]

Hi,
  I updated the CL according to your feedbacks. Please kindly review it again.

Best Regards
James Su

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method.h#ne...
views/ime/input_method.h:32: virtual void
set_delegate(internal::InputMethodDelegate* delegate) = 0;
On 2011/03/22 22:35:51, oshima wrote:
> may be we can move this to protected if we know who should access this?

Currently native widget implementations may call it as well, so that we can
replace the input method instance on-the-fly. Though it's mainly for testing
purpose.

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_base.h
File views/ime/input_method_base.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_base...
views/ime/input_method_base.h:64: // Check is the focused text input client
supports text input, i.e. if its
On 2011/03/22 22:35:51, oshima wrote:
> Check if

Done.

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_base...
views/ime/input_method_base.h:66: bool IsClientSupportTextInput() const;
On 2011/03/22 22:35:51, oshima wrote:
> IfClientSupports..,DoesClientSupports.. ? don't have better suggestion, but
> IsClientSupport sounds a bit weird.

I renamed it to IsTextInputTypeNone() (the behavior is inverted).

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_gtk.h
File views/ime/input_method_gtk.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/ime/input_method_gtk....
views/ime/input_method_gtk.h:99: 
On 2011/03/22 22:35:51, oshima wrote:
> // Signal ids for corresponding gtk signals.

Done.

http://codereview.chromium.org/6688049/diff/12001/views/widget/widget.h
File views/widget/widget.h (right):

http://codereview.chromium.org/6688049/diff/12001/views/widget/widget.h#newco...
views/widget/widget.h:98: static bool ConvertRect(const Widget* from,
On 2011/03/22 20:45:14, Ben Goodger wrote:
> source/target

Done.

http://codereview.chromium.org/6688049/diff/12001/views/widget/widget.h#newco...
views/widget/widget.h:271: NativeWidget* native_widget() const { return
native_widget_; }
On 2011/03/22 20:45:14, Ben Goodger wrote:
> This method is deliberately non-const.
> 
> If you need const, add a 
> 
> const NativeWidget* native_widget() const { .. }

This change is not necessary anymore.

[Peng, 2011-03-24 03:49:02]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
Should do this? Maybe some view will handle them by self.

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:415: if (!handling_key_event_ &&
!IsTextInputTypeNone()) {
I suggest always send the fake key event, even if handling_key_event_ is true.
So that the logic will be simpler.

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:440: if (widget->window) {
How about create GtkIMContext here?

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:446: void
InputMethodGtk::OnWidgetUnrealize(GtkWidget* widget) {
And destroy IMContext here?

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.h
File views/ime/input_method_gtk.h (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.h:10: #include <pango/pango-attributes.h>
Is it necessary to include pango header file?

[James Su, 2011-03-24 18:01:35]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
On 2011/03/24 03:49:02, Peng wrote:
> Should do this? Maybe some view will handle them by self.

We need it to keep consistent with Windows. And a View may not have enough
information to get the correct character out of a keyboard event.
See ProcessFilteredKeyPressEvent() above and ProcessInputMethodResult() below.
If the input method returns a result text with only one character, and there is
no composition text change, then the key event will be treated as if it's not
handled by the input method, and the only character in the result text will be
sent to the View by calling InsertChar(), the same way here.

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:415: if (!handling_key_event_ &&
!IsTextInputTypeNone()) {
On 2011/03/24 03:49:02, Peng wrote:
> I suggest always send the fake key event, even if handling_key_event_ is true.
> So that the logic will be simpler.

No. This method could be called for multiple times when handling a key event, it
makes no sense to send multiple VKEY_PROCESSKEY events for one real key event.

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:440: if (widget->window) {
On 2011/03/24 03:49:02, Peng wrote:
> How about create GtkIMContext here?
If the creation of GtkIMContext happens here, then I need to check if the
pointers are NULL in other places. And I don't think there is much difference by
creating the object at very beginning. Because the Widget will usually be
realized immediately after creation.

Actually I was thinking about delaying the creation of InputMethodGtk object
until the first key event, but as the creation may need some time, it may affect
the user experience because when the user starts typing the browser may be stuck
for a short period.

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.h
File views/ime/input_method_gtk.h (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.h:10: #include <pango/pango-attributes.h>
On 2011/03/24 03:49:02, Peng wrote:
> Is it necessary to include pango header file?
It should be removed.

[Peng, 2011-03-24 18:39:03]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
On 2011/03/24 18:01:35, James Su wrote:
> On 2011/03/24 03:49:02, Peng wrote:
> > Should do this? Maybe some view will handle them by self.
> 
> We need it to keep consistent with Windows. And a View may not have enough
> information to get the correct character out of a keyboard event.
> See ProcessFilteredKeyPressEvent() above and ProcessInputMethodResult() below.
> If the input method returns a result text with only one character, and there
is
> no composition text change, then the key event will be treated as if it's not
> handled by the input method, and the only character in the result text will be
> sent to the View by calling InsertChar(), the same way here.

I do not understand. I am thinking in some cases, IME is disabled, and view want
to handle some special key by self. For example a view want to handle 'Ctrl + o'
by self, and do not want to insert a char \0xxx. Is it OK?

[James Su, 2011-03-24 18:56:28]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
On 2011/03/24 18:39:03, Peng wrote:
> On 2011/03/24 18:01:35, James Su wrote:
> > On 2011/03/24 03:49:02, Peng wrote:
> > > Should do this? Maybe some view will handle them by self.
> > 
> > We need it to keep consistent with Windows. And a View may not have enough
> > information to get the correct character out of a keyboard event.
> > See ProcessFilteredKeyPressEvent() above and ProcessInputMethodResult()
below.
> > If the input method returns a result text with only one character, and there
> is
> > no composition text change, then the key event will be treated as if it's
not
> > handled by the input method, and the only character in the result text will
be
> > sent to the View by calling InsertChar(), the same way here.
> 
> I do not understand. I am thinking in some cases, IME is disabled, and view
want
> to handle some special key by self. For example a view want to handle 'Ctrl +
o'
> by self, and do not want to insert a char \0xxx. Is it OK?

Of course, it's free for a View to ignore any character and text inside its own
InsertChar() and InsertText() implementation. Check my CL to see how
NativeTextfieldViews ignores control characters.

[Peng, 2011-03-24 19:25:08]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
On 2011/03/24 18:56:28, James Su wrote:
> On 2011/03/24 18:39:03, Peng wrote:
> > On 2011/03/24 18:01:35, James Su wrote:
> > > On 2011/03/24 03:49:02, Peng wrote:
> > > > Should do this? Maybe some view will handle them by self.
> > > 
> > > We need it to keep consistent with Windows. And a View may not have enough
> > > information to get the correct character out of a keyboard event.
> > > See ProcessFilteredKeyPressEvent() above and ProcessInputMethodResult()
> below.
> > > If the input method returns a result text with only one character, and
there
> > is
> > > no composition text change, then the key event will be treated as if it's
> not
> > > handled by the input method, and the only character in the result text
will
> be
> > > sent to the View by calling InsertChar(), the same way here.
> > 
> > I do not understand. I am thinking in some cases, IME is disabled, and view
> want
> > to handle some special key by self. For example a view want to handle 'Ctrl
+
> o'
> > by self, and do not want to insert a char \0xxx. Is it OK?
> 
> Of course, it's free for a View to ignore any character and text inside its
own
> InsertChar() and InsertText() implementation. Check my CL to see how
> NativeTextfieldViews ignores control characters.

So do you mean the view can still receive this event? But I can not find calling
DispatchPostIMEKeyEvent after this function. Please point it out. Thanks.

And for InsertChar(), it is not very reasonable to let view ignore some chars in
InsertChar() function. Or maybe the name InsertChar() is not good, need rename
it. for example OnChar() like WM_CHAR on windows.

[James Su, 2011-03-24 19:48:49]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
On 2011/03/24 19:25:08, Peng wrote:
> On 2011/03/24 18:56:28, James Su wrote:
> > On 2011/03/24 18:39:03, Peng wrote:
> > > On 2011/03/24 18:01:35, James Su wrote:
> > > > On 2011/03/24 03:49:02, Peng wrote:
> > > > > Should do this? Maybe some view will handle them by self.
> > > > 
> > > > We need it to keep consistent with Windows. And a View may not have
enough
> > > > information to get the correct character out of a keyboard event.
> > > > See ProcessFilteredKeyPressEvent() above and ProcessInputMethodResult()
> > below.
> > > > If the input method returns a result text with only one character, and
> there
> > > is
> > > > no composition text change, then the key event will be treated as if
it's
> > not
> > > > handled by the input method, and the only character in the result text
> will
> > be
> > > > sent to the View by calling InsertChar(), the same way here.
> > > 
> > > I do not understand. I am thinking in some cases, IME is disabled, and
view
> > want
> > > to handle some special key by self. For example a view want to handle
'Ctrl
> +
> > o'
> > > by self, and do not want to insert a char \0xxx. Is it OK?
> > 
> > Of course, it's free for a View to ignore any character and text inside its
> own
> > InsertChar() and InsertText() implementation. Check my CL to see how
> > NativeTextfieldViews ignores control characters.
> 
> So do you mean the view can still receive this event? But I can not find
calling
> DispatchPostIMEKeyEvent after this function. Please point it out. Thanks.
I don't understand your question. Which event do you mean?

> 
> And for InsertChar(), it is not very reasonable to let view ignore some chars
in
> InsertChar() function. Or maybe the name InsertChar() is not good, need rename
> it. for example OnChar() like WM_CHAR on windows.
It's just about method name, I actually have no preference. I can rename it to
OnChar if you insist. The reason to use InsertChar is to keep in sync with
InsertText.
Regardless of the name, it's of course reasonable for the View to decided its
behavior freely. Not only for InsertChar, but also for InsertText.

[Peng, 2011-03-24 20:26:48]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:161: if (key.type() == ui::ET_KEY_PRESSED &&
!filtered)
I mean, if the event is not not filtered by imcontext, it calls
ProcessUnfilteredKeyPressEvent(), and then should it call
DispatchKeyEventPostIME(key)? View may want to handle some key events by self.

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
On 2011/03/24 19:48:49, James Su wrote:
> On 2011/03/24 19:25:08, Peng wrote:
> > On 2011/03/24 18:56:28, James Su wrote:
> > > On 2011/03/24 18:39:03, Peng wrote:
> > > > On 2011/03/24 18:01:35, James Su wrote:
> > > > > On 2011/03/24 03:49:02, Peng wrote:
> > > > > > Should do this? Maybe some view will handle them by self.
> > > > > 
> > > > > We need it to keep consistent with Windows. And a View may not have
> enough
> > > > > information to get the correct character out of a keyboard event.
> > > > > See ProcessFilteredKeyPressEvent() above and
ProcessInputMethodResult()
> > > below.
> > > > > If the input method returns a result text with only one character, and
> > there
> > > > is
> > > > > no composition text change, then the key event will be treated as if
> it's
> > > not
> > > > > handled by the input method, and the only character in the result text
> > will
> > > be
> > > > > sent to the View by calling InsertChar(), the same way here.
> > > > 
> > > > I do not understand. I am thinking in some cases, IME is disabled, and
> view
> > > want
> > > > to handle some special key by self. For example a view want to handle
> 'Ctrl
> > +
> > > o'
> > > > by self, and do not want to insert a char \0xxx. Is it OK?
> > > 
> > > Of course, it's free for a View to ignore any character and text inside
its
> > own
> > > InsertChar() and InsertText() implementation. Check my CL to see how
> > > NativeTextfieldViews ignores control characters.
> > 
> > So do you mean the view can still receive this event? But I can not find
> calling
> > DispatchPostIMEKeyEvent after this function. Please point it out. Thanks.
> I don't understand your question. Which event do you mean?
> 
> > 
> > And for InsertChar(), it is not very reasonable to let view ignore some
chars
> in
> > InsertChar() function. Or maybe the name InsertChar() is not good, need
rename
> > it. for example OnChar() like WM_CHAR on windows.
> It's just about method name, I actually have no preference. I can rename it to
> OnChar if you insist. The reason to use InsertChar is to keep in sync with
> InsertText.
> Regardless of the name, it's of course reasonable for the View to decided its
> behavior freely. Not only for InsertChar, but also for InsertText.

I think InsertChar() & InsertText() are very clear meaning for InputClient.

I did not say only renaming is OK. I believe InsertText() and InsertChar()
should be only for result from IME. For unfiltered key events, that should be
handled by view self. view could call keyevent.GetCharacter() to get char or do
other things as well. right?

[James Su, 2011-03-24 21:01:23]

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk.cc
File views/ime/input_method_gtk.cc (right):

http://codereview.chromium.org/6688049/diff/13004/views/ime/input_method_gtk....
views/ime/input_method_gtk.cc:294: // focused text input client by calling
TextInputClient::InsertChar().
On 2011/03/24 20:26:48, Peng wrote:
> On 2011/03/24 19:48:49, James Su wrote:
> > On 2011/03/24 19:25:08, Peng wrote:
> > > On 2011/03/24 18:56:28, James Su wrote:
> > > > On 2011/03/24 18:39:03, Peng wrote:
> > > > > On 2011/03/24 18:01:35, James Su wrote:
> > > > > > On 2011/03/24 03:49:02, Peng wrote:
> > > > > > > Should do this? Maybe some view will handle them by self.
> > > > > > 
> > > > > > We need it to keep consistent with Windows. And a View may not have
> > enough
> > > > > > information to get the correct character out of a keyboard event.
> > > > > > See ProcessFilteredKeyPressEvent() above and
> ProcessInputMethodResult()
> > > > below.
> > > > > > If the input method returns a result text with only one character,
and
> > > there
> > > > > is
> > > > > > no composition text change, then the key event will be treated as if
> > it's
> > > > not
> > > > > > handled by the input method, and the only character in the result
text
> > > will
> > > > be
> > > > > > sent to the View by calling InsertChar(), the same way here.
> > > > > 
> > > > > I do not understand. I am thinking in some cases, IME is disabled, and
> > view
> > > > want
> > > > > to handle some special key by self. For example a view want to handle
> > 'Ctrl
> > > +
> > > > o'
> > > > > by self, and do not want to insert a char \0xxx. Is it OK?
> > > > 
> > > > Of course, it's free for a View to ignore any character and text inside
> its
> > > own
> > > > InsertChar() and InsertText() implementation. Check my CL to see how
> > > > NativeTextfieldViews ignores control characters.
> > > 
> > > So do you mean the view can still receive this event? But I can not find
> > calling
> > > DispatchPostIMEKeyEvent after this function. Please point it out. Thanks.
> > I don't understand your question. Which event do you mean?
> > 
> > > 
> > > And for InsertChar(), it is not very reasonable to let view ignore some
> chars
> > in
> > > InsertChar() function. Or maybe the name InsertChar() is not good, need
> rename
> > > it. for example OnChar() like WM_CHAR on windows.
> > It's just about method name, I actually have no preference. I can rename it
to
> > OnChar if you insist. The reason to use InsertChar is to keep in sync with
> > InsertText.
> > Regardless of the name, it's of course reasonable for the View to decided
its
> > behavior freely. Not only for InsertChar, but also for InsertText.
> 
> I think InsertChar() & InsertText() are very clear meaning for InputClient.
> 
> I did not say only renaming is OK. I believe InsertText() and InsertChar()
> should be only for result from IME. For unfiltered key events, that should be
> handled by view self. view could call keyevent.GetCharacter() to get char or
do
> other things as well. right?
> 
> 
DispatchKeyEventPostIME() is called 7 lines above in the code, didn't you see
it?

[Peng, 2011-03-29 19:12:19]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
It is better to add some API for browser to trace InputClient focus. So that
browser could use it for controlling virtual keyboard and etc.
Like:

void AddInputMethodFocusListener(Listener *listener);

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:90: // TODO(suzhe): Support mouse/touch event.
It is better to have a static helper function like:

static InputMethod *FocusedInputMetthod();
or
static InputClient *FocusedInputClient();

We may need it for touchui. we could use it to talk with focused view.

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client...
views/ime/text_input_client.h:35: virtual void SetCompositionText(const
ui::CompositionText& composition) = 0;
For Touch UI, sometimes, we want to show composition in candidates panel or
virtual keyboard instead of in input control views. Could you add this support
for Chrome IME API?

[James Su, 2011-03-29 19:39:15]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
On 2011/03/29 19:12:19, Peng wrote:
> It is better to add some API for browser to trace InputClient focus. So that
> browser could use it for controlling virtual keyboard and etc.
> Like:
> 
> void AddInputMethodFocusListener(Listener *listener);

I think it may not be necessary to add such thing here. Because it's not a
common feature for all platforms.

Instead, we can do following things:
1. In code for handling/dispatching touch events, after handling a touch event,
the focused view can be checked explicitly to see if it supports text input.
Then we can popup/hide virtual keyboard accordingly.
2. Encapsulate everything inside the actual InputMethod implementation, which
should know better about how to do it.

And if we really need such thing, I think we can just add it to the Delegate
interface.

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:90: // TODO(suzhe): Support mouse/touch event.
On 2011/03/29 19:12:19, Peng wrote:
> It is better to have a static helper function like:
> 
> static InputMethod *FocusedInputMetthod();
> or
> static InputClient *FocusedInputClient();
> 
> We may need it for touchui. we could use it to talk with focused view.

Good point. I'll add some helper methods.

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client...
views/ime/text_input_client.h:35: virtual void SetCompositionText(const
ui::CompositionText& composition) = 0;
On 2011/03/29 19:12:19, Peng wrote:
> For Touch UI, sometimes, we want to show composition in candidates panel or
> virtual keyboard instead of in input control views. Could you add this support
> for Chrome IME API?

It should be handled inside the actual InputMethod implementation, which should
send the composition text to appropriate component. And it's not necessary for a
text input client to know it.

[Peng, 2011-03-29 19:50:46]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
Your suggestion can work. But it is not idea solution. That need input Browser
UI code understand the detail of InputMethod implementation.

It will be more convenient if the Listener can receive two arguments (InputMehod
*im, InputClient *ic).

On 2011/03/29 19:39:15, James Su wrote:
> On 2011/03/29 19:12:19, Peng wrote:
> > It is better to add some API for browser to trace InputClient focus. So that
> > browser could use it for controlling virtual keyboard and etc.
> > Like:
> > 
> > void AddInputMethodFocusListener(Listener *listener);
> 
> I think it may not be necessary to add such thing here. Because it's not a
> common feature for all platforms.
> 
> Instead, we can do following things:
> 1. In code for handling/dispatching touch events, after handling a touch
event,
> the focused view can be checked explicitly to see if it supports text input.
> Then we can popup/hide virtual keyboard accordingly.
> 2. Encapsulate everything inside the actual InputMethod implementation, which
> should know better about how to do it.
> 
> And if we really need such thing, I think we can just add it to the Delegate
> interface.

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client...
views/ime/text_input_client.h:35: virtual void SetCompositionText(const
ui::CompositionText& composition) = 0;
I think that code should be part of browser UI, instead of views or a special
InputMethod implementations.

On 2011/03/29 19:39:15, James Su wrote:
> On 2011/03/29 19:12:19, Peng wrote:
> > For Touch UI, sometimes, we want to show composition in candidates panel or
> > virtual keyboard instead of in input control views. Could you add this
support
> > for Chrome IME API?
> 
> It should be handled inside the actual InputMethod implementation, which
should
> send the composition text to appropriate component. And it's not necessary for
a
> text input client to know it.

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client...
views/ime/text_input_client.h:117: };
It is better to have a function to get view pointer of the InputClient.

[James Su, 2011-03-29 22:32:38]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
Let's add it later when we figure out the actual way for supporting virtual
keyboard. I still believe such thing should be part of the actual InputMethod
implementation.

On 2011/03/29 19:50:47, Peng wrote:
> Your suggestion can work. But it is not idea solution. That need input Browser
> UI code understand the detail of InputMethod implementation.
> 
> It will be more convenient if the Listener can receive two arguments
(InputMehod
> *im, InputClient *ic).
> 
> On 2011/03/29 19:39:15, James Su wrote:
> > On 2011/03/29 19:12:19, Peng wrote:
> > > It is better to add some API for browser to trace InputClient focus. So
that
> > > browser could use it for controlling virtual keyboard and etc.
> > > Like:
> > > 
> > > void AddInputMethodFocusListener(Listener *listener);
> > 
> > I think it may not be necessary to add such thing here. Because it's not a
> > common feature for all platforms.
> > 
> > Instead, we can do following things:
> > 1. In code for handling/dispatching touch events, after handling a touch
> event,
> > the focused view can be checked explicitly to see if it supports text input.
> > Then we can popup/hide virtual keyboard accordingly.
> > 2. Encapsulate everything inside the actual InputMethod implementation,
which
> > should know better about how to do it.
> > 
> > And if we really need such thing, I think we can just add it to the Delegate
> > interface.
>

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client...
views/ime/text_input_client.h:35: virtual void SetCompositionText(const
ui::CompositionText& composition) = 0;
On 2011/03/29 19:50:47, Peng wrote:
> I think that code should be part of browser UI, instead of views or a special
> InputMethod implementations.
If it's part of TouchUI's browser code, then it makes sense to add specific
API's to InputMethodIBus class for calling from TouchUI's specific browser code.
It's not a generic requirement that should be added to this interface.

> 
> On 2011/03/29 19:39:15, James Su wrote:
> > On 2011/03/29 19:12:19, Peng wrote:
> > > For Touch UI, sometimes, we want to show composition in candidates panel
or
> > > virtual keyboard instead of in input control views. Could you add this
> support
> > > for Chrome IME API?
> > 
> > It should be handled inside the actual InputMethod implementation, which
> should
> > send the composition text to appropriate component. And it's not necessary
for
> a
> > text input client to know it.
>

http://codereview.chromium.org/6688049/diff/28001/views/ime/text_input_client...
views/ime/text_input_client.h:117: };
On 2011/03/29 19:50:47, Peng wrote:
> It is better to have a function to get view pointer of the InputClient.

Done.

[Peng, 2011-03-29 22:47:05]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
Do you mean we should let class InputMethodIBus control virtual keyboard
directly? 

On 2011/03/29 22:32:38, James Su wrote:
> Let's add it later when we figure out the actual way for supporting virtual
> keyboard. I still believe such thing should be part of the actual InputMethod
> implementation.
> 
> On 2011/03/29 19:50:47, Peng wrote:
> > Your suggestion can work. But it is not idea solution. That need input
Browser
> > UI code understand the detail of InputMethod implementation.
> > 
> > It will be more convenient if the Listener can receive two arguments
> (InputMehod
> > *im, InputClient *ic).
> > 
> > On 2011/03/29 19:39:15, James Su wrote:
> > > On 2011/03/29 19:12:19, Peng wrote:
> > > > It is better to add some API for browser to trace InputClient focus. So
> that
> > > > browser could use it for controlling virtual keyboard and etc.
> > > > Like:
> > > > 
> > > > void AddInputMethodFocusListener(Listener *listener);
> > > 
> > > I think it may not be necessary to add such thing here. Because it's not a
> > > common feature for all platforms.
> > > 
> > > Instead, we can do following things:
> > > 1. In code for handling/dispatching touch events, after handling a touch
> > event,
> > > the focused view can be checked explicitly to see if it supports text
input.
> > > Then we can popup/hide virtual keyboard accordingly.
> > > 2. Encapsulate everything inside the actual InputMethod implementation,
> which
> > > should know better about how to do it.
> > > 
> > > And if we really need such thing, I think we can just add it to the
Delegate
> > > interface.
> > 
>

[James Su, 2011-03-29 23:13:13]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
On 2011/03/29 22:47:05, Peng wrote:
> Do you mean we should let class InputMethodIBus control virtual keyboard
> directly? 
It depends on how it's implemented in InputMethodIBus. It may or may not be
direct control. No matter how we implement InputMethodIBus, virtual keyboards
and input methods should be managed in the same way. We can discuss it
separately.

> 
> On 2011/03/29 22:32:38, James Su wrote:
> > Let's add it later when we figure out the actual way for supporting virtual
> > keyboard. I still believe such thing should be part of the actual
InputMethod
> > implementation.
> > 
> > On 2011/03/29 19:50:47, Peng wrote:
> > > Your suggestion can work. But it is not idea solution. That need input
> Browser
> > > UI code understand the detail of InputMethod implementation.
> > > 
> > > It will be more convenient if the Listener can receive two arguments
> > (InputMehod
> > > *im, InputClient *ic).
> > > 
> > > On 2011/03/29 19:39:15, James Su wrote:
> > > > On 2011/03/29 19:12:19, Peng wrote:
> > > > > It is better to add some API for browser to trace InputClient focus.
So
> > that
> > > > > browser could use it for controlling virtual keyboard and etc.
> > > > > Like:
> > > > > 
> > > > > void AddInputMethodFocusListener(Listener *listener);
> > > > 
> > > > I think it may not be necessary to add such thing here. Because it's not
a
> > > > common feature for all platforms.
> > > > 
> > > > Instead, we can do following things:
> > > > 1. In code for handling/dispatching touch events, after handling a touch
> > > event,
> > > > the focused view can be checked explicitly to see if it supports text
> input.
> > > > Then we can popup/hide virtual keyboard accordingly.
> > > > 2. Encapsulate everything inside the actual InputMethod implementation,
> > which
> > > > should know better about how to do it.
> > > > 
> > > > And if we really need such thing, I think we can just add it to the
> Delegate
> > > > interface.
> > > 
> > 
>

[Peng, 2011-03-30 18:41:35]

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:48: virtual void OnFocusIn() = 0;
Maybe rename to OnFocus() and OnBlur(). I changed them to OnFocus() and OnBlur()
in my origin design by someone's review comments.

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:53: virtual void OnFocusOut() = 0;
I think it leaks some functions for tracing focus of input client. I think that
is necessary for some InputMethod implementations to get notification when input
client is changed.

for example:

virtual void OnInputClientFocusIn(TextInputClient *client) and virtual void
OnInputClientFocusOut(TextInputClient *client);
or
virtual void SetInputClient(TextInputClient *client)

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:66: virtual void OnTextInputTypeChanged(View* view) =
0;
You defined TextInputClient in the design. It should be used for communicating
with InputMethod. So maybe you should replace View* to TextInputClient * here?
And for other functions.

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method_base.cc
File views/ime/input_method_base.cc (right):

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method_base...
views/ime/input_method_base.cc:42:
widget->GetFocusManager()->AddFocusChangeListener(this);
Maybe it is better to move those code to Widget?

It is not very reasonable to let InputMethod add focus listener by self. Looks
those code, seems we have to implement all InputMethod base on InputMethodBase.

[James Su, 2011-03-30 19:44:22]

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:48: virtual void OnFocusIn() = 0;
On 2011/03/30 18:41:35, Peng wrote:
> Maybe rename to OnFocus() and OnBlur(). I changed them to OnFocus() and
OnBlur()
> in my origin design by someone's review comments.

Done.

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:53: virtual void OnFocusOut() = 0;
On 2011/03/30 18:41:35, Peng wrote:
> I think it leaks some functions for tracing focus of input client. I think
that
> is necessary for some InputMethod implementations to get notification when
input
> client is changed.
> 
> for example:
> 
> virtual void OnInputClientFocusIn(TextInputClient *client) and virtual void
> OnInputClientFocusOut(TextInputClient *client);
> or
> virtual void SetInputClient(TextInputClient *client)

TextInputClient and View are 1:1 mapping, so they share the same focus state, so
we don't need another set of methods for TextInputClient's focus state.

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:66: virtual void OnTextInputTypeChanged(View* view) =
0;
On 2011/03/30 18:41:35, Peng wrote:
> You defined TextInputClient in the design. It should be used for communicating
> with InputMethod. So maybe you should replace View* to TextInputClient * here?
> And for other functions.

View* is used here intentionally, because we allow a View to return NULL
TextInputClient* to indicate that it doesn't support text input at all. Note
that NULL TextInputClient* and ui::TEXT_INPUT_TYPE_NONE is different, a non-NULL
TextInputClient* will always receive character events through InsertChar()
regardless of its text input type.

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method_base.cc
File views/ime/input_method_base.cc (right):

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method_base...
views/ime/input_method_base.cc:42:
widget->GetFocusManager()->AddFocusChangeListener(this);
On 2011/03/30 18:41:35, Peng wrote:
> Maybe it is better to move those code to Widget?
> 
> It is not very reasonable to let InputMethod add focus listener by self. Looks
> those code, seems we have to implement all InputMethod base on
InputMethodBase.

The intention to keep the change to Widget/NativeWidget as small as possible.
And deriving InputMethodBase is optional. If an input method needs very
different implementation, it can implement InputMethod directly.

[Peng, 2011-03-30 21:32:37]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
I think it is better to have a reasonable idea to support it, before checking
this CL.

For me, it is not reasonable to let InputMethodIBus to control Virtual keyboard
directly. Because InputMethodIBus is in views/. And virtual keyboard is a chrome
extension. It is not a good idea to let views depend on chrome browser code.


On 2011/03/29 23:13:14, James Su wrote:
> On 2011/03/29 22:47:05, Peng wrote:
> > Do you mean we should let class InputMethodIBus control virtual keyboard
> > directly? 
> It depends on how it's implemented in InputMethodIBus. It may or may not be
> direct control. No matter how we implement InputMethodIBus, virtual keyboards
> and input methods should be managed in the same way. We can discuss it
> separately.
> 
> > 
> > On 2011/03/29 22:32:38, James Su wrote:
> > > Let's add it later when we figure out the actual way for supporting
virtual
> > > keyboard. I still believe such thing should be part of the actual
> InputMethod
> > > implementation.
> > > 
> > > On 2011/03/29 19:50:47, Peng wrote:
> > > > Your suggestion can work. But it is not idea solution. That need input
> > Browser
> > > > UI code understand the detail of InputMethod implementation.
> > > > 
> > > > It will be more convenient if the Listener can receive two arguments
> > > (InputMehod
> > > > *im, InputClient *ic).
> > > > 
> > > > On 2011/03/29 19:39:15, James Su wrote:
> > > > > On 2011/03/29 19:12:19, Peng wrote:
> > > > > > It is better to add some API for browser to trace InputClient focus.
> So
> > > that
> > > > > > browser could use it for controlling virtual keyboard and etc.
> > > > > > Like:
> > > > > > 
> > > > > > void AddInputMethodFocusListener(Listener *listener);
> > > > > 
> > > > > I think it may not be necessary to add such thing here. Because it's
not
> a
> > > > > common feature for all platforms.
> > > > > 
> > > > > Instead, we can do following things:
> > > > > 1. In code for handling/dispatching touch events, after handling a
touch
> > > > event,
> > > > > the focused view can be checked explicitly to see if it supports text
> > input.
> > > > > Then we can popup/hide virtual keyboard accordingly.
> > > > > 2. Encapsulate everything inside the actual InputMethod
implementation,
> > > which
> > > > > should know better about how to do it.
> > > > > 
> > > > > And if we really need such thing, I think we can just add it to the
> > Delegate
> > > > > interface.
> > > > 
> > > 
> > 
>

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:53: // Dispatch a key event to the input method. The
key event will be dispatched
IC. The functions is for tracing view too. I did not find any virtual functions
for geting notify when focused view (or TextInputClient) is changed.

On 2011/03/30 19:44:22, James Su wrote:
> On 2011/03/30 18:41:35, Peng wrote:
> > I think it leaks some functions for tracing focus of input client. I think
> that
> > is necessary for some InputMethod implementations to get notification when
> input
> > client is changed.
> > 
> > for example:
> > 
> > virtual void OnInputClientFocusIn(TextInputClient *client) and virtual void
> > OnInputClientFocusOut(TextInputClient *client);
> > or
> > virtual void SetInputClient(TextInputClient *client)
> 
> TextInputClient and View are 1:1 mapping, so they share the same focus state,
so
> we don't need another set of methods for TextInputClient's focus state.

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:66: // Called by the focused |view| whenever its caret
bounds is changed.
It is OK. But this design is weird.

On 2011/03/30 19:44:22, James Su wrote:
> On 2011/03/30 18:41:35, Peng wrote:
> > You defined TextInputClient in the design. It should be used for
communicating
> > with InputMethod. So maybe you should replace View* to TextInputClient *
here?
> > And for other functions.
> 
> View* is used here intentionally, because we allow a View to return NULL
> TextInputClient* to indicate that it doesn't support text input at all. Note
> that NULL TextInputClient* and ui::TEXT_INPUT_TYPE_NONE is different, a
non-NULL
> TextInputClient* will always receive character events through InsertChar()
> regardless of its text input type.

[James Su, 2011-03-30 21:44:00]

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/28001/views/ime/input_method.h#ne...
views/ime/input_method.h:51: virtual void OnFocusOut() = 0;
On 2011/03/30 21:32:37, Peng wrote:
> I think it is better to have a reasonable idea to support it, before checking
> this CL.
> 
> For me, it is not reasonable to let InputMethodIBus to control Virtual
keyboard
> directly. Because InputMethodIBus is in views/. And virtual keyboard is a
chrome
> extension. It is not a good idea to let views depend on chrome browser code.
I didn't say to let InputMethodIBus to control virtual keyboards directly.
Virtual keyboards should be controlled directly by ibus itself through the
extension API for virtual keyboards and input methods. That part is out of this
CL's scope.

But anyway, we can always improve this API incrementally according to our future
requirements. So let's keep it as it for now, and move on to other part.

> 
> 
> On 2011/03/29 23:13:14, James Su wrote:
> > On 2011/03/29 22:47:05, Peng wrote:
> > > Do you mean we should let class InputMethodIBus control virtual keyboard
> > > directly? 
> > It depends on how it's implemented in InputMethodIBus. It may or may not be
> > direct control. No matter how we implement InputMethodIBus, virtual
keyboards
> > and input methods should be managed in the same way. We can discuss it
> > separately.
> > 
> > > 
> > > On 2011/03/29 22:32:38, James Su wrote:
> > > > Let's add it later when we figure out the actual way for supporting
> virtual
> > > > keyboard. I still believe such thing should be part of the actual
> > InputMethod
> > > > implementation.
> > > > 
> > > > On 2011/03/29 19:50:47, Peng wrote:
> > > > > Your suggestion can work. But it is not idea solution. That need input
> > > Browser
> > > > > UI code understand the detail of InputMethod implementation.
> > > > > 
> > > > > It will be more convenient if the Listener can receive two arguments
> > > > (InputMehod
> > > > > *im, InputClient *ic).
> > > > > 
> > > > > On 2011/03/29 19:39:15, James Su wrote:
> > > > > > On 2011/03/29 19:12:19, Peng wrote:
> > > > > > > It is better to add some API for browser to trace InputClient
focus.
> > So
> > > > that
> > > > > > > browser could use it for controlling virtual keyboard and etc.
> > > > > > > Like:
> > > > > > > 
> > > > > > > void AddInputMethodFocusListener(Listener *listener);
> > > > > > 
> > > > > > I think it may not be necessary to add such thing here. Because it's
> not
> > a
> > > > > > common feature for all platforms.
> > > > > > 
> > > > > > Instead, we can do following things:
> > > > > > 1. In code for handling/dispatching touch events, after handling a
> touch
> > > > > event,
> > > > > > the focused view can be checked explicitly to see if it supports
text
> > > input.
> > > > > > Then we can popup/hide virtual keyboard accordingly.
> > > > > > 2. Encapsulate everything inside the actual InputMethod
> implementation,
> > > > which
> > > > > > should know better about how to do it.
> > > > > > 
> > > > > > And if we really need such thing, I think we can just add it to the
> > > Delegate
> > > > > > interface.
> > > > > 
> > > > 
> > > 
> > 
>

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:53: // Dispatch a key event to the input method. The
key event will be dispatched
On 2011/03/30 21:32:37, Peng wrote:
> IC. The functions is for tracing view too. I did not find any virtual
functions
> for geting notify when focused view (or TextInputClient) is changed.
That's the purpose of FocusChangeListener provided by FocusManager.

> 
> On 2011/03/30 19:44:22, James Su wrote:
> > On 2011/03/30 18:41:35, Peng wrote:
> > > I think it leaks some functions for tracing focus of input client. I think
> > that
> > > is necessary for some InputMethod implementations to get notification when
> > input
> > > client is changed.
> > > 
> > > for example:
> > > 
> > > virtual void OnInputClientFocusIn(TextInputClient *client) and virtual
void
> > > OnInputClientFocusOut(TextInputClient *client);
> > > or
> > > virtual void SetInputClient(TextInputClient *client)
> > 
> > TextInputClient and View are 1:1 mapping, so they share the same focus
state,
> so
> > we don't need another set of methods for TextInputClient's focus state.
>

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:66: // Called by the focused |view| whenever its caret
bounds is changed.
On 2011/03/30 21:32:37, Peng wrote:
> It is OK. But this design is weird.
It's pretty normal. Think in this way: the InputMethod object provide text input
service to View objects through (optional)TextInputClient interface. So the
major bodies in this relationship are InputMethod and View.

> 
> On 2011/03/30 19:44:22, James Su wrote:
> > On 2011/03/30 18:41:35, Peng wrote:
> > > You defined TextInputClient in the design. It should be used for
> communicating
> > > with InputMethod. So maybe you should replace View* to TextInputClient *
> here?
> > > And for other functions.
> > 
> > View* is used here intentionally, because we allow a View to return NULL
> > TextInputClient* to indicate that it doesn't support text input at all. Note
> > that NULL TextInputClient* and ui::TEXT_INPUT_TYPE_NONE is different, a
> non-NULL
> > TextInputClient* will always receive character events through InsertChar()
> > regardless of its text input type.
>

[Peng, 2011-03-30 23:56:33]

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:53: virtual void OnFocusOut() = 0;
I think that is not convenient. I don't think it is good idea to let InputMethod
implementation to do it by self. Especially InputMethodBase has similar code,
but it can not be used by Child classes.

I think widget should know changes of focus and it also own an instance of
InputMethod . And let widget call InputMethod::OnInputClientFocus{In,Out}, it
could be simpler.

On 2011/03/30 21:44:00, James Su wrote:
> On 2011/03/30 21:32:37, Peng wrote:
> > IC. The functions is for tracing view too. I did not find any virtual
> functions
> > for geting notify when focused view (or TextInputClient) is changed.
> That's the purpose of FocusChangeListener provided by FocusManager.
> 
> > 
> > On 2011/03/30 19:44:22, James Su wrote:
> > > On 2011/03/30 18:41:35, Peng wrote:
> > > > I think it leaks some functions for tracing focus of input client. I
think
> > > that
> > > > is necessary for some InputMethod implementations to get notification
when
> > > input
> > > > client is changed.
> > > > 
> > > > for example:
> > > > 
> > > > virtual void OnInputClientFocusIn(TextInputClient *client) and virtual
> void
> > > > OnInputClientFocusOut(TextInputClient *client);
> > > > or
> > > > virtual void SetInputClient(TextInputClient *client)
> > > 
> > > TextInputClient and View are 1:1 mapping, so they share the same focus
> state,
> > so
> > > we don't need another set of methods for TextInputClient's focus state.
> > 
>

[James Su, 2011-03-31 00:08:08]

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h
File views/ime/input_method.h (right):

http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
views/ime/input_method.h:53: virtual void OnFocusOut() = 0;
On 2011/03/30 23:56:33, Peng wrote:
> I think that is not convenient. I don't think it is good idea to let
InputMethod
> implementation to do it by self. Especially InputMethodBase has similar code,
> but it can not be used by Child classes.
> 
> I think widget should know changes of focus and it also own an instance of
> InputMethod . And let widget call InputMethod::OnInputClientFocus{In,Out}, it
> could be simpler.
No. Otherwise why we need FocusChangeListener at all? Widget won't know the
focus change event without implementing FocusChangeListener and hooking into
FocusManager.
This is the only way for monitoring focus change.

> 
> On 2011/03/30 21:44:00, James Su wrote:
> > On 2011/03/30 21:32:37, Peng wrote:
> > > IC. The functions is for tracing view too. I did not find any virtual
> > functions
> > > for geting notify when focused view (or TextInputClient) is changed.
> > That's the purpose of FocusChangeListener provided by FocusManager.
> > 
> > > 
> > > On 2011/03/30 19:44:22, James Su wrote:
> > > > On 2011/03/30 18:41:35, Peng wrote:
> > > > > I think it leaks some functions for tracing focus of input client. I
> think
> > > > that
> > > > > is necessary for some InputMethod implementations to get notification
> when
> > > > input
> > > > > client is changed.
> > > > > 
> > > > > for example:
> > > > > 
> > > > > virtual void OnInputClientFocusIn(TextInputClient *client) and virtual
> > void
> > > > > OnInputClientFocusOut(TextInputClient *client);
> > > > > or
> > > > > virtual void SetInputClient(TextInputClient *client)
> > > > 
> > > > TextInputClient and View are 1:1 mapping, so they share the same focus
> > state,
> > > so
> > > > we don't need another set of methods for TextInputClient's focus state.
> > > 
> > 
>

[Peng, 2011-03-31 19:14:06]

Most LGTM. But I am not very qualified for reviewing some code for Window.

On 2011/03/31 00:08:08, James Su wrote:
> http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h
> File views/ime/input_method.h (right):
> 
>
http://codereview.chromium.org/6688049/diff/19009/views/ime/input_method.h#ne...
> views/ime/input_method.h:53: virtual void OnFocusOut() = 0;
> On 2011/03/30 23:56:33, Peng wrote:
> > I think that is not convenient. I don't think it is good idea to let
> InputMethod
> > implementation to do it by self. Especially InputMethodBase has similar
code,
> > but it can not be used by Child classes.
> > 
> > I think widget should know changes of focus and it also own an instance of
> > InputMethod . And let widget call InputMethod::OnInputClientFocus{In,Out},
it
> > could be simpler.
> No. Otherwise why we need FocusChangeListener at all? Widget won't know the
> focus change event without implementing FocusChangeListener and hooking into
> FocusManager.
> This is the only way for monitoring focus change.
> 
> > 
> > On 2011/03/30 21:44:00, James Su wrote:
> > > On 2011/03/30 21:32:37, Peng wrote:
> > > > IC. The functions is for tracing view too. I did not find any virtual
> > > functions
> > > > for geting notify when focused view (or TextInputClient) is changed.
> > > That's the purpose of FocusChangeListener provided by FocusManager.
> > > 
> > > > 
> > > > On 2011/03/30 19:44:22, James Su wrote:
> > > > > On 2011/03/30 18:41:35, Peng wrote:
> > > > > > I think it leaks some functions for tracing focus of input client. I
> > think
> > > > > that
> > > > > > is necessary for some InputMethod implementations to get
notification
> > when
> > > > > input
> > > > > > client is changed.
> > > > > > 
> > > > > > for example:
> > > > > > 
> > > > > > virtual void OnInputClientFocusIn(TextInputClient *client) and
virtual
> > > void
> > > > > > OnInputClientFocusOut(TextInputClient *client);
> > > > > > or
> > > > > > virtual void SetInputClient(TextInputClient *client)
> > > > > 
> > > > > TextInputClient and View are 1:1 mapping, so they share the same focus
> > > state,
> > > > so
> > > > > we don't need another set of methods for TextInputClient's focus
state.
> > > > 
> > > 
> >

[Ben Goodger (Google), 2011-03-31 21:07:24]

LGTM for me on the View integration bits.