Issue 6823055: Consolidate OnKeyEvent and InputMethod code.

msw

Hey Ben, I was hoping to achieve something closer to Patch Set 2, but got ...

9 years, 8 months ago (2011-04-12 20:57:00 UTC) #1

Ben Goodger (Google)

http://codereview.chromium.org/6823055/diff/2013/views/widget/widget.cc File views/widget/widget.cc (right): http://codereview.chromium.org/6823055/diff/2013/views/widget/widget.cc#newcode333 views/widget/widget.cc:333: DispatchKeyEventPostIME(event); I looked at InputMethod's DispatchKeyEvent/calling of DispatchKeyEventPostIME. I ...

9 years, 8 months ago (2011-04-12 21:11:15 UTC) #2

Ben Goodger (Google)

On 2011/04/12 21:11:15, Ben Goodger wrote: > bool handled = true; > if (GetInputMethod()) > ...

9 years, 8 months ago (2011-04-12 21:15:18 UTC) #3

msw

This is a good first step to reduce some KeyEvent handling complexity while I learn ...

9 years, 8 months ago (2011-04-14 21:47:39 UTC) #4

This is a good first step to reduce some KeyEvent handling complexity while I
learn more about IMEs and our need for an asynchronous pattern (and
InputMethodDelegate / DispatchKeyEventPostIME) in InputMethodGtk and
InputMethodIBus. Please take a look.

http://codereview.chromium.org/6823055/diff/2013/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/6823055/diff/2013/views/widget/widget.cc#newco...
views/widget/widget.cc:333: DispatchKeyEventPostIME(event);
On 2011/04/12 21:11:15, Ben Goodger wrote:
> I looked at InputMethod's DispatchKeyEvent/calling of DispatchKeyEventPostIME.
I
> don't think there's any asynchronousness here. This function should be
> re-written as:
> 
> bool handled = true;
> if (GetInputMethod())
>   handled = GetInputMethod()->OnKeyEvent(event);
> return handled ? GetRootView()->OnKeyEvent(event) : false;
> 
> ... you are renaming InputMethod::DispatchKeyEvent to OnKeyEvent, and making
it
> return a bool if further processing is required.
> 
> Then you can also delete the InputMethodDelegate implementation in this class.

Done. My updated changes roughly follow this approach.

http://codereview.chromium.org/6823055/diff/2013/views/widget/widget_gtk.cc
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/6823055/diff/2013/views/widget/widget_gtk.cc#n...
views/widget/widget_gtk.cc:1356: if (key.key_code() == ui::VKEY_PROCESSKEY ||
handled)
On 2011/04/12 21:11:15, Ben Goodger wrote:
> See my comment in widget.cc.
> 
> Once you make those changes... code below this line should be inserted into
> WidgetGtk::OnKeyEvent() after the call to delegate_->OnKeyEvent().
> 
> This override can then die.

Not so, if I understand suzhe and InputHandlerGtk correctly; this override is
needed for Gtk (and IBus?) IME asynchronous handling.

http://codereview.chromium.org/6823055/diff/2013/views/widget/widget_win.cc
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/6823055/diff/2013/views/widget/widget_win.cc#n...
views/widget/widget_win.cc:1110: SetMsgHandled(handled);
On 2011/04/12 21:11:15, Ben Goodger wrote:
> I prefer these SetMsgHandled things to only be called in windows message
> handlers. What if this is called from somewhere else?
> 
> It seems like delegate_->OnKeyEvent() should return a bool for whether or not
> it's handled, and WidgetWin::OnKeyEvent should call this function.

Done.

James Su

Sorry that I plan to have a vacation today for some personal affair. I'll try ...

9 years, 8 months ago (2011-04-15 02:04:17 UTC) #5

Ben Goodger (Google)

I am really liking where this change is going Mike. A few comments: http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_delegate.h File ...

9 years, 8 months ago (2011-04-15 18:23:19 UTC) #6

msw

On 2011/04/15 18:23:19, Ben Goodger wrote: > I am really liking where this change is ...

9 years, 8 months ago (2011-04-15 19:19:32 UTC) #7

On 2011/04/15 18:23:19, Ben Goodger wrote:
> I am really liking where this change is going Mike. A few comments:
> 
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
> File views/ime/input_method_delegate.h (right):
> 
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
> views/ime/input_method_delegate.h:22: virtual bool
DispatchKeyEventPostIME(const
> KeyEvent& key) = 0;
> Do you even still need this function?
> 
> I was thinking InputMethod::OnKeyEvent would return a bool indicating whether
or
> not the calling Widget should process further... which is effectively what
this
> function is doing. If you remove this then you can delete the whole delegate
> interface.
> 
> http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
> File views/widget/widget.cc (right):
> 
>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
> If you get rid of the delegate interface this whole function becomes:
> 
> InputMethod* input_method = GetInputMethod();
> if (input_method && input_method->OnKeyEvent(event))
>   return GetRootView()->OnKeyEvent(event);
> return false;
> 
> ... and you can delete DispatchKeyEventPostIME from this class.

I'm still trying to grok the code 100% in the hopes that I can achieve your
suggested pattern.
Here is my e-mail with James; he suggests against eliminating the delegate /
DispachKeyEventPostIME:

[James]: Maybe we can have a VC meeting sometime next week. Feel free to book my
time for your convenience.
Before having more in-depth discussion, it'll be best if you could give me some
background of your refactoring work. E.g. what's your ultimate goal? And what's
the problem of the current code?

在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：

[Mike]: I'm trying to better comprehend the necessity of our asynchronous
pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
[James]: Because we are going to have an extension API to allow third-party
input method extensions, it's mandatory to have asynchronous communication
between these extensions and Chrome UI to avoid blocking UI thread. It'll be
true for ChromeOS and will probably be true for Windows if we want to support
the extension API on Windows as well.
The current DispatchKeyEvent/DispachKeyEventPostIME design pattern was actually
borrowed from Android, which needs to deal with the same problem (where input
methods run out of application's process).

[Mike]: Is it possible to synchronously handle our KeyEvents, electing to ignore
those destined for the IME? In principle it seems that conditional logic (around
[James]: I'm afraid that it's not possible, at least on ChromeOS for now. In
order to make sure input methods work as expected, we need to dispatch key
events to the input method before actually handling them. But the input method
can only decide if it'll hand a key event until it receives the event and then
asks the application to continue to handle it when necessary. It's not possible
to determine whether or not we should send a key event to the input method   
instead of handling it by ourselves beforehand.
In all existing desktop systems, including Windows, Mac and Linux, the
application will be simply blocked after sending a key event to the input method
until the input method returns the result. Though such model simplifies the
logic, it's not acceptable for Chrome, as we don't want to block UI thread at
all.

[Mike]: the presence of an IME and the contents of the KeyEvent), and (if truly
needed) a callback disambiguated from OnKeyEvent (for the IME to commit
result_text to the InputMethod/Widget), ought to clean up some of our code and
eliminate the need for an InputMethodDelegate and DispatchKeyEventPostIME.
[James]: Actually I'd prefer to stick with the current
DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has very clear
behavior definition: Widget's OnKeyEvent() needs to do nothing except for
calling InputMethod::DispatchKeyEvent(), and all actual logics should be
implemented in InputMethodDelegate::DispachKeyEventPostIME(), which doesn't need
to care about whether or not the key event is sent back from the input method
asynchronously. And in fact, we can eliminate the check in
Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_| is always
not NULL.

[Mike]: Additionally, may I consolidate some cross-platform InputMethod code
into InputMethodBase?
[James]: Yes. It's the purpose of InputMethodBase. I'm just wondering which part
you want to move?

[Mike]:What are your thoughts, and do you have time to discuss this in person or
over IM again? Thanks again!

Mike

msw

Forgot to send along inline responses... http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_delegate.h File views/ime/input_method_delegate.h (right): http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_delegate.h#newcode22 views/ime/input_method_delegate.h:22: virtual bool DispatchKeyEventPostIME(const ...

9 years, 8 months ago (2011-04-15 19:19:57 UTC) #8

Ben Goodger (Google)

But it's not really asynchronous. i.e. the code currently does this: WidgetWin::OnKeyFoo input_method_->OnKeyEvent(..); InputMethod::OnKeyEvent ...

9 years, 8 months ago (2011-04-15 19:58:16 UTC) #9

But it's not really asynchronous.

i.e. the code currently does this:

WidgetWin::OnKeyFoo
  input_method_->OnKeyEvent(..);

InputMethod::OnKeyEvent
  ...
  widget_->PostProcess(...)

WidgetWin::PostProcess
  GetRootView()->OnKeyEvent(..);

This isn't asynchronous, it's just more complex code :-)

How is the input method going to actually deal with delayed input method
response from extensions? How does it plan to block the key event from being
processed by the root view?

-Ben

On Fri, Apr 15, 2011 at 12:19 PM, <msw@chromium.org> wrote:

> On 2011/04/15 18:23:19, Ben Goodger wrote:
>
>> I am really liking where this change is going Mike. A few comments:
>>
>
>
>
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>
>> File views/ime/input_method_delegate.h (right):
>>
>
>
>
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>
>> views/ime/input_method_delegate.h:22: virtual bool
>>
> DispatchKeyEventPostIME(const
>
>> KeyEvent& key) = 0;
>> Do you even still need this function?
>>
>
>  I was thinking InputMethod::OnKeyEvent would return a bool indicating
>> whether
>>
> or
>
>> not the calling Widget should process further... which is effectively what
>>
> this
>
>> function is doing. If you remove this then you can delete the whole
>> delegate
>> interface.
>>
>
>  http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
>> File views/widget/widget.cc (right):
>>
>
>
>
>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
>
>> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
>> If you get rid of the delegate interface this whole function becomes:
>>
>
>  InputMethod* input_method = GetInputMethod();
>> if (input_method && input_method->OnKeyEvent(event))
>>   return GetRootView()->OnKeyEvent(event);
>> return false;
>>
>
>  ... and you can delete DispatchKeyEventPostIME from this class.
>>
>
> I'm still trying to grok the code 100% in the hopes that I can achieve your
> suggested pattern.
> Here is my e-mail with James; he suggests against eliminating the delegate
> /
> DispachKeyEventPostIME:
>
> [James]: Maybe we can have a VC meeting sometime next week. Feel free to
> book my
> time for your convenience.
> Before having more in-depth discussion, it'll be best if you could give me
> some
> background of your refactoring work. E.g. what's your ultimate goal? And
> what's
> the problem of the current code?
>
> 在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：
>
> [Mike]: I'm trying to better comprehend the necessity of our asynchronous
> pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
> [James]: Because we are going to have an extension API to allow third-party
> input method extensions, it's mandatory to have asynchronous communication
> between these extensions and Chrome UI to avoid blocking UI thread. It'll
> be
> true for ChromeOS and will probably be true for Windows if we want to
> support
> the extension API on Windows as well.
> The current DispatchKeyEvent/DispachKeyEventPostIME design pattern was
> actually
> borrowed from Android, which needs to deal with the same problem (where
> input
> methods run out of application's process).
>
> [Mike]: Is it possible to synchronously handle our KeyEvents, electing to
> ignore
> those destined for the IME? In principle it seems that conditional logic
> (around
> [James]: I'm afraid that it's not possible, at least on ChromeOS for now.
> In
> order to make sure input methods work as expected, we need to dispatch key
> events to the input method before actually handling them. But the input
> method
> can only decide if it'll hand a key event until it receives the event and
> then
> asks the application to continue to handle it when necessary. It's not
> possible
> to determine whether or not we should send a key event to the input method
> instead of handling it by ourselves beforehand.
> In all existing desktop systems, including Windows, Mac and Linux, the
> application will be simply blocked after sending a key event to the input
> method
> until the input method returns the result. Though such model simplifies the
> logic, it's not acceptable for Chrome, as we don't want to block UI thread
> at
> all.
>
> [Mike]: the presence of an IME and the contents of the KeyEvent), and (if
> truly
> needed) a callback disambiguated from OnKeyEvent (for the IME to commit
> result_text to the InputMethod/Widget), ought to clean up some of our code
> and
> eliminate the need for an InputMethodDelegate and DispatchKeyEventPostIME.
> [James]: Actually I'd prefer to stick with the current
> DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has very
> clear
> behavior definition: Widget's OnKeyEvent() needs to do nothing except for
> calling InputMethod::DispatchKeyEvent(), and all actual logics should be
> implemented in InputMethodDelegate::DispachKeyEventPostIME(), which doesn't
> need
> to care about whether or not the key event is sent back from the input
> method
> asynchronously. And in fact, we can eliminate the check in
> Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_| is
> always
> not NULL.
>
> [Mike]: Additionally, may I consolidate some cross-platform InputMethod
> code
> into InputMethodBase?
> [James]: Yes. It's the purpose of InputMethodBase. I'm just wondering which
> part
> you want to move?
>
> [Mike]:What are your thoughts, and do you have time to discuss this in
> person or
> over IM again? Thanks again!
>
> Mike
>
>
> http://codereview.chromium.org/6823055/
>

James Su

InputMethodWin is currently just a simple synchronous implementation, but InputMethodIBus is a full asynchronous implementation. ...

9 years, 8 months ago (2011-04-18 07:59:34 UTC) #10

InputMethodWin is currently just a simple synchronous implementation, but
InputMethodIBus is a full asynchronous implementation. Using the same API for
all InputMethod implementations can make related code simpler and easy to
maintain. And we may also need to add asynchronous ability to InputMethodWin in
the future when we want to support input method extensions on Windows.

On 2011/04/15 19:58:16, Ben Goodger wrote:
> But it's not really asynchronous.
> 
> i.e. the code currently does this:
> 
> WidgetWin::OnKeyFoo
>   input_method_->OnKeyEvent(..);
> 
> InputMethod::OnKeyEvent
>   ...
>   widget_->PostProcess(...)
> 
> WidgetWin::PostProcess
>   GetRootView()->OnKeyEvent(..);
> 
> This isn't asynchronous, it's just more complex code :-)
> 
> How is the input method going to actually deal with delayed input method
> response from extensions? How does it plan to block the key event from being
> processed by the root view?
If we want to support input method extensions on Windows, InputMethodWin will
need to dispatch a key event to the input method extension asynchronously
without waiting for the reply, and call DispatchKeyEventPostIME() when receiving
the reply from the extension.

> 
> -Ben
> 
> On Fri, Apr 15, 2011 at 12:19 PM, <mailto:msw@chromium.org> wrote:
> 
> > On 2011/04/15 18:23:19, Ben Goodger wrote:
> >
> >> I am really liking where this change is going Mike. A few comments:
> >>
> >
> >
> >
> >
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
> >
> >> File views/ime/input_method_delegate.h (right):
> >>
> >
> >
> >
> >
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
> >
> >> views/ime/input_method_delegate.h:22: virtual bool
> >>
> > DispatchKeyEventPostIME(const
> >
> >> KeyEvent& key) = 0;
> >> Do you even still need this function?
> >>
> >
> >  I was thinking InputMethod::OnKeyEvent would return a bool indicating
> >> whether
> >>
> > or
> >
> >> not the calling Widget should process further... which is effectively what
> >>
> > this
> >
> >> function is doing. If you remove this then you can delete the whole
> >> delegate
> >> interface.
> >>
> >
> >  http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
> >> File views/widget/widget.cc (right):
> >>
> >
> >
> >
> >
>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
> >
> >> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
> >> If you get rid of the delegate interface this whole function becomes:
> >>
> >
> >  InputMethod* input_method = GetInputMethod();
> >> if (input_method && input_method->OnKeyEvent(event))
> >>   return GetRootView()->OnKeyEvent(event);
> >> return false;
> >>
> >
> >  ... and you can delete DispatchKeyEventPostIME from this class.
> >>
> >
> > I'm still trying to grok the code 100% in the hopes that I can achieve your
> > suggested pattern.
> > Here is my e-mail with James; he suggests against eliminating the delegate
> > /
> > DispachKeyEventPostIME:
> >
> > [James]: Maybe we can have a VC meeting sometime next week. Feel free to
> > book my
> > time for your convenience.
> > Before having more in-depth discussion, it'll be best if you could give me
> > some
> > background of your refactoring work. E.g. what's your ultimate goal? And
> > what's
> > the problem of the current code?
> >
> > 在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：
> >
> > [Mike]: I'm trying to better comprehend the necessity of our asynchronous
> > pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
> > [James]: Because we are going to have an extension API to allow third-party
> > input method extensions, it's mandatory to have asynchronous communication
> > between these extensions and Chrome UI to avoid blocking UI thread. It'll
> > be
> > true for ChromeOS and will probably be true for Windows if we want to
> > support
> > the extension API on Windows as well.
> > The current DispatchKeyEvent/DispachKeyEventPostIME design pattern was
> > actually
> > borrowed from Android, which needs to deal with the same problem (where
> > input
> > methods run out of application's process).
> >
> > [Mike]: Is it possible to synchronously handle our KeyEvents, electing to
> > ignore
> > those destined for the IME? In principle it seems that conditional logic
> > (around
> > [James]: I'm afraid that it's not possible, at least on ChromeOS for now.
> > In
> > order to make sure input methods work as expected, we need to dispatch key
> > events to the input method before actually handling them. But the input
> > method
> > can only decide if it'll hand a key event until it receives the event and
> > then
> > asks the application to continue to handle it when necessary. It's not
> > possible
> > to determine whether or not we should send a key event to the input method
> > instead of handling it by ourselves beforehand.
> > In all existing desktop systems, including Windows, Mac and Linux, the
> > application will be simply blocked after sending a key event to the input
> > method
> > until the input method returns the result. Though such model simplifies the
> > logic, it's not acceptable for Chrome, as we don't want to block UI thread
> > at
> > all.
> >
> > [Mike]: the presence of an IME and the contents of the KeyEvent), and (if
> > truly
> > needed) a callback disambiguated from OnKeyEvent (for the IME to commit
> > result_text to the InputMethod/Widget), ought to clean up some of our code
> > and
> > eliminate the need for an InputMethodDelegate and DispatchKeyEventPostIME.
> > [James]: Actually I'd prefer to stick with the current
> > DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has very
> > clear
> > behavior definition: Widget's OnKeyEvent() needs to do nothing except for
> > calling InputMethod::DispatchKeyEvent(), and all actual logics should be
> > implemented in InputMethodDelegate::DispachKeyEventPostIME(), which doesn't
> > need
> > to care about whether or not the key event is sent back from the input
> > method
> > asynchronously. And in fact, we can eliminate the check in
> > Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_| is
> > always
> > not NULL.
> >
> > [Mike]: Additionally, may I consolidate some cross-platform InputMethod
> > code
> > into InputMethodBase?
> > [James]: Yes. It's the purpose of InputMethodBase. I'm just wondering which
> > part
> > you want to move?
> >
> > [Mike]:What are your thoughts, and do you have time to discuss this in
> > person or
> > over IM again? Thanks again!
> >
> > Mike
> >
> >
> > http://codereview.chromium.org/6823055/
> >

Ben Goodger (Google)

OK this makes more sense now. However, I don't think the platform-specific widget subclasses should ...

9 years, 8 months ago (2011-04-18 17:16:25 UTC) #11

OK this makes more sense now.

However, I don't think the platform-specific widget subclasses should be
overriding DispatchKeyEventPostIME. This will be a problem when we split the
inheritance. Mike, can you create a NativeWidget method so WidgetGtk can do
its additional processing instead of overriding in WidgetGtk?

-Ben

On Mon, Apr 18, 2011 at 12:59 AM, <suzhe@chromium.org> wrote:

> InputMethodWin is currently just a simple synchronous implementation, but
> InputMethodIBus is a full asynchronous implementation. Using the same API
> for
> all InputMethod implementations can make related code simpler and easy to
> maintain. And we may also need to add asynchronous ability to
> InputMethodWin in
> the future when we want to support input method extensions on Windows.
>
>
> On 2011/04/15 19:58:16, Ben Goodger wrote:
>
>> But it's not really asynchronous.
>>
>
>  i.e. the code currently does this:
>>
>
>  WidgetWin::OnKeyFoo
>>   input_method_->OnKeyEvent(..);
>>
>
>  InputMethod::OnKeyEvent
>>   ...
>>   widget_->PostProcess(...)
>>
>
>  WidgetWin::PostProcess
>>   GetRootView()->OnKeyEvent(..);
>>
>
>  This isn't asynchronous, it's just more complex code :-)
>>
>
>  How is the input method going to actually deal with delayed input method
>> response from extensions? How does it plan to block the key event from
>> being
>> processed by the root view?
>>
> If we want to support input method extensions on Windows, InputMethodWin
> will
> need to dispatch a key event to the input method extension asynchronously
> without waiting for the reply, and call DispatchKeyEventPostIME() when
> receiving
> the reply from the extension.
>
>
>  -Ben
>>
>
>  On Fri, Apr 15, 2011 at 12:19 PM, <mailto:msw@chromium.org> wrote:
>>
>
>  > On 2011/04/15 18:23:19, Ben Goodger wrote:
>> >
>> >> I am really liking where this change is going Mike. A few comments:
>> >>
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>
>> >
>> >> File views/ime/input_method_delegate.h (right):
>> >>
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>
>> >
>> >> views/ime/input_method_delegate.h:22: virtual bool
>> >>
>> > DispatchKeyEventPostIME(const
>> >
>> >> KeyEvent& key) = 0;
>> >> Do you even still need this function?
>> >>
>> >
>> >  I was thinking InputMethod::OnKeyEvent would return a bool indicating
>> >> whether
>> >>
>> > or
>> >
>> >> not the calling Widget should process further... which is effectively
>> what
>> >>
>> > this
>> >
>> >> function is doing. If you remove this then you can delete the whole
>> >> delegate
>> >> interface.
>> >>
>> >
>> >
>> http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
>> >> File views/widget/widget.cc (right):
>> >>
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
>
>> >
>> >> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
>> >> If you get rid of the delegate interface this whole function becomes:
>> >>
>> >
>> >  InputMethod* input_method = GetInputMethod();
>> >> if (input_method && input_method->OnKeyEvent(event))
>> >>   return GetRootView()->OnKeyEvent(event);
>> >> return false;
>> >>
>> >
>> >  ... and you can delete DispatchKeyEventPostIME from this class.
>> >>
>> >
>> > I'm still trying to grok the code 100% in the hopes that I can achieve
>> your
>> > suggested pattern.
>> > Here is my e-mail with James; he suggests against eliminating the
>> delegate
>> > /
>> > DispachKeyEventPostIME:
>> >
>> > [James]: Maybe we can have a VC meeting sometime next week. Feel free to
>> > book my
>> > time for your convenience.
>> > Before having more in-depth discussion, it'll be best if you could give
>> me
>> > some
>> > background of your refactoring work. E.g. what's your ultimate goal? And
>> > what's
>> > the problem of the current code?
>> >
>> > 在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：
>> >
>> > [Mike]: I'm trying to better comprehend the necessity of our
>> asynchronous
>> > pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
>> > [James]: Because we are going to have an extension API to allow
>> third-party
>> > input method extensions, it's mandatory to have asynchronous
>> communication
>> > between these extensions and Chrome UI to avoid blocking UI thread.
>> It'll
>> > be
>> > true for ChromeOS and will probably be true for Windows if we want to
>> > support
>> > the extension API on Windows as well.
>> > The current DispatchKeyEvent/DispachKeyEventPostIME design pattern was
>> > actually
>> > borrowed from Android, which needs to deal with the same problem (where
>> > input
>> > methods run out of application's process).
>> >
>> > [Mike]: Is it possible to synchronously handle our KeyEvents, electing
>> to
>> > ignore
>> > those destined for the IME? In principle it seems that conditional logic
>> > (around
>> > [James]: I'm afraid that it's not possible, at least on ChromeOS for
>> now.
>> > In
>> > order to make sure input methods work as expected, we need to dispatch
>> key
>> > events to the input method before actually handling them. But the input
>> > method
>> > can only decide if it'll hand a key event until it receives the event
>> and
>> > then
>> > asks the application to continue to handle it when necessary. It's not
>> > possible
>> > to determine whether or not we should send a key event to the input
>> method
>> > instead of handling it by ourselves beforehand.
>> > In all existing desktop systems, including Windows, Mac and Linux, the
>> > application will be simply blocked after sending a key event to the
>> input
>> > method
>> > until the input method returns the result. Though such model simplifies
>> the
>> > logic, it's not acceptable for Chrome, as we don't want to block UI
>> thread
>> > at
>> > all.
>> >
>> > [Mike]: the presence of an IME and the contents of the KeyEvent), and
>> (if
>> > truly
>> > needed) a callback disambiguated from OnKeyEvent (for the IME to commit
>> > result_text to the InputMethod/Widget), ought to clean up some of our
>> code
>> > and
>> > eliminate the need for an InputMethodDelegate and
>> DispatchKeyEventPostIME.
>> > [James]: Actually I'd prefer to stick with the current
>> > DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has very
>> > clear
>> > behavior definition: Widget's OnKeyEvent() needs to do nothing except
>> for
>> > calling InputMethod::DispatchKeyEvent(), and all actual logics should be
>> > implemented in InputMethodDelegate::DispachKeyEventPostIME(), which
>> doesn't
>> > need
>> > to care about whether or not the key event is sent back from the input
>> > method
>> > asynchronously. And in fact, we can eliminate the check in
>> > Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_| is
>> > always
>> > not NULL.
>> >
>> > [Mike]: Additionally, may I consolidate some cross-platform InputMethod
>> > code
>> > into InputMethodBase?
>> > [James]: Yes. It's the purpose of InputMethodBase. I'm just wondering
>> which
>> > part
>> > you want to move?
>> >
>> > [Mike]:What are your thoughts, and do you have time to discuss this in
>> > person or
>> > over IM again? Thanks again!
>> >
>> > Mike
>> >
>> >
>> > http://codereview.chromium.org/6823055/
>> >
>>
>
>
>
> http://codereview.chromium.org/6823055/
>

msw

On 2011/04/18 17:16:25, Ben Goodger wrote: > OK this makes more sense now. > > ...

9 years, 8 months ago (2011-04-18 20:48:14 UTC) #12

suzhe

I'd still prefer to implement InputMethodDelegate in NativeWidget classes. Regards James Su 在 2011年4月19日上午1:16，Ben ...

9 years, 8 months ago (2011-04-18 22:12:12 UTC) #13

I'd still prefer to implement InputMethodDelegate in NativeWidget classes.

Regards
James Su

在 2011年4月19日 上午1:16，Ben Goodger (Google) <ben@chromium.org>写道：

> OK this makes more sense now.
>
> However, I don't think the platform-specific widget subclasses should be
> overriding DispatchKeyEventPostIME. This will be a problem when we split the
> inheritance. Mike, can you create a NativeWidget method so WidgetGtk can do
> its additional processing instead of overriding in WidgetGtk?
>
> -Ben
>
>
> On Mon, Apr 18, 2011 at 12:59 AM, <suzhe@chromium.org> wrote:
>
>> InputMethodWin is currently just a simple synchronous implementation, but
>> InputMethodIBus is a full asynchronous implementation. Using the same API
>> for
>> all InputMethod implementations can make related code simpler and easy to
>> maintain. And we may also need to add asynchronous ability to
>> InputMethodWin in
>> the future when we want to support input method extensions on Windows.
>>
>>
>> On 2011/04/15 19:58:16, Ben Goodger wrote:
>>
>>> But it's not really asynchronous.
>>>
>>
>>  i.e. the code currently does this:
>>>
>>
>>  WidgetWin::OnKeyFoo
>>>   input_method_->OnKeyEvent(..);
>>>
>>
>>  InputMethod::OnKeyEvent
>>>   ...
>>>   widget_->PostProcess(...)
>>>
>>
>>  WidgetWin::PostProcess
>>>   GetRootView()->OnKeyEvent(..);
>>>
>>
>>  This isn't asynchronous, it's just more complex code :-)
>>>
>>
>>  How is the input method going to actually deal with delayed input method
>>> response from extensions? How does it plan to block the key event from
>>> being
>>> processed by the root view?
>>>
>> If we want to support input method extensions on Windows, InputMethodWin
>> will
>> need to dispatch a key event to the input method extension asynchronously
>> without waiting for the reply, and call DispatchKeyEventPostIME() when
>> receiving
>> the reply from the extension.
>>
>>
>>  -Ben
>>>
>>
>>  On Fri, Apr 15, 2011 at 12:19 PM, <mailto:msw@chromium.org> wrote:
>>>
>>
>>  > On 2011/04/15 18:23:19, Ben Goodger wrote:
>>> >
>>> >> I am really liking where this change is going Mike. A few comments:
>>> >>
>>> >
>>> >
>>> >
>>> >
>>>
>>
>>
>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>
>>> >
>>> >> File views/ime/input_method_delegate.h (right):
>>> >>
>>> >
>>> >
>>> >
>>> >
>>>
>>
>>
>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>
>>> >
>>> >> views/ime/input_method_delegate.h:22: virtual bool
>>> >>
>>> > DispatchKeyEventPostIME(const
>>> >
>>> >> KeyEvent& key) = 0;
>>> >> Do you even still need this function?
>>> >>
>>> >
>>> >  I was thinking InputMethod::OnKeyEvent would return a bool indicating
>>> >> whether
>>> >>
>>> > or
>>> >
>>> >> not the calling Widget should process further... which is effectively
>>> what
>>> >>
>>> > this
>>> >
>>> >> function is doing. If you remove this then you can delete the whole
>>> >> delegate
>>> >> interface.
>>> >>
>>> >
>>> >
>>> http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
>>> >> File views/widget/widget.cc (right):
>>> >>
>>> >
>>> >
>>> >
>>> >
>>>
>>
>>
>>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
>>
>>> >
>>> >> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
>>> >> If you get rid of the delegate interface this whole function becomes:
>>> >>
>>> >
>>> >  InputMethod* input_method = GetInputMethod();
>>> >> if (input_method && input_method->OnKeyEvent(event))
>>> >>   return GetRootView()->OnKeyEvent(event);
>>> >> return false;
>>> >>
>>> >
>>> >  ... and you can delete DispatchKeyEventPostIME from this class.
>>> >>
>>> >
>>> > I'm still trying to grok the code 100% in the hopes that I can achieve
>>> your
>>> > suggested pattern.
>>> > Here is my e-mail with James; he suggests against eliminating the
>>> delegate
>>> > /
>>> > DispachKeyEventPostIME:
>>> >
>>> > [James]: Maybe we can have a VC meeting sometime next week. Feel free
>>> to
>>> > book my
>>> > time for your convenience.
>>> > Before having more in-depth discussion, it'll be best if you could give
>>> me
>>> > some
>>> > background of your refactoring work. E.g. what's your ultimate goal?
>>> And
>>> > what's
>>> > the problem of the current code?
>>> >
>>> > 在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：
>>> >
>>> > [Mike]: I'm trying to better comprehend the necessity of our
>>> asynchronous
>>> > pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
>>> > [James]: Because we are going to have an extension API to allow
>>> third-party
>>> > input method extensions, it's mandatory to have asynchronous
>>> communication
>>> > between these extensions and Chrome UI to avoid blocking UI thread.
>>> It'll
>>> > be
>>> > true for ChromeOS and will probably be true for Windows if we want to
>>> > support
>>> > the extension API on Windows as well.
>>> > The current DispatchKeyEvent/DispachKeyEventPostIME design pattern was
>>> > actually
>>> > borrowed from Android, which needs to deal with the same problem (where
>>> > input
>>> > methods run out of application's process).
>>> >
>>> > [Mike]: Is it possible to synchronously handle our KeyEvents, electing
>>> to
>>> > ignore
>>> > those destined for the IME? In principle it seems that conditional
>>> logic
>>> > (around
>>> > [James]: I'm afraid that it's not possible, at least on ChromeOS for
>>> now.
>>> > In
>>> > order to make sure input methods work as expected, we need to dispatch
>>> key
>>> > events to the input method before actually handling them. But the input
>>> > method
>>> > can only decide if it'll hand a key event until it receives the event
>>> and
>>> > then
>>> > asks the application to continue to handle it when necessary. It's not
>>> > possible
>>> > to determine whether or not we should send a key event to the input
>>> method
>>> > instead of handling it by ourselves beforehand.
>>> > In all existing desktop systems, including Windows, Mac and Linux, the
>>> > application will be simply blocked after sending a key event to the
>>> input
>>> > method
>>> > until the input method returns the result. Though such model simplifies
>>> the
>>> > logic, it's not acceptable for Chrome, as we don't want to block UI
>>> thread
>>> > at
>>> > all.
>>> >
>>> > [Mike]: the presence of an IME and the contents of the KeyEvent), and
>>> (if
>>> > truly
>>> > needed) a callback disambiguated from OnKeyEvent (for the IME to commit
>>> > result_text to the InputMethod/Widget), ought to clean up some of our
>>> code
>>> > and
>>> > eliminate the need for an InputMethodDelegate and
>>> DispatchKeyEventPostIME.
>>> > [James]: Actually I'd prefer to stick with the current
>>> > DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has very
>>> > clear
>>> > behavior definition: Widget's OnKeyEvent() needs to do nothing except
>>> for
>>> > calling InputMethod::DispatchKeyEvent(), and all actual logics should
>>> be
>>> > implemented in InputMethodDelegate::DispachKeyEventPostIME(), which
>>> doesn't
>>> > need
>>> > to care about whether or not the key event is sent back from the input
>>> > method
>>> > asynchronously. And in fact, we can eliminate the check in
>>> > Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_| is
>>> > always
>>> > not NULL.
>>> >
>>> > [Mike]: Additionally, may I consolidate some cross-platform InputMethod
>>> > code
>>> > into InputMethodBase?
>>> > [James]: Yes. It's the purpose of InputMethodBase. I'm just wondering
>>> which
>>> > part
>>> > you want to move?
>>> >
>>> > [Mike]:What are your thoughts, and do you have time to discuss this in
>>> > person or
>>> > over IM again? Thanks again!
>>> >
>>> > Mike
>>> >
>>> >
>>> > http://codereview.chromium.org/6823055/
>>> >
>>>
>>
>>
>>
>> http://codereview.chromium.org/6823055/
>>
>
>

Ben Goodger (Google)

Is the consultation with input method extensions going to be handled internally to each platform ...

9 years, 8 months ago (2011-04-18 23:30:37 UTC) #14

Is the consultation with input method extensions going to be handled
internally to each platform input method implementation? It seems like some
of that code is cross platform (the extension portion at least).

-Ben

On Mon, Apr 18, 2011 at 3:11 PM, James Su <suzhe@google.com> wrote:

> I'd still prefer to implement InputMethodDelegate in NativeWidget classes.
>
> Regards
> James Su
>
> 在 2011年4月19日 上午1:16，Ben Goodger (Google) <ben@chromium.org>写道：
>
> OK this makes more sense now.
>>
>> However, I don't think the platform-specific widget subclasses should be
>> overriding DispatchKeyEventPostIME. This will be a problem when we split the
>> inheritance. Mike, can you create a NativeWidget method so WidgetGtk can do
>> its additional processing instead of overriding in WidgetGtk?
>>
>> -Ben
>>
>>
>> On Mon, Apr 18, 2011 at 12:59 AM, <suzhe@chromium.org> wrote:
>>
>>> InputMethodWin is currently just a simple synchronous implementation, but
>>> InputMethodIBus is a full asynchronous implementation. Using the same API
>>> for
>>> all InputMethod implementations can make related code simpler and easy to
>>> maintain. And we may also need to add asynchronous ability to
>>> InputMethodWin in
>>> the future when we want to support input method extensions on Windows.
>>>
>>>
>>> On 2011/04/15 19:58:16, Ben Goodger wrote:
>>>
>>>> But it's not really asynchronous.
>>>>
>>>
>>>  i.e. the code currently does this:
>>>>
>>>
>>>  WidgetWin::OnKeyFoo
>>>>   input_method_->OnKeyEvent(..);
>>>>
>>>
>>>  InputMethod::OnKeyEvent
>>>>   ...
>>>>   widget_->PostProcess(...)
>>>>
>>>
>>>  WidgetWin::PostProcess
>>>>   GetRootView()->OnKeyEvent(..);
>>>>
>>>
>>>  This isn't asynchronous, it's just more complex code :-)
>>>>
>>>
>>>  How is the input method going to actually deal with delayed input method
>>>> response from extensions? How does it plan to block the key event from
>>>> being
>>>> processed by the root view?
>>>>
>>> If we want to support input method extensions on Windows, InputMethodWin
>>> will
>>> need to dispatch a key event to the input method extension asynchronously
>>> without waiting for the reply, and call DispatchKeyEventPostIME() when
>>> receiving
>>> the reply from the extension.
>>>
>>>
>>>  -Ben
>>>>
>>>
>>>  On Fri, Apr 15, 2011 at 12:19 PM, <mailto:msw@chromium.org> wrote:
>>>>
>>>
>>>  > On 2011/04/15 18:23:19, Ben Goodger wrote:
>>>> >
>>>> >> I am really liking where this change is going Mike. A few comments:
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> >
>>>>
>>>
>>>
>>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>>
>>>> >
>>>> >> File views/ime/input_method_delegate.h (right):
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> >
>>>>
>>>
>>>
>>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>>
>>>> >
>>>> >> views/ime/input_method_delegate.h:22: virtual bool
>>>> >>
>>>> > DispatchKeyEventPostIME(const
>>>> >
>>>> >> KeyEvent& key) = 0;
>>>> >> Do you even still need this function?
>>>> >>
>>>> >
>>>> >  I was thinking InputMethod::OnKeyEvent would return a bool indicating
>>>> >> whether
>>>> >>
>>>> > or
>>>> >
>>>> >> not the calling Widget should process further... which is effectively
>>>> what
>>>> >>
>>>> > this
>>>> >
>>>> >> function is doing. If you remove this then you can delete the whole
>>>> >> delegate
>>>> >> interface.
>>>> >>
>>>> >
>>>> >
>>>> http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
>>>> >> File views/widget/widget.cc (right):
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> >
>>>>
>>>
>>>
>>>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
>>>
>>>> >
>>>> >> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
>>>> >> If you get rid of the delegate interface this whole function becomes:
>>>> >>
>>>> >
>>>> >  InputMethod* input_method = GetInputMethod();
>>>> >> if (input_method && input_method->OnKeyEvent(event))
>>>> >>   return GetRootView()->OnKeyEvent(event);
>>>> >> return false;
>>>> >>
>>>> >
>>>> >  ... and you can delete DispatchKeyEventPostIME from this class.
>>>> >>
>>>> >
>>>> > I'm still trying to grok the code 100% in the hopes that I can achieve
>>>> your
>>>> > suggested pattern.
>>>> > Here is my e-mail with James; he suggests against eliminating the
>>>> delegate
>>>> > /
>>>> > DispachKeyEventPostIME:
>>>> >
>>>> > [James]: Maybe we can have a VC meeting sometime next week. Feel free
>>>> to
>>>> > book my
>>>> > time for your convenience.
>>>> > Before having more in-depth discussion, it'll be best if you could
>>>> give me
>>>> > some
>>>> > background of your refactoring work. E.g. what's your ultimate goal?
>>>> And
>>>> > what's
>>>> > the problem of the current code?
>>>> >
>>>> > 在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：
>>>> >
>>>> > [Mike]: I'm trying to better comprehend the necessity of our
>>>> asynchronous
>>>> > pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
>>>> > [James]: Because we are going to have an extension API to allow
>>>> third-party
>>>> > input method extensions, it's mandatory to have asynchronous
>>>> communication
>>>> > between these extensions and Chrome UI to avoid blocking UI thread.
>>>> It'll
>>>> > be
>>>> > true for ChromeOS and will probably be true for Windows if we want to
>>>> > support
>>>> > the extension API on Windows as well.
>>>> > The current DispatchKeyEvent/DispachKeyEventPostIME design pattern was
>>>> > actually
>>>> > borrowed from Android, which needs to deal with the same problem
>>>> (where
>>>> > input
>>>> > methods run out of application's process).
>>>> >
>>>> > [Mike]: Is it possible to synchronously handle our KeyEvents, electing
>>>> to
>>>> > ignore
>>>> > those destined for the IME? In principle it seems that conditional
>>>> logic
>>>> > (around
>>>> > [James]: I'm afraid that it's not possible, at least on ChromeOS for
>>>> now.
>>>> > In
>>>> > order to make sure input methods work as expected, we need to dispatch
>>>> key
>>>> > events to the input method before actually handling them. But the
>>>> input
>>>> > method
>>>> > can only decide if it'll hand a key event until it receives the event
>>>> and
>>>> > then
>>>> > asks the application to continue to handle it when necessary. It's not
>>>> > possible
>>>> > to determine whether or not we should send a key event to the input
>>>> method
>>>> > instead of handling it by ourselves beforehand.
>>>> > In all existing desktop systems, including Windows, Mac and Linux, the
>>>> > application will be simply blocked after sending a key event to the
>>>> input
>>>> > method
>>>> > until the input method returns the result. Though such model
>>>> simplifies the
>>>> > logic, it's not acceptable for Chrome, as we don't want to block UI
>>>> thread
>>>> > at
>>>> > all.
>>>> >
>>>> > [Mike]: the presence of an IME and the contents of the KeyEvent), and
>>>> (if
>>>> > truly
>>>> > needed) a callback disambiguated from OnKeyEvent (for the IME to
>>>> commit
>>>> > result_text to the InputMethod/Widget), ought to clean up some of our
>>>> code
>>>> > and
>>>> > eliminate the need for an InputMethodDelegate and
>>>> DispatchKeyEventPostIME.
>>>> > [James]: Actually I'd prefer to stick with the current
>>>> > DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has
>>>> very
>>>> > clear
>>>> > behavior definition: Widget's OnKeyEvent() needs to do nothing except
>>>> for
>>>> > calling InputMethod::DispatchKeyEvent(), and all actual logics should
>>>> be
>>>> > implemented in InputMethodDelegate::DispachKeyEventPostIME(), which
>>>> doesn't
>>>> > need
>>>> > to care about whether or not the key event is sent back from the input
>>>> > method
>>>> > asynchronously. And in fact, we can eliminate the check in
>>>> > Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_|
>>>> is
>>>> > always
>>>> > not NULL.
>>>> >
>>>> > [Mike]: Additionally, may I consolidate some cross-platform
>>>> InputMethod
>>>> > code
>>>> > into InputMethodBase?
>>>> > [James]: Yes. It's the purpose of InputMethodBase. I'm just wondering
>>>> which
>>>> > part
>>>> > you want to move?
>>>> >
>>>> > [Mike]:What are your thoughts, and do you have time to discuss this in
>>>> > person or
>>>> > over IM again? Thanks again!
>>>> >
>>>> > Mike
>>>> >
>>>> >
>>>> > http://codereview.chromium.org/6823055/
>>>> >
>>>>
>>>
>>>
>>>
>>> http://codereview.chromium.org/6823055/
>>>
>>
>>
>

suzhe

在 2011年4月19日上午7:30，Ben Goodger (Google) <ben@chromium.org>写道： > Is the consultation with input method extensions going ...

9 years, 8 months ago (2011-04-19 00:44:36 UTC) #15

在 2011年4月19日 上午7:30，Ben Goodger (Google) <ben@chromium.org>写道：

> Is the consultation with input method extensions going to be handled
> internally to each platform input method implementation? It seems like some
> of that code is cross platform (the extension portion at least).

As far as I can see, they may need different implementations. E.g. on
ChromeOS, the communication between the InputMethod implementation and input
method extensions may go through ibus, while we may have different mechanism
on Windows.


> -Ben
>
>
> On Mon, Apr 18, 2011 at 3:11 PM, James Su <suzhe@google.com> wrote:
>
>> I'd still prefer to implement InputMethodDelegate in NativeWidget classes.
>>
>> Regards
>> James Su
>>
>> 在 2011年4月19日 上午1:16，Ben Goodger (Google) <ben@chromium.org>写道：
>>
>> OK this makes more sense now.
>>>
>>> However, I don't think the platform-specific widget subclasses should be
>>> overriding DispatchKeyEventPostIME. This will be a problem when we split the
>>> inheritance. Mike, can you create a NativeWidget method so WidgetGtk can do
>>> its additional processing instead of overriding in WidgetGtk?
>>>
>>> -Ben
>>>
>>>
>>> On Mon, Apr 18, 2011 at 12:59 AM, <suzhe@chromium.org> wrote:
>>>
>>>> InputMethodWin is currently just a simple synchronous implementation,
>>>> but
>>>> InputMethodIBus is a full asynchronous implementation. Using the same
>>>> API for
>>>> all InputMethod implementations can make related code simpler and easy
>>>> to
>>>> maintain. And we may also need to add asynchronous ability to
>>>> InputMethodWin in
>>>> the future when we want to support input method extensions on Windows.
>>>>
>>>>
>>>> On 2011/04/15 19:58:16, Ben Goodger wrote:
>>>>
>>>>> But it's not really asynchronous.
>>>>>
>>>>
>>>>  i.e. the code currently does this:
>>>>>
>>>>
>>>>  WidgetWin::OnKeyFoo
>>>>>   input_method_->OnKeyEvent(..);
>>>>>
>>>>
>>>>  InputMethod::OnKeyEvent
>>>>>   ...
>>>>>   widget_->PostProcess(...)
>>>>>
>>>>
>>>>  WidgetWin::PostProcess
>>>>>   GetRootView()->OnKeyEvent(..);
>>>>>
>>>>
>>>>  This isn't asynchronous, it's just more complex code :-)
>>>>>
>>>>
>>>>  How is the input method going to actually deal with delayed input
>>>>> method
>>>>> response from extensions? How does it plan to block the key event from
>>>>> being
>>>>> processed by the root view?
>>>>>
>>>> If we want to support input method extensions on Windows, InputMethodWin
>>>> will
>>>> need to dispatch a key event to the input method extension
>>>> asynchronously
>>>> without waiting for the reply, and call DispatchKeyEventPostIME() when
>>>> receiving
>>>> the reply from the extension.
>>>>
>>>>
>>>>  -Ben
>>>>>
>>>>
>>>>  On Fri, Apr 15, 2011 at 12:19 PM, <mailto:msw@chromium.org> wrote:
>>>>>
>>>>
>>>>  > On 2011/04/15 18:23:19, Ben Goodger wrote:
>>>>> >
>>>>> >> I am really liking where this change is going Mike. A few comments:
>>>>> >>
>>>>> >
>>>>> >
>>>>> >
>>>>> >
>>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>>>
>>>>> >
>>>>> >> File views/ime/input_method_delegate.h (right):
>>>>> >>
>>>>> >
>>>>> >
>>>>> >
>>>>> >
>>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>>>
>>>>> >
>>>>> >> views/ime/input_method_delegate.h:22: virtual bool
>>>>> >>
>>>>> > DispatchKeyEventPostIME(const
>>>>> >
>>>>> >> KeyEvent& key) = 0;
>>>>> >> Do you even still need this function?
>>>>> >>
>>>>> >
>>>>> >  I was thinking InputMethod::OnKeyEvent would return a bool
>>>>> indicating
>>>>> >> whether
>>>>> >>
>>>>> > or
>>>>> >
>>>>> >> not the calling Widget should process further... which is
>>>>> effectively what
>>>>> >>
>>>>> > this
>>>>> >
>>>>> >> function is doing. If you remove this then you can delete the whole
>>>>> >> delegate
>>>>> >> interface.
>>>>> >>
>>>>> >
>>>>> >
>>>>> http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
>>>>> >> File views/widget/widget.cc (right):
>>>>> >>
>>>>> >
>>>>> >
>>>>> >
>>>>> >
>>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
>>>>
>>>>> >
>>>>> >> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
>>>>> >> If you get rid of the delegate interface this whole function
>>>>> becomes:
>>>>> >>
>>>>> >
>>>>> >  InputMethod* input_method = GetInputMethod();
>>>>> >> if (input_method && input_method->OnKeyEvent(event))
>>>>> >>   return GetRootView()->OnKeyEvent(event);
>>>>> >> return false;
>>>>> >>
>>>>> >
>>>>> >  ... and you can delete DispatchKeyEventPostIME from this class.
>>>>> >>
>>>>> >
>>>>> > I'm still trying to grok the code 100% in the hopes that I can
>>>>> achieve your
>>>>> > suggested pattern.
>>>>> > Here is my e-mail with James; he suggests against eliminating the
>>>>> delegate
>>>>> > /
>>>>> > DispachKeyEventPostIME:
>>>>> >
>>>>> > [James]: Maybe we can have a VC meeting sometime next week. Feel free
>>>>> to
>>>>> > book my
>>>>> > time for your convenience.
>>>>> > Before having more in-depth discussion, it'll be best if you could
>>>>> give me
>>>>> > some
>>>>> > background of your refactoring work. E.g. what's your ultimate goal?
>>>>> And
>>>>> > what's
>>>>> > the problem of the current code?
>>>>> >
>>>>> > 在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：
>>>>> >
>>>>> > [Mike]: I'm trying to better comprehend the necessity of our
>>>>> asynchronous
>>>>> > pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
>>>>> > [James]: Because we are going to have an extension API to allow
>>>>> third-party
>>>>> > input method extensions, it's mandatory to have asynchronous
>>>>> communication
>>>>> > between these extensions and Chrome UI to avoid blocking UI thread.
>>>>> It'll
>>>>> > be
>>>>> > true for ChromeOS and will probably be true for Windows if we want to
>>>>> > support
>>>>> > the extension API on Windows as well.
>>>>> > The current DispatchKeyEvent/DispachKeyEventPostIME design pattern
>>>>> was
>>>>> > actually
>>>>> > borrowed from Android, which needs to deal with the same problem
>>>>> (where
>>>>> > input
>>>>> > methods run out of application's process).
>>>>> >
>>>>> > [Mike]: Is it possible to synchronously handle our KeyEvents,
>>>>> electing to
>>>>> > ignore
>>>>> > those destined for the IME? In principle it seems that conditional
>>>>> logic
>>>>> > (around
>>>>> > [James]: I'm afraid that it's not possible, at least on ChromeOS for
>>>>> now.
>>>>> > In
>>>>> > order to make sure input methods work as expected, we need to
>>>>> dispatch key
>>>>> > events to the input method before actually handling them. But the
>>>>> input
>>>>> > method
>>>>> > can only decide if it'll hand a key event until it receives the event
>>>>> and
>>>>> > then
>>>>> > asks the application to continue to handle it when necessary. It's
>>>>> not
>>>>> > possible
>>>>> > to determine whether or not we should send a key event to the input
>>>>> method
>>>>> > instead of handling it by ourselves beforehand.
>>>>> > In all existing desktop systems, including Windows, Mac and Linux,
>>>>> the
>>>>> > application will be simply blocked after sending a key event to the
>>>>> input
>>>>> > method
>>>>> > until the input method returns the result. Though such model
>>>>> simplifies the
>>>>> > logic, it's not acceptable for Chrome, as we don't want to block UI
>>>>> thread
>>>>> > at
>>>>> > all.
>>>>> >
>>>>> > [Mike]: the presence of an IME and the contents of the KeyEvent), and
>>>>> (if
>>>>> > truly
>>>>> > needed) a callback disambiguated from OnKeyEvent (for the IME to
>>>>> commit
>>>>> > result_text to the InputMethod/Widget), ought to clean up some of our
>>>>> code
>>>>> > and
>>>>> > eliminate the need for an InputMethodDelegate and
>>>>> DispatchKeyEventPostIME.
>>>>> > [James]: Actually I'd prefer to stick with the current
>>>>> > DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has
>>>>> very
>>>>> > clear
>>>>> > behavior definition: Widget's OnKeyEvent() needs to do nothing except
>>>>> for
>>>>> > calling InputMethod::DispatchKeyEvent(), and all actual logics should
>>>>> be
>>>>> > implemented in InputMethodDelegate::DispachKeyEventPostIME(), which
>>>>> doesn't
>>>>> > need
>>>>> > to care about whether or not the key event is sent back from the
>>>>> input
>>>>> > method
>>>>> > asynchronously. And in fact, we can eliminate the check in
>>>>> > Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_|
>>>>> is
>>>>> > always
>>>>> > not NULL.
>>>>> >
>>>>> > [Mike]: Additionally, may I consolidate some cross-platform
>>>>> InputMethod
>>>>> > code
>>>>> > into InputMethodBase?
>>>>> > [James]: Yes. It's the purpose of InputMethodBase. I'm just wondering
>>>>> which
>>>>> > part
>>>>> > you want to move?
>>>>> >
>>>>> > [Mike]:What are your thoughts, and do you have time to discuss this
>>>>> in
>>>>> > person or
>>>>> > over IM again? Thanks again!
>>>>> >
>>>>> > Mike
>>>>> >
>>>>> >
>>>>> > http://codereview.chromium.org/6823055/
>>>>> >
>>>>>
>>>>
>>>>
>>>>
>>>> http://codereview.chromium.org/6823055/
>>>>
>>>
>>>
>>
>

Ben Goodger (Google)

Are the extensions like regular chrome extensions? Or something else? -Ben On Mon, Apr 18, ...

9 years, 8 months ago (2011-04-19 00:47:17 UTC) #16

Are the extensions like regular chrome extensions? Or something else?
-Ben

On Mon, Apr 18, 2011 at 5:44 PM, James Su <suzhe@google.com> wrote:

>
>
> 在 2011年4月19日 上午7:30，Ben Goodger (Google) <ben@chromium.org>写道：
>
> Is the consultation with input method extensions going to be handled
>> internally to each platform input method implementation? It seems like some
>> of that code is cross platform (the extension portion at least).
>
> As far as I can see, they may need different implementations. E.g. on
> ChromeOS, the communication between the InputMethod implementation and input
> method extensions may go through ibus, while we may have different mechanism
> on Windows.
>
>
>> -Ben
>>
>>
>> On Mon, Apr 18, 2011 at 3:11 PM, James Su <suzhe@google.com> wrote:
>>
>>> I'd still prefer to implement InputMethodDelegate in NativeWidget
>>> classes.
>>>
>>> Regards
>>> James Su
>>>
>>> 在 2011年4月19日 上午1:16，Ben Goodger (Google) <ben@chromium.org>写道：
>>>
>>> OK this makes more sense now.
>>>>
>>>> However, I don't think the platform-specific widget subclasses should be
>>>> overriding DispatchKeyEventPostIME. This will be a problem when we split
the
>>>> inheritance. Mike, can you create a NativeWidget method so WidgetGtk can do
>>>> its additional processing instead of overriding in WidgetGtk?
>>>>
>>>> -Ben
>>>>
>>>>
>>>> On Mon, Apr 18, 2011 at 12:59 AM, <suzhe@chromium.org> wrote:
>>>>
>>>>> InputMethodWin is currently just a simple synchronous implementation,
>>>>> but
>>>>> InputMethodIBus is a full asynchronous implementation. Using the same
>>>>> API for
>>>>> all InputMethod implementations can make related code simpler and easy
>>>>> to
>>>>> maintain. And we may also need to add asynchronous ability to
>>>>> InputMethodWin in
>>>>> the future when we want to support input method extensions on Windows.
>>>>>
>>>>>
>>>>> On 2011/04/15 19:58:16, Ben Goodger wrote:
>>>>>
>>>>>> But it's not really asynchronous.
>>>>>>
>>>>>
>>>>>  i.e. the code currently does this:
>>>>>>
>>>>>
>>>>>  WidgetWin::OnKeyFoo
>>>>>>   input_method_->OnKeyEvent(..);
>>>>>>
>>>>>
>>>>>  InputMethod::OnKeyEvent
>>>>>>   ...
>>>>>>   widget_->PostProcess(...)
>>>>>>
>>>>>
>>>>>  WidgetWin::PostProcess
>>>>>>   GetRootView()->OnKeyEvent(..);
>>>>>>
>>>>>
>>>>>  This isn't asynchronous, it's just more complex code :-)
>>>>>>
>>>>>
>>>>>  How is the input method going to actually deal with delayed input
>>>>>> method
>>>>>> response from extensions? How does it plan to block the key event from
>>>>>> being
>>>>>> processed by the root view?
>>>>>>
>>>>> If we want to support input method extensions on Windows,
>>>>> InputMethodWin will
>>>>> need to dispatch a key event to the input method extension
>>>>> asynchronously
>>>>> without waiting for the reply, and call DispatchKeyEventPostIME() when
>>>>> receiving
>>>>> the reply from the extension.
>>>>>
>>>>>
>>>>>  -Ben
>>>>>>
>>>>>
>>>>>  On Fri, Apr 15, 2011 at 12:19 PM, <mailto:msw@chromium.org> wrote:
>>>>>>
>>>>>
>>>>>  > On 2011/04/15 18:23:19, Ben Goodger wrote:
>>>>>> >
>>>>>> >> I am really liking where this change is going Mike. A few comments:
>>>>>> >>
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>>
>>>>>
>>>>>
>>>>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>>>>
>>>>>> >
>>>>>> >> File views/ime/input_method_delegate.h (right):
>>>>>> >>
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>>
>>>>>
>>>>>
>>>>>
http://codereview.chromium.org/6823055/diff/12023/views/ime/input_method_dele...
>>>>>
>>>>>> >
>>>>>> >> views/ime/input_method_delegate.h:22: virtual bool
>>>>>> >>
>>>>>> > DispatchKeyEventPostIME(const
>>>>>> >
>>>>>> >> KeyEvent& key) = 0;
>>>>>> >> Do you even still need this function?
>>>>>> >>
>>>>>> >
>>>>>> >  I was thinking InputMethod::OnKeyEvent would return a bool
>>>>>> indicating
>>>>>> >> whether
>>>>>> >>
>>>>>> > or
>>>>>> >
>>>>>> >> not the calling Widget should process further... which is
>>>>>> effectively what
>>>>>> >>
>>>>>> > this
>>>>>> >
>>>>>> >> function is doing. If you remove this then you can delete the whole
>>>>>> >> delegate
>>>>>> >> interface.
>>>>>> >>
>>>>>> >
>>>>>> >
>>>>>> http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc
>>>>>> >> File views/widget/widget.cc (right):
>>>>>> >>
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>>
>>>>>
>>>>>
>>>>>
http://codereview.chromium.org/6823055/diff/12023/views/widget/widget.cc#newc...
>>>>>
>>>>>> >
>>>>>> >> views/widget/widget.cc:336: DispatchKeyEventPostIME(event);
>>>>>> >> If you get rid of the delegate interface this whole function
>>>>>> becomes:
>>>>>> >>
>>>>>> >
>>>>>> >  InputMethod* input_method = GetInputMethod();
>>>>>> >> if (input_method && input_method->OnKeyEvent(event))
>>>>>> >>   return GetRootView()->OnKeyEvent(event);
>>>>>> >> return false;
>>>>>> >>
>>>>>> >
>>>>>> >  ... and you can delete DispatchKeyEventPostIME from this class.
>>>>>> >>
>>>>>> >
>>>>>> > I'm still trying to grok the code 100% in the hopes that I can
>>>>>> achieve your
>>>>>> > suggested pattern.
>>>>>> > Here is my e-mail with James; he suggests against eliminating the
>>>>>> delegate
>>>>>> > /
>>>>>> > DispachKeyEventPostIME:
>>>>>> >
>>>>>> > [James]: Maybe we can have a VC meeting sometime next week. Feel
>>>>>> free to
>>>>>> > book my
>>>>>> > time for your convenience.
>>>>>> > Before having more in-depth discussion, it'll be best if you could
>>>>>> give me
>>>>>> > some
>>>>>> > background of your refactoring work. E.g. what's your ultimate goal?
>>>>>> And
>>>>>> > what's
>>>>>> > the problem of the current code?
>>>>>> >
>>>>>> > 在 2011年4月15日 上午6:05，Michael Wasserman <msw@google.com>写道：
>>>>>> >
>>>>>> > [Mike]: I'm trying to better comprehend the necessity of our
>>>>>> asynchronous
>>>>>> > pattern surrounding KeyEvents on InputMethodGtk and InputMethodIBus.
>>>>>> > [James]: Because we are going to have an extension API to allow
>>>>>> third-party
>>>>>> > input method extensions, it's mandatory to have asynchronous
>>>>>> communication
>>>>>> > between these extensions and Chrome UI to avoid blocking UI thread.
>>>>>> It'll
>>>>>> > be
>>>>>> > true for ChromeOS and will probably be true for Windows if we want
>>>>>> to
>>>>>> > support
>>>>>> > the extension API on Windows as well.
>>>>>> > The current DispatchKeyEvent/DispachKeyEventPostIME design pattern
>>>>>> was
>>>>>> > actually
>>>>>> > borrowed from Android, which needs to deal with the same problem
>>>>>> (where
>>>>>> > input
>>>>>> > methods run out of application's process).
>>>>>> >
>>>>>> > [Mike]: Is it possible to synchronously handle our KeyEvents,
>>>>>> electing to
>>>>>> > ignore
>>>>>> > those destined for the IME? In principle it seems that conditional
>>>>>> logic
>>>>>> > (around
>>>>>> > [James]: I'm afraid that it's not possible, at least on ChromeOS for
>>>>>> now.
>>>>>> > In
>>>>>> > order to make sure input methods work as expected, we need to
>>>>>> dispatch key
>>>>>> > events to the input method before actually handling them. But the
>>>>>> input
>>>>>> > method
>>>>>> > can only decide if it'll hand a key event until it receives the
>>>>>> event and
>>>>>> > then
>>>>>> > asks the application to continue to handle it when necessary. It's
>>>>>> not
>>>>>> > possible
>>>>>> > to determine whether or not we should send a key event to the input
>>>>>> method
>>>>>> > instead of handling it by ourselves beforehand.
>>>>>> > In all existing desktop systems, including Windows, Mac and Linux,
>>>>>> the
>>>>>> > application will be simply blocked after sending a key event to the
>>>>>> input
>>>>>> > method
>>>>>> > until the input method returns the result. Though such model
>>>>>> simplifies the
>>>>>> > logic, it's not acceptable for Chrome, as we don't want to block UI
>>>>>> thread
>>>>>> > at
>>>>>> > all.
>>>>>> >
>>>>>> > [Mike]: the presence of an IME and the contents of the KeyEvent),
>>>>>> and (if
>>>>>> > truly
>>>>>> > needed) a callback disambiguated from OnKeyEvent (for the IME to
>>>>>> commit
>>>>>> > result_text to the InputMethod/Widget), ought to clean up some of
>>>>>> our code
>>>>>> > and
>>>>>> > eliminate the need for an InputMethodDelegate and
>>>>>> DispatchKeyEventPostIME.
>>>>>> > [James]: Actually I'd prefer to stick with the current
>>>>>> > DispatchKeyEvent/DispatchKeyEventPostIME pattern, because it's has
>>>>>> very
>>>>>> > clear
>>>>>> > behavior definition: Widget's OnKeyEvent() needs to do nothing
>>>>>> except for
>>>>>> > calling InputMethod::DispatchKeyEvent(), and all actual logics
>>>>>> should be
>>>>>> > implemented in InputMethodDelegate::DispachKeyEventPostIME(), which
>>>>>> doesn't
>>>>>> > need
>>>>>> > to care about whether or not the key event is sent back from the
>>>>>> input
>>>>>> > method
>>>>>> > asynchronously. And in fact, we can eliminate the check in
>>>>>> > Widget{Gtk|Win}::OnKeyEvent(), if we can ensure that |input_method_|
>>>>>> is
>>>>>> > always
>>>>>> > not NULL.
>>>>>> >
>>>>>> > [Mike]: Additionally, may I consolidate some cross-platform
>>>>>> InputMethod
>>>>>> > code
>>>>>> > into InputMethodBase?
>>>>>> > [James]: Yes. It's the purpose of InputMethodBase. I'm just
>>>>>> wondering which
>>>>>> > part
>>>>>> > you want to move?
>>>>>> >
>>>>>> > [Mike]:What are your thoughts, and do you have time to discuss this
>>>>>> in
>>>>>> > person or
>>>>>> > over IM again? Thanks again!
>>>>>> >
>>>>>> > Mike
>>>>>> >
>>>>>> >
>>>>>> > http://codereview.chromium.org/6823055/
>>>>>> >
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/6823055/
>>>>>
>>>>
>>>>
>>>
>>
>

James Su

I can't see much benefit of the changes to InputMethod and InputMethodDelegate interfaces. Can you ...

9 years, 8 months ago (2011-04-19 09:38:56 UTC) #17

Ben Goodger (Google)

If the extensions are Chrome extensions, then it makes sense that the code that dispatches ...

9 years, 8 months ago (2011-04-19 15:00:12 UTC) #18

suzhe

在 2011年4月19日下午11:00，Ben Goodger (Google) <ben@chromium.org>写道： > If the extensions are Chrome extensions, then it ...

9 years, 8 months ago (2011-04-19 15:34:57 UTC) #19

在 2011年4月19日 下午11:00，Ben Goodger (Google) <ben@chromium.org>写道：

> If the extensions are Chrome extensions, then it makes sense that the code
> that dispatches into them be cross platform too, no?
>
> What is the control flow?
>
> 1. Incoming native event
> 2. chrome extension
> 3. view hierarchy
>
> Where does native IME fit into this sequence?
>
We have several native IMEs on ChromeOS, which are connected to chrome
through ibus. AFAIK, the plan is to attach extension IMEs to ibus as well
along with those native IMEs, so that they can be managed and used in a
unified way. E.g. the user can select among all of them in one language menu
But on Windows, as we cannot control system IMEs, a different built-in
mechanism is expected in chrome to manage extension IMEs, so that the user
can use them when no system IME is activated.


>
> -Ben
>
>
> On Tue, Apr 19, 2011 at 2:38 AM, <suzhe@chromium.org> wrote:
>
>> I can't see much benefit of the changes to InputMethod and
>> InputMethodDelegate
>> interfaces. Can you please give me some more explanation about your
>> purpose and
>> goal?
>>
>> Some of my points after checking the CL roughly:
>> 1. It would be ok to add Widget::OnKeyEvent(), which dispatches key events
>> to
>> the input method.
>> 2. I'd prefer to implement InputMethodDelegate::DispatchKeyEventPostIME()
>> in
>> NativeWidget classes, because most of its code is platform dependent.
>> 3. If you want to avoid implementing
>> InputMethodDelegate::DispatchKeyEventPostIME() on Windows, we may use this
>> approach:
>> 1), Let InputMethod::DispatchKeyEvent() return a boolean indicating if no
>> further handling is required.
>> 2), Let InputMethodBase::DispatchKeyEventPostIME() return a boolean
>> indicating
>> if delegate_->DispatchKeyEventPostIME() has been called successfully.
>> 3), Return DispatchKeyEventPostIME() in InputMethodWin::DispatchKeyEvent()
>> and
>> InputMethodGtk::DispatchKeyEvent(), but always true in
>> InputMethodIBus::DispatchKeyEvent().
>> 4), Simply do not implement InputMethodDelegate in WidgetWin and handle
>> key
>> event as normal.
>>
>> But I'd rather to keep the current InputMethod and InputMethodDelegate
>> interface
>> unchanged to keep the code consistent across platforms.
>>
>>
>> http://codereview.chromium.org/6823055/
>>
>
>

Ben Goodger (Google)

Where does the native IME translation happen on ChromeOS - before the native event reaches ...

9 years, 8 months ago (2011-04-19 15:45:51 UTC) #20

Where does the native IME translation happen on ChromeOS - before the native
event reaches the widget?

-Ben

On Tue, Apr 19, 2011 at 8:34 AM, James Su <suzhe@google.com> wrote:

>
>
> 在 2011年4月19日 下午11:00，Ben Goodger (Google) <ben@chromium.org>写道：
>
> If the extensions are Chrome extensions, then it makes sense that the code
>> that dispatches into them be cross platform too, no?
>>
>> What is the control flow?
>>
>> 1. Incoming native event
>> 2. chrome extension
>> 3. view hierarchy
>>
>> Where does native IME fit into this sequence?
>>
> We have several native IMEs on ChromeOS, which are connected to chrome
> through ibus. AFAIK, the plan is to attach extension IMEs to ibus as well
> along with those native IMEs, so that they can be managed and used in a
> unified way. E.g. the user can select among all of them in one language menu
> But on Windows, as we cannot control system IMEs, a different built-in
> mechanism is expected in chrome to manage extension IMEs, so that the user
> can use them when no system IME is activated.
>
>
>>
>> -Ben
>>
>>
>> On Tue, Apr 19, 2011 at 2:38 AM, <suzhe@chromium.org> wrote:
>>
>>> I can't see much benefit of the changes to InputMethod and
>>> InputMethodDelegate
>>> interfaces. Can you please give me some more explanation about your
>>> purpose and
>>> goal?
>>>
>>> Some of my points after checking the CL roughly:
>>> 1. It would be ok to add Widget::OnKeyEvent(), which dispatches key
>>> events to
>>> the input method.
>>> 2. I'd prefer to implement InputMethodDelegate::DispatchKeyEventPostIME()
>>> in
>>> NativeWidget classes, because most of its code is platform dependent.
>>> 3. If you want to avoid implementing
>>> InputMethodDelegate::DispatchKeyEventPostIME() on Windows, we may use
>>> this
>>> approach:
>>> 1), Let InputMethod::DispatchKeyEvent() return a boolean indicating if no
>>> further handling is required.
>>> 2), Let InputMethodBase::DispatchKeyEventPostIME() return a boolean
>>> indicating
>>> if delegate_->DispatchKeyEventPostIME() has been called successfully.
>>> 3), Return DispatchKeyEventPostIME() in
>>> InputMethodWin::DispatchKeyEvent() and
>>> InputMethodGtk::DispatchKeyEvent(), but always true in
>>> InputMethodIBus::DispatchKeyEvent().
>>> 4), Simply do not implement InputMethodDelegate in WidgetWin and handle
>>> key
>>> event as normal.
>>>
>>> But I'd rather to keep the current InputMethod and InputMethodDelegate
>>> interface
>>> unchanged to keep the code consistent across platforms.
>>>
>>>
>>> http://codereview.chromium.org/6823055/
>>>
>>
>>
>

Ben Goodger (Google)

Where does the native IME translation happen on ChromeOS - before the native event reaches ...

9 years, 8 months ago (2011-04-19 15:47:54 UTC) #21

Where does the native IME translation happen on ChromeOS - before the native
event reaches the widget?

-Ben

On Tue, Apr 19, 2011 at 8:34 AM, James Su <suzhe@google.com> wrote:

>
>
> 在 2011年4月19日 下午11:00，Ben Goodger (Google) <ben@chromium.org>写道：
>
> If the extensions are Chrome extensions, then it makes sense that the code
>> that dispatches into them be cross platform too, no?
>>
>> What is the control flow?
>>
>> 1. Incoming native event
>> 2. chrome extension
>> 3. view hierarchy
>>
>> Where does native IME fit into this sequence?
>>
> We have several native IMEs on ChromeOS, which are connected to chrome
> through ibus. AFAIK, the plan is to attach extension IMEs to ibus as well
> along with those native IMEs, so that they can be managed and used in a
> unified way. E.g. the user can select among all of them in one language menu
> But on Windows, as we cannot control system IMEs, a different built-in
> mechanism is expected in chrome to manage extension IMEs, so that the user
> can use them when no system IME is activated.
>
>
>>
>> -Ben
>>
>>
>> On Tue, Apr 19, 2011 at 2:38 AM, <suzhe@chromium.org> wrote:
>>
>>> I can't see much benefit of the changes to InputMethod and
>>> InputMethodDelegate
>>> interfaces. Can you please give me some more explanation about your
>>> purpose and
>>> goal?
>>>
>>> Some of my points after checking the CL roughly:
>>> 1. It would be ok to add Widget::OnKeyEvent(), which dispatches key
>>> events to
>>> the input method.
>>> 2. I'd prefer to implement InputMethodDelegate::DispatchKeyEventPostIME()
>>> in
>>> NativeWidget classes, because most of its code is platform dependent.
>>> 3. If you want to avoid implementing
>>> InputMethodDelegate::DispatchKeyEventPostIME() on Windows, we may use
>>> this
>>> approach:
>>> 1), Let InputMethod::DispatchKeyEvent() return a boolean indicating if no
>>> further handling is required.
>>> 2), Let InputMethodBase::DispatchKeyEventPostIME() return a boolean
>>> indicating
>>> if delegate_->DispatchKeyEventPostIME() has been called successfully.
>>> 3), Return DispatchKeyEventPostIME() in
>>> InputMethodWin::DispatchKeyEvent() and
>>> InputMethodGtk::DispatchKeyEvent(), but always true in
>>> InputMethodIBus::DispatchKeyEvent().
>>> 4), Simply do not implement InputMethodDelegate in WidgetWin and handle
>>> key
>>> event as normal.
>>>
>>> But I'd rather to keep the current InputMethod and InputMethodDelegate
>>> interface
>>> unchanged to keep the code consistent across platforms.
>>>
>>>
>>> http://codereview.chromium.org/6823055/
>>>
>>
>>
>

suzhe

在 2011年4月19日下午11:45，Ben Goodger (Google) <ben@chromium.org>写道： > Where does the native IME translation happen on ...

9 years, 8 months ago (2011-04-19 16:03:45 UTC) #22

在 2011年4月19日 下午11:45，Ben Goodger (Google) <ben@chromium.org>写道：

> Where does the native IME translation happen on ChromeOS - before the
> native event reaches the widget?

Key events reach the native widget first and then are dispatched to the
native IME through ibus in InputMethodIBus. But before reaching the native
IME, ibus itself may intercept a key event as a global hotkey, such as
alt-shift for switching to the next IME in the language menu. An IME may
also register its special hotkey to ibus, so that it can be activated by
ibus when the hotkey is pressed. E.g. a Korean IME may register Hangul key
as its hotkey. Such mechanism should be applied to extension IMEs as well.
Besides key event, ibus is also in charge of the system candidate window and
the language menu, an IME needs to use these UIs through ibus.

Things are different on Windows, which dispatches all key events to the
system active IME before reaching the application, so we actually can only
use extension IMEs when no system IME is activated. A mechanism similar than
ibus could be built into InputMethodWin directly to manage all extension
IMEs, provide candidate window and language menu UIs, and dispatch key
events.


>
> -Ben
>
>
> On Tue, Apr 19, 2011 at 8:34 AM, James Su <suzhe@google.com> wrote:
>
>>
>>
>> 在 2011年4月19日 下午11:00，Ben Goodger (Google) <ben@chromium.org>写道：
>>
>> If the extensions are Chrome extensions, then it makes sense that the code
>>> that dispatches into them be cross platform too, no?
>>>
>>> What is the control flow?
>>>
>>> 1. Incoming native event
>>> 2. chrome extension
>>> 3. view hierarchy
>>>
>>> Where does native IME fit into this sequence?
>>>
>> We have several native IMEs on ChromeOS, which are connected to chrome
>> through ibus. AFAIK, the plan is to attach extension IMEs to ibus as well
>> along with those native IMEs, so that they can be managed and used in a
>> unified way. E.g. the user can select among all of them in one language menu
>> But on Windows, as we cannot control system IMEs, a different built-in
>> mechanism is expected in chrome to manage extension IMEs, so that the user
>> can use them when no system IME is activated.
>>
>>
>>>
>>> -Ben
>>>
>>>
>>> On Tue, Apr 19, 2011 at 2:38 AM, <suzhe@chromium.org> wrote:
>>>
>>>> I can't see much benefit of the changes to InputMethod and
>>>> InputMethodDelegate
>>>> interfaces. Can you please give me some more explanation about your
>>>> purpose and
>>>> goal?
>>>>
>>>> Some of my points after checking the CL roughly:
>>>> 1. It would be ok to add Widget::OnKeyEvent(), which dispatches key
>>>> events to
>>>> the input method.
>>>> 2. I'd prefer to implement
>>>> InputMethodDelegate::DispatchKeyEventPostIME() in
>>>> NativeWidget classes, because most of its code is platform dependent.
>>>> 3. If you want to avoid implementing
>>>> InputMethodDelegate::DispatchKeyEventPostIME() on Windows, we may use
>>>> this
>>>> approach:
>>>> 1), Let InputMethod::DispatchKeyEvent() return a boolean indicating if
>>>> no
>>>> further handling is required.
>>>> 2), Let InputMethodBase::DispatchKeyEventPostIME() return a boolean
>>>> indicating
>>>> if delegate_->DispatchKeyEventPostIME() has been called successfully.
>>>> 3), Return DispatchKeyEventPostIME() in
>>>> InputMethodWin::DispatchKeyEvent() and
>>>> InputMethodGtk::DispatchKeyEvent(), but always true in
>>>> InputMethodIBus::DispatchKeyEvent().
>>>> 4), Simply do not implement InputMethodDelegate in WidgetWin and handle
>>>> key
>>>> event as normal.
>>>>
>>>> But I'd rather to keep the current InputMethod and InputMethodDelegate
>>>> interface
>>>> unchanged to keep the code consistent across platforms.
>>>>
>>>>
>>>> http://codereview.chromium.org/6823055/
>>>>
>>>
>>>
>>
>

Ben Goodger (Google)

I guess my main question is - is there a good reason to have cross ...

9 years, 8 months ago (2011-04-19 16:26:50 UTC) #23

I guess my main question is - is there a good reason to have cross platform
functionality like chrome extension input methods hook up differently
between Windows and ChromeOS? In general I would like ChromeOS and Windows
to be as similar as possible. This means we can test and fix bugs in both
places more easily.

-Ben

On Tue, Apr 19, 2011 at 9:03 AM, James Su <suzhe@google.com> wrote:

>
>
> 在 2011年4月19日 下午11:45，Ben Goodger (Google) <ben@chromium.org>写道：
>
> Where does the native IME translation happen on ChromeOS - before the
>> native event reaches the widget?
>
> Key events reach the native widget first and then are dispatched to the
> native IME through ibus in InputMethodIBus. But before reaching the native
> IME, ibus itself may intercept a key event as a global hotkey, such as
> alt-shift for switching to the next IME in the language menu. An IME may
> also register its special hotkey to ibus, so that it can be activated by
> ibus when the hotkey is pressed. E.g. a Korean IME may register Hangul key
> as its hotkey. Such mechanism should be applied to extension IMEs as well.
> Besides key event, ibus is also in charge of the system candidate window
> and the language menu, an IME needs to use these UIs through ibus.
>
> Things are different on Windows, which dispatches all key events to the
> system active IME before reaching the application, so we actually can only
> use extension IMEs when no system IME is activated. A mechanism similar than
> ibus could be built into InputMethodWin directly to manage all extension
> IMEs, provide candidate window and language menu UIs, and dispatch key
> events.
>
>
>>
>> -Ben
>>
>>
>> On Tue, Apr 19, 2011 at 8:34 AM, James Su <suzhe@google.com> wrote:
>>
>>>
>>>
>>> 在 2011年4月19日 下午11:00，Ben Goodger (Google) <ben@chromium.org>写道：
>>>
>>> If the extensions are Chrome extensions, then it makes sense that the
>>>> code that dispatches into them be cross platform too, no?
>>>>
>>>> What is the control flow?
>>>>
>>>> 1. Incoming native event
>>>> 2. chrome extension
>>>> 3. view hierarchy
>>>>
>>>> Where does native IME fit into this sequence?
>>>>
>>> We have several native IMEs on ChromeOS, which are connected to chrome
>>> through ibus. AFAIK, the plan is to attach extension IMEs to ibus as well
>>> along with those native IMEs, so that they can be managed and used in a
>>> unified way. E.g. the user can select among all of them in one language menu
>>> But on Windows, as we cannot control system IMEs, a different built-in
>>> mechanism is expected in chrome to manage extension IMEs, so that the user
>>> can use them when no system IME is activated.
>>>
>>>
>>>>
>>>> -Ben
>>>>
>>>>
>>>> On Tue, Apr 19, 2011 at 2:38 AM, <suzhe@chromium.org> wrote:
>>>>
>>>>> I can't see much benefit of the changes to InputMethod and
>>>>> InputMethodDelegate
>>>>> interfaces. Can you please give me some more explanation about your
>>>>> purpose and
>>>>> goal?
>>>>>
>>>>> Some of my points after checking the CL roughly:
>>>>> 1. It would be ok to add Widget::OnKeyEvent(), which dispatches key
>>>>> events to
>>>>> the input method.
>>>>> 2. I'd prefer to implement
>>>>> InputMethodDelegate::DispatchKeyEventPostIME() in
>>>>> NativeWidget classes, because most of its code is platform dependent.
>>>>> 3. If you want to avoid implementing
>>>>> InputMethodDelegate::DispatchKeyEventPostIME() on Windows, we may use
>>>>> this
>>>>> approach:
>>>>> 1), Let InputMethod::DispatchKeyEvent() return a boolean indicating if
>>>>> no
>>>>> further handling is required.
>>>>> 2), Let InputMethodBase::DispatchKeyEventPostIME() return a boolean
>>>>> indicating
>>>>> if delegate_->DispatchKeyEventPostIME() has been called successfully.
>>>>> 3), Return DispatchKeyEventPostIME() in
>>>>> InputMethodWin::DispatchKeyEvent() and
>>>>> InputMethodGtk::DispatchKeyEvent(), but always true in
>>>>> InputMethodIBus::DispatchKeyEvent().
>>>>> 4), Simply do not implement InputMethodDelegate in WidgetWin and handle
>>>>> key
>>>>> event as normal.
>>>>>
>>>>> But I'd rather to keep the current InputMethod and InputMethodDelegate
>>>>> interface
>>>>> unchanged to keep the code consistent across platforms.
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/6823055/
>>>>>
>>>>
>>>>
>>>
>>
>

suzhe

在 2011年4月20日上午12:26，Ben Goodger (Google) <ben@chromium.org>写道： > I guess my main question is - is ...

9 years, 8 months ago (2011-04-20 01:37:35 UTC) #24

在 2011年4月20日 上午12:26，Ben Goodger (Google) <ben@chromium.org>写道：

> I guess my main question is - is there a good reason to have cross platform
> functionality like chrome extension input methods hook up differently
> between Windows and ChromeOS? In general I would like ChromeOS and Windows
> to be as similar as possible. This means we can test and fix bugs in both
> places more easily.

I think the hookup code itself could be divided into several layers, most of
them should be cross platform. There could be an abstract layer isolating
the platform difference, but I don't think it's possible to make the whole
part cross platform, as their key event dispatching and system IME
management mechanisms are different.


>
> -Ben
>
>
> On Tue, Apr 19, 2011 at 9:03 AM, James Su <suzhe@google.com> wrote:
>
>>
>>
>> 在 2011年4月19日 下午11:45，Ben Goodger (Google) <ben@chromium.org>写道：
>>
>> Where does the native IME translation happen on ChromeOS - before the
>>> native event reaches the widget?
>>
>> Key events reach the native widget first and then are dispatched to the
>> native IME through ibus in InputMethodIBus. But before reaching the native
>> IME, ibus itself may intercept a key event as a global hotkey, such as
>> alt-shift for switching to the next IME in the language menu. An IME may
>> also register its special hotkey to ibus, so that it can be activated by
>> ibus when the hotkey is pressed. E.g. a Korean IME may register Hangul key
>> as its hotkey. Such mechanism should be applied to extension IMEs as well.
>> Besides key event, ibus is also in charge of the system candidate window
>> and the language menu, an IME needs to use these UIs through ibus.
>>
>> Things are different on Windows, which dispatches all key events to the
>> system active IME before reaching the application, so we actually can only
>> use extension IMEs when no system IME is activated. A mechanism similar than
>> ibus could be built into InputMethodWin directly to manage all extension
>> IMEs, provide candidate window and language menu UIs, and dispatch key
>> events.
>>
>>
>>>
>>> -Ben
>>>
>>>
>>> On Tue, Apr 19, 2011 at 8:34 AM, James Su <suzhe@google.com> wrote:
>>>
>>>>
>>>>
>>>> 在 2011年4月19日 下午11:00，Ben Goodger (Google) <ben@chromium.org>写道：
>>>>
>>>> If the extensions are Chrome extensions, then it makes sense that the
>>>>> code that dispatches into them be cross platform too, no?
>>>>>
>>>>> What is the control flow?
>>>>>
>>>>> 1. Incoming native event
>>>>> 2. chrome extension
>>>>> 3. view hierarchy
>>>>>
>>>>> Where does native IME fit into this sequence?
>>>>>
>>>> We have several native IMEs on ChromeOS, which are connected to chrome
>>>> through ibus. AFAIK, the plan is to attach extension IMEs to ibus as well
>>>> along with those native IMEs, so that they can be managed and used in a
>>>> unified way. E.g. the user can select among all of them in one language
menu
>>>> But on Windows, as we cannot control system IMEs, a different built-in
>>>> mechanism is expected in chrome to manage extension IMEs, so that the user
>>>> can use them when no system IME is activated.
>>>>
>>>>
>>>>>
>>>>> -Ben
>>>>>
>>>>>
>>>>> On Tue, Apr 19, 2011 at 2:38 AM, <suzhe@chromium.org> wrote:
>>>>>
>>>>>> I can't see much benefit of the changes to InputMethod and
>>>>>> InputMethodDelegate
>>>>>> interfaces. Can you please give me some more explanation about your
>>>>>> purpose and
>>>>>> goal?
>>>>>>
>>>>>> Some of my points after checking the CL roughly:
>>>>>> 1. It would be ok to add Widget::OnKeyEvent(), which dispatches key
>>>>>> events to
>>>>>> the input method.
>>>>>> 2. I'd prefer to implement
>>>>>> InputMethodDelegate::DispatchKeyEventPostIME() in
>>>>>> NativeWidget classes, because most of its code is platform dependent.
>>>>>> 3. If you want to avoid implementing
>>>>>> InputMethodDelegate::DispatchKeyEventPostIME() on Windows, we may use
>>>>>> this
>>>>>> approach:
>>>>>> 1), Let InputMethod::DispatchKeyEvent() return a boolean indicating if
>>>>>> no
>>>>>> further handling is required.
>>>>>> 2), Let InputMethodBase::DispatchKeyEventPostIME() return a boolean
>>>>>> indicating
>>>>>> if delegate_->DispatchKeyEventPostIME() has been called successfully.
>>>>>> 3), Return DispatchKeyEventPostIME() in
>>>>>> InputMethodWin::DispatchKeyEvent() and
>>>>>> InputMethodGtk::DispatchKeyEvent(), but always true in
>>>>>> InputMethodIBus::DispatchKeyEvent().
>>>>>> 4), Simply do not implement InputMethodDelegate in WidgetWin and
>>>>>> handle key
>>>>>> event as normal.
>>>>>>
>>>>>> But I'd rather to keep the current InputMethod and InputMethodDelegate
>>>>>> interface
>>>>>> unchanged to keep the code consistent across platforms.
>>>>>>
>>>>>>
>>>>>> http://codereview.chromium.org/6823055/
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

Ben Goodger (Google)

OK. I am OK with isolating the IME stuff in the native widget for now. ...

9 years, 8 months ago (2011-04-20 15:08:22 UTC) #25

OK. I am OK with isolating the IME stuff in the native widget for now. I do
want the final callback into cross platform Widget to be called OnKeyEvent
however. The Widget should receive a post-processed key event. Mike - make
sense?

-Ben

On Tue, Apr 19, 2011 at 6:29 PM, James Su <suzhe@google.com> wrote:

>
>
> 在 2011年4月20日 上午12:26，Ben Goodger (Google) <ben@chromium.org>写道：
>
> I guess my main question is - is there a good reason to have cross platform
>> functionality like chrome extension input methods hook up differently
>> between Windows and ChromeOS? In general I would like ChromeOS and Windows
>> to be as similar as possible. This means we can test and fix bugs in both
>> places more easily.
>
> I think the hookup code itself could be divided into several layers, most
> of them should be cross platform. There could be an abstract layer isolating
> the platform difference, but I don't think it's possible to make the whole
> part cross platform, as their key event dispatching and system IME
> management mechanisms are different.
>
>
>>
>> -Ben
>>
>>
>> On Tue, Apr 19, 2011 at 9:03 AM, James Su <suzhe@google.com> wrote:
>>
>>>
>>>
>>> 在 2011年4月19日 下午11:45，Ben Goodger (Google) <ben@chromium.org>写道：
>>>
>>> Where does the native IME translation happen on ChromeOS - before the
>>>> native event reaches the widget?
>>>
>>> Key events reach the native widget first and then are dispatched to the
>>> native IME through ibus in InputMethodIBus. But before reaching the native
>>> IME, ibus itself may intercept a key event as a global hotkey, such as
>>> alt-shift for switching to the next IME in the language menu. An IME may
>>> also register its special hotkey to ibus, so that it can be activated by
>>> ibus when the hotkey is pressed. E.g. a Korean IME may register Hangul key
>>> as its hotkey. Such mechanism should be applied to extension IMEs as well.
>>> Besides key event, ibus is also in charge of the system candidate window
>>> and the language menu, an IME needs to use these UIs through ibus.
>>>
>>> Things are different on Windows, which dispatches all key events to the
>>> system active IME before reaching the application, so we actually can only
>>> use extension IMEs when no system IME is activated. A mechanism similar than
>>> ibus could be built into InputMethodWin directly to manage all extension
>>> IMEs, provide candidate window and language menu UIs, and dispatch key
>>> events.
>>>
>>>
>>>>
>>>> -Ben
>>>>
>>>>
>>>> On Tue, Apr 19, 2011 at 8:34 AM, James Su <suzhe@google.com> wrote:
>>>>
>>>>>
>>>>>
>>>>> 在 2011年4月19日 下午11:00，Ben Goodger (Google) <ben@chromium.org>写道：
>>>>>
>>>>> If the extensions are Chrome extensions, then it makes sense that the
>>>>>> code that dispatches into them be cross platform too, no?
>>>>>>
>>>>>> What is the control flow?
>>>>>>
>>>>>> 1. Incoming native event
>>>>>> 2. chrome extension
>>>>>> 3. view hierarchy
>>>>>>
>>>>>> Where does native IME fit into this sequence?
>>>>>>
>>>>> We have several native IMEs on ChromeOS, which are connected to chrome
>>>>> through ibus. AFAIK, the plan is to attach extension IMEs to ibus as well
>>>>> along with those native IMEs, so that they can be managed and used in a
>>>>> unified way. E.g. the user can select among all of them in one language
menu
>>>>> But on Windows, as we cannot control system IMEs, a different built-in
>>>>> mechanism is expected in chrome to manage extension IMEs, so that the user
>>>>> can use them when no system IME is activated.
>>>>>
>>>>>
>>>>>>
>>>>>> -Ben
>>>>>>
>>>>>>
>>>>>> On Tue, Apr 19, 2011 at 2:38 AM, <suzhe@chromium.org> wrote:
>>>>>>
>>>>>>> I can't see much benefit of the changes to InputMethod and
>>>>>>> InputMethodDelegate
>>>>>>> interfaces. Can you please give me some more explanation about your
>>>>>>> purpose and
>>>>>>> goal?
>>>>>>>
>>>>>>> Some of my points after checking the CL roughly:
>>>>>>> 1. It would be ok to add Widget::OnKeyEvent(), which dispatches key
>>>>>>> events to
>>>>>>> the input method.
>>>>>>> 2. I'd prefer to implement
>>>>>>> InputMethodDelegate::DispatchKeyEventPostIME() in
>>>>>>> NativeWidget classes, because most of its code is platform dependent.
>>>>>>> 3. If you want to avoid implementing
>>>>>>> InputMethodDelegate::DispatchKeyEventPostIME() on Windows, we may use
>>>>>>> this
>>>>>>> approach:
>>>>>>> 1), Let InputMethod::DispatchKeyEvent() return a boolean indicating
>>>>>>> if no
>>>>>>> further handling is required.
>>>>>>> 2), Let InputMethodBase::DispatchKeyEventPostIME() return a boolean
>>>>>>> indicating
>>>>>>> if delegate_->DispatchKeyEventPostIME() has been called successfully.
>>>>>>> 3), Return DispatchKeyEventPostIME() in
>>>>>>> InputMethodWin::DispatchKeyEvent() and
>>>>>>> InputMethodGtk::DispatchKeyEvent(), but always true in
>>>>>>> InputMethodIBus::DispatchKeyEvent().
>>>>>>> 4), Simply do not implement InputMethodDelegate in WidgetWin and
>>>>>>> handle key
>>>>>>> event as normal.
>>>>>>>
>>>>>>> But I'd rather to keep the current InputMethod and
>>>>>>> InputMethodDelegate interface
>>>>>>> unchanged to keep the code consistent across platforms.
>>>>>>>
>>>>>>>
>>>>>>> http://codereview.chromium.org/6823055/
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

msw

This simpler change should capture our current goals; PTAL. Sadrul: please review accelerator_handler_touch.cc.

9 years, 8 months ago (2011-04-20 22:26:16 UTC) #26

Ben Goodger (Google)

http://codereview.chromium.org/6823055/diff/29008/chrome/browser/extensions/extension_input_api.cc File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6823055/diff/29008/chrome/browser/extensions/extension_input_api.cc#newcode118 chrome/browser/extensions/extension_input_api.cc:118: views::RootView* root_view = GetRootView(); RootView is going to become ...

9 years, 8 months ago (2011-04-20 23:12:05 UTC) #27

sadrul

On 2011/04/20 22:26:16, msw wrote: > This simpler change should capture our current goals; PTAL. ...

9 years, 8 months ago (2011-04-21 13:56:19 UTC) #28

msw

PTAL; thanks! http://codereview.chromium.org/6823055/diff/29008/chrome/browser/extensions/extension_input_api.cc File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6823055/diff/29008/chrome/browser/extensions/extension_input_api.cc#newcode118 chrome/browser/extensions/extension_input_api.cc:118: views::RootView* root_view = GetRootView(); On 2011/04/20 23:12:05, ...

9 years, 8 months ago (2011-04-21 19:19:24 UTC) #29

msw

Ping! I removed RootView use from extension_input_api and reverted the mouse and touch event changes ...

9 years, 8 months ago (2011-04-22 17:52:02 UTC) #30

Ben Goodger (Google)

LGTM http://codereview.chromium.org/6823055/diff/32001/chrome/browser/extensions/extension_input_api.h File chrome/browser/extensions/extension_input_api.h (right): http://codereview.chromium.org/6823055/diff/32001/chrome/browser/extensions/extension_input_api.h#newcode12 chrome/browser/extensions/extension_input_api.h:12: class Widget; nit: outdent 2 spaces

9 years, 8 months ago (2011-04-22 20:23:45 UTC) #31

msw

Just FYI; addressed your nit. http://codereview.chromium.org/6823055/diff/32001/chrome/browser/extensions/extension_input_api.h File chrome/browser/extensions/extension_input_api.h (right): http://codereview.chromium.org/6823055/diff/32001/chrome/browser/extensions/extension_input_api.h#newcode12 chrome/browser/extensions/extension_input_api.h:12: class Widget; On 2011/04/22 ...

9 years, 8 months ago (2011-04-22 21:34:38 UTC) #32

msw

Please take one more quick look, sorry! I fixed a Linux Views Clang error by ...

9 years, 8 months ago (2011-04-22 22:15:18 UTC) #33

msw

Third time's a charm! PTAL. Linux_ChromiumOS and ARM failures should now be fixed with updated ...

9 years, 8 months ago (2011-04-23 00:27:25 UTC) #35

suzhe

I'm out of office this week (offsite), will check this CL again next Monday. 在 ...

9 years, 8 months ago (2011-04-23 03:17:01 UTC) #36

James Su

LGTM, except one issue: http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_handler_touch.cc#newcode182 views/focus/accelerator_handler_touch.cc:182: return widget->OnKeyEvent(keyev); This change breaks ...

9 years, 8 months ago (2011-04-25 03:37:03 UTC) #37

James Su

http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_handler_touch.cc#newcode182 views/focus/accelerator_handler_touch.cc:182: return widget->OnKeyEvent(keyev); On 2011/04/25 03:37:03, James Su wrote: > ...

9 years, 8 months ago (2011-04-25 03:46:32 UTC) #38

msw

Issue addressed. PTAL, James. http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_handler_touch.cc#newcode182 views/focus/accelerator_handler_touch.cc:182: return widget->OnKeyEvent(keyev); On 2011/04/25 03:46:32, ...

9 years, 8 months ago (2011-04-25 17:52:38 UTC) #39

James Su

9 years, 8 months ago (2011-04-26 01:24:22 UTC) #40

Thanks, LGTM now.

On 2011/04/25 17:52:38, msw wrote:
> Issue addressed. PTAL, James.
> 
>
http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_han...
> File views/focus/accelerator_handler_touch.cc (right):
> 
>
http://codereview.chromium.org/6823055/diff/36008/views/focus/accelerator_han...
> views/focus/accelerator_handler_touch.cc:182: return
widget->OnKeyEvent(keyev);
> On 2011/04/25 03:46:32, James Su wrote:
> > On 2011/04/25 03:37:03, James Su wrote:
> > > This change breaks IME when using hardware keyboard.
> > 
> > To be more clear: we always need to dispatch a key event to the input method
> > before views hierarchy. Otherwise, even though most key events may be
> dispatched
> > to the input method through WidgetGtk::OnEventKey if
widget->OnKeyEvent(keyev)
> > returns false, functions of the input method may be broken if some keys are
> > handled in widget->OnKeyEvent(keyev).
> > And this change may cause a key event to be dispatched to views hierarchy
> twice.
> 
> Done. Logic restored with updated call to Widget.

Issue 6823055: Consolidate OnKeyEvent and InputMethod code. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix KeyEvent construction, InputMethod functions, etc. #

Patch Set 3 : Reduce the scope of this change. #

Patch Set 4 : Refactor X2Event handling, nix Windows InputMethodDelegate, etc. #

Patch Set 5 : Resore most InputMethod code. #

Patch Set 6 : Encapsulate RootView use to SendKeyboardEventInputFunction::GetTopLevelWidget. #

Patch Set 7 : Nix RootView from extension_input_api, revert mouse and touch event changes. #

Patch Set 8 : Rename WidgetGtk::OnEventKey to avoid overload with Widget::OnKeyEvent. #

Patch Set 9 : Update additional OnEventKey references. #

Patch Set 10 : Restore IME call in accelerator_handler_touch.cc. #

Messages