Issue 8576005: IME (input method editor) support for Aura, part 3 of 3: Use ui::InputMethod in ash.

Yusuke Sato

James, I've just finished implementing IME support for Aura following your design at http://codereview.chromium.org/8342026/. Looks ...

9 years, 1 month ago (2011-11-17 05:23:25 UTC) #1

James Su

Great! I'll review it asap. 在 2011年11月17日下午1:23， <yusukes@chromium.org>写道： > Reviewers: James Su, kinaba, Peng, ...

9 years, 1 month ago (2011-11-17 06:15:58 UTC) #2

Great! I'll review it asap.

在 2011年11月17日 下午1:23， <yusukes@chromium.org>写道：

> Reviewers: James Su, kinaba, Peng,
>
> Message:
> James,
> I've just finished implementing IME support for Aura following your design
> at
>
http://codereview.chromium.**org/8342026/<http://codereview.chromium.org/8342....
> Looks like it's working fine at least
> on Chrome OS and Linux. Can I ask you (and penghuang, kinaba) to review the
> change?
>
> If the CL is too big to review, I'll split it into 3 parts: ui/views/ime/*,
> ui/base/ime/*, and the rest.
>
>
> Description:
> Add IME (input method editor) support to Aura.
>
> The basic design of the feature is that to add an input method object to
> DesktopHost to feed all KeyPress and KeyRelease events from the system
> message
> loop to the input method, and let the input method send IME results (e.g.
> composition text) to RenderWidgetHostViewAura or NativeWidgetAura as
> needed.
>
> RenderWidgetHostViewAura:
> - Implement ui::TextInputClient interface so that RWHVA could receive an
> event
> like 'SetComposition' and 'InsertChar' from an input method object owned by
> DesktopHost.
> - Send a notification to the input method object on focus, blur, text
> input type
> change, etc.
> - OnKeyEvent() handler is now able to handle a non-native key event, e.g. a
> VKEY_PROCESSKEY event, which is fabricated by the input method editor.
>
> NativeWidgetAura:
> - Use views::InputMethodBridge instead of views::InputMethodIBus.
> - InputMethodBridge, which implements ui::TextInputClient interface, just
> receives IME results (e.g. composition text) from ui::InputMethod and
> forwards
> them to UI (e.g. a text field).
> - InputMethodBridge also receives a request like 'CancelComposition' from
> the UI
> and forwards the request to ui::InputMethod.
>
> DesktopHostLinux:
> - Creates and owns a ui::InputMethodIBusAura object. If IBus-1.4 is not
> available (e.g. Goobuntu), creates an instance of ui::MockInputMethod.
> - In Dispatch(), send a KeyPress and KeyRelease aura event to the input
> method.
> - Implement ui::InputMethodDelegate interface so that DesktopHostLinux
> could
> receive an key press and key release event which should be sent to NWA or
> RWHVA.
> Note that an "is_char" event is always sent directly from the input method
> to a
> TextInputClient. the ui::InputMethodDelegate is not used for that purpose.
> - ShouldSendCharEventForKeyboard**Code() is moved to ui::InputMethod since
> DesktopHostLinux no longer generates a Char event.
>
> DesktopHostWin:
> - IME is not supported yet.
>
> views::InputMethodBridge:
> - See the description above.
>
> ui::InputMethod:
> - An interface similar to views::InputMethod, but does not have Views
> dependency.
>
> ui::InputMethodIBus
> - Port of views::InputMethodIBus. Does not have Views dependency.
>
> ui::InputMethodIBusAura
> - IME implementation for Aura for Linux/ChromeOS.
> - Impelements aura::DesktopObserver to keep track of the active
> aura::Window.
>
>
> Supported platforms:
> - Aura + Chrome OS (IME works!)
> - Aura + Chrome OS + TOUCH_UI (compiles, but virtual keyboard is not shown
> since
> views::TextInputTypeTracker is not ported to Aura yet.)
> - Aura + Linux, with and without IBus-1.4
> - Aura + Windows (compiles, but IME does not work. views::InputMethodWin
> is not
> ported to Aura yet.)
>
> BUG=97261
> TEST=(writing unit tests for DesktopHostLinux using the new
> ui::MockInputMethod.)
>
>
> Please review this at
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M content/browser/renderer_host/**native_web_keyboard_event_**aura.cc
>  M content/browser/renderer_host/**render_widget_host_view_aura.h
>  M content/browser/renderer_host/**render_widget_host_view_aura.**cc
>  M content/public/browser/native_**web_keyboard_event.h
>  M ui/aura/aura.gyp
>  M ui/aura/desktop.h
>  M ui/aura/desktop.cc
>  M ui/aura/desktop_host.h
>  M ui/aura/desktop_host_linux.cc
>  M ui/aura/desktop_host_win.h
>  M ui/aura/desktop_host_win.cc
>  M ui/aura/event.h
>  A ui/base/ime/input_method.h
>  A ui/base/ime/input_method.cc
>  A ui/base/ime/input_method_base.**h
>  A ui/base/ime/input_method_base.**cc
>  A ui/base/ime/input_method_**delegate.h
>  A + ui/base/ime/input_method_ibus.**h
>  A + ui/base/ime/input_method_ibus.**cc
>  A ui/base/ime/input_method_ibus_**aura.h
>  A ui/base/ime/input_method_ibus_**aura.cc
>  A ui/base/ime/mock_input_method.**h
>  A ui/base/ime/mock_input_method.**cc
>  M ui/base/keycodes/keyboard_**code_conversion.cc
>  M ui/ui.gyp
>  A ui/views/ime/input_method_**bridge.h
>  A ui/views/ime/input_method_**bridge.cc
>  M ui/views/ime/input_method_**ibus.h
>  M ui/views/ime/input_method_**ibus.cc
>  M views/views.gyp
>  M views/widget/native_widget_**aura.cc
>
>
>


-- 
- James Su

Yusuke Sato

Hi James, I know you're busy working on your main project, but do you have ...

9 years, 1 month ago (2011-11-21 01:27:53 UTC) #3

Hi James, I know you're busy working on your main project, but do you have any
ETA of the initial review of this change? This is actually one of the high
priority issues in Feature-Aura..

On 2011/11/17 06:15:58, James Su wrote:
> Great! I'll review it asap.
> 
> 在 2011年11月17日 下午1:23， <yusukes@chromium.org>写道：
> 
> > Reviewers: James Su, kinaba, Peng,
> >
> > Message:
> > James,
> > I've just finished implementing IME support for Aura following your design
> > at
> >
>
http://codereview.chromium.**org/8342026/%3Chttp://codereview.chromium.org/83...>.
> > Looks like it's working fine at least
> > on Chrome OS and Linux. Can I ask you (and penghuang, kinaba) to review the
> > change?
> >
> > If the CL is too big to review, I'll split it into 3 parts: ui/views/ime/*,
> > ui/base/ime/*, and the rest.
> >
> >
> > Description:
> > Add IME (input method editor) support to Aura.
> >
> > The basic design of the feature is that to add an input method object to
> > DesktopHost to feed all KeyPress and KeyRelease events from the system
> > message
> > loop to the input method, and let the input method send IME results (e.g.
> > composition text) to RenderWidgetHostViewAura or NativeWidgetAura as
> > needed.
> >
> > RenderWidgetHostViewAura:
> > - Implement ui::TextInputClient interface so that RWHVA could receive an
> > event
> > like 'SetComposition' and 'InsertChar' from an input method object owned by
> > DesktopHost.
> > - Send a notification to the input method object on focus, blur, text
> > input type
> > change, etc.
> > - OnKeyEvent() handler is now able to handle a non-native key event, e.g. a
> > VKEY_PROCESSKEY event, which is fabricated by the input method editor.
> >
> > NativeWidgetAura:
> > - Use views::InputMethodBridge instead of views::InputMethodIBus.
> > - InputMethodBridge, which implements ui::TextInputClient interface, just
> > receives IME results (e.g. composition text) from ui::InputMethod and
> > forwards
> > them to UI (e.g. a text field).
> > - InputMethodBridge also receives a request like 'CancelComposition' from
> > the UI
> > and forwards the request to ui::InputMethod.
> >
> > DesktopHostLinux:
> > - Creates and owns a ui::InputMethodIBusAura object. If IBus-1.4 is not
> > available (e.g. Goobuntu), creates an instance of ui::MockInputMethod.
> > - In Dispatch(), send a KeyPress and KeyRelease aura event to the input
> > method.
> > - Implement ui::InputMethodDelegate interface so that DesktopHostLinux
> > could
> > receive an key press and key release event which should be sent to NWA or
> > RWHVA.
> > Note that an "is_char" event is always sent directly from the input method
> > to a
> > TextInputClient. the ui::InputMethodDelegate is not used for that purpose.
> > - ShouldSendCharEventForKeyboard**Code() is moved to ui::InputMethod since
> > DesktopHostLinux no longer generates a Char event.
> >
> > DesktopHostWin:
> > - IME is not supported yet.
> >
> > views::InputMethodBridge:
> > - See the description above.
> >
> > ui::InputMethod:
> > - An interface similar to views::InputMethod, but does not have Views
> > dependency.
> >
> > ui::InputMethodIBus
> > - Port of views::InputMethodIBus. Does not have Views dependency.
> >
> > ui::InputMethodIBusAura
> > - IME implementation for Aura for Linux/ChromeOS.
> > - Impelements aura::DesktopObserver to keep track of the active
> > aura::Window.
> >
> >
> > Supported platforms:
> > - Aura + Chrome OS (IME works!)
> > - Aura + Chrome OS + TOUCH_UI (compiles, but virtual keyboard is not shown
> > since
> > views::TextInputTypeTracker is not ported to Aura yet.)
> > - Aura + Linux, with and without IBus-1.4
> > - Aura + Windows (compiles, but IME does not work. views::InputMethodWin
> > is not
> > ported to Aura yet.)
> >
> > BUG=97261
> > TEST=(writing unit tests for DesktopHostLinux using the new
> > ui::MockInputMethod.)
> >
> >
> > Please review this at
>
http://codereview.chromium.**org/8576005/%3Chttp://codereview.chromium.org/85...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >  M content/browser/renderer_host/**native_web_keyboard_event_**aura.cc
> >  M content/browser/renderer_host/**render_widget_host_view_aura.h
> >  M content/browser/renderer_host/**render_widget_host_view_aura.**cc
> >  M content/public/browser/native_**web_keyboard_event.h
> >  M ui/aura/aura.gyp
> >  M ui/aura/desktop.h
> >  M ui/aura/desktop.cc
> >  M ui/aura/desktop_host.h
> >  M ui/aura/desktop_host_linux.cc
> >  M ui/aura/desktop_host_win.h
> >  M ui/aura/desktop_host_win.cc
> >  M ui/aura/event.h
> >  A ui/base/ime/input_method.h
> >  A ui/base/ime/input_method.cc
> >  A ui/base/ime/input_method_base.**h
> >  A ui/base/ime/input_method_base.**cc
> >  A ui/base/ime/input_method_**delegate.h
> >  A + ui/base/ime/input_method_ibus.**h
> >  A + ui/base/ime/input_method_ibus.**cc
> >  A ui/base/ime/input_method_ibus_**aura.h
> >  A ui/base/ime/input_method_ibus_**aura.cc
> >  A ui/base/ime/mock_input_method.**h
> >  A ui/base/ime/mock_input_method.**cc
> >  M ui/base/keycodes/keyboard_**code_conversion.cc
> >  M ui/ui.gyp
> >  A ui/views/ime/input_method_**bridge.h
> >  A ui/views/ime/input_method_**bridge.cc
> >  M ui/views/ime/input_method_**ibus.h
> >  M ui/views/ime/input_method_**ibus.cc
> >  M views/views.gyp
> >  M views/widget/native_widget_**aura.cc
> >
> >
> >
> 
> 
> -- 
> - James Su

James Su

Sorry for delay. I'm reviewing it. Will give you initial feedback today. 在 2011年11月21日上午9:27， ...

9 years, 1 month ago (2011-11-21 02:32:06 UTC) #4

Sorry for delay. I'm reviewing it. Will give you initial feedback today.

在 2011年11月21日 上午9:27， <yusukes@chromium.org>写道：

> Hi James, I know you're busy working on your main project, but do you have
> any
> ETA of the initial review of this change? This is actually one of the high
> priority issues in Feature-Aura..
>
>
> On 2011/11/17 06:15:58, James Su wrote:
>
>> Great! I'll review it asap.
>>
>
>  在 2011年11月17日 下午1:23， <yusukes@chromium.org>写道：
>>
>
>  > Reviewers: James Su, kinaba, Peng,
>> >
>> > Message:
>> > James,
>> > I've just finished implementing IME support for Aura following your
>> design
>> > at
>> >
>>
>
> http://codereview.chromium.****org/8342026/%3Chttp://coderevi**
> ew.chromium.org/8342026/ <http://codereview.chromium.org/8342026/>>.
>
>> > Looks like it's working fine at least
>> > on Chrome OS and Linux. Can I ask you (and penghuang, kinaba) to review
>> the
>> > change?
>> >
>> > If the CL is too big to review, I'll split it into 3 parts:
>> ui/views/ime/*,
>> > ui/base/ime/*, and the rest.
>> >
>> >
>> > Description:
>> > Add IME (input method editor) support to Aura.
>> >
>> > The basic design of the feature is that to add an input method object to
>> > DesktopHost to feed all KeyPress and KeyRelease events from the system
>> > message
>> > loop to the input method, and let the input method send IME results
>> (e.g.
>> > composition text) to RenderWidgetHostViewAura or NativeWidgetAura as
>> > needed.
>> >
>> > RenderWidgetHostViewAura:
>> > - Implement ui::TextInputClient interface so that RWHVA could receive an
>> > event
>> > like 'SetComposition' and 'InsertChar' from an input method object
>> owned by
>> > DesktopHost.
>> > - Send a notification to the input method object on focus, blur, text
>> > input type
>> > change, etc.
>> > - OnKeyEvent() handler is now able to handle a non-native key event,
>> e.g. a
>> > VKEY_PROCESSKEY event, which is fabricated by the input method editor.
>> >
>> > NativeWidgetAura:
>> > - Use views::InputMethodBridge instead of views::InputMethodIBus.
>> > - InputMethodBridge, which implements ui::TextInputClient interface,
>> just
>> > receives IME results (e.g. composition text) from ui::InputMethod and
>> > forwards
>> > them to UI (e.g. a text field).
>> > - InputMethodBridge also receives a request like 'CancelComposition'
>> from
>> > the UI
>> > and forwards the request to ui::InputMethod.
>> >
>> > DesktopHostLinux:
>> > - Creates and owns a ui::InputMethodIBusAura object. If IBus-1.4 is not
>> > available (e.g. Goobuntu), creates an instance of ui::MockInputMethod.
>> > - In Dispatch(), send a KeyPress and KeyRelease aura event to the input
>> > method.
>> > - Implement ui::InputMethodDelegate interface so that DesktopHostLinux
>> > could
>> > receive an key press and key release event which should be sent to NWA
>> or
>> > RWHVA.
>> > Note that an "is_char" event is always sent directly from the input
>> method
>> > to a
>> > TextInputClient. the ui::InputMethodDelegate is not used for that
>> purpose.
>> > - ShouldSendCharEventForKeyboard****Code() is moved to ui::InputMethod
>> since
>>
>> > DesktopHostLinux no longer generates a Char event.
>> >
>> > DesktopHostWin:
>> > - IME is not supported yet.
>> >
>> > views::InputMethodBridge:
>> > - See the description above.
>> >
>> > ui::InputMethod:
>> > - An interface similar to views::InputMethod, but does not have Views
>> > dependency.
>> >
>> > ui::InputMethodIBus
>> > - Port of views::InputMethodIBus. Does not have Views dependency.
>> >
>> > ui::InputMethodIBusAura
>> > - IME implementation for Aura for Linux/ChromeOS.
>> > - Impelements aura::DesktopObserver to keep track of the active
>> > aura::Window.
>> >
>> >
>> > Supported platforms:
>> > - Aura + Chrome OS (IME works!)
>> > - Aura + Chrome OS + TOUCH_UI (compiles, but virtual keyboard is not
>> shown
>> > since
>> > views::TextInputTypeTracker is not ported to Aura yet.)
>> > - Aura + Linux, with and without IBus-1.4
>> > - Aura + Windows (compiles, but IME does not work. views::InputMethodWin
>> > is not
>> > ported to Aura yet.)
>> >
>> > BUG=97261
>> > TEST=(writing unit tests for DesktopHostLinux using the new
>> > ui::MockInputMethod.)
>> >
>> >
>> > Please review this at
>>
>
> http://codereview.chromium.****org/8576005/%3Chttp://coderevi**
> ew.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
>
>> >
>> > SVN Base:
>>
>
>
svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src>
>
<http://svn.**chromium.org/chrome/trunk/src<http://svn.chromium.org/chrome/tru...
> >
>
>> >
>> > Affected files:
>> >  M content/browser/renderer_host/****native_web_keyboard_event_****
>> aura.cc
>> >  M content/browser/renderer_host/****render_widget_host_view_**aura.h
>> >  M content/browser/renderer_host/****render_widget_host_view_**
>> aura.**cc
>> >  M content/public/browser/native_****web_keyboard_event.h
>>
>> >  M ui/aura/aura.gyp
>> >  M ui/aura/desktop.h
>> >  M ui/aura/desktop.cc
>> >  M ui/aura/desktop_host.h
>> >  M ui/aura/desktop_host_linux.cc
>> >  M ui/aura/desktop_host_win.h
>> >  M ui/aura/desktop_host_win.cc
>> >  M ui/aura/event.h
>> >  A ui/base/ime/input_method.h
>> >  A ui/base/ime/input_method.cc
>> >  A ui/base/ime/input_method_base.****h
>> >  A ui/base/ime/input_method_base.****cc
>> >  A ui/base/ime/input_method_****delegate.h
>> >  A + ui/base/ime/input_method_ibus.****h
>> >  A + ui/base/ime/input_method_ibus.****cc
>> >  A ui/base/ime/input_method_ibus_****aura.h
>> >  A ui/base/ime/input_method_ibus_****aura.cc
>> >  A ui/base/ime/mock_input_method.****h
>> >  A ui/base/ime/mock_input_method.****cc
>> >  M ui/base/keycodes/keyboard_****code_conversion.cc
>> >  M ui/ui.gyp
>> >  A ui/views/ime/input_method_****bridge.h
>> >  A ui/views/ime/input_method_****bridge.cc
>> >  M ui/views/ime/input_method_****ibus.h
>> >  M ui/views/ime/input_method_****ibus.cc
>> >  M views/views.gyp
>> >  M views/widget/native_widget_****aura.cc
>> >
>> >
>> >
>>
>
>
>  --
>> - James Su
>>
>
>
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>



-- 
- James Su

Yusuke Sato

Thanks a lot! On 2011/11/21 02:32:06, James Su wrote: > Sorry for delay. I'm reviewing ...

9 years, 1 month ago (2011-11-21 03:06:45 UTC) #5

Thanks a lot!

On 2011/11/21 02:32:06, James Su wrote:
> Sorry for delay. I'm reviewing it. Will give you initial feedback today.
> 
> 在 2011年11月21日 上午9:27， <yusukes@chromium.org>写道：
> 
> > Hi James, I know you're busy working on your main project, but do you have
> > any
> > ETA of the initial review of this change? This is actually one of the high
> > priority issues in Feature-Aura..
> >
> >
> > On 2011/11/17 06:15:58, James Su wrote:
> >
> >> Great! I'll review it asap.
> >>
> >
> >  在 2011年11月17日 下午1:23， <yusukes@chromium.org>写道：
> >>
> >
> >  > Reviewers: James Su, kinaba, Peng,
> >> >
> >> > Message:
> >> > James,
> >> > I've just finished implementing IME support for Aura following your
> >> design
> >> > at
> >> >
> >>
> >
> > http://codereview.chromium.****org/8342026/%253Chttp://coderevi**
> > ew.chromium.org/8342026/ <http://codereview.chromium.org/8342026/>>.
> >
> >> > Looks like it's working fine at least
> >> > on Chrome OS and Linux. Can I ask you (and penghuang, kinaba) to review
> >> the
> >> > change?
> >> >
> >> > If the CL is too big to review, I'll split it into 3 parts:
> >> ui/views/ime/*,
> >> > ui/base/ime/*, and the rest.
> >> >
> >> >
> >> > Description:
> >> > Add IME (input method editor) support to Aura.
> >> >
> >> > The basic design of the feature is that to add an input method object to
> >> > DesktopHost to feed all KeyPress and KeyRelease events from the system
> >> > message
> >> > loop to the input method, and let the input method send IME results
> >> (e.g.
> >> > composition text) to RenderWidgetHostViewAura or NativeWidgetAura as
> >> > needed.
> >> >
> >> > RenderWidgetHostViewAura:
> >> > - Implement ui::TextInputClient interface so that RWHVA could receive an
> >> > event
> >> > like 'SetComposition' and 'InsertChar' from an input method object
> >> owned by
> >> > DesktopHost.
> >> > - Send a notification to the input method object on focus, blur, text
> >> > input type
> >> > change, etc.
> >> > - OnKeyEvent() handler is now able to handle a non-native key event,
> >> e.g. a
> >> > VKEY_PROCESSKEY event, which is fabricated by the input method editor.
> >> >
> >> > NativeWidgetAura:
> >> > - Use views::InputMethodBridge instead of views::InputMethodIBus.
> >> > - InputMethodBridge, which implements ui::TextInputClient interface,
> >> just
> >> > receives IME results (e.g. composition text) from ui::InputMethod and
> >> > forwards
> >> > them to UI (e.g. a text field).
> >> > - InputMethodBridge also receives a request like 'CancelComposition'
> >> from
> >> > the UI
> >> > and forwards the request to ui::InputMethod.
> >> >
> >> > DesktopHostLinux:
> >> > - Creates and owns a ui::InputMethodIBusAura object. If IBus-1.4 is not
> >> > available (e.g. Goobuntu), creates an instance of ui::MockInputMethod.
> >> > - In Dispatch(), send a KeyPress and KeyRelease aura event to the input
> >> > method.
> >> > - Implement ui::InputMethodDelegate interface so that DesktopHostLinux
> >> > could
> >> > receive an key press and key release event which should be sent to NWA
> >> or
> >> > RWHVA.
> >> > Note that an "is_char" event is always sent directly from the input
> >> method
> >> > to a
> >> > TextInputClient. the ui::InputMethodDelegate is not used for that
> >> purpose.
> >> > - ShouldSendCharEventForKeyboard****Code() is moved to ui::InputMethod
> >> since
> >>
> >> > DesktopHostLinux no longer generates a Char event.
> >> >
> >> > DesktopHostWin:
> >> > - IME is not supported yet.
> >> >
> >> > views::InputMethodBridge:
> >> > - See the description above.
> >> >
> >> > ui::InputMethod:
> >> > - An interface similar to views::InputMethod, but does not have Views
> >> > dependency.
> >> >
> >> > ui::InputMethodIBus
> >> > - Port of views::InputMethodIBus. Does not have Views dependency.
> >> >
> >> > ui::InputMethodIBusAura
> >> > - IME implementation for Aura for Linux/ChromeOS.
> >> > - Impelements aura::DesktopObserver to keep track of the active
> >> > aura::Window.
> >> >
> >> >
> >> > Supported platforms:
> >> > - Aura + Chrome OS (IME works!)
> >> > - Aura + Chrome OS + TOUCH_UI (compiles, but virtual keyboard is not
> >> shown
> >> > since
> >> > views::TextInputTypeTracker is not ported to Aura yet.)
> >> > - Aura + Linux, with and without IBus-1.4
> >> > - Aura + Windows (compiles, but IME does not work. views::InputMethodWin
> >> > is not
> >> > ported to Aura yet.)
> >> >
> >> > BUG=97261
> >> > TEST=(writing unit tests for DesktopHostLinux using the new
> >> > ui::MockInputMethod.)
> >> >
> >> >
> >> > Please review this at
> >>
> >
> > http://codereview.chromium.****org/8576005/%253Chttp://coderevi**
> > ew.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
> >
> >> >
> >> > SVN Base:
> >>
> >
> >
>
svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src>
> >
>
<http://svn.**chromium.org/chrome/trunk/src%3Chttp://svn.chromium.org/chrome/t...>
> > >
> >
> >> >
> >> > Affected files:
> >> >  M content/browser/renderer_host/****native_web_keyboard_event_****
> >> aura.cc
> >> >  M content/browser/renderer_host/****render_widget_host_view_**aura.h
> >> >  M content/browser/renderer_host/****render_widget_host_view_**
> >> aura.**cc
> >> >  M content/public/browser/native_****web_keyboard_event.h
> >>
> >> >  M ui/aura/aura.gyp
> >> >  M ui/aura/desktop.h
> >> >  M ui/aura/desktop.cc
> >> >  M ui/aura/desktop_host.h
> >> >  M ui/aura/desktop_host_linux.cc
> >> >  M ui/aura/desktop_host_win.h
> >> >  M ui/aura/desktop_host_win.cc
> >> >  M ui/aura/event.h
> >> >  A ui/base/ime/input_method.h
> >> >  A ui/base/ime/input_method.cc
> >> >  A ui/base/ime/input_method_base.****h
> >> >  A ui/base/ime/input_method_base.****cc
> >> >  A ui/base/ime/input_method_****delegate.h
> >> >  A + ui/base/ime/input_method_ibus.****h
> >> >  A + ui/base/ime/input_method_ibus.****cc
> >> >  A ui/base/ime/input_method_ibus_****aura.h
> >> >  A ui/base/ime/input_method_ibus_****aura.cc
> >> >  A ui/base/ime/mock_input_method.****h
> >> >  A ui/base/ime/mock_input_method.****cc
> >> >  M ui/base/keycodes/keyboard_****code_conversion.cc
> >> >  M ui/ui.gyp
> >> >  A ui/views/ime/input_method_****bridge.h
> >> >  A ui/views/ime/input_method_****bridge.cc
> >> >  M ui/views/ime/input_method_****ibus.h
> >> >  M ui/views/ime/input_method_****ibus.cc
> >> >  M views/views.gyp
> >> >  M views/widget/native_widget_****aura.cc
> >> >
> >> >
> >> >
> >>
> >
> >
> >  --
> >> - James Su
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/8576005/%3Chttp://codereview.chromium.org/85...>
> >
> 
> 
> 
> -- 
> - James Su

James Su

I may need some time to give you thorough feedback for such a huge CL. ...

9 years, 1 month ago (2011-11-21 03:35:59 UTC) #6

James Su

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h File ui/base/ime/input_method.h (right): http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#newcode62 ui/base/ime/input_method.h:62: virtual void set_text_input_client(TextInputClient* client) = 0; How about rename ...

9 years, 1 month ago (2011-11-21 08:23:35 UTC) #7

oshima

On 2011/11/21 03:35:59, James Su wrote: > I may need some time to give you ...

9 years, 1 month ago (2011-11-21 08:34:26 UTC) #8

Yusuke Sato

Sure, I'll remove ui/aura/ dependency from ui/base/ime/ tomorrow. Patch Set #2 is the work-in-progress version ...

9 years, 1 month ago (2011-11-21 09:28:41 UTC) #9

Yusuke Sato

Fixed all. Now files under ui/base/ime/ have neither aura:: nor USE_AURA. Please take another look. ...

9 years, 1 month ago (2011-11-22 11:09:29 UTC) #10

Fixed all. Now files under ui/base/ime/ have neither aura:: nor USE_AURA. Please
take another look.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h
File ui/base/ime/input_method.h (right):

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:62: virtual void
set_text_input_client(TextInputClient* client) = 0;
On 2011/11/21 08:23:36, James Su wrote:
> How about rename it to SetFocusedTextInputClient() to emphasize that we are
> tracing the input focus per TextInputClient basis. The actual ui::InputMethod
> implementation may toggle the system input method state in this method.
> 
> In additional we may still need something similar than
> views::InputMethod::OnFocus()/OnBlur() to know if the toplevel native(system)
> window, where the ui::InputMethod object is attached to, is focused or not.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:65: virtual TextInputClient* text_input_client()
const = 0;
On 2011/11/21 08:23:36, James Su wrote:
> We may rename it to GetTextInputClient() as well.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:68: virtual void Init() = 0;
On 2011/11/21 08:23:36, James Su wrote:
> This method should have a parameter to tell this ui::InputMethod object which
> native(system) window should be attached to. But we can't use
gfx::NativeWindow,
> as it's typedefed to aura::Window when USE_AURA is defined, which is not what
we
> want.
> 
> Perhaps we may introduce a new header file similar than base/event_types.h for
> this purpose.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:74: virtual void DispatchKeyEvent(gfx::NativeEvent
native_key_event) = 0;
On 2011/11/21 08:23:36, James Su wrote:
> It's better to use the real system event type (base/event_types.h) to avoid
> depending on aura code.
> And it's better to use const reference here.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:81: virtual void
OnTextInputTypeChanged(gfx::NativeWindow window) = 0;
On 2011/11/21 08:23:36, James Su wrote:
> Using |window| as parameter makes no sense here. We can remove the parameter,
> indicating it's change of the currently focused TextInputClient. Or maybe we
can
> use ui::TextInputClient* focused_client instead, so that we can make sure it's
> called by the correct TextInputClient object.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:85: virtual void
OnCaretBoundsChanged(gfx::NativeWindow window) = 0;
On 2011/11/21 08:23:36, James Su wrote:
> Same as above method.
> The only problem is how to translate the caret bounds to desired coordinates
we
> want(usually it can be the native window we are attached to, then we can
> translate it to the whole screen when necessary).
> 
> One possible solution is to let the TextInputClient object's GetCaretBounds()
> method return the translated caret bounds for us. But then the definition of
> TextInputClient::GetCaretBounds method may be different for views::InputMethod
> and ui::InputMethod, though I don't think it's a big deal.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:90: virtual void CancelComposition(gfx::NativeWindow
window) = 0;
On 2011/11/21 08:23:36, James Su wrote:
> ditto.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method.h#n...
ui/base/ime/input_method.h:116: static bool
ShouldSendCharEventForKeyboardCode(KeyboardCode keycode);
On 2011/11/21 08:23:36, James Su wrote:
> I'm wondering why we need this method. On Linux, if a key press event
> corresponds to a valid character, then we always need to send the character.

Removed.
 
> btw, can you help rename ui::DefaultSymbolFromXEvent() defined in
> ui/base/keycodes/keyboard_code_conversion_x.h to something more reasonable?
e.g.
> ui::GetCharacterFromXEvent().

Added TODO.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method_base.h
File ui/base/ime/input_method_base.h (right):

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method_bas...
ui/base/ime/input_method_base.h:43: virtual bool
IsWindowFocused(gfx::NativeWindow window) const = 0;
On 2011/11/21 08:23:36, James Su wrote:
> Instead of introducing this method, I'd suggest to use the code similar than
> views::InputMethodBase::OnFocus()/OnBlur().
> 
> Here we need to track the focus state of the toplevel system window, which the
> input method is attached to, even if USE_AURA is defined.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method_bas...
ui/base/ime/input_method_base.h:46: void ActiveWindowChanged(gfx::NativeWindow
active);
On 2011/11/21 08:23:36, James Su wrote:
> This method is not necessary. Instead, the owner should call
> ui::InputMethod::OnFocus()/OnBlur() whenever the toplevel system window
> gains/loses keyboard focus.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method_del...
File ui/base/ime/input_method_delegate.h (right):

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method_del...
ui/base/ime/input_method_delegate.h:22: virtual void
DispatchKeyEventPostIME(gfx::NativeEvent native_key_event) = 0;
On 2011/11/21 08:23:36, James Su wrote:
> use const base::NativeEvent& instead.

Done.

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method_ibu...
File ui/base/ime/input_method_ibus_aura.h (right):

http://codereview.chromium.org/8576005/diff/2001/ui/base/ime/input_method_ibu...
ui/base/ime/input_method_ibus_aura.h:26: class UI_EXPORT InputMethodIBusAura :
public InputMethodIBus,
On 2011/11/21 08:23:36, James Su wrote:
> This class is not necessary, as we don't need to track the focus state of aura
> window.

Done. Removed.

http://codereview.chromium.org/8576005/diff/2001/ui/base/keycodes/keyboard_co...
File ui/base/keycodes/keyboard_code_conversion.cc (right):

http://codereview.chromium.org/8576005/diff/2001/ui/base/keycodes/keyboard_co...
ui/base/keycodes/keyboard_code_conversion.cc:61: return 0xE5;
On 2011/11/21 08:23:36, James Su wrote:
> Why do we need it?

I think this is necessary to allow RWHVA::OnKeyEvent() create a correct
NativeWebKeyboardEvent object (see the code snippet below). There might be a
better way to do so, though.

 617 #if defined(USE_X11)
 618     if (!event->native_event()) {
 619       // Send a fabricated event, which is usually a VKEY_PROCESSKEY IME
event.
 620       NativeWebKeyboardEvent webkit_event(event->type(),
 621                                           false /* is_char */,
 622                                           event->GetCharacter(),
 623                                           event->flags(),
 624                                           base::Time::Now().ToDoubleT());
 625       host_->ForwardKeyboardEvent(webkit_event);

http://codereview.chromium.org/8576005/diff/2001/views/widget/native_widget_a...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8576005/diff/2001/views/widget/native_widget_a...
views/widget/native_widget_aura.cc:28: #if defined(HAVE_IBUS)
On 2011/11/21 08:23:36, James Su wrote:
> do we still need this check?

If we use InputMethodBridge here, views_unittests will fail. It seems that the
unit tests call views::InputMethod's OnFocus() method twice (and therefore,
DCHECK in views::InputMethodBridge::OnFocus fails).

I believe this is a problem in the unit test. I'll file a bug for this issue
later and will put the bug number here.

http://codereview.chromium.org/8576005/diff/2001/views/widget/native_widget_a...
views/widget/native_widget_aura.cc:266: #if defined(HAVE_IBUS)
On 2011/11/21 08:23:36, James Su wrote:
> Do we still need this check?

ditto.

Peng

http://codereview.chromium.org/8576005/diff/16002/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/16002/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode489 content/browser/renderer_host/render_widget_host_view_aura.cc:489: aura::Window::ConvertPointToWindow(window_, desktop, &end); Is the desktop origin always same ...

9 years, 1 month ago (2011-11-22 16:26:26 UTC) #11

Yusuke Sato

http://codereview.chromium.org/8576005/diff/16002/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/16002/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode489 content/browser/renderer_host/render_widget_host_view_aura.cc:489: aura::Window::ConvertPointToWindow(window_, desktop, &end); On 2011/11/22 16:26:27, Peng wrote: > ...

9 years, 1 month ago (2011-11-23 15:42:47 UTC) #12

James Su

Below are feedback related to ui/base/ime/*. I'll review other parts asap. http://codereview.chromium.org/8576005/diff/29034/base/event_types.h File base/event_types.h (right): ...

9 years, 1 month ago (2011-11-24 03:57:02 UTC) #13

Yusuke Sato

Please take another look. http://codereview.chromium.org/8576005/diff/29034/base/event_types.h File base/event_types.h (right): http://codereview.chromium.org/8576005/diff/29034/base/event_types.h#newcode6 base/event_types.h:6: #define BASE_EVENT_TYPES_H On 2011/11/24 03:57:03, ...

9 years ago (2011-11-28 06:29:39 UTC) #14

James Su

Overall, LGTM except these style issues. Looking forward to CLs for those TODOs. http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_host/render_widget_host_view_aura.cc File ...

9 years ago (2011-11-29 04:25:53 UTC) #15

kinaba

LGTM from my side. On 2011/11/29 04:25:53, James Su wrote: > Overall, LGTM except these ...

9 years ago (2011-11-29 05:38:28 UTC) #16

Yusuke Sato

http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode240 content/browser/renderer_host/render_widget_host_view_aura.cc:240: // TODO(yusukes): Should we call ImeCancelComposition() here? On 2011/11/29 ...

9 years ago (2011-11-29 09:06:36 UTC) #17

James Su

http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode633 content/browser/renderer_host/render_widget_host_view_aura.cc:633: input_method->OnTextInputTypeChanged(this); On 2011/11/29 09:06:36, Yusuke Sato wrote: > Let ...

9 years ago (2011-11-30 01:39:22 UTC) #20

Yusuke Sato

http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode633 content/browser/renderer_host/render_widget_host_view_aura.cc:633: input_method->OnTextInputTypeChanged(this); sure, removed input_method->OnTextInputTypeChanged(this); from RWHVA and added some ...

9 years ago (2011-11-30 02:25:49 UTC) #21

http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8576005/diff/42001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:633:
input_method->OnTextInputTypeChanged(this);
sure, removed input_method->OnTextInputTypeChanged(this); from RWHVA and added
some code to ui::InputMethodBase::SetFocusedTextInputClient(). Please take
another look.

On 2011/11/30 01:39:23, James Su wrote:
> On 2011/11/29 09:06:36, Yusuke Sato wrote:
> > Let me keep the input_method->OnTextInputTypeChanged(this); call. Here is
the
> > reason why:
> > 
> > On 2011/11/29 04:25:53, James Su wrote:
> > > This should be called automatically. So we don't need to call
> > > ImeCancelComposition() either.
> > 
> > I tried to remove L.633 but it didn't work well. It turned out that
> > RWHVA::TextInputStateChanged() is not always called when RWHVA gets focused.
> > 
> > For example, in the following scenario, the IME candidate window remains
after
> > step 6.
> > 
> > 1. Switch to Pinyin IME
> > 2. navigate to http://www.google.com
> > 3. click the search box
> > 4. click Omnibox
> > 5. show the candidate window by typing some Chinese text
> > 6. before commiting the text, click the search box again
> > 
> > Expected:
> > The composition text is committed to Omnibox, and the candidate window is
> > hidden.
> > 
> > Actual:
> > The composition text is committed to Omnibox, but the candidate window is
> still
> > shown.
> > 
> > This is likely because RenderWidget::UpdateTextInputState() does not call
> > RWHVA's TextInputStateChanged() method when old and new text_input_type
> matches.
> > Furthermore, RWHVA::TextInputStateChanged() also has the same logic like 'if
> > (text_input_type_ != type) { ...'.
> 
> Then I think ui::InputMethod::SetFocusedTextInputClient() should do the trick,
> just like what we've done in views::InputMethodXXX::OnWillChangeFocus() and
> OnDidChangeFocus().

James Su

http://codereview.chromium.org/8576005/diff/53001/ui/base/ime/input_method_base.cc File ui/base/ime/input_method_base.cc (right): http://codereview.chromium.org/8576005/diff/53001/ui/base/ime/input_method_base.cc#newcode58 ui/base/ime/input_method_base.cc:58: if (!client) I think we still need to reset ...

9 years ago (2011-11-30 02:32:18 UTC) #22

Yusuke Sato

http://codereview.chromium.org/8576005/diff/53001/ui/base/ime/input_method_base.cc File ui/base/ime/input_method_base.cc (right): http://codereview.chromium.org/8576005/diff/53001/ui/base/ime/input_method_base.cc#newcode58 ui/base/ime/input_method_base.cc:58: if (!client) Added such functions. Please take another look ...

9 years ago (2011-11-30 04:23:08 UTC) #23

James Su

LGTM. Once you merge the latest change to the new separated CLs, I'll give LGTM ...

9 years ago (2011-11-30 04:41:39 UTC) #24

Yusuke Sato

Thanks. Update http://codereview.chromium.org/8659033/ . On 2011/11/30 04:41:39, James Su wrote: > LGTM. > Once you ...

9 years ago (2011-11-30 05:02:25 UTC) #25

Yusuke Sato

sky: This CL is the final piece for supporting IME on Aura. Could you review ...

9 years ago (2011-12-01 07:29:42 UTC) #27

James Su

LGTM. Just one question: do you expect the desktop_host_unittest.cc to be extended to cover more ...

9 years ago (2011-12-01 07:40:56 UTC) #28

Yusuke Sato

No immediate plan to add non-IME tests. Renamed it to desktop_host_ime_unittest.cc. On 2011/12/01 07:40:56, James ...

9 years ago (2011-12-01 08:00:06 UTC) #29

sky

http://codereview.chromium.org/8576005/diff/67001/content/browser/renderer_host/native_web_keyboard_event_aura.cc File content/browser/renderer_host/native_web_keyboard_event_aura.cc (right): http://codereview.chromium.org/8576005/diff/67001/content/browser/renderer_host/native_web_keyboard_event_aura.cc#newcode44 content/browser/renderer_host/native_web_keyboard_event_aura.cc:44: // From chrome/common/native_web_keyboard_event_views.cc. Is this comment right (meaning I ...

9 years ago (2011-12-01 17:01:09 UTC) #30

Yusuke Sato

Thanks. Please take another look. http://codereview.chromium.org/8576005/diff/67001/content/browser/renderer_host/native_web_keyboard_event_aura.cc File content/browser/renderer_host/native_web_keyboard_event_aura.cc (right): http://codereview.chromium.org/8576005/diff/67001/content/browser/renderer_host/native_web_keyboard_event_aura.cc#newcode44 content/browser/renderer_host/native_web_keyboard_event_aura.cc:44: // From chrome/common/native_web_keyboard_event_views.cc. On ...

9 years ago (2011-12-02 02:18:31 UTC) #31

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusukes@chromium.org/8576005/78001

9 years ago (2011-12-06 07:14:52 UTC) #33

commit-bot: I haz the power

Presubmit check for 8576005-78001 failed and returned exit status 1. Running presubmit commit checks ...

9 years ago (2011-12-06 07:15:00 UTC) #34

Yusuke Sato

+avi, +joi Could you review content/public/browser/native_web_keyboard_event.h? Thanks. On 2011/12/06 07:15:00, I haz the power (commit-bot) ...

9 years ago (2011-12-06 07:16:35 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusukes@chromium.org/8576005/78001

9 years ago (2011-12-06 12:30:22 UTC) #37

commit-bot: I haz the power

Can't apply patch for file ui/aura/desktop.h. While running patch -p1 --forward --force; patching file ui/aura/desktop.h ...

9 years ago (2011-12-06 12:30:28 UTC) #38

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusukes@chromium.org/8576005/80002

9 years ago (2011-12-06 14:39:56 UTC) #39

Ben Goodger (Google)

I disagree with the design of this change. As I have mentioned previously IME should ...

9 years ago (2011-12-06 20:34:01 UTC) #41

Ben Goodger (Google)

Let me provide some more specifics. Also, check out the new design overviews at http://dev.chromium.org/developers/design-documents/aura ...

9 years ago (2011-12-07 05:43:59 UTC) #42

James Su

Let me try to explain the problem: We can split the whole system related to ...

9 years ago (2011-12-07 06:51:24 UTC) #43

Let me try to explain the problem:

We can split the whole system related to IME into two parts:

1. The IME system itself that provides input method services, including the
language decoder, the candidate UI, etc.
This part can be further split into two separated parts: the backend server
(including IME engines, a.k.a language decoders) and the UI (including
candidate UI, the language icon on the status bar, the language menu, etc.).
The fact is that chrome itself doesn't have an built-in IME system. We
simply use the IME system provided by each platform. On Windows we use
IME32, on ChromeOS we use ibus. The difference is that we have our own
built-in IME UI on ChromeOS, but not on Windows.
I agree that for ChromeOS, we probably want to move our built-in IME UI
from chrome itself into aura_shell. And in the future, we'll probably want
to introduce a new IME system specially designed for aura desktop, but it's
not going to happen very soon. So for aura desktop, we still need to use
IME32 (or maybe TSF in the future) on Windows and ibus on ChromeOS. But we
probably can use the same built-in IME UI on both Windows (*) and ChromeOS.
But it's a different story.

(*) Windows (both IME32 and TSF) allows an application to provide
customized IME UI, but we probably want to use TSF instead to get better
functionality.

2. The client that makes use of the service provided by the IME system.
The code in ui/base/ime/*, that yusukes are working on, are actually the
client code of actual IME systems. More specifically, it's the wrapper
layer that abstracts the difference among different IME systems, to isolate
the chrome itself, the actual client, from the IME system. This part has
following facts:
1). It provides a platform independent API that can be used by chrome
itself to communicate with the actual IME system.
2). It supports the actual IME system by a wrapper layer (like
InputMethodWin), so we can provide new wrapper layers to support new IME
systems, without changing the client (chrome) code.
3). We need a central place for the client (chrome) to get the pointer to
the wrapper. In old views architecture, NativeWidget is that place. In aura
desktop, we think that the root window would be the most preferable place
for this purpose. Because one wrapper object is enough for the whole aura
desktop, and the root window object is the only top level object that's
accessible from every UI elements in the chrome (for views controls, it's
accessible through NativeWidget and views::InputMethodBridge). And I don't
think aura_shell is a good place for this purpose, as it's outside chrome's
scope.

Hope the problem can be clarified by above explanation.

Regards
James Su

在 2011年12月7日 下午1:43， <ben@chromium.org>写道：

> Let me provide some more specifics. Also, check out the new design
> overviews at
>
http://dev.chromium.org/**developers/design-documents/**aura<http://dev.chrom...
are intended to
> provide more context on the Aura design.
>
> A few notes:
>
> I don't believe IME is something that belongs in Aura itself. Aura is
> intentionally very simple and delegates most application-specific
> processing to
> its client, in this case Aura Shell.
>
> IME UI that you might show is going to have to play with the rest of the
> Aura
> UI, and as such can only be reasonably shown by the Aura Shell, which has
> the
> context about windows etc.
>
> The pattern is typically that the underlying OS sends base::NativeEvents
> to the
> RootWindowHost implementation, and that that forwards them to the
> RootWindow,
> and the RootWindow then sends them to the relevant target (after giving
> EventFilters an opportunity to intercept). The Shell registers several
> EventFilters for various containers/circumstances. I believe this pattern
> or a
> variant would be useful here.
>
> FInally, the Aura Shell is an implementation of the "ChromeOS product",
> which
> can actually also run on Windows. From a product perspective, this means
> 100% of
> the ChromeOS experience must be brought through this UI, including the
> ChromeOS
> input method system.  It is meaningless (and probably impossible) to run
> the
> Windows IME system in the ChromeOS desktop built in Aura Shell.
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>



-- 
- James Su

Yusuke Sato

First, I'm sorry for mischoosing the reviewers. Next time I'll ask you for a review ...

9 years ago (2011-12-07 15:30:28 UTC) #44

First, I'm sorry for mischoosing the reviewers. Next time I'll ask you for a
review as well.

> The Shell registers several EventFilters for various containers/circumstances.
I believe
> this pattern or a variant would be useful here.

Moving the key event handling code in this CL to somewhere in ui/aura_shell/,
probably toplevel_window_event_filter.cc, would be easy. However, as James
mentioned, we still need a single ui::InputMethod object which is accessible
from RWHVA and NWA so that they could tell the current text field type, caret
position, etc. to the input method object. Would it be acceptable to add a
member variable like 'ui::InputMethod* input_method_;' and its setter/getter to
the RootWindow class?

James:
Thanks for providing the detailed background information.

-Yusuke


On 2011/12/07 06:51:24, James Su wrote:
> Let me try to explain the problem:
> 
> We can split the whole system related to IME into two parts:
> 
> 1. The IME system itself that provides input method services, including the
> language decoder, the candidate UI, etc.
> This part can be further split into two separated parts: the backend server
> (including IME engines, a.k.a language decoders) and the UI (including
> candidate UI, the language icon on the status bar, the language menu, etc.).
> The fact is that chrome itself doesn't have an built-in IME system. We
> simply use the IME system provided by each platform. On Windows we use
> IME32, on ChromeOS we use ibus. The difference is that we have our own
> built-in IME UI on ChromeOS, but not on Windows.
> I agree that for ChromeOS, we probably want to move our built-in IME UI
> from chrome itself into aura_shell. And in the future, we'll probably want
> to introduce a new IME system specially designed for aura desktop, but it's
> not going to happen very soon. So for aura desktop, we still need to use
> IME32 (or maybe TSF in the future) on Windows and ibus on ChromeOS. But we
> probably can use the same built-in IME UI on both Windows (*) and ChromeOS.
> But it's a different story.
> 
> (*) Windows (both IME32 and TSF) allows an application to provide
> customized IME UI, but we probably want to use TSF instead to get better
> functionality.
> 
> 2. The client that makes use of the service provided by the IME system.
> The code in ui/base/ime/*, that yusukes are working on, are actually the
> client code of actual IME systems. More specifically, it's the wrapper
> layer that abstracts the difference among different IME systems, to isolate
> the chrome itself, the actual client, from the IME system. This part has
> following facts:
> 1). It provides a platform independent API that can be used by chrome
> itself to communicate with the actual IME system.
> 2). It supports the actual IME system by a wrapper layer (like
> InputMethodWin), so we can provide new wrapper layers to support new IME
> systems, without changing the client (chrome) code.
> 3). We need a central place for the client (chrome) to get the pointer to
> the wrapper. In old views architecture, NativeWidget is that place. In aura
> desktop, we think that the root window would be the most preferable place
> for this purpose. Because one wrapper object is enough for the whole aura
> desktop, and the root window object is the only top level object that's
> accessible from every UI elements in the chrome (for views controls, it's
> accessible through NativeWidget and views::InputMethodBridge). And I don't
> think aura_shell is a good place for this purpose, as it's outside chrome's
> scope.
> 
> Hope the problem can be clarified by above explanation.
> 
> Regards
> James Su
> 
> 在 2011年12月7日 下午1:43， <ben@chromium.org>写道：
> 
> > Let me provide some more specifics. Also, check out the new design
> > overviews at
> >
>
http://dev.chromium.org/**developers/design-documents/**aura%3Chttp://dev.chr...
> are intended to
> > provide more context on the Aura design.
> >
> > A few notes:
> >
> > I don't believe IME is something that belongs in Aura itself. Aura is
> > intentionally very simple and delegates most application-specific
> > processing to
> > its client, in this case Aura Shell.
> >
> > IME UI that you might show is going to have to play with the rest of the
> > Aura
> > UI, and as such can only be reasonably shown by the Aura Shell, which has
> > the
> > context about windows etc.
> >
> > The pattern is typically that the underlying OS sends base::NativeEvents
> > to the
> > RootWindowHost implementation, and that that forwards them to the
> > RootWindow,
> > and the RootWindow then sends them to the relevant target (after giving
> > EventFilters an opportunity to intercept). The Shell registers several
> > EventFilters for various containers/circumstances. I believe this pattern
> > or a
> > variant would be useful here.
> >
> > FInally, the Aura Shell is an implementation of the "ChromeOS product",
> > which
> > can actually also run on Windows. From a product perspective, this means
> > 100% of
> > the ChromeOS experience must be brought through this UI, including the
> > ChromeOS
> > input method system.  It is meaningless (and probably impossible) to run
> > the
> > Windows IME system in the ChromeOS desktop built in Aura Shell.
> >
> >
>
http://codereview.chromium.**org/8576005/%3Chttp://codereview.chromium.org/85...>
> >
> 
> 
> 
> -- 
> - James Su

Ben Goodger (Google)

2 questions: 1. How is the Chrome IME UI implemented? Using what frameworks? 2. Can ...

9 years ago (2011-12-07 17:04:40 UTC) #45

2 questions:

1. How is the Chrome IME UI implemented? Using what frameworks?
2. Can you describe the event flow from raw base::NativeEvent through to
insertion of relevant characters. My understanding from Windows is that
your app receives raw keydowns pre-composition and WM_CHAR post composition.

Second... we don't need to worry about supporting the ChromeOS IME on
Windows right away, since we don't have a product there yet. I just want to
make sure that the architecture is set up that we can implement it there,
rather than stubbing out for the Windows IME, which would not make sense in
an otherwise All-Chrome environment.

-Ben

On Wed, Dec 7, 2011 at 7:30 AM, <yusukes@chromium.org> wrote:

> First, I'm sorry for mischoosing the reviewers. Next time I'll ask you for
> a
> review as well.
>
>
>  The Shell registers several EventFilters for various
>> containers/circumstances.
>>
> I believe
>
>> this pattern or a variant would be useful here.
>>
>
> Moving the key event handling code in this CL to somewhere in
> ui/aura_shell/,
> probably toplevel_window_event_filter.**cc, would be easy. However, as
> James
> mentioned, we still need a single ui::InputMethod object which is
> accessible
> from RWHVA and NWA so that they could tell the current text field type,
> caret
> position, etc. to the input method object. Would it be acceptable to add a
> member variable like 'ui::InputMethod* input_method_;' and its
> setter/getter to
> the RootWindow class?
>
> James:
> Thanks for providing the detailed background information.
>
> -Yusuke
>
>
>
> On 2011/12/07 06:51:24, James Su wrote:
>
>> Let me try to explain the problem:
>>
>
>  We can split the whole system related to IME into two parts:
>>
>
>  1. The IME system itself that provides input method services, including
>> the
>> language decoder, the candidate UI, etc.
>> This part can be further split into two separated parts: the backend
>> server
>> (including IME engines, a.k.a language decoders) and the UI (including
>> candidate UI, the language icon on the status bar, the language menu,
>> etc.).
>> The fact is that chrome itself doesn't have an built-in IME system. We
>> simply use the IME system provided by each platform. On Windows we use
>> IME32, on ChromeOS we use ibus. The difference is that we have our own
>> built-in IME UI on ChromeOS, but not on Windows.
>> I agree that for ChromeOS, we probably want to move our built-in IME UI
>> from chrome itself into aura_shell. And in the future, we'll probably want
>> to introduce a new IME system specially designed for aura desktop, but
>> it's
>> not going to happen very soon. So for aura desktop, we still need to use
>> IME32 (or maybe TSF in the future) on Windows and ibus on ChromeOS. But we
>> probably can use the same built-in IME UI on both Windows (*) and
>> ChromeOS.
>> But it's a different story.
>>
>
>  (*) Windows (both IME32 and TSF) allows an application to provide
>> customized IME UI, but we probably want to use TSF instead to get better
>> functionality.
>>
>
>  2. The client that makes use of the service provided by the IME system.
>> The code in ui/base/ime/*, that yusukes are working on, are actually the
>> client code of actual IME systems. More specifically, it's the wrapper
>> layer that abstracts the difference among different IME systems, to
>> isolate
>> the chrome itself, the actual client, from the IME system. This part has
>> following facts:
>> 1). It provides a platform independent API that can be used by chrome
>> itself to communicate with the actual IME system.
>> 2). It supports the actual IME system by a wrapper layer (like
>> InputMethodWin), so we can provide new wrapper layers to support new IME
>> systems, without changing the client (chrome) code.
>> 3). We need a central place for the client (chrome) to get the pointer to
>> the wrapper. In old views architecture, NativeWidget is that place. In
>> aura
>> desktop, we think that the root window would be the most preferable place
>> for this purpose. Because one wrapper object is enough for the whole aura
>> desktop, and the root window object is the only top level object that's
>> accessible from every UI elements in the chrome (for views controls, it's
>> accessible through NativeWidget and views::InputMethodBridge). And I don't
>> think aura_shell is a good place for this purpose, as it's outside
>> chrome's
>> scope.
>>
>
>  Hope the problem can be clarified by above explanation.
>>
>
>  Regards
>> James Su
>>
>
>  在 2011年12月7日 下午1:43， <ben@chromium.org>写道：
>>
>
>  > Let me provide some more specifics. Also, check out the new design
>> > overviews at
>> >
>>
>
> http://dev.chromium.org/****developers/design-documents/****
> aura%3Chttp://dev.chromium.**org/developers/design-**
>
documents/aura%3Ewhich<http://dev.chromium.org/**developers/design-documents/**aura%3Chttp://dev.chromium.org/developers/design-documents/aura%3Ewhich>
>
>  are intended to
>> > provide more context on the Aura design.
>> >
>> > A few notes:
>> >
>> > I don't believe IME is something that belongs in Aura itself. Aura is
>> > intentionally very simple and delegates most application-specific
>> > processing to
>> > its client, in this case Aura Shell.
>> >
>> > IME UI that you might show is going to have to play with the rest of the
>> > Aura
>> > UI, and as such can only be reasonably shown by the Aura Shell, which
>> has
>> > the
>> > context about windows etc.
>> >
>> > The pattern is typically that the underlying OS sends base::NativeEvents
>> > to the
>> > RootWindowHost implementation, and that that forwards them to the
>> > RootWindow,
>> > and the RootWindow then sends them to the relevant target (after giving
>> > EventFilters an opportunity to intercept). The Shell registers several
>> > EventFilters for various containers/circumstances. I believe this
>> pattern
>> > or a
>> > variant would be useful here.
>> >
>> > FInally, the Aura Shell is an implementation of the "ChromeOS product",
>> > which
>> > can actually also run on Windows. From a product perspective, this means
>> > 100% of
>> > the ChromeOS experience must be brought through this UI, including the
>> > ChromeOS
>> > input method system.  It is meaningless (and probably impossible) to run
>> > the
>> > Windows IME system in the ChromeOS desktop built in Aura Shell.
>> >
>> >
>>
>
> http://codereview.chromium.****org/8576005/%3Chttp://coderevi**
> ew.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
>
>> >
>>
>
>
>
>  --
>> - James Su
>>
>
>
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>

Yusuke Sato

My answers are inlined: > 2 questions: > 1. How is the Chrome IME UI ...

9 years ago (2011-12-08 05:49:41 UTC) #46

My answers are inlined:

> 2 questions:
> 1. How is the Chrome IME UI implemented? Using what frameworks?

IME needs two UIs: one is a candidate window (http://goo.gl/GS1BX) which shows a
list of CJK characters that have e.g. the same reading. The other is a UI called
"language menu" or "language bar" (see screen shots in
http://en.wikipedia.org/wiki/Text_Services_Framework), which helps a user switch
the current input method (e.g. from Korean IME to English layout), or switch
internal modes of the current IME.  Let me explain about the internal state
switching a little bit more since it might be confusing. For example, on Korean
IME, a user might want to switch back and forth between its two internal modes;
Korean character (like '계산기') input mode and Chinese character (like '計算機')
input mode, and a language bar can help the user to do so.

Here is the implementation details of these two UIs:

* Chrome OS:

The short answer is Views. Here is the detail:

- candidate window

Chrome for Chrome OS shows a candidate window when the system IME service (i.e.
ibus-daemon process) notifies Chrome to do so. Please note that ibus-daemon
communicates with Chrome using D-Bus protocol just like other Chrome OS services
such as power/network. The candidate window is implemented as a TYPE_POPUP
views::Widget.

- language menu

As you know, Chrome for Chrome OS has several status buttons
(chrome/browser/chromeos/status/*.h) for power, network, time, and so on. The
language menu is also implemented as a status button.

Again, just like power/network buttons, the language menu button receives
notifications from ibus-daemon via D-Bus and shows appropriate information to
user.

* Windows and Mac (FYI):

- candidate window

Practically, every IME has its own candidate window and renders its own by
itself since system APIs like IMM32/TSF don't help an IME render a candidate
window. As a result, every candidate window has its own feature set and look &
feel. For example, Baidu's Japanese IME has a "theme" or "skin" like feature
while others don't. Apple's has two-dimensional candidate display mode while
others don't. Google's has a word-dictionary feature but others sometimes don't,
etc. etc.

- language menu

If I understand correctly, old Windows IMEs that rely on Windows' IMM32 API
render a language menu by themselves since IMM32 does not help an IME implement
such menu. Recent Windows IMEs that use Windows' TSF API and Mac IMEs use
system's API to render a menu.

> 2. Can you describe the event flow from raw base::NativeEvent through to
> insertion of relevant characters. My understanding from Windows is that
> your app receives raw keydowns pre-composition and WM_CHAR post composition.

Unlike Windows, on Chrome OS (and Linux) it is an application's responsibility
to communicate with an IME to get a post composition character. In other words,
Chrome can receive only raw keypress and keyrelease X events and has to send
them to ibus-daemon as needed. Therefore, when CJK IME is in use, the event flow
would be like this:

1. X sends a raw keypress event to RootWindowHost
2. RoowWindowHost forwards it to RootWindow
3. RootWindow sends it to an ui::InputMethod instance (likely via EventFilter),
which is ui::InputMethodIBus on Chrome OS.
4. ui::InputMethodIBus asynchronously sends the keypress event to the external
IME daemon process, ibus-daemon.
5. Return to the event loop.

(scenario A) if the IME didn't consume the key event.

6. ibus-daemon asynchronously sends a D-Bus signal 'not-consumed' to Chrome. A
callback function in ui::InputMethodIBus is called.
7. ui::InputMethodIBus calls ui::InputMethodDelegate::DispatchKeyEventPostIME()
with the unhandled X keypress event.
8. ui::InputMethodDelegate::DispatchKeyEventPostIME() implementation, which is
likely aura_shell::ToplevelWindowEventFilter, forwards the event to an
aura::Window which is currently focused.
9. ui::InputMethodIBus also calls ui::TextInputClient::InsertChar(), which is
implemented by RWHVA and NWA, to send a Char event.  (NWA ignores Char events
though.)

(scenario B) if the IME consumes the key event and generates a composition text.

6. ibus-daemon asynchronously sends a D-Bus signal 'update-preedit-text' with a
composition text (like 'あ') to Chrome. A callback function in
ui::InputMethodIBus is called.
7. ui::InputMethodIBus remembers the composition text.
8. Return to the event loop.
9. ibus-daemon asynchronously sends a D-Bus signal 'consumed' to Chrome. A
callback function in ui::InputMethodIBus is called again.
10. ui::InputMethodIBus calls ui::InputMethodDelegate::DispatchKeyEventPostIME()
with VKEY_PROCESSKEY fake keypress event. A focused aura::Window receives the
fake keypress event. 
11. Since ui::InputMethodIBus has a composition text, it also calls
ui::TextInputClient::SetCompositionText() to ask RWHVA or NWA to show the text.

Although ui::InputMethodIBus covers many other cases such as dead key
composition, I believe the two scenarios above would explain basic event flows.

Please note that when CJK IME is not in use (e.g. only US Qwerty, US Dvorak,
French Azerty, ... is in use), ui::InputMethodIBus does not perform any async
communications with the external IME daemon process. Instead, it calls
ui::InputMethodDelegate::DispatchKeyEventPostIME() immediately.

> Second... we don't need to worry about supporting the ChromeOS IME on
> Windows right away, since we don't have a product there yet. I just want to
> make sure that the architecture is set up that we can implement it there,
> rather than stubbing out for the Windows IME, which would not make sense in
> an otherwise All-Chrome environment.

I think there're two ways to support IME in Aura Shell for Windows.

-----------
1) Use native Windows IMEs in Aura Shell for WIndows

I believe this is the easiest way to support IME in Aura Shell for Windows,
although IME UX could be different from Aura Shell for Chrome OS. To do this,
what we have to implement would be as follows:

- Disable the language menu (status area button) for IME since we can use a
native language bar for each Windows IME.
- Likewise, disable the views::Widget candidate window.
- Implement ui::InputMethodWin, which is a thin wrapper of the system IME API,
and use it in Aura Shell.

2) Use Chrome OS IMEs in Aura Shell for Windows

Probably this is better from product perspective as you mentioned. To do this,
we have to do lots of things something like:

- Port all IMEs for Chrome OS, Simplified Chinese, Traditional Chinese,
Japanese, Korean, Vietnamese, and many more, to Windows.
- Implement a new ui::InputMethod class that could communicate with the new IMEs
above. Use it in Aura Shell.
- Modify the candidate window and language menu for Chrome OS so that they could
communicate with the new IMEs above.
-----------

The point here is that in both cases, we don't have to modify Aura Shell to
support IME on Windows since the actual IME implementation is hidden behind the
abstraction layer, ui::InputMethod.

Thanks,
Yusuke

On 2011/12/07 17:04:40, Ben Goodger (Google) wrote:
> 2 questions:
> 
> 1. How is the Chrome IME UI implemented? Using what frameworks?
> 2. Can you describe the event flow from raw base::NativeEvent through to
> insertion of relevant characters. My understanding from Windows is that
> your app receives raw keydowns pre-composition and WM_CHAR post composition.
> 
> Second... we don't need to worry about supporting the ChromeOS IME on
> Windows right away, since we don't have a product there yet. I just want to
> make sure that the architecture is set up that we can implement it there,
> rather than stubbing out for the Windows IME, which would not make sense in
> an otherwise All-Chrome environment.
> 
> -Ben

Ben Goodger (Google)

Who shows the candidate window? Are there any hidden X/Gtk dependencies? -Ben

9 years ago (2011-12-08 20:20:33 UTC) #47

Peng

The candidates window uses views, It is a part of chrome browser UI process. The ...

9 years ago (2011-12-08 20:59:31 UTC) #48

Yusuke Sato

Thanks Peng. - There's no X/Gtk dependency. - On Chrome OS, as he mentioned, Chrome ...

9 years ago (2011-12-08 23:02:40 UTC) #49

James Su

Please see my comment inline. On 2011/12/07 15:30:28, Yusuke Sato wrote: > First, I'm sorry ...

9 years ago (2011-12-09 03:34:56 UTC) #50

Please see my comment inline.

On 2011/12/07 15:30:28, Yusuke Sato wrote:
> First, I'm sorry for mischoosing the reviewers. Next time I'll ask you for a
> review as well.
> 
> > The Shell registers several EventFilters for various
containers/circumstances.
> I believe
> > this pattern or a variant would be useful here.
> 
> Moving the key event handling code in this CL to somewhere in ui/aura_shell/,
> probably toplevel_window_event_filter.cc, would be easy. However, as James
> mentioned, we still need a single ui::InputMethod object which is accessible
> from RWHVA and NWA so that they could tell the current text field type, caret
> position, etc. to the input method object. Would it be acceptable to add a
> member variable like 'ui::InputMethod* input_method_;' and its setter/getter
to
> the RootWindow class?

Besides the single ui::InputMethod object issue, I still think it's more
reasonable to add IME related event handling in RootWindowHost, because:
1. We want to let the IME to handle key events first before dispatching them to
other parts of the application, because:
a) Make sure IME's hotkeys have highest priority, to avoid surprise behavior for
IME users when there are hotkey conflicts between IME and other event filters.
b) Make sure IME's hotkeys can still work even without focused window.
c) It's the behavior on Windows, and we cannot change it. It's better to keep it
consistent on all platforms to reduce the code difference.

2. ibus handles keyevents asynchronously, RootWindowHost is the simplest place
to support this behavior. Because all upper level event handling APIs have
synchronous semantics, such as RootWindow::ProcessKeyEvent(),
EventFilter::PreHandleKeyEvent(), WindowDelegate::OnKeyEvent(), etc.


> 
> James:
> Thanks for providing the detailed background information.
> 
> -Yusuke
> 
> 
> On 2011/12/07 06:51:24, James Su wrote:
> > Let me try to explain the problem:
> > 
> > We can split the whole system related to IME into two parts:
> > 
> > 1. The IME system itself that provides input method services, including the
> > language decoder, the candidate UI, etc.
> > This part can be further split into two separated parts: the backend server
> > (including IME engines, a.k.a language decoders) and the UI (including
> > candidate UI, the language icon on the status bar, the language menu, etc.).
> > The fact is that chrome itself doesn't have an built-in IME system. We
> > simply use the IME system provided by each platform. On Windows we use
> > IME32, on ChromeOS we use ibus. The difference is that we have our own
> > built-in IME UI on ChromeOS, but not on Windows.
> > I agree that for ChromeOS, we probably want to move our built-in IME UI
> > from chrome itself into aura_shell. And in the future, we'll probably want
> > to introduce a new IME system specially designed for aura desktop, but it's
> > not going to happen very soon. So for aura desktop, we still need to use
> > IME32 (or maybe TSF in the future) on Windows and ibus on ChromeOS. But we
> > probably can use the same built-in IME UI on both Windows (*) and ChromeOS.
> > But it's a different story.
> > 
> > (*) Windows (both IME32 and TSF) allows an application to provide
> > customized IME UI, but we probably want to use TSF instead to get better
> > functionality.
> > 
> > 2. The client that makes use of the service provided by the IME system.
> > The code in ui/base/ime/*, that yusukes are working on, are actually the
> > client code of actual IME systems. More specifically, it's the wrapper
> > layer that abstracts the difference among different IME systems, to isolate
> > the chrome itself, the actual client, from the IME system. This part has
> > following facts:
> > 1). It provides a platform independent API that can be used by chrome
> > itself to communicate with the actual IME system.
> > 2). It supports the actual IME system by a wrapper layer (like
> > InputMethodWin), so we can provide new wrapper layers to support new IME
> > systems, without changing the client (chrome) code.
> > 3). We need a central place for the client (chrome) to get the pointer to
> > the wrapper. In old views architecture, NativeWidget is that place. In aura
> > desktop, we think that the root window would be the most preferable place
> > for this purpose. Because one wrapper object is enough for the whole aura
> > desktop, and the root window object is the only top level object that's
> > accessible from every UI elements in the chrome (for views controls, it's
> > accessible through NativeWidget and views::InputMethodBridge). And I don't
> > think aura_shell is a good place for this purpose, as it's outside chrome's
> > scope.
> > 
> > Hope the problem can be clarified by above explanation.
> > 
> > Regards
> > James Su
> > 
> > 在 2011年12月7日 下午1:43， <ben@chromium.org>写道：
> > 
> > > Let me provide some more specifics. Also, check out the new design
> > > overviews at
> > >
> >
>
http://dev.chromium.org/**developers/design-documents/**aura%253Chttp://dev.c...
> > are intended to
> > > provide more context on the Aura design.
> > >
> > > A few notes:
> > >
> > > I don't believe IME is something that belongs in Aura itself. Aura is
> > > intentionally very simple and delegates most application-specific
> > > processing to
> > > its client, in this case Aura Shell.
> > >
> > > IME UI that you might show is going to have to play with the rest of the
> > > Aura
> > > UI, and as such can only be reasonably shown by the Aura Shell, which has
> > > the
> > > context about windows etc.
> > >
> > > The pattern is typically that the underlying OS sends base::NativeEvents
> > > to the
> > > RootWindowHost implementation, and that that forwards them to the
> > > RootWindow,
> > > and the RootWindow then sends them to the relevant target (after giving
> > > EventFilters an opportunity to intercept). The Shell registers several
> > > EventFilters for various containers/circumstances. I believe this pattern
> > > or a
> > > variant would be useful here.
> > >
> > > FInally, the Aura Shell is an implementation of the "ChromeOS product",
> > > which
> > > can actually also run on Windows. From a product perspective, this means
> > > 100% of
> > > the ChromeOS experience must be brought through this UI, including the
> > > ChromeOS
> > > input method system.  It is meaningless (and probably impossible) to run
> > > the
> > > Windows IME system in the ChromeOS desktop built in Aura Shell.
> > >
> > >
> >
>
http://codereview.chromium.**org/8576005/%253Chttp://codereview.chromium.org/...>
> > >
> > 
> > 
> > 
> > -- 
> > - James Su

Ben Goodger (Google)

Much like none of this stuff is built into X, none of this stuff should ...

9 years ago (2011-12-09 04:16:53 UTC) #51

Much like none of this stuff is built into X, none of this stuff should be
built into Aura.

We have one EventFilter in the shell attached to the RootWindow. You can
add support for this there. It gets attached first, and you can guarantee
your code gets first crack.

This is also the right place to show your views-based UI.

-Ben

On Thu, Dec 8, 2011 at 7:34 PM, <suzhe@chromium.org> wrote:

> Please see my comment inline.
>
>
> On 2011/12/07 15:30:28, Yusuke Sato wrote:
>
>> First, I'm sorry for mischoosing the reviewers. Next time I'll ask you
>> for a
>> review as well.
>>
>
>  > The Shell registers several EventFilters for various
>>
> containers/circumstances.
>
>> I believe
>> > this pattern or a variant would be useful here.
>>
>
>  Moving the key event handling code in this CL to somewhere in
>> ui/aura_shell/,
>> probably toplevel_window_event_filter.**cc, would be easy. However, as
>> James
>> mentioned, we still need a single ui::InputMethod object which is
>> accessible
>> from RWHVA and NWA so that they could tell the current text field type,
>> caret
>> position, etc. to the input method object. Would it be acceptable to add a
>> member variable like 'ui::InputMethod* input_method_;' and its
>> setter/getter
>>
> to
>
>> the RootWindow class?
>>
>
> Besides the single ui::InputMethod object issue, I still think it's more
> reasonable to add IME related event handling in RootWindowHost, because:
> 1. We want to let the IME to handle key events first before dispatching
> them to
> other parts of the application, because:
> a) Make sure IME's hotkeys have highest priority, to avoid surprise
> behavior for
> IME users when there are hotkey conflicts between IME and other event
> filters.
> b) Make sure IME's hotkeys can still work even without focused window.
> c) It's the behavior on Windows, and we cannot change it. It's better to
> keep it
> consistent on all platforms to reduce the code difference.
>
> 2. ibus handles keyevents asynchronously, RootWindowHost is the simplest
> place
> to support this behavior. Because all upper level event handling APIs have
> synchronous semantics, such as RootWindow::ProcessKeyEvent(),
> EventFilter::**PreHandleKeyEvent(), WindowDelegate::OnKeyEvent(), etc.
>
>
>
>
>  James:
>> Thanks for providing the detailed background information.
>>
>
>  -Yusuke
>>
>
>
>  On 2011/12/07 06:51:24, James Su wrote:
>> > Let me try to explain the problem:
>> >
>> > We can split the whole system related to IME into two parts:
>> >
>> > 1. The IME system itself that provides input method services, including
>> the
>> > language decoder, the candidate UI, etc.
>> > This part can be further split into two separated parts: the backend
>> server
>> > (including IME engines, a.k.a language decoders) and the UI (including
>> > candidate UI, the language icon on the status bar, the language menu,
>> etc.).
>> > The fact is that chrome itself doesn't have an built-in IME system. We
>> > simply use the IME system provided by each platform. On Windows we use
>> > IME32, on ChromeOS we use ibus. The difference is that we have our own
>> > built-in IME UI on ChromeOS, but not on Windows.
>> > I agree that for ChromeOS, we probably want to move our built-in IME UI
>> > from chrome itself into aura_shell. And in the future, we'll probably
>> want
>> > to introduce a new IME system specially designed for aura desktop, but
>> it's
>> > not going to happen very soon. So for aura desktop, we still need to use
>> > IME32 (or maybe TSF in the future) on Windows and ibus on ChromeOS. But
>> we
>> > probably can use the same built-in IME UI on both Windows (*) and
>> ChromeOS.
>> > But it's a different story.
>> >
>> > (*) Windows (both IME32 and TSF) allows an application to provide
>> > customized IME UI, but we probably want to use TSF instead to get better
>> > functionality.
>> >
>> > 2. The client that makes use of the service provided by the IME system.
>> > The code in ui/base/ime/*, that yusukes are working on, are actually the
>> > client code of actual IME systems. More specifically, it's the wrapper
>> > layer that abstracts the difference among different IME systems, to
>> isolate
>> > the chrome itself, the actual client, from the IME system. This part has
>> > following facts:
>> > 1). It provides a platform independent API that can be used by chrome
>> > itself to communicate with the actual IME system.
>> > 2). It supports the actual IME system by a wrapper layer (like
>> > InputMethodWin), so we can provide new wrapper layers to support new IME
>> > systems, without changing the client (chrome) code.
>> > 3). We need a central place for the client (chrome) to get the pointer
>> to
>> > the wrapper. In old views architecture, NativeWidget is that place. In
>> aura
>> > desktop, we think that the root window would be the most preferable
>> place
>> > for this purpose. Because one wrapper object is enough for the whole
>> aura
>> > desktop, and the root window object is the only top level object that's
>> > accessible from every UI elements in the chrome (for views controls,
>> it's
>> > accessible through NativeWidget and views::InputMethodBridge). And I
>> don't
>> > think aura_shell is a good place for this purpose, as it's outside
>> chrome's
>> > scope.
>> >
>> > Hope the problem can be clarified by above explanation.
>> >
>> > Regards
>> > James Su
>> >
>> > 在 2011年12月7日 下午1:43， <ben@chromium.org>写道：
>> >
>> > > Let me provide some more specifics. Also, check out the new design
>> > > overviews at
>> > >
>> >
>>
>
> http://dev.chromium.org/****developers/design-documents/****
> aura%253Chttp://dev.chromium.**org/developers/design-**
>
documents/aura%253Ewhich<http://dev.chromium.org/**developers/design-documents/**aura%253Chttp://dev.chromium.org/developers/design-documents/aura%253Ewhich>
>
>  > are intended to
>> > > provide more context on the Aura design.
>> > >
>> > > A few notes:
>> > >
>> > > I don't believe IME is something that belongs in Aura itself. Aura is
>> > > intentionally very simple and delegates most application-specific
>> > > processing to
>> > > its client, in this case Aura Shell.
>> > >
>> > > IME UI that you might show is going to have to play with the rest of
>> the
>> > > Aura
>> > > UI, and as such can only be reasonably shown by the Aura Shell, which
>> has
>> > > the
>> > > context about windows etc.
>> > >
>> > > The pattern is typically that the underlying OS sends
>> base::NativeEvents
>> > > to the
>> > > RootWindowHost implementation, and that that forwards them to the
>> > > RootWindow,
>> > > and the RootWindow then sends them to the relevant target (after
>> giving
>> > > EventFilters an opportunity to intercept). The Shell registers several
>> > > EventFilters for various containers/circumstances. I believe this
>> pattern
>> > > or a
>> > > variant would be useful here.
>> > >
>> > > FInally, the Aura Shell is an implementation of the "ChromeOS
>> product",
>> > > which
>> > > can actually also run on Windows. From a product perspective, this
>> means
>> > > 100% of
>> > > the ChromeOS experience must be brought through this UI, including the
>> > > ChromeOS
>> > > input method system.  It is meaningless (and probably impossible) to
>> run
>> > > the
>> > > Windows IME system in the ChromeOS desktop built in Aura Shell.
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.****org/8576005/%253Chttp://codere**
> view.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
>
>  > >
>> >
>> >
>> >
>> > --
>> > - James Su
>>
>
>
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>

James Su

在 2011年12月9日下午12:16，Ben Goodger (Google) <ben@chromium.org>写道： > Much like none of this stuff is built ...

9 years ago (2011-12-09 04:39:48 UTC) #52

在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：

> Much like none of this stuff is built into X, none of this stuff should be
> built into Aura.

I'm wondering what's the exact reason that we cannot integrate
ui::InputMethod into aura?


> We have one EventFilter in the shell attached to the RootWindow. You can
> add support for this there. It gets attached first, and you can guarantee
> your code gets first crack.
>
We need to resolve at least two problems in order to integrate
ui::InputMethod into RootWindowEventFilter:
1. Who should own the ui::InputMethod object and how to expose it to RWHVA
and NWA?
2. How to deal with asynchronous event handling model required by ibus?

IMHO, we will make current code more complicated and harder to understand
if we want to go this way.

And if we want to support IMM32 on Windows, we can only support it by
integrating ui::InputMethodWin into RootWindowHostWin, because we need to
intercept a bunch of Windows events. Then it'll make the code different on
ChromeOS and Windows, making it harder to maintain.


> This is also the right place to show your views-based UI.
>
> -Ben
>
>
> On Thu, Dec 8, 2011 at 7:34 PM, <suzhe@chromium.org> wrote:
>
>> Please see my comment inline.
>>
>>
>> On 2011/12/07 15:30:28, Yusuke Sato wrote:
>>
>>> First, I'm sorry for mischoosing the reviewers. Next time I'll ask you
>>> for a
>>> review as well.
>>>
>>
>>  > The Shell registers several EventFilters for various
>>>
>> containers/circumstances.
>>
>>> I believe
>>> > this pattern or a variant would be useful here.
>>>
>>
>>  Moving the key event handling code in this CL to somewhere in
>>> ui/aura_shell/,
>>> probably toplevel_window_event_filter.**cc, would be easy. However, as
>>> James
>>> mentioned, we still need a single ui::InputMethod object which is
>>> accessible
>>> from RWHVA and NWA so that they could tell the current text field type,
>>> caret
>>> position, etc. to the input method object. Would it be acceptable to add
>>> a
>>> member variable like 'ui::InputMethod* input_method_;' and its
>>> setter/getter
>>>
>> to
>>
>>> the RootWindow class?
>>>
>>
>> Besides the single ui::InputMethod object issue, I still think it's more
>> reasonable to add IME related event handling in RootWindowHost, because:
>> 1. We want to let the IME to handle key events first before dispatching
>> them to
>> other parts of the application, because:
>> a) Make sure IME's hotkeys have highest priority, to avoid surprise
>> behavior for
>> IME users when there are hotkey conflicts between IME and other event
>> filters.
>> b) Make sure IME's hotkeys can still work even without focused window.
>> c) It's the behavior on Windows, and we cannot change it. It's better to
>> keep it
>> consistent on all platforms to reduce the code difference.
>>
>> 2. ibus handles keyevents asynchronously, RootWindowHost is the simplest
>> place
>> to support this behavior. Because all upper level event handling APIs have
>> synchronous semantics, such as RootWindow::ProcessKeyEvent(),
>> EventFilter::**PreHandleKeyEvent(), WindowDelegate::OnKeyEvent(), etc.
>>
>>
>>
>>
>>  James:
>>> Thanks for providing the detailed background information.
>>>
>>
>>  -Yusuke
>>>
>>
>>
>>  On 2011/12/07 06:51:24, James Su wrote:
>>> > Let me try to explain the problem:
>>> >
>>> > We can split the whole system related to IME into two parts:
>>> >
>>> > 1. The IME system itself that provides input method services,
>>> including the
>>> > language decoder, the candidate UI, etc.
>>> > This part can be further split into two separated parts: the backend
>>> server
>>> > (including IME engines, a.k.a language decoders) and the UI (including
>>> > candidate UI, the language icon on the status bar, the language menu,
>>> etc.).
>>> > The fact is that chrome itself doesn't have an built-in IME system. We
>>> > simply use the IME system provided by each platform. On Windows we use
>>> > IME32, on ChromeOS we use ibus. The difference is that we have our own
>>> > built-in IME UI on ChromeOS, but not on Windows.
>>> > I agree that for ChromeOS, we probably want to move our built-in IME UI
>>> > from chrome itself into aura_shell. And in the future, we'll probably
>>> want
>>> > to introduce a new IME system specially designed for aura desktop, but
>>> it's
>>> > not going to happen very soon. So for aura desktop, we still need to
>>> use
>>> > IME32 (or maybe TSF in the future) on Windows and ibus on ChromeOS.
>>> But we
>>> > probably can use the same built-in IME UI on both Windows (*) and
>>> ChromeOS.
>>> > But it's a different story.
>>> >
>>> > (*) Windows (both IME32 and TSF) allows an application to provide
>>> > customized IME UI, but we probably want to use TSF instead to get
>>> better
>>> > functionality.
>>> >
>>> > 2. The client that makes use of the service provided by the IME system.
>>> > The code in ui/base/ime/*, that yusukes are working on, are actually
>>> the
>>> > client code of actual IME systems. More specifically, it's the wrapper
>>> > layer that abstracts the difference among different IME systems, to
>>> isolate
>>> > the chrome itself, the actual client, from the IME system. This part
>>> has
>>> > following facts:
>>> > 1). It provides a platform independent API that can be used by chrome
>>> > itself to communicate with the actual IME system.
>>> > 2). It supports the actual IME system by a wrapper layer (like
>>> > InputMethodWin), so we can provide new wrapper layers to support new
>>> IME
>>> > systems, without changing the client (chrome) code.
>>> > 3). We need a central place for the client (chrome) to get the pointer
>>> to
>>> > the wrapper. In old views architecture, NativeWidget is that place. In
>>> aura
>>> > desktop, we think that the root window would be the most preferable
>>> place
>>> > for this purpose. Because one wrapper object is enough for the whole
>>> aura
>>> > desktop, and the root window object is the only top level object that's
>>> > accessible from every UI elements in the chrome (for views controls,
>>> it's
>>> > accessible through NativeWidget and views::InputMethodBridge). And I
>>> don't
>>> > think aura_shell is a good place for this purpose, as it's outside
>>> chrome's
>>> > scope.
>>> >
>>> > Hope the problem can be clarified by above explanation.
>>> >
>>> > Regards
>>> > James Su
>>> >
>>> > 在 2011年12月7日 下午1:43， <ben@chromium.org>写道：
>>> >
>>> > > Let me provide some more specifics. Also, check out the new design
>>> > > overviews at
>>> > >
>>> >
>>>
>>
>> http://dev.chromium.org/****developers/design-documents/****
>> aura%253Chttp://dev.chromium.**org/developers/design-**
>>
documents/aura%253Ewhich<http://dev.chromium.org/**developers/design-documents/**aura%253Chttp://dev.chromium.org/developers/design-documents/aura%253Ewhich>
>>
>>  > are intended to
>>> > > provide more context on the Aura design.
>>> > >
>>> > > A few notes:
>>> > >
>>> > > I don't believe IME is something that belongs in Aura itself. Aura is
>>> > > intentionally very simple and delegates most application-specific
>>> > > processing to
>>> > > its client, in this case Aura Shell.
>>> > >
>>> > > IME UI that you might show is going to have to play with the rest of
>>> the
>>> > > Aura
>>> > > UI, and as such can only be reasonably shown by the Aura Shell,
>>> which has
>>> > > the
>>> > > context about windows etc.
>>> > >
>>> > > The pattern is typically that the underlying OS sends
>>> base::NativeEvents
>>> > > to the
>>> > > RootWindowHost implementation, and that that forwards them to the
>>> > > RootWindow,
>>> > > and the RootWindow then sends them to the relevant target (after
>>> giving
>>> > > EventFilters an opportunity to intercept). The Shell registers
>>> several
>>> > > EventFilters for various containers/circumstances. I believe this
>>> pattern
>>> > > or a
>>> > > variant would be useful here.
>>> > >
>>> > > FInally, the Aura Shell is an implementation of the "ChromeOS
>>> product",
>>> > > which
>>> > > can actually also run on Windows. From a product perspective, this
>>> means
>>> > > 100% of
>>> > > the ChromeOS experience must be brought through this UI, including
>>> the
>>> > > ChromeOS
>>> > > input method system.  It is meaningless (and probably impossible) to
>>> run
>>> > > the
>>> > > Windows IME system in the ChromeOS desktop built in Aura Shell.
>>> > >
>>> > >
>>> >
>>>
>>
>> http://codereview.chromium.****org/8576005/%253Chttp://codere**
>> view.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
>>
>>  > >
>>> >
>>> >
>>> >
>>> > --
>>> > - James Su
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>>
>
>


-- 
- James Su

Yusuke Sato

On 2011/12/09 04:39:48, James Su wrote: > 在 2011年12月9日下午12:16，Ben Goodger (Google) <ben@chromium.org>写道： > > ...

9 years ago (2011-12-09 06:30:26 UTC) #53

On 2011/12/09 04:39:48, James Su wrote:
> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
> 
> > Much like none of this stuff is built into X, none of this stuff should be
> > built into Aura.
> 
> I'm wondering what's the exact reason that we cannot integrate
> ui::InputMethod into aura?
> 
> 
> > We have one EventFilter in the shell attached to the RootWindow. You can
> > add support for this there. It gets attached first, and you can guarantee
> > your code gets first crack.
> >
> We need to resolve at least two problems in order to integrate
> ui::InputMethod into RootWindowEventFilter:
> 1. Who should own the ui::InputMethod object and how to expose it to RWHVA
> and NWA?
> 2. How to deal with asynchronous event handling model required by ibus?

Let me give a real example of the problem 2.

In Japanese IME, Control+t might be used for transliterating a composition
string (e.g. あ -> a) depending on its internal state. In other IMEs, Control+t
is likely not consumed. At the same time, Control+t is the shortcut for adding a
new tab to Chrome. If we implement IME features in an event filter, the IME
event filter must return true (= 'consumed') when the Japanese IME is active and
in a certain state, and must return false (= 'not consumed') otherwise so that
Chrome (ui/aura_shell/shell_accelerator_controller.cc) could consume it.
However, it's not possible for the IME event filter to decide the correct return
value since the filter cannot talk to ibus-daemon in a synchronous manner.

I think I can figure out a way to workaround the issue, but it might make the
key event flow even more complicated. Anyway, I'll give it a try.

> IMHO, we will make current code more complicated and harder to understand
> if we want to go this way.
> 
> And if we want to support IMM32 on Windows, we can only support it by
> integrating ui::InputMethodWin into RootWindowHostWin, because we need to
> intercept a bunch of Windows events. Then it'll make the code different on
> ChromeOS and Windows, making it harder to maintain.

Oh I was not aware of the IMM32 issue..  I guess the corresponding part in the
old views::InputMethodWin implementation is the following, correct?

LRESULT InputMethodWin::OnImeMessages(
    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
  LRESULT result = 0;
  switch (message) {
    case WM_IME_SETCONTEXT:
      result = OnImeSetContext(message, w_param, l_param, handled);
      break;
    case WM_IME_STARTCOMPOSITION:
      result = OnImeStartComposition(message, w_param, l_param, handled);
      break;
    case WM_IME_COMPOSITION:
      result = OnImeComposition(message, w_param, l_param, handled);
      break;
    case WM_IME_ENDCOMPOSITION:
      result = OnImeEndComposition(message, w_param, l_param, handled);
      break;
    case WM_IME_REQUEST:
      result = OnImeRequest(message, w_param, l_param, handled);
      break;
    case WM_CHAR:
    case WM_SYSCHAR:
      result = OnChar(message, w_param, l_param, handled);
      break;
    case WM_DEADCHAR:
    case WM_SYSDEADCHAR:
      result = OnDeadChar(message, w_param, l_param, handled);
      break;
    default:
      NOTREACHED() << "Unknown IME message:" << message;
      break;
  }
  return result;
}

-Yusuke

James Su

在 2011年12月9日下午2:30， <yusukes@chromium.org>写道： > On 2011/12/09 04:39:48, James Su wrote: > >> 在 2011年12月9日 ...

9 years ago (2011-12-09 06:37:26 UTC) #54

在 2011年12月9日 下午2:30， <yusukes@chromium.org>写道：

> On 2011/12/09 04:39:48, James Su wrote:
>
>> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
>>
>
>  > Much like none of this stuff is built into X, none of this stuff should
>> be
>> > built into Aura.
>>
>
>  I'm wondering what's the exact reason that we cannot integrate
>> ui::InputMethod into aura?
>>
>
>
>  > We have one EventFilter in the shell attached to the RootWindow. You can
>> > add support for this there. It gets attached first, and you can
>> guarantee
>> > your code gets first crack.
>> >
>> We need to resolve at least two problems in order to integrate
>> ui::InputMethod into RootWindowEventFilter:
>> 1. Who should own the ui::InputMethod object and how to expose it to RWHVA
>> and NWA?
>> 2. How to deal with asynchronous event handling model required by ibus?
>>
>
> Let me give a real example of the problem 2.
>
> In Japanese IME, Control+t might be used for transliterating a composition
> string (e.g. あ -> a) depending on its internal state. In other IMEs,
> Control+t
> is likely not consumed. At the same time, Control+t is the shortcut for
> adding a
> new tab to Chrome. If we implement IME features in an event filter, the IME
> event filter must return true (= 'consumed') when the Japanese IME is
> active and
> in a certain state, and must return false (= 'not consumed') otherwise so
> that
> Chrome (ui/aura_shell/shell_**accelerator_controller.cc) could consume it.
> However, it's not possible for the IME event filter to decide the correct
> return
> value since the filter cannot talk to ibus-daemon in a synchronous manner.
>
> I think I can figure out a way to workaround the issue, but it might make
> the
> key event flow even more complicated. Anyway, I'll give it a try.
>
>
>  IMHO, we will make current code more complicated and harder to understand
>> if we want to go this way.
>>
>
>  And if we want to support IMM32 on Windows, we can only support it by
>> integrating ui::InputMethodWin into RootWindowHostWin, because we need to
>> intercept a bunch of Windows events. Then it'll make the code different on
>> ChromeOS and Windows, making it harder to maintain.
>>
>
> Oh I was not aware of the IMM32 issue..  I guess the corresponding part in
> the
> old views::InputMethodWin implementation is the following, correct?
>
Yes exactly.


>
> LRESULT InputMethodWin::OnImeMessages(
>    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
>  LRESULT result = 0;
>  switch (message) {
>    case WM_IME_SETCONTEXT:
>      result = OnImeSetContext(message, w_param, l_param, handled);
>      break;
>    case WM_IME_STARTCOMPOSITION:
>      result = OnImeStartComposition(message, w_param, l_param, handled);
>      break;
>    case WM_IME_COMPOSITION:
>      result = OnImeComposition(message, w_param, l_param, handled);
>      break;
>    case WM_IME_ENDCOMPOSITION:
>      result = OnImeEndComposition(message, w_param, l_param, handled);
>      break;
>    case WM_IME_REQUEST:
>      result = OnImeRequest(message, w_param, l_param, handled);
>      break;
>    case WM_CHAR:
>    case WM_SYSCHAR:
>      result = OnChar(message, w_param, l_param, handled);
>      break;
>    case WM_DEADCHAR:
>    case WM_SYSDEADCHAR:
>      result = OnDeadChar(message, w_param, l_param, handled);
>      break;
>    default:
>      NOTREACHED() << "Unknown IME message:" << message;
>      break;
>  }
>  return result;
> }
>
> -Yusuke
>
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>



-- 
- James Su

Peng

On Fri, Dec 9, 2011 at 1:30 AM, <yusukes@chromium.org> wrote: > On 2011/12/09 04:39:48, James ...

9 years ago (2011-12-09 13:56:38 UTC) #55

On Fri, Dec 9, 2011 at 1:30 AM, <yusukes@chromium.org> wrote:

> On 2011/12/09 04:39:48, James Su wrote:
>
>> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
>>
>
>  > Much like none of this stuff is built into X, none of this stuff should
>> be
>> > built into Aura.
>>
>
>  I'm wondering what's the exact reason that we cannot integrate
>> ui::InputMethod into aura?
>>
>

>  > We have one EventFilter in the shell attached to the RootWindow. You can
>> > add support for this there. It gets attached first, and you can
>> guarantee
>> > your code gets first crack.
>> >
>> We need to resolve at least two problems in order to integrate
>> ui::InputMethod into RootWindowEventFilter:
>> 1. Who should own the ui::InputMethod object and how to expose it to RWHVA
>> and NWA?
>>
>
I think we could let each RHWVA and views which need input method service
own an InputMethod instance. Or make ui::InputMethod as singleton class,
and let them share one instance.



> 2. How to deal with asynchronous event handling model required by ibus?
>>
>
If views own or share ui::InputMethod instance, that will be easy. The
views just need pass the the key events to the ui::InputMehtod, and the
ui::InputMethod use TextInputClient(or rename to InputMethodDelegate) to
send result back, like gtk or qt does. But the event path will be a little
different on Linux and Windows. Anyway, it is impossible to make Linux and
Window use 100% same event path even if the current design.


> Let me give a real example of the problem 2.
>
> In Japanese IME, Control+t might be used for transliterating a composition
> string (e.g. あ -> a) depending on its internal state. In other IMEs,
> Control+t
> is likely not consumed. At the same time, Control+t is the shortcut for
> adding a
> new tab to Chrome. If we implement IME features in an event filter, the IME
> event filter must return true (= 'consumed') when the Japanese IME is
> active and
> in a certain state, and must return false (= 'not consumed') otherwise so
> that
> Chrome (ui/aura_shell/shell_**accelerator_controller.cc) could consume it.
> However, it's not possible for the IME event filter to decide the correct
> return
> value since the filter cannot talk to ibus-daemon in a synchronous manner.
>
> I think I can figure out a way to workaround the issue, but it might make
> the
> key event flow even more complicated. Anyway, I'll give it a try.
>
>
>  IMHO, we will make current code more complicated and harder to understand
>> if we want to go this way.
>>
>
>  And if we want to support IMM32 on Windows, we can only support it by
>> integrating ui::InputMethodWin into RootWindowHostWin, because we need to
>> intercept a bunch of Windows events. Then it'll make the code different on
>> ChromeOS and Windows, making it harder to maintain.
>>
>
> Oh I was not aware of the IMM32 issue..  I guess the corresponding part in
> the
> old views::InputMethodWin implementation is the following, correct?
>
> LRESULT InputMethodWin::OnImeMessages(
>    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
>  LRESULT result = 0;
>  switch (message) {
>    case WM_IME_SETCONTEXT:
>      result = OnImeSetContext(message, w_param, l_param, handled);
>      break;
>    case WM_IME_STARTCOMPOSITION:
>      result = OnImeStartComposition(message, w_param, l_param, handled);
>      break;
>    case WM_IME_COMPOSITION:
>      result = OnImeComposition(message, w_param, l_param, handled);
>      break;
>    case WM_IME_ENDCOMPOSITION:
>      result = OnImeEndComposition(message, w_param, l_param, handled);
>      break;
>    case WM_IME_REQUEST:
>      result = OnImeRequest(message, w_param, l_param, handled);
>      break;
>    case WM_CHAR:
>    case WM_SYSCHAR:
>      result = OnChar(message, w_param, l_param, handled);
>      break;
>    case WM_DEADCHAR:
>    case WM_SYSDEADCHAR:
>      result = OnDeadChar(message, w_param, l_param, handled);
>      break;
>    default:
>      NOTREACHED() << "Unknown IME message:" << message;
>      break;
>  }
>  return result;
> }
>
>
For IMM32 supporting, aura need provide a filter to allow pre-process the
native messages before translating them to aura events.

Peng

-Yusuke
>
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>

Ben Goodger (Google)

Why would RWHVA need to know anything about IME? (Other than the "I might contain ...

9 years ago (2011-12-09 17:40:04 UTC) #56

Why would RWHVA need to know anything about IME? (Other than the "I might
contain a textfield" stuff). RWHVA is just another aura::Window* embedded
in a hierarchy within the RootWindow.

I am pondering this some more today, I will provide some more
feedback/questions in a couple of hours.

-Ben

On Fri, Dec 9, 2011 at 5:56 AM, Peng Huang <penghuang@chromium.org> wrote:

>
>
> On Fri, Dec 9, 2011 at 1:30 AM, <yusukes@chromium.org> wrote:
>
>> On 2011/12/09 04:39:48, James Su wrote:
>>
>>> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
>>>
>>
>>  > Much like none of this stuff is built into X, none of this stuff
>>> should be
>>> > built into Aura.
>>>
>>
>>  I'm wondering what's the exact reason that we cannot integrate
>>> ui::InputMethod into aura?
>>>
>>
>
>>  > We have one EventFilter in the shell attached to the RootWindow. You
>>> can
>>> > add support for this there. It gets attached first, and you can
>>> guarantee
>>> > your code gets first crack.
>>> >
>>> We need to resolve at least two problems in order to integrate
>>> ui::InputMethod into RootWindowEventFilter:
>>> 1. Who should own the ui::InputMethod object and how to expose it to
>>> RWHVA
>>> and NWA?
>>>
>>
> I think we could let each RHWVA and views which need input method service
> own an InputMethod instance. Or make ui::InputMethod as singleton class,
> and let them share one instance.
>
>
>
>> 2. How to deal with asynchronous event handling model required by ibus?
>>>
>>
> If views own or share ui::InputMethod instance, that will be easy. The
> views just need pass the the key events to the ui::InputMehtod, and the
> ui::InputMethod use TextInputClient(or rename to InputMethodDelegate) to
> send result back, like gtk or qt does. But the event path will be a little
> different on Linux and Windows. Anyway, it is impossible to make Linux and
> Window use 100% same event path even if the current design.
>
>
>> Let me give a real example of the problem 2.
>>
>> In Japanese IME, Control+t might be used for transliterating a composition
>> string (e.g. あ -> a) depending on its internal state. In other IMEs,
>> Control+t
>> is likely not consumed. At the same time, Control+t is the shortcut for
>> adding a
>> new tab to Chrome. If we implement IME features in an event filter, the
>> IME
>> event filter must return true (= 'consumed') when the Japanese IME is
>> active and
>> in a certain state, and must return false (= 'not consumed') otherwise so
>> that
>> Chrome (ui/aura_shell/shell_**accelerator_controller.cc) could consume
>> it.
>> However, it's not possible for the IME event filter to decide the correct
>> return
>> value since the filter cannot talk to ibus-daemon in a synchronous manner.
>>
>> I think I can figure out a way to workaround the issue, but it might make
>> the
>> key event flow even more complicated. Anyway, I'll give it a try.
>>
>>
>>  IMHO, we will make current code more complicated and harder to understand
>>> if we want to go this way.
>>>
>>
>>  And if we want to support IMM32 on Windows, we can only support it by
>>> integrating ui::InputMethodWin into RootWindowHostWin, because we need to
>>> intercept a bunch of Windows events. Then it'll make the code different
>>> on
>>> ChromeOS and Windows, making it harder to maintain.
>>>
>>
>> Oh I was not aware of the IMM32 issue..  I guess the corresponding part
>> in the
>> old views::InputMethodWin implementation is the following, correct?
>>
>> LRESULT InputMethodWin::OnImeMessages(
>>    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
>>  LRESULT result = 0;
>>  switch (message) {
>>    case WM_IME_SETCONTEXT:
>>      result = OnImeSetContext(message, w_param, l_param, handled);
>>      break;
>>    case WM_IME_STARTCOMPOSITION:
>>      result = OnImeStartComposition(message, w_param, l_param, handled);
>>      break;
>>    case WM_IME_COMPOSITION:
>>      result = OnImeComposition(message, w_param, l_param, handled);
>>      break;
>>    case WM_IME_ENDCOMPOSITION:
>>      result = OnImeEndComposition(message, w_param, l_param, handled);
>>      break;
>>    case WM_IME_REQUEST:
>>      result = OnImeRequest(message, w_param, l_param, handled);
>>      break;
>>    case WM_CHAR:
>>    case WM_SYSCHAR:
>>      result = OnChar(message, w_param, l_param, handled);
>>      break;
>>    case WM_DEADCHAR:
>>    case WM_SYSDEADCHAR:
>>      result = OnDeadChar(message, w_param, l_param, handled);
>>      break;
>>    default:
>>      NOTREACHED() << "Unknown IME message:" << message;
>>      break;
>>  }
>>  return result;
>> }
>>
>>
> For IMM32 supporting, aura need provide a filter to allow pre-process the
> native messages before translating them to aura events.
>
> Peng
>
> -Yusuke
>>
>>
>>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>>
>
>

Peng

Because IME need talk with textfields in webkit process via RHWVA. textfield need tell IME ...

9 years ago (2011-12-09 18:57:42 UTC) #57

Because IME need talk with textfields in webkit process via RHWVA.

textfield need tell IME what is the text input type (none, password, text,
url and etc), where is the input cursor position on screen? and etc.

IME also need send some information to textfield, like updating composition
text in textfield, committing text to textfield, and etc.

Current implementation is RHWVA implements ui::TextInputClient interface,
ui::InputMethod uses this interface to interact with RHWVA.

Related source code:
http://codereview.chromium.org/8576005/diff/93001/content/browser/renderer_ho...
http://codereview.chromium.org/8576005/diff/93001/content/browser/renderer_ho...

Peng

On Fri, Dec 9, 2011 at 12:40 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> Why would RWHVA need to know anything about IME? (Other than the "I might
> contain a textfield" stuff). RWHVA is just another aura::Window* embedded
> in a hierarchy within the RootWindow.
>
> I am pondering this some more today, I will provide some more
> feedback/questions in a couple of hours.
>
> -Ben
>
>
> On Fri, Dec 9, 2011 at 5:56 AM, Peng Huang <penghuang@chromium.org> wrote:
>
>>
>>
>> On Fri, Dec 9, 2011 at 1:30 AM, <yusukes@chromium.org> wrote:
>>
>>> On 2011/12/09 04:39:48, James Su wrote:
>>>
>>>> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
>>>>
>>>
>>>  > Much like none of this stuff is built into X, none of this stuff
>>>> should be
>>>> > built into Aura.
>>>>
>>>
>>>  I'm wondering what's the exact reason that we cannot integrate
>>>> ui::InputMethod into aura?
>>>>
>>>
>>
>>>  > We have one EventFilter in the shell attached to the RootWindow. You
>>>> can
>>>> > add support for this there. It gets attached first, and you can
>>>> guarantee
>>>> > your code gets first crack.
>>>> >
>>>> We need to resolve at least two problems in order to integrate
>>>> ui::InputMethod into RootWindowEventFilter:
>>>> 1. Who should own the ui::InputMethod object and how to expose it to
>>>> RWHVA
>>>> and NWA?
>>>>
>>>
>> I think we could let each RHWVA and views which need input method service
>> own an InputMethod instance. Or make ui::InputMethod as singleton class,
>> and let them share one instance.
>>
>>
>>
>>> 2. How to deal with asynchronous event handling model required by ibus?
>>>>
>>>
>> If views own or share ui::InputMethod instance, that will be easy. The
>> views just need pass the the key events to the ui::InputMehtod, and the
>> ui::InputMethod use TextInputClient(or rename to InputMethodDelegate) to
>> send result back, like gtk or qt does. But the event path will be a little
>> different on Linux and Windows. Anyway, it is impossible to make Linux and
>> Window use 100% same event path even if the current design.
>>
>>
>>> Let me give a real example of the problem 2.
>>>
>>> In Japanese IME, Control+t might be used for transliterating a
>>> composition
>>> string (e.g. あ -> a) depending on its internal state. In other IMEs,
>>> Control+t
>>> is likely not consumed. At the same time, Control+t is the shortcut for
>>> adding a
>>> new tab to Chrome. If we implement IME features in an event filter, the
>>> IME
>>> event filter must return true (= 'consumed') when the Japanese IME is
>>> active and
>>> in a certain state, and must return false (= 'not consumed') otherwise
>>> so that
>>> Chrome (ui/aura_shell/shell_**accelerator_controller.cc) could consume
>>> it.
>>> However, it's not possible for the IME event filter to decide the
>>> correct return
>>> value since the filter cannot talk to ibus-daemon in a synchronous
>>> manner.
>>>
>>> I think I can figure out a way to workaround the issue, but it might
>>> make the
>>> key event flow even more complicated. Anyway, I'll give it a try.
>>>
>>>
>>>  IMHO, we will make current code more complicated and harder to
>>>> understand
>>>> if we want to go this way.
>>>>
>>>
>>>  And if we want to support IMM32 on Windows, we can only support it by
>>>> integrating ui::InputMethodWin into RootWindowHostWin, because we need
>>>> to
>>>> intercept a bunch of Windows events. Then it'll make the code different
>>>> on
>>>> ChromeOS and Windows, making it harder to maintain.
>>>>
>>>
>>> Oh I was not aware of the IMM32 issue..  I guess the corresponding part
>>> in the
>>> old views::InputMethodWin implementation is the following, correct?
>>>
>>> LRESULT InputMethodWin::OnImeMessages(
>>>    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
>>>  LRESULT result = 0;
>>>  switch (message) {
>>>    case WM_IME_SETCONTEXT:
>>>      result = OnImeSetContext(message, w_param, l_param, handled);
>>>      break;
>>>    case WM_IME_STARTCOMPOSITION:
>>>      result = OnImeStartComposition(message, w_param, l_param, handled);
>>>      break;
>>>    case WM_IME_COMPOSITION:
>>>      result = OnImeComposition(message, w_param, l_param, handled);
>>>      break;
>>>    case WM_IME_ENDCOMPOSITION:
>>>      result = OnImeEndComposition(message, w_param, l_param, handled);
>>>      break;
>>>    case WM_IME_REQUEST:
>>>      result = OnImeRequest(message, w_param, l_param, handled);
>>>      break;
>>>    case WM_CHAR:
>>>    case WM_SYSCHAR:
>>>      result = OnChar(message, w_param, l_param, handled);
>>>      break;
>>>    case WM_DEADCHAR:
>>>    case WM_SYSDEADCHAR:
>>>      result = OnDeadChar(message, w_param, l_param, handled);
>>>      break;
>>>    default:
>>>      NOTREACHED() << "Unknown IME message:" << message;
>>>      break;
>>>  }
>>>  return result;
>>> }
>>>
>>>
>> For IMM32 supporting, aura need provide a filter to allow pre-process the
>> native messages before translating them to aura events.
>>
>> Peng
>>
>>  -Yusuke
>>>
>>>
>>>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>>>
>>
>>
>

Yusuke Sato

On 2011/12/09 13:56:38, Peng wrote: > On Fri, Dec 9, 2011 at 1:30 AM, <mailto:yusukes@chromium.org> ...

9 years ago (2011-12-10 00:52:38 UTC) #58

On 2011/12/09 13:56:38, Peng wrote:
> On Fri, Dec 9, 2011 at 1:30 AM, <mailto:yusukes@chromium.org> wrote:
> 
> > On 2011/12/09 04:39:48, James Su wrote:
> >
> >> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
> >>
> >
> >  > Much like none of this stuff is built into X, none of this stuff should
> >> be
> >> > built into Aura.
> >>
> >
> >  I'm wondering what's the exact reason that we cannot integrate
> >> ui::InputMethod into aura?
> >>
> >
> 
> >  > We have one EventFilter in the shell attached to the RootWindow. You can
> >> > add support for this there. It gets attached first, and you can
> >> guarantee
> >> > your code gets first crack.
> >> >
> >> We need to resolve at least two problems in order to integrate
> >> ui::InputMethod into RootWindowEventFilter:
> >> 1. Who should own the ui::InputMethod object and how to expose it to RWHVA
> >> and NWA?
> >>
> >
> I think we could let each RHWVA and views which need input method service
> own an InputMethod instance. 

Peng,
In this design, how could we handle a key event correctly that matches Chrome
shortcut? I guess your design would make this more complicated.
See the Control+t example I mentioned earlier.

-Yusuke

> Or make ui::InputMethod as singleton class,
> and let them share one instance.
> 
> 
> 
> > 2. How to deal with asynchronous event handling model required by ibus?
> >>
> >
> If views own or share ui::InputMethod instance, that will be easy. The
> views just need pass the the key events to the ui::InputMehtod, and the
> ui::InputMethod use TextInputClient(or rename to InputMethodDelegate) to
> send result back, like gtk or qt does. But the event path will be a little
> different on Linux and Windows. Anyway, it is impossible to make Linux and
> Window use 100% same event path even if the current design.
> 
> 
> > Let me give a real example of the problem 2.
> >
> > In Japanese IME, Control+t might be used for transliterating a composition
> > string (e.g. あ -> a) depending on its internal state. In other IMEs,
> > Control+t
> > is likely not consumed. At the same time, Control+t is the shortcut for
> > adding a
> > new tab to Chrome. If we implement IME features in an event filter, the IME
> > event filter must return true (= 'consumed') when the Japanese IME is
> > active and
> > in a certain state, and must return false (= 'not consumed') otherwise so
> > that
> > Chrome (ui/aura_shell/shell_**accelerator_controller.cc) could consume it.
> > However, it's not possible for the IME event filter to decide the correct
> > return
> > value since the filter cannot talk to ibus-daemon in a synchronous manner.
> >
> > I think I can figure out a way to workaround the issue, but it might make
> > the
> > key event flow even more complicated. Anyway, I'll give it a try.
> >
> >
> >  IMHO, we will make current code more complicated and harder to understand
> >> if we want to go this way.
> >>
> >
> >  And if we want to support IMM32 on Windows, we can only support it by
> >> integrating ui::InputMethodWin into RootWindowHostWin, because we need to
> >> intercept a bunch of Windows events. Then it'll make the code different on
> >> ChromeOS and Windows, making it harder to maintain.
> >>
> >
> > Oh I was not aware of the IMM32 issue..  I guess the corresponding part in
> > the
> > old views::InputMethodWin implementation is the following, correct?
> >
> > LRESULT InputMethodWin::OnImeMessages(
> >    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
> >  LRESULT result = 0;
> >  switch (message) {
> >    case WM_IME_SETCONTEXT:
> >      result = OnImeSetContext(message, w_param, l_param, handled);
> >      break;
> >    case WM_IME_STARTCOMPOSITION:
> >      result = OnImeStartComposition(message, w_param, l_param, handled);
> >      break;
> >    case WM_IME_COMPOSITION:
> >      result = OnImeComposition(message, w_param, l_param, handled);
> >      break;
> >    case WM_IME_ENDCOMPOSITION:
> >      result = OnImeEndComposition(message, w_param, l_param, handled);
> >      break;
> >    case WM_IME_REQUEST:
> >      result = OnImeRequest(message, w_param, l_param, handled);
> >      break;
> >    case WM_CHAR:
> >    case WM_SYSCHAR:
> >      result = OnChar(message, w_param, l_param, handled);
> >      break;
> >    case WM_DEADCHAR:
> >    case WM_SYSDEADCHAR:
> >      result = OnDeadChar(message, w_param, l_param, handled);
> >      break;
> >    default:
> >      NOTREACHED() << "Unknown IME message:" << message;
> >      break;
> >  }
> >  return result;
> > }
> >
> >
> For IMM32 supporting, aura need provide a filter to allow pre-process the
> native messages before translating them to aura events.
> 
> Peng
> 
> -Yusuke
> >
> >
> >
>
http://codereview.chromium.**org/8576005/%3Chttp://codereview.chromium.org/85...>
> >

Peng

On Fri, Dec 9, 2011 at 7:52 PM, <yusukes@chromium.org> wrote: > On 2011/12/09 13:56:38, Peng ...

9 years ago (2011-12-10 03:07:26 UTC) #59

On Fri, Dec 9, 2011 at 7:52 PM, <yusukes@chromium.org> wrote:

> On 2011/12/09 13:56:38, Peng wrote:
>
>  On Fri, Dec 9, 2011 at 1:30 AM, <mailto:yusukes@chromium.org> wrote:
>>
>
>  > On 2011/12/09 04:39:48, James Su wrote:
>> >
>> >> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
>> >>
>> >
>> >  > Much like none of this stuff is built into X, none of this stuff
>> should
>> >> be
>> >> > built into Aura.
>> >>
>> >
>> >  I'm wondering what's the exact reason that we cannot integrate
>> >> ui::InputMethod into aura?
>> >>
>> >
>>
>
>  >  > We have one EventFilter in the shell attached to the RootWindow. You
>> can
>> >> > add support for this there. It gets attached first, and you can
>> >> guarantee
>> >> > your code gets first crack.
>> >> >
>> >> We need to resolve at least two problems in order to integrate
>> >> ui::InputMethod into RootWindowEventFilter:
>> >> 1. Who should own the ui::InputMethod object and how to expose it to
>> RWHVA
>> >> and NWA?
>> >>
>> >
>> I think we could let each RHWVA and views which need input method service
>> own an InputMethod instance.
>>
>
> Peng,
> In this design, how could we handle a key event correctly that matches
> Chrome
> shortcut? I guess your design would make this more complicated.
> See the Control+t example I mentioned earlier.
>
>
Sorry. I forget the shortcut key issue. I think adding a new
method OnKeyEventBeforeFilter() in aura:WindowDelegate can resolve this
issue. With it, aura windows will have chance to handle all key events
before any filters.

Peng

-Yusuke
>
>
>  Or make ui::InputMethod as singleton class,
>> and let them share one instance.
>>
>
>
>
>  > 2. How to deal with asynchronous event handling model required by ibus?
>> >>
>> >
>> If views own or share ui::InputMethod instance, that will be easy. The
>> views just need pass the the key events to the ui::InputMehtod, and the
>> ui::InputMethod use TextInputClient(or rename to InputMethodDelegate) to
>> send result back, like gtk or qt does. But the event path will be a little
>> different on Linux and Windows. Anyway, it is impossible to make Linux and
>> Window use 100% same event path even if the current design.
>>
>
>
>  > Let me give a real example of the problem 2.
>> >
>> > In Japanese IME, Control+t might be used for transliterating a
>> composition
>> > string (e.g. あ -> a) depending on its internal state. In other IMEs,
>> > Control+t
>> > is likely not consumed. At the same time, Control+t is the shortcut for
>> > adding a
>> > new tab to Chrome. If we implement IME features in an event filter, the
>> IME
>> > event filter must return true (= 'consumed') when the Japanese IME is
>> > active and
>> > in a certain state, and must return false (= 'not consumed') otherwise
>> so
>> > that
>> > Chrome (ui/aura_shell/shell_****accelerator_controller.cc) could
>> consume it.
>>
>> > However, it's not possible for the IME event filter to decide the
>> correct
>> > return
>> > value since the filter cannot talk to ibus-daemon in a synchronous
>> manner.
>> >
>> > I think I can figure out a way to workaround the issue, but it might
>> make
>> > the
>> > key event flow even more complicated. Anyway, I'll give it a try.
>> >
>> >
>> >  IMHO, we will make current code more complicated and harder to
>> understand
>> >> if we want to go this way.
>> >>
>> >
>> >  And if we want to support IMM32 on Windows, we can only support it by
>> >> integrating ui::InputMethodWin into RootWindowHostWin, because we need
>> to
>> >> intercept a bunch of Windows events. Then it'll make the code
>> different on
>> >> ChromeOS and Windows, making it harder to maintain.
>> >>
>> >
>> > Oh I was not aware of the IMM32 issue..  I guess the corresponding part
>> in
>> > the
>> > old views::InputMethodWin implementation is the following, correct?
>> >
>> > LRESULT InputMethodWin::OnImeMessages(
>> >    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
>> >  LRESULT result = 0;
>> >  switch (message) {
>> >    case WM_IME_SETCONTEXT:
>> >      result = OnImeSetContext(message, w_param, l_param, handled);
>> >      break;
>> >    case WM_IME_STARTCOMPOSITION:
>> >      result = OnImeStartComposition(message, w_param, l_param, handled);
>> >      break;
>> >    case WM_IME_COMPOSITION:
>> >      result = OnImeComposition(message, w_param, l_param, handled);
>> >      break;
>> >    case WM_IME_ENDCOMPOSITION:
>> >      result = OnImeEndComposition(message, w_param, l_param, handled);
>> >      break;
>> >    case WM_IME_REQUEST:
>> >      result = OnImeRequest(message, w_param, l_param, handled);
>> >      break;
>> >    case WM_CHAR:
>> >    case WM_SYSCHAR:
>> >      result = OnChar(message, w_param, l_param, handled);
>> >      break;
>> >    case WM_DEADCHAR:
>> >    case WM_SYSDEADCHAR:
>> >      result = OnDeadChar(message, w_param, l_param, handled);
>> >      break;
>> >    default:
>> >      NOTREACHED() << "Unknown IME message:" << message;
>> >      break;
>> >  }
>> >  return result;
>> > }
>> >
>> >
>> For IMM32 supporting, aura need provide a filter to allow pre-process the
>> native messages before translating them to aura events.
>>
>
>  Peng
>>
>
>  -Yusuke
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/8576005/%3Chttp://coderevi**
> ew.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>

Yusuke Sato

On 2011/12/10 03:07:26, Peng wrote: > On Fri, Dec 9, 2011 at 7:52 PM, <mailto:yusukes@chromium.org> ...

9 years ago (2011-12-10 08:34:03 UTC) #60

On 2011/12/10 03:07:26, Peng wrote:
> On Fri, Dec 9, 2011 at 7:52 PM, <mailto:yusukes@chromium.org> wrote:
> 
> > On 2011/12/09 13:56:38, Peng wrote:
> >
> >  On Fri, Dec 9, 2011 at 1:30 AM, <mailto:yusukes@chromium.org> wrote:
> >>
> >
> >  > On 2011/12/09 04:39:48, James Su wrote:
> >> >
> >> >> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
> >> >>
> >> >
> >> >  > Much like none of this stuff is built into X, none of this stuff
> >> should
> >> >> be
> >> >> > built into Aura.
> >> >>
> >> >
> >> >  I'm wondering what's the exact reason that we cannot integrate
> >> >> ui::InputMethod into aura?
> >> >>
> >> >
> >>
> >
> >  >  > We have one EventFilter in the shell attached to the RootWindow. You
> >> can
> >> >> > add support for this there. It gets attached first, and you can
> >> >> guarantee
> >> >> > your code gets first crack.
> >> >> >
> >> >> We need to resolve at least two problems in order to integrate
> >> >> ui::InputMethod into RootWindowEventFilter:
> >> >> 1. Who should own the ui::InputMethod object and how to expose it to
> >> RWHVA
> >> >> and NWA?
> >> >>
> >> >
> >> I think we could let each RHWVA and views which need input method service
> >> own an InputMethod instance.
> >>
> >
> > Peng,
> > In this design, how could we handle a key event correctly that matches
> > Chrome
> > shortcut? I guess your design would make this more complicated.
> > See the Control+t example I mentioned earlier.
> >
> >
> Sorry. I forget the shortcut key issue. I think adding a new
> method OnKeyEventBeforeFilter() in aura:WindowDelegate can resolve this
> issue. With it, aura windows will have chance to handle all key events
> before any filters.
> 
> Peng

But the new interface still has the sync/async issue, right? Adding a new
interface that has a problem sounds like a bad idea. I'm also concerned that
having multiple instances of ui::InputMethod that are essentially unnecessary
would make things complicated. 

-Yusuke


> 
> -Yusuke
> >
> >
> >  Or make ui::InputMethod as singleton class,
> >> and let them share one instance.
> >>
> >
> >
> >
> >  > 2. How to deal with asynchronous event handling model required by ibus?
> >> >>
> >> >
> >> If views own or share ui::InputMethod instance, that will be easy. The
> >> views just need pass the the key events to the ui::InputMehtod, and the
> >> ui::InputMethod use TextInputClient(or rename to InputMethodDelegate) to
> >> send result back, like gtk or qt does. But the event path will be a little
> >> different on Linux and Windows. Anyway, it is impossible to make Linux and
> >> Window use 100% same event path even if the current design.
> >>
> >
> >
> >  > Let me give a real example of the problem 2.
> >> >
> >> > In Japanese IME, Control+t might be used for transliterating a
> >> composition
> >> > string (e.g. あ -> a) depending on its internal state. In other IMEs,
> >> > Control+t
> >> > is likely not consumed. At the same time, Control+t is the shortcut for
> >> > adding a
> >> > new tab to Chrome. If we implement IME features in an event filter, the
> >> IME
> >> > event filter must return true (= 'consumed') when the Japanese IME is
> >> > active and
> >> > in a certain state, and must return false (= 'not consumed') otherwise
> >> so
> >> > that
> >> > Chrome (ui/aura_shell/shell_****accelerator_controller.cc) could
> >> consume it.
> >>
> >> > However, it's not possible for the IME event filter to decide the
> >> correct
> >> > return
> >> > value since the filter cannot talk to ibus-daemon in a synchronous
> >> manner.
> >> >
> >> > I think I can figure out a way to workaround the issue, but it might
> >> make
> >> > the
> >> > key event flow even more complicated. Anyway, I'll give it a try.
> >> >
> >> >
> >> >  IMHO, we will make current code more complicated and harder to
> >> understand
> >> >> if we want to go this way.
> >> >>
> >> >
> >> >  And if we want to support IMM32 on Windows, we can only support it by
> >> >> integrating ui::InputMethodWin into RootWindowHostWin, because we need
> >> to
> >> >> intercept a bunch of Windows events. Then it'll make the code
> >> different on
> >> >> ChromeOS and Windows, making it harder to maintain.
> >> >>
> >> >
> >> > Oh I was not aware of the IMM32 issue..  I guess the corresponding part
> >> in
> >> > the
> >> > old views::InputMethodWin implementation is the following, correct?
> >> >
> >> > LRESULT InputMethodWin::OnImeMessages(
> >> >    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
> >> >  LRESULT result = 0;
> >> >  switch (message) {
> >> >    case WM_IME_SETCONTEXT:
> >> >      result = OnImeSetContext(message, w_param, l_param, handled);
> >> >      break;
> >> >    case WM_IME_STARTCOMPOSITION:
> >> >      result = OnImeStartComposition(message, w_param, l_param, handled);
> >> >      break;
> >> >    case WM_IME_COMPOSITION:
> >> >      result = OnImeComposition(message, w_param, l_param, handled);
> >> >      break;
> >> >    case WM_IME_ENDCOMPOSITION:
> >> >      result = OnImeEndComposition(message, w_param, l_param, handled);
> >> >      break;
> >> >    case WM_IME_REQUEST:
> >> >      result = OnImeRequest(message, w_param, l_param, handled);
> >> >      break;
> >> >    case WM_CHAR:
> >> >    case WM_SYSCHAR:
> >> >      result = OnChar(message, w_param, l_param, handled);
> >> >      break;
> >> >    case WM_DEADCHAR:
> >> >    case WM_SYSDEADCHAR:
> >> >      result = OnDeadChar(message, w_param, l_param, handled);
> >> >      break;
> >> >    default:
> >> >      NOTREACHED() << "Unknown IME message:" << message;
> >> >      break;
> >> >  }
> >> >  return result;
> >> > }
> >> >
> >> >
> >> For IMM32 supporting, aura need provide a filter to allow pre-process the
> >> native messages before translating them to aura events.
> >>
> >
> >  Peng
> >>
> >
> >  -Yusuke
> >> >
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.****org/8576005/%253Chttp://coderevi**
> > ew.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
> >
> >> >
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/8576005/%3Chttp://codereview.chromium.org/85...>
> >

Peng

On Sat, Dec 10, 2011 at 3:34 AM, <yusukes@chromium.org> wrote: > On 2011/12/10 03:07:26, Peng ...

9 years ago (2011-12-11 16:52:33 UTC) #61

On Sat, Dec 10, 2011 at 3:34 AM, <yusukes@chromium.org> wrote:

> On 2011/12/10 03:07:26, Peng wrote:
>
>  On Fri, Dec 9, 2011 at 7:52 PM, <mailto:yusukes@chromium.org> wrote:
>>
>
>  > On 2011/12/09 13:56:38, Peng wrote:
>> >
>> >  On Fri, Dec 9, 2011 at 1:30 AM, <mailto:yusukes@chromium.org> wrote:
>> >>
>> >
>> >  > On 2011/12/09 04:39:48, James Su wrote:
>> >> >
>> >> >> 在 2011年12月9日 下午12:16，Ben Goodger (Google) <ben@chromium.org>写道：
>> >> >>
>> >> >
>> >> >  > Much like none of this stuff is built into X, none of this stuff
>> >> should
>> >> >> be
>> >> >> > built into Aura.
>> >> >>
>> >> >
>> >> >  I'm wondering what's the exact reason that we cannot integrate
>> >> >> ui::InputMethod into aura?
>> >> >>
>> >> >
>> >>
>> >
>> >  >  > We have one EventFilter in the shell attached to the RootWindow.
>> You
>> >> can
>> >> >> > add support for this there. It gets attached first, and you can
>> >> >> guarantee
>> >> >> > your code gets first crack.
>> >> >> >
>> >> >> We need to resolve at least two problems in order to integrate
>> >> >> ui::InputMethod into RootWindowEventFilter:
>> >> >> 1. Who should own the ui::InputMethod object and how to expose it to
>> >> RWHVA
>> >> >> and NWA?
>> >> >>
>> >> >
>> >> I think we could let each RHWVA and views which need input method
>> service
>> >> own an InputMethod instance.
>> >>
>> >
>> > Peng,
>> > In this design, how could we handle a key event correctly that matches
>> > Chrome
>> > shortcut? I guess your design would make this more complicated.
>> > See the Control+t example I mentioned earlier.
>> >
>> >
>> Sorry. I forget the shortcut key issue. I think adding a new
>> method OnKeyEventBeforeFilter() in aura:WindowDelegate can resolve this
>> issue. With it, aura windows will have chance to handle all key events
>> before any filters.
>>
>
>  Peng
>>
>
> But the new interface still has the sync/async issue, right? Adding a new
> interface that has a problem sounds like a bad idea. I'm also concerned
> that
> having multiple instances of ui::InputMethod that are essentially
> unnecessary
> would make things complicated.



For async communication, Aura just need provide an API to allow sending
unconsumed events back to aura::RootWindow. So that RootWindow can continue
processing accelerator and dispatching events to target aura window.

I do not insist this proposal, because we already have the implementation
in views. Just want to give an alternative idea for considering and
comparing.

Thanks,
Peng


> -Yusuke
>
>
>
>  -Yusuke
>> >
>> >
>> >  Or make ui::InputMethod as singleton class,
>> >> and let them share one instance.
>> >>
>> >
>> >
>> >
>> >  > 2. How to deal with asynchronous event handling model required by
>> ibus?
>> >> >>
>> >> >
>> >> If views own or share ui::InputMethod instance, that will be easy. The
>> >> views just need pass the the key events to the ui::InputMehtod, and the
>> >> ui::InputMethod use TextInputClient(or rename to InputMethodDelegate)
>> to
>> >> send result back, like gtk or qt does. But the event path will be a
>> little
>> >> different on Linux and Windows. Anyway, it is impossible to make Linux
>> and
>> >> Window use 100% same event path even if the current design.
>> >>
>> >
>> >
>> >  > Let me give a real example of the problem 2.
>> >> >
>> >> > In Japanese IME, Control+t might be used for transliterating a
>> >> composition
>> >> > string (e.g. あ -> a) depending on its internal state. In other IMEs,
>> >> > Control+t
>> >> > is likely not consumed. At the same time, Control+t is the shortcut
>> for
>> >> > adding a
>> >> > new tab to Chrome. If we implement IME features in an event filter,
>> the
>> >> IME
>> >> > event filter must return true (= 'consumed') when the Japanese IME is
>> >> > active and
>> >> > in a certain state, and must return false (= 'not consumed')
>> otherwise
>> >> so
>> >> > that
>> >> > Chrome (ui/aura_shell/shell_******accelerator_controller.cc) could
>>
>> >> consume it.
>> >>
>> >> > However, it's not possible for the IME event filter to decide the
>> >> correct
>> >> > return
>> >> > value since the filter cannot talk to ibus-daemon in a synchronous
>> >> manner.
>> >> >
>> >> > I think I can figure out a way to workaround the issue, but it might
>> >> make
>> >> > the
>> >> > key event flow even more complicated. Anyway, I'll give it a try.
>> >> >
>> >> >
>> >> >  IMHO, we will make current code more complicated and harder to
>> >> understand
>> >> >> if we want to go this way.
>> >> >>
>> >> >
>> >> >  And if we want to support IMM32 on Windows, we can only support it
>> by
>> >> >> integrating ui::InputMethodWin into RootWindowHostWin, because we
>> need
>> >> to
>> >> >> intercept a bunch of Windows events. Then it'll make the code
>> >> different on
>> >> >> ChromeOS and Windows, making it harder to maintain.
>> >> >>
>> >> >
>> >> > Oh I was not aware of the IMM32 issue..  I guess the corresponding
>> part
>> >> in
>> >> > the
>> >> > old views::InputMethodWin implementation is the following, correct?
>> >> >
>> >> > LRESULT InputMethodWin::OnImeMessages(
>> >> >    UINT message, WPARAM w_param, LPARAM l_param, BOOL* handled) {
>> >> >  LRESULT result = 0;
>> >> >  switch (message) {
>> >> >    case WM_IME_SETCONTEXT:
>> >> >      result = OnImeSetContext(message, w_param, l_param, handled);
>> >> >      break;
>> >> >    case WM_IME_STARTCOMPOSITION:
>> >> >      result = OnImeStartComposition(message, w_param, l_param,
>> handled);
>> >> >      break;
>> >> >    case WM_IME_COMPOSITION:
>> >> >      result = OnImeComposition(message, w_param, l_param, handled);
>> >> >      break;
>> >> >    case WM_IME_ENDCOMPOSITION:
>> >> >      result = OnImeEndComposition(message, w_param, l_param,
>> handled);
>> >> >      break;
>> >> >    case WM_IME_REQUEST:
>> >> >      result = OnImeRequest(message, w_param, l_param, handled);
>> >> >      break;
>> >> >    case WM_CHAR:
>> >> >    case WM_SYSCHAR:
>> >> >      result = OnChar(message, w_param, l_param, handled);
>> >> >      break;
>> >> >    case WM_DEADCHAR:
>> >> >    case WM_SYSDEADCHAR:
>> >> >      result = OnDeadChar(message, w_param, l_param, handled);
>> >> >      break;
>> >> >    default:
>> >> >      NOTREACHED() << "Unknown IME message:" << message;
>> >> >      break;
>> >> >  }
>> >> >  return result;
>> >> > }
>> >> >
>> >> >
>> >> For IMM32 supporting, aura need provide a filter to allow pre-process
>> the
>> >> native messages before translating them to aura events.
>> >>
>> >
>> >  Peng
>> >>
>> >
>> >  -Yusuke
>> >> >
>> >> >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.******org/8576005/%253Chttp://**coderevi**
>> > ew.chromium.org/8576005/
<http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>> >>
>> >
>> >> >
>> >>
>> >
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/8576005/%3Chttp://coderevi**
> ew.chromium.org/8576005/ <http://codereview.chromium.org/8576005/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8576005/<http://codereview.chromium.org/8576...
>

Yusuke Sato

Ben: I've moved most of the IME code to aura_shell/ using the EventFilter interface in ...

9 years ago (2011-12-12 07:51:53 UTC) #62

Ben Goodger (Google)

In general like the approach. A few comments follow. http://codereview.chromium.org/8576005/diff/101001/content/public/browser/native_web_keyboard_event.h File content/public/browser/native_web_keyboard_event.h (right): http://codereview.chromium.org/8576005/diff/101001/content/public/browser/native_web_keyboard_event.h#newcode33 content/public/browser/native_web_keyboard_event.h:33: ...

9 years ago (2011-12-12 16:51:02 UTC) #63

James Su

Hello Ben, Please allow me to express my opinion again: I do believe integrating ui::InputMethodXxx ...

9 years ago (2011-12-13 04:03:44 UTC) #64

Hello Ben,

Please allow me to express my opinion again: I do believe integrating
ui::InputMethodXxx into aura::RootWindowHostXxx is the correct way we should go.

Following are my arguments:
1. Looks like we are going to use aura on MacOSX as well. So we will support
three platforms: ChromeOS, Windows and MacOSX. (How about Linux desktop?)

2. As Yusuke already said, it'd be a huge effort for us to develop a specific
IME system for aura. So in the foreseeable future, we will continue to depend on
the IME system provided by each platform: ibus on ChromeOS, IMM32(TSF) on
Windows and InputMethod Kit Framework on MacOSX.

3. From an IME system's point of view, the Chrome as a whole is a client
application.

4. Inside Chrome, in order to avoid dealing with different IME systems
everywhere, we introduced an abstract layer, which consists of two interfaces:
ui::TextInputClient and ui::InputMethod, and several implementation classes:
ui::InputMethodXxx. Much like what we've done for native window and event types
in aura.

5. The IME systems are more or less bound to platforms' native window and event
systems: on ChromeOS, ibus depends on X Window's key event definition. On
Windows, IMM32(TSF) is tightly bound to HWND. On MacOSX, IME support is tightly
bound to NSView through NSTextInputClient protocol.

6. In aura world, aura::RootWindowHost is the single point for abstracting
platform differences. It's the only place that we can freely access platform's
native window and event API.

7. So it's pretty nature to integrate ui::InputMethodXxx into
aura::RootWindowHostXxx, because: 1) they are both abstract layers for hiding
platform differences; 2) ui::InputMethodXxx needs to access platform's native
API.

8. According to aura design doc, aura_shell is optional for Windows and MacOSX
(is it correct?), but IME support is mandatory on all platforms. So it makes no
sense to integrate ui::InputMethodXxx into aura_shell, despite of the reason
stated in item 7.

9. Although it may look ok to integrate ui::InputMethodIBus into aura_shell,
because ibus doesn't depend on X library (though it depends on X key event
definition). It's not possible for ui::InputMethodWin and ui::InputMethodMac in
the future. So if we insist on integrating ui::InputMethodIBus into aura_shell,
we must use different approaches for Windows and MacOSX, which is obviously not
a good way to go.

Conclusion: integrating ui::InputMethodXxx into aura::RootWindowHostXxx is the
best way to go.

Hope my explanation is enough. But please give me your arguments if you still
think we should do it aura_shell.

Best Regards
James Su

On 2011/12/12 16:51:02, Ben Goodger (Google) wrote:
> In general like the approach. A few comments follow.
> 
>
http://codereview.chromium.org/8576005/diff/101001/content/public/browser/nat...
> File content/public/browser/native_web_keyboard_event.h (right):
> 
>
http://codereview.chromium.org/8576005/diff/101001/content/public/browser/nat...
> content/public/browser/native_web_keyboard_event.h:33: #if defined(USE_AURA)
> I believe you may want to run this before the ones above it:
> 
> #if defined(USE_AURA)
> ..
> #elif defined(OS_MACOSX) || defined(TOOLKIT_USES_GTK)
> ..
> #endif
> 
> since dhollowa@ is getting aura to build with OS_MACOSX.
> 
> http://codereview.chromium.org/8576005/diff/101001/ui/aura/event.h
> File ui/aura/event.h (right):
> 
> http://codereview.chromium.org/8576005/diff/101001/ui/aura/event.h#newcode183
> ui/aura/event.h:183: bool skip_ime_;
> On 2011/12/12 07:51:53, Yusuke Sato wrote:
> > This flag is for resoving the sync/async issue.
> 
> It'd be good to come up with a neater way to express this. What this really
> means is, "when true, the event is the result of an IME translation". Can this
> occur with any type of key event (e.g. both KeyDown and KeyUp)? Seems like you
> could make an argument that this is a new event type. Windows treats it as
such
> (WM_IME_CHAR).
> 
> http://codereview.chromium.org/8576005/diff/101001/ui/aura/root_window.h
> File ui/aura/root_window.h (right):
> 
>
http://codereview.chromium.org/8576005/diff/101001/ui/aura/root_window.h#newc...
> ui/aura/root_window.h:69: void SetInputMethod(ui::InputMethod* input_method);
> The Shell can actually set this on the RootWindow via a property.
> 
> Because you need to access this value from views and content, the property
name
> can live in src/ui/aura/client/aura_constants.h/cc.
> 
>
http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/input_method...
> File ui/aura_shell/input_method_event_filter.cc (right):
> 
>
http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/input_method...
> ui/aura_shell/input_method_event_filter.cc:27: input_method = new
> ui::InputMethodIBus(this);
> I've seen this pattern (and the associated includes) in a few places. Have you
> considered having a utility header/.cc in ui/base/ime called
> input_method_factory that defines ui::CreateInputMethod(..); that hides these
> ifdefs?
> 
> http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/shell.cc
> File ui/aura_shell/shell.cc (right):
> 
>
http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/shell.cc#new...
> ui/aura_shell/shell.cc:240: // filter has the highest priority.
> To enforce this, you might want to expose a has_event_filters_registered()
> method on RootWindow, and then DCHECK(!hefr()) here.

Ben Goodger (Google)

At this time, Windows and Mac support is preliminary and meant only for developer use/testing ...

9 years ago (2011-12-13 04:33:50 UTC) #65

At this time, Windows and Mac support is preliminary and meant only for
developer use/testing and debugging. Addressed the rest of your comments
inline below:

On Mon, Dec 12, 2011 at 8:03 PM, <suzhe@chromium.org> wrote:

> Hello Ben,
>
> Please allow me to express my opinion again: I do believe integrating
> ui::InputMethodXxx into aura::RootWindowHostXxx is the correct way we
> should go.
>
> Following are my arguments:
> 1. Looks like we are going to use aura on MacOSX as well. So we will
> support
> three platforms: ChromeOS, Windows and MacOSX. (How about Linux desktop?)
>

There aren't clear product directives for non-ChromeOS yet, they exist
primarily for testing and debugging.

> 2. As Yusuke already said, it'd be a huge effort for us to develop a
> specific
> IME system for aura. So in the foreseeable future, we will continue to
> depend on
> the IME system provided by each platform: ibus on ChromeOS, IMM32(TSF) on
> Windows and InputMethod Kit Framework on MacOSX.
>

This is fine. Not every aspect of Cros is available on Windows yet. We are
still determining what (if any) product mandate will materialize.

> 4. Inside Chrome, in order to avoid dealing with different IME systems
> everywhere, we introduced an abstract layer, which consists of two
> interfaces:
> ui::TextInputClient and ui::InputMethod, and several implementation
> classes:
> ui::InputMethodXxx. Much like what we've done for native window and event
> types
> in aura.
>
> 5. The IME systems are more or less bound to platforms' native window and
> event
> systems: on ChromeOS, ibus depends on X Window's key event definition.
>

While it's not on the agenda for the next few quarters, you should
definitely be aware that once we've isolated X11 usage in Chrome, there are
many that would like to cut the cord completely. So any dependency we have
on X11 is likely to become a liability at some point. I don't think this
affects this CL though so this is just an FYI.

> 6. In aura world, aura::RootWindowHost is the single point for abstracting
> platform differences. It's the only place that we can freely access
> platform's
> native window and event API.
>

This is not quite right. RootWindowHost is the single point for abstracting
platform differences relating to the window hierarchy. If you look at my
checkins over the past couple of weeks, and some design decisions made in
Aura, you'll see that RootWindow/Host is a lot less like
views::NativeWidget/Widget than it might look at first glance. Rather than
being a singleton instance integrating various functionality (cross
platform and platform specific), it is meant to simply be the root of the
window hierarchy, controlling rendering and basic event dispatch. My
assertion is that IME, with its links into significant subsystems (ibus,
shell UI to show IME candidate windows) steps beyond this basic
functionality and is something better implemented in the shell.

> 8. According to aura design doc, aura_shell is optional for Windows and
> MacOSX
> (is it correct?), but IME support is mandatory on all platforms. So it
> makes no
> sense to integrate ui::InputMethodXxx into aura_shell, despite of the
> reason
> stated in item 7.
>

IME support may differ depending on whether or not Aura shell is used. For
example there are two potential products on Windows. One is an Aura
Shell-like environment that presents the entire ChromeOS UI. This would
include the ChromeOS IME candidate window UI and subsystem, and not present
any of the Windows UI. The other is a desktop Chrome like experience using
Aura to provide hardware accelerated UI transitions. This would
(potentially) use the Windows IME UI.

> 9. Although it may look ok to integrate ui::InputMethodIBus into
> aura_shell,
> because ibus doesn't depend on X library (though it depends on X key event
> definition). It's not possible for ui::InputMethodWin and
> ui::InputMethodMac in
> the future. So if we insist on integrating ui::InputMethodIBus into
> aura_shell,
> we must use different approaches for Windows and MacOSX, which is
> obviously not
> a good way to go.
>

The Mac side is a little nebulous so I am deferring thinking about it. As
to InputMethodWin, what difficulties do you see? One that I can see is that
the native system UI may not support the window transitions that we do, so
that if we ever transform a textfield, the candidate window if anchored to
that textfield may appear in the wrong position.

-Ben

Ben Goodger (Google)

Long story short: think of src/ui/aura as the equivalent of the X11 client API. It's ...

9 years ago (2011-12-13 04:48:07 UTC) #66

Long story short: think of src/ui/aura as the equivalent of the X11 client
API. It's not the platform abstraction, it's just the window hierarchy.

-Ben

On Mon, Dec 12, 2011 at 8:33 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> At this time, Windows and Mac support is preliminary and meant only for
> developer use/testing and debugging. Addressed the rest of your comments
> inline below:
>
> On Mon, Dec 12, 2011 at 8:03 PM, <suzhe@chromium.org> wrote:
>
>> Hello Ben,
>>
>> Please allow me to express my opinion again: I do believe integrating
>> ui::InputMethodXxx into aura::RootWindowHostXxx is the correct way we
>> should go.
>>
>> Following are my arguments:
>> 1. Looks like we are going to use aura on MacOSX as well. So we will
>> support
>> three platforms: ChromeOS, Windows and MacOSX. (How about Linux desktop?)
>>
>
> There aren't clear product directives for non-ChromeOS yet, they exist
> primarily for testing and debugging.
>
>
>> 2. As Yusuke already said, it'd be a huge effort for us to develop a
>> specific
>> IME system for aura. So in the foreseeable future, we will continue to
>> depend on
>> the IME system provided by each platform: ibus on ChromeOS, IMM32(TSF) on
>> Windows and InputMethod Kit Framework on MacOSX.
>>
>
> This is fine. Not every aspect of Cros is available on Windows yet. We are
> still determining what (if any) product mandate will materialize.
>
>
>> 4. Inside Chrome, in order to avoid dealing with different IME systems
>> everywhere, we introduced an abstract layer, which consists of two
>> interfaces:
>> ui::TextInputClient and ui::InputMethod, and several implementation
>> classes:
>> ui::InputMethodXxx. Much like what we've done for native window and event
>> types
>> in aura.
>>
>> 5. The IME systems are more or less bound to platforms' native window and
>> event
>> systems: on ChromeOS, ibus depends on X Window's key event definition.
>>
>
> While it's not on the agenda for the next few quarters, you should
> definitely be aware that once we've isolated X11 usage in Chrome, there are
> many that would like to cut the cord completely. So any dependency we have
> on X11 is likely to become a liability at some point. I don't think this
> affects this CL though so this is just an FYI.
>
>
>> 6. In aura world, aura::RootWindowHost is the single point for abstracting
>> platform differences. It's the only place that we can freely access
>> platform's
>> native window and event API.
>>
>
> This is not quite right. RootWindowHost is the single point for
> abstracting platform differences relating to the window hierarchy. If you
> look at my checkins over the past couple of weeks, and some design
> decisions made in Aura, you'll see that RootWindow/Host is a lot less like
> views::NativeWidget/Widget than it might look at first glance. Rather than
> being a singleton instance integrating various functionality (cross
> platform and platform specific), it is meant to simply be the root of the
> window hierarchy, controlling rendering and basic event dispatch. My
> assertion is that IME, with its links into significant subsystems (ibus,
> shell UI to show IME candidate windows) steps beyond this basic
> functionality and is something better implemented in the shell.
>
>
>
>> 8. According to aura design doc, aura_shell is optional for Windows and
>> MacOSX
>> (is it correct?), but IME support is mandatory on all platforms. So it
>> makes no
>> sense to integrate ui::InputMethodXxx into aura_shell, despite of the
>> reason
>> stated in item 7.
>>
>
> IME support may differ depending on whether or not Aura shell is used. For
> example there are two potential products on Windows. One is an Aura
> Shell-like environment that presents the entire ChromeOS UI. This would
> include the ChromeOS IME candidate window UI and subsystem, and not present
> any of the Windows UI. The other is a desktop Chrome like experience using
> Aura to provide hardware accelerated UI transitions. This would
> (potentially) use the Windows IME UI.
>
>
>> 9. Although it may look ok to integrate ui::InputMethodIBus into
>> aura_shell,
>> because ibus doesn't depend on X library (though it depends on X key event
>> definition). It's not possible for ui::InputMethodWin and
>> ui::InputMethodMac in
>> the future. So if we insist on integrating ui::InputMethodIBus into
>> aura_shell,
>> we must use different approaches for Windows and MacOSX, which is
>> obviously not
>> a good way to go.
>>
>
> The Mac side is a little nebulous so I am deferring thinking about it. As
> to InputMethodWin, what difficulties do you see? One that I can see is that
> the native system UI may not support the window transitions that we do, so
> that if we ever transform a textfield, the candidate window if anchored to
> that textfield may appear in the wrong position.
>
> -Ben
>

Yusuke Sato

On 2011/12/13 04:48:07, Ben Goodger (Google) wrote: > Long story short: think of src/ui/aura as ...

9 years ago (2011-12-13 05:01:07 UTC) #67

On 2011/12/13 04:48:07, Ben Goodger (Google) wrote:
> Long story short: think of src/ui/aura as the equivalent of the X11 client
> API. It's not the platform abstraction, it's just the window hierarchy.
> 
> -Ben
> 
> On Mon, Dec 12, 2011 at 8:33 PM, Ben Goodger (Google) <ben@chromium.org>wrote:
> 
> > At this time, Windows and Mac support is preliminary and meant only for
> > developer use/testing and debugging. Addressed the rest of your comments
> > inline below:
> >
> > On Mon, Dec 12, 2011 at 8:03 PM, <mailto:suzhe@chromium.org> wrote:
> >
> >> Hello Ben,
> >>
> >> Please allow me to express my opinion again: I do believe integrating
> >> ui::InputMethodXxx into aura::RootWindowHostXxx is the correct way we
> >> should go.
> >>
> >> Following are my arguments:
> >> 1. Looks like we are going to use aura on MacOSX as well. So we will
> >> support
> >> three platforms: ChromeOS, Windows and MacOSX. (How about Linux desktop?)
> >>
> >
> > There aren't clear product directives for non-ChromeOS yet, they exist
> > primarily for testing and debugging.
> >
> >
> >> 2. As Yusuke already said, it'd be a huge effort for us to develop a
> >> specific
> >> IME system for aura. So in the foreseeable future, we will continue to
> >> depend on
> >> the IME system provided by each platform: ibus on ChromeOS, IMM32(TSF) on
> >> Windows and InputMethod Kit Framework on MacOSX.
> >>
> >
> > This is fine. Not every aspect of Cros is available on Windows yet. We are
> > still determining what (if any) product mandate will materialize.
> >
> >
> >> 4. Inside Chrome, in order to avoid dealing with different IME systems
> >> everywhere, we introduced an abstract layer, which consists of two
> >> interfaces:
> >> ui::TextInputClient and ui::InputMethod, and several implementation
> >> classes:
> >> ui::InputMethodXxx. Much like what we've done for native window and event
> >> types
> >> in aura.
> >>
> >> 5. The IME systems are more or less bound to platforms' native window and
> >> event
> >> systems: on ChromeOS, ibus depends on X Window's key event definition.
> >>
> >
> > While it's not on the agenda for the next few quarters, you should
> > definitely be aware that once we've isolated X11 usage in Chrome, there are
> > many that would like to cut the cord completely. So any dependency we have
> > on X11 is likely to become a liability at some point. I don't think this
> > affects this CL though so this is just an FYI.
> >
> >
> >> 6. In aura world, aura::RootWindowHost is the single point for abstracting
> >> platform differences. It's the only place that we can freely access
> >> platform's
> >> native window and event API.
> >>
> >
> > This is not quite right. RootWindowHost is the single point for
> > abstracting platform differences relating to the window hierarchy. If you
> > look at my checkins over the past couple of weeks, and some design
> > decisions made in Aura, you'll see that RootWindow/Host is a lot less like
> > views::NativeWidget/Widget than it might look at first glance. Rather than
> > being a singleton instance integrating various functionality (cross
> > platform and platform specific), it is meant to simply be the root of the
> > window hierarchy, controlling rendering and basic event dispatch. My
> > assertion is that IME, with its links into significant subsystems (ibus,
> > shell UI to show IME candidate windows) steps beyond this basic
> > functionality and is something better implemented in the shell.
> >
> >
> >
> >> 8. According to aura design doc, aura_shell is optional for Windows and
> >> MacOSX
> >> (is it correct?), but IME support is mandatory on all platforms. So it
> >> makes no
> >> sense to integrate ui::InputMethodXxx into aura_shell, despite of the
> >> reason
> >> stated in item 7.
> >>
> >
> > IME support may differ depending on whether or not Aura shell is used. For
> > example there are two potential products on Windows. One is an Aura
> > Shell-like environment that presents the entire ChromeOS UI. This would
> > include the ChromeOS IME candidate window UI and subsystem, and not present
> > any of the Windows UI. The other is a desktop Chrome like experience using
> > Aura to provide hardware accelerated UI transitions. This would
> > (potentially) use the Windows IME UI.
> >

Ah this is a very good point. I agree that we likely have to have two different
ui::InputMethod implementations for Windows for the two potential products.

> >
> >> 9. Although it may look ok to integrate ui::InputMethodIBus into
> >> aura_shell,
> >> because ibus doesn't depend on X library (though it depends on X key event
> >> definition). It's not possible for ui::InputMethodWin and
> >> ui::InputMethodMac in
> >> the future. So if we insist on integrating ui::InputMethodIBus into
> >> aura_shell,
> >> we must use different approaches for Windows and MacOSX, which is
> >> obviously not
> >> a good way to go.
> >>
> >
> > The Mac side is a little nebulous so I am deferring thinking about it. As
> > to InputMethodWin, what difficulties do you see? One that I can see is that
> > the native system UI may not support the window transitions that we do, so
> > that if we ever transform a textfield, the candidate window if anchored to
> > that textfield may appear in the wrong position.

The problem he mentioned earlier in this thread was that ui::InputMethodWin has
to handle raw Windows events like WM_IME_SETCONTEXT, WM_IME_STARTCOMPOSITION,
WM_IME_COMPOSITION, WM_IME_ENDCOMPOSITION, and WM_IME_REQUEST, and
RootWindowHost would be the best place to intercept such events.

However, I think we could solve the problem by e.g. implementing a new Aura
event called ImeEvent and encapsulate OS dependent parameters like WPARAM and
LPARAM in it, creating an ImeEvent object in RootWindowHostWin, and passing it
to RootWindow, then an EventFilter.

-Yusuke

Ben Goodger (Google)

On Mon, Dec 12, 2011 at 9:01 PM, <yusukes@chromium.org> wrote: > However, I think we ...

9 years ago (2011-12-13 05:13:36 UTC) #68

James Su

Please read my feedback inline. 在 2011年12月13日下午12:33，Ben Goodger (Google) <ben@chromium.org>写道： > At this time, ...

9 years ago (2011-12-13 08:53:27 UTC) #69

Please read my feedback inline.

在 2011年12月13日 下午12:33，Ben Goodger (Google) <ben@chromium.org>写道：

> At this time, Windows and Mac support is preliminary and meant only for
> developer use/testing and debugging. Addressed the rest of your comments
> inline below:
>
> On Mon, Dec 12, 2011 at 8:03 PM, <suzhe@chromium.org> wrote:
>
>> Hello Ben,
>>
>> Please allow me to express my opinion again: I do believe integrating
>> ui::InputMethodXxx into aura::RootWindowHostXxx is the correct way we
>> should go.
>>
>> Following are my arguments:
>> 1. Looks like we are going to use aura on MacOSX as well. So we will
>> support
>> three platforms: ChromeOS, Windows and MacOSX. (How about Linux desktop?)
>>
>
> There aren't clear product directives for non-ChromeOS yet, they exist
> primarily for testing and debugging.
>
>
>> 2. As Yusuke already said, it'd be a huge effort for us to develop a
>> specific
>> IME system for aura. So in the foreseeable future, we will continue to
>> depend on
>> the IME system provided by each platform: ibus on ChromeOS, IMM32(TSF) on
>> Windows and InputMethod Kit Framework on MacOSX.
>>
>
> This is fine. Not every aspect of Cros is available on Windows yet. We are
> still determining what (if any) product mandate will materialize.
>
>
>> 4. Inside Chrome, in order to avoid dealing with different IME systems
>> everywhere, we introduced an abstract layer, which consists of two
>> interfaces:
>> ui::TextInputClient and ui::InputMethod, and several implementation
>> classes:
>> ui::InputMethodXxx. Much like what we've done for native window and event
>> types
>> in aura.
>>
>> 5. The IME systems are more or less bound to platforms' native window and
>> event
>> systems: on ChromeOS, ibus depends on X Window's key event definition.
>>
>
> While it's not on the agenda for the next few quarters, you should
> definitely be aware that once we've isolated X11 usage in Chrome, there are
> many that would like to cut the cord completely. So any dependency we have
> on X11 is likely to become a liability at some point. I don't think this
> affects this CL though so this is just an FYI.
>
Agree. On ChromeOS we can continue to use ibus, as it doesn't depend on X
library. When we move off X11 completely (e.g. use Wayland instead) we may
need to convert the platform event into X key event definition so that ibus
can recognize it. We always need to do such conversion if the platform
event definition is different than X definition no matter where we
integrate ui::InputMethodIBus.


>
>
>> 6. In aura world, aura::RootWindowHost is the single point for abstracting
>> platform differences. It's the only place that we can freely access
>> platform's
>> native window and event API.
>>
>
> This is not quite right. RootWindowHost is the single point for
> abstracting platform differences relating to the window hierarchy. If you
> look at my checkins over the past couple of weeks, and some design
> decisions made in Aura, you'll see that RootWindow/Host is a lot less like
> views::NativeWidget/Widget than it might look at first glance. Rather than
> being a singleton instance integrating various functionality (cross
> platform and platform specific), it is meant to simply be the root of the
> window hierarchy, controlling rendering and basic event dispatch. My
> assertion is that IME, with its links into significant subsystems (ibus,
> shell UI to show IME candidate windows) steps beyond this basic
> functionality and is something better implemented in the shell.
>
Though ui::InputMethod* communicate with platform IME systems, they are not
part of the platform IME systems. They are just abstract layers to hide the
actual IME systems, similar than aura::KeyEvent for hiding native event
types and aura::RootWindowHost for hiding native window. And more
importantly ui::InputMethod* are actually very thin, the only work they do
is to convert/forward events between platform IME system and upper level
Chrome client code.
On the other hand, I agree that we should integrate other part of IME
system (e.g. IME UI) into aura_shell when appropriate.


>
>
>
>> 8. According to aura design doc, aura_shell is optional for Windows and
>> MacOSX
>> (is it correct?), but IME support is mandatory on all platforms. So it
>> makes no
>> sense to integrate ui::InputMethodXxx into aura_shell, despite of the
>> reason
>> stated in item 7.
>>
>
> IME support may differ depending on whether or not Aura shell is used. For
> example there are two potential products on Windows. One is an Aura
> Shell-like environment that presents the entire ChromeOS UI. This would
> include the ChromeOS IME candidate window UI and subsystem, and not present
> any of the Windows UI. The other is a desktop Chrome like experience using
> Aura to provide hardware accelerated UI transitions. This would
> (potentially) use the Windows IME UI.
>
I agree that we should integrate our own IME UI into aura_shell and make
use of it if the platform allow us to do so. And using our own IME UI is
actually optional if the platform IME system provides its own UI (from
functionalities' point of view).
But ui::InputMethod* are mandatory for us to support IME in Chrome.


>
>
>> 9. Although it may look ok to integrate ui::InputMethodIBus into
>> aura_shell,
>> because ibus doesn't depend on X library (though it depends on X key event
>> definition). It's not possible for ui::InputMethodWin and
>> ui::InputMethodMac in
>> the future. So if we insist on integrating ui::InputMethodIBus into
>> aura_shell,
>> we must use different approaches for Windows and MacOSX, which is
>> obviously not
>> a good way to go.
>>
>
> The Mac side is a little nebulous so I am deferring thinking about it. As
> to InputMethodWin, what difficulties do you see? One that I can see is that
> the native system UI may not support the window transitions that we do, so
> that if we ever transform a textfield, the candidate window if anchored to
> that textfield may appear in the wrong position.
>
The difficulty is ui::InputMethodWin needs intercept a bunch of windows
messages (e.g. WM_IME_START_COMPOSITION) and call a bunch of windows
functions which are bound to HWND. We cannot simply do it in aura_shell.
I'm actually not afraid of the IME UI too much. As long as we are not going
to rotate a textfield to a strange degree, we can always position the IME
UI nicely.


>
> -Ben
>



-- 
- James Su

Ben Goodger (Google)

Hi James, Thanks for the detailed feedback as always. You raise a number of interesting ...

9 years ago (2011-12-13 17:23:13 UTC) #70

Hi James,

Thanks for the detailed feedback as always. You raise a number of
interesting technical challenges that we will doubtless have to consider in
the fullness of time.

With that said, I believe the right approach for right now is to continue
the work that Yusuke is doing in his most recent CL, and so that's the
approach I'm going to endorse. Further along, when it comes time to
evaluate more platform requirements, we can look to see if and how we need
to make adjustments. Nothing in Chrome is set in stone for all eternity.

Yusuke, please continue with your CL, see my most recent comments on your
idea of a different event type for post-processed key events.

-Ben

On Tue, Dec 13, 2011 at 12:53 AM, James Su <suzhe@chromium.org> wrote:

> Please read my feedback inline.
>
> 在 2011年12月13日 下午12:33，Ben Goodger (Google) <ben@chromium.org>写道：
>
> At this time, Windows and Mac support is preliminary and meant only for
>> developer use/testing and debugging. Addressed the rest of your comments
>> inline below:
>>
>> On Mon, Dec 12, 2011 at 8:03 PM, <suzhe@chromium.org> wrote:
>>
>>> Hello Ben,
>>>
>>> Please allow me to express my opinion again: I do believe integrating
>>> ui::InputMethodXxx into aura::RootWindowHostXxx is the correct way we
>>> should go.
>>>
>>> Following are my arguments:
>>> 1. Looks like we are going to use aura on MacOSX as well. So we will
>>> support
>>> three platforms: ChromeOS, Windows and MacOSX. (How about Linux desktop?)
>>>
>>
>> There aren't clear product directives for non-ChromeOS yet, they exist
>> primarily for testing and debugging.
>>
>>
>>> 2. As Yusuke already said, it'd be a huge effort for us to develop a
>>> specific
>>> IME system for aura. So in the foreseeable future, we will continue to
>>> depend on
>>> the IME system provided by each platform: ibus on ChromeOS, IMM32(TSF) on
>>> Windows and InputMethod Kit Framework on MacOSX.
>>>
>>
>> This is fine. Not every aspect of Cros is available on Windows yet. We
>> are still determining what (if any) product mandate will materialize.
>>
>>
>>> 4. Inside Chrome, in order to avoid dealing with different IME systems
>>> everywhere, we introduced an abstract layer, which consists of two
>>> interfaces:
>>> ui::TextInputClient and ui::InputMethod, and several implementation
>>> classes:
>>> ui::InputMethodXxx. Much like what we've done for native window and
>>> event types
>>> in aura.
>>>
>>> 5. The IME systems are more or less bound to platforms' native window
>>> and event
>>> systems: on ChromeOS, ibus depends on X Window's key event definition.
>>>
>>
>> While it's not on the agenda for the next few quarters, you should
>> definitely be aware that once we've isolated X11 usage in Chrome, there are
>> many that would like to cut the cord completely. So any dependency we have
>> on X11 is likely to become a liability at some point. I don't think this
>> affects this CL though so this is just an FYI.
>>
> Agree. On ChromeOS we can continue to use ibus, as it doesn't depend on X
> library. When we move off X11 completely (e.g. use Wayland instead) we may
> need to convert the platform event into X key event definition so that ibus
> can recognize it. We always need to do such conversion if the platform
> event definition is different than X definition no matter where we
> integrate ui::InputMethodIBus.
>
>
>>
>>
>>> 6. In aura world, aura::RootWindowHost is the single point for
>>> abstracting
>>> platform differences. It's the only place that we can freely access
>>> platform's
>>> native window and event API.
>>>
>>
>> This is not quite right. RootWindowHost is the single point for
>> abstracting platform differences relating to the window hierarchy. If you
>> look at my checkins over the past couple of weeks, and some design
>> decisions made in Aura, you'll see that RootWindow/Host is a lot less like
>> views::NativeWidget/Widget than it might look at first glance. Rather than
>> being a singleton instance integrating various functionality (cross
>> platform and platform specific), it is meant to simply be the root of the
>> window hierarchy, controlling rendering and basic event dispatch. My
>> assertion is that IME, with its links into significant subsystems (ibus,
>> shell UI to show IME candidate windows) steps beyond this basic
>> functionality and is something better implemented in the shell.
>>
> Though ui::InputMethod* communicate with platform IME systems, they are
> not part of the platform IME systems. They are just abstract layers to hide
> the actual IME systems, similar than aura::KeyEvent for hiding native event
> types and aura::RootWindowHost for hiding native window. And more
> importantly ui::InputMethod* are actually very thin, the only work they do
> is to convert/forward events between platform IME system and upper level
> Chrome client code.
> On the other hand, I agree that we should integrate other part of IME
> system (e.g. IME UI) into aura_shell when appropriate.
>
>
>>
>>
>>
>>> 8. According to aura design doc, aura_shell is optional for Windows and
>>> MacOSX
>>> (is it correct?), but IME support is mandatory on all platforms. So it
>>> makes no
>>> sense to integrate ui::InputMethodXxx into aura_shell, despite of the
>>> reason
>>> stated in item 7.
>>>
>>
>> IME support may differ depending on whether or not Aura shell is used.
>> For example there are two potential products on Windows. One is an Aura
>> Shell-like environment that presents the entire ChromeOS UI. This would
>> include the ChromeOS IME candidate window UI and subsystem, and not present
>> any of the Windows UI. The other is a desktop Chrome like experience using
>> Aura to provide hardware accelerated UI transitions. This would
>> (potentially) use the Windows IME UI.
>>
> I agree that we should integrate our own IME UI into aura_shell and make
> use of it if the platform allow us to do so. And using our own IME UI is
> actually optional if the platform IME system provides its own UI (from
> functionalities' point of view).
> But ui::InputMethod* are mandatory for us to support IME in Chrome.
>
>
>>
>>
>>> 9. Although it may look ok to integrate ui::InputMethodIBus into
>>> aura_shell,
>>> because ibus doesn't depend on X library (though it depends on X key
>>> event
>>> definition). It's not possible for ui::InputMethodWin and
>>> ui::InputMethodMac in
>>> the future. So if we insist on integrating ui::InputMethodIBus into
>>> aura_shell,
>>> we must use different approaches for Windows and MacOSX, which is
>>> obviously not
>>> a good way to go.
>>>
>>
>> The Mac side is a little nebulous so I am deferring thinking about it. As
>> to InputMethodWin, what difficulties do you see? One that I can see is that
>> the native system UI may not support the window transitions that we do, so
>> that if we ever transform a textfield, the candidate window if anchored to
>> that textfield may appear in the wrong position.
>>
> The difficulty is ui::InputMethodWin needs intercept a bunch of windows
> messages (e.g. WM_IME_START_COMPOSITION) and call a bunch of windows
> functions which are bound to HWND. We cannot simply do it in aura_shell.
> I'm actually not afraid of the IME UI too much. As long as we are not
> going to rotate a textfield to a strange degree, we can always position the
> IME UI nicely.
>
>
>>
>> -Ben
>>
>
>
>
> --
> - James Su
>

Yusuke Sato

Fixed all. Please take another look at Patch Set #23. Since I added a new ...

9 years ago (2011-12-14 13:42:04 UTC) #71

Fixed all. Please take another look at Patch Set #23.

Since I added a new event type (as well as an event filter function for the new
type), the change bloated. If the change is too huge to review, I'll split it
into several parts. Please let me know if it's necessary.

http://codereview.chromium.org/8576005/diff/101001/content/public/browser/nat...
File content/public/browser/native_web_keyboard_event.h (right):

http://codereview.chromium.org/8576005/diff/101001/content/public/browser/nat...
content/public/browser/native_web_keyboard_event.h:33: #if defined(USE_AURA)
On 2011/12/12 16:51:03, Ben Goodger (Google) wrote:
> I believe you may want to run this before the ones above it:
> 
> #if defined(USE_AURA)
> ..
> #elif defined(OS_MACOSX) || defined(TOOLKIT_USES_GTK)
> ..
> #endif
> 
> since dhollowa@ is getting aura to build with OS_MACOSX.

Done.

http://codereview.chromium.org/8576005/diff/101001/ui/aura/event.h
File ui/aura/event.h (right):

http://codereview.chromium.org/8576005/diff/101001/ui/aura/event.h#newcode183
ui/aura/event.h:183: bool skip_ime_;
Thanks for the suggestion. I've added a new event type called
TranslatedKeyEvent.

On 2011/12/12 16:51:03, Ben Goodger (Google) wrote:
> On 2011/12/12 07:51:53, Yusuke Sato wrote:
> > This flag is for resoving the sync/async issue.
> 
> It'd be good to come up with a neater way to express this. What this really
> means is, "when true, the event is the result of an IME translation". Can this
> occur with any type of key event (e.g. both KeyDown and KeyUp)? Seems like you
> could make an argument that this is a new event type. Windows treats it as
such
> (WM_IME_CHAR).

http://codereview.chromium.org/8576005/diff/101001/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/8576005/diff/101001/ui/aura/root_window.h#newc...
ui/aura/root_window.h:69: void SetInputMethod(ui::InputMethod* input_method);
On 2011/12/12 16:51:03, Ben Goodger (Google) wrote:
> The Shell can actually set this on the RootWindow via a property.
> 
> Because you need to access this value from views and content, the property
name
> can live in src/ui/aura/client/aura_constants.h/cc.

Done.

http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/input_method...
File ui/aura_shell/input_method_event_filter.cc (right):

http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/input_method...
ui/aura_shell/input_method_event_filter.cc:27: input_method = new
ui::InputMethodIBus(this);
On 2011/12/12 16:51:03, Ben Goodger (Google) wrote:
> I've seen this pattern (and the associated includes) in a few places. Have you
> considered having a utility header/.cc in ui/base/ime called
> input_method_factory that defines ui::CreateInputMethod(..); that hides these
> ifdefs?

Done.

http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8576005/diff/101001/ui/aura_shell/shell.cc#new...
ui/aura_shell/shell.cc:240: // filter has the highest priority.
On 2011/12/12 16:51:03, Ben Goodger (Google) wrote:
> To enforce this, you might want to expose a has_event_filters_registered()
> method on RootWindow, and then DCHECK(!hefr()) here.

Removed the restriction by adding the new event type, TranslatedKeyEvent.

James Su

Hi Ben, Though I'll continue to hold my opinion, I'll respect your decision. But as ...

9 years ago (2011-12-14 14:34:09 UTC) #72

Hi Ben,
  Though I'll continue to hold my opinion, I'll respect your decision. But as
this CL goes onto a different way than what I expected, and it becomes more
complicated than before, I'm not going to review it and give LGTM again. Please
help review it.

Best Regards
James Su

On 2011/12/13 17:23:13, Ben Goodger (Google) wrote:
> Hi James,
> 
> Thanks for the detailed feedback as always. You raise a number of
> interesting technical challenges that we will doubtless have to consider in
> the fullness of time.
> 
> With that said, I believe the right approach for right now is to continue
> the work that Yusuke is doing in his most recent CL, and so that's the
> approach I'm going to endorse. Further along, when it comes time to
> evaluate more platform requirements, we can look to see if and how we need
> to make adjustments. Nothing in Chrome is set in stone for all eternity.
> 
> Yusuke, please continue with your CL, see my most recent comments on your
> idea of a different event type for post-processed key events.
> 
> -Ben
> 
> On Tue, Dec 13, 2011 at 12:53 AM, James Su <mailto:suzhe@chromium.org> wrote:
> 
> > Please read my feedback inline.
> >
> > 在 2011年12月13日 下午12:33，Ben Goodger (Google) <ben@chromium.org>写道：
> >
> > At this time, Windows and Mac support is preliminary and meant only for
> >> developer use/testing and debugging. Addressed the rest of your comments
> >> inline below:
> >>
> >> On Mon, Dec 12, 2011 at 8:03 PM, <mailto:suzhe@chromium.org> wrote:
> >>
> >>> Hello Ben,
> >>>
> >>> Please allow me to express my opinion again: I do believe integrating
> >>> ui::InputMethodXxx into aura::RootWindowHostXxx is the correct way we
> >>> should go.
> >>>
> >>> Following are my arguments:
> >>> 1. Looks like we are going to use aura on MacOSX as well. So we will
> >>> support
> >>> three platforms: ChromeOS, Windows and MacOSX. (How about Linux desktop?)
> >>>
> >>
> >> There aren't clear product directives for non-ChromeOS yet, they exist
> >> primarily for testing and debugging.
> >>
> >>
> >>> 2. As Yusuke already said, it'd be a huge effort for us to develop a
> >>> specific
> >>> IME system for aura. So in the foreseeable future, we will continue to
> >>> depend on
> >>> the IME system provided by each platform: ibus on ChromeOS, IMM32(TSF) on
> >>> Windows and InputMethod Kit Framework on MacOSX.
> >>>
> >>
> >> This is fine. Not every aspect of Cros is available on Windows yet. We
> >> are still determining what (if any) product mandate will materialize.
> >>
> >>
> >>> 4. Inside Chrome, in order to avoid dealing with different IME systems
> >>> everywhere, we introduced an abstract layer, which consists of two
> >>> interfaces:
> >>> ui::TextInputClient and ui::InputMethod, and several implementation
> >>> classes:
> >>> ui::InputMethodXxx. Much like what we've done for native window and
> >>> event types
> >>> in aura.
> >>>
> >>> 5. The IME systems are more or less bound to platforms' native window
> >>> and event
> >>> systems: on ChromeOS, ibus depends on X Window's key event definition.
> >>>
> >>
> >> While it's not on the agenda for the next few quarters, you should
> >> definitely be aware that once we've isolated X11 usage in Chrome, there are
> >> many that would like to cut the cord completely. So any dependency we have
> >> on X11 is likely to become a liability at some point. I don't think this
> >> affects this CL though so this is just an FYI.
> >>
> > Agree. On ChromeOS we can continue to use ibus, as it doesn't depend on X
> > library. When we move off X11 completely (e.g. use Wayland instead) we may
> > need to convert the platform event into X key event definition so that ibus
> > can recognize it. We always need to do such conversion if the platform
> > event definition is different than X definition no matter where we
> > integrate ui::InputMethodIBus.
> >
> >
> >>
> >>
> >>> 6. In aura world, aura::RootWindowHost is the single point for
> >>> abstracting
> >>> platform differences. It's the only place that we can freely access
> >>> platform's
> >>> native window and event API.
> >>>
> >>
> >> This is not quite right. RootWindowHost is the single point for
> >> abstracting platform differences relating to the window hierarchy. If you
> >> look at my checkins over the past couple of weeks, and some design
> >> decisions made in Aura, you'll see that RootWindow/Host is a lot less like
> >> views::NativeWidget/Widget than it might look at first glance. Rather than
> >> being a singleton instance integrating various functionality (cross
> >> platform and platform specific), it is meant to simply be the root of the
> >> window hierarchy, controlling rendering and basic event dispatch. My
> >> assertion is that IME, with its links into significant subsystems (ibus,
> >> shell UI to show IME candidate windows) steps beyond this basic
> >> functionality and is something better implemented in the shell.
> >>
> > Though ui::InputMethod* communicate with platform IME systems, they are
> > not part of the platform IME systems. They are just abstract layers to hide
> > the actual IME systems, similar than aura::KeyEvent for hiding native event
> > types and aura::RootWindowHost for hiding native window. And more
> > importantly ui::InputMethod* are actually very thin, the only work they do
> > is to convert/forward events between platform IME system and upper level
> > Chrome client code.
> > On the other hand, I agree that we should integrate other part of IME
> > system (e.g. IME UI) into aura_shell when appropriate.
> >
> >
> >>
> >>
> >>
> >>> 8. According to aura design doc, aura_shell is optional for Windows and
> >>> MacOSX
> >>> (is it correct?), but IME support is mandatory on all platforms. So it
> >>> makes no
> >>> sense to integrate ui::InputMethodXxx into aura_shell, despite of the
> >>> reason
> >>> stated in item 7.
> >>>
> >>
> >> IME support may differ depending on whether or not Aura shell is used.
> >> For example there are two potential products on Windows. One is an Aura
> >> Shell-like environment that presents the entire ChromeOS UI. This would
> >> include the ChromeOS IME candidate window UI and subsystem, and not present
> >> any of the Windows UI. The other is a desktop Chrome like experience using
> >> Aura to provide hardware accelerated UI transitions. This would
> >> (potentially) use the Windows IME UI.
> >>
> > I agree that we should integrate our own IME UI into aura_shell and make
> > use of it if the platform allow us to do so. And using our own IME UI is
> > actually optional if the platform IME system provides its own UI (from
> > functionalities' point of view).
> > But ui::InputMethod* are mandatory for us to support IME in Chrome.
> >
> >
> >>
> >>
> >>> 9. Although it may look ok to integrate ui::InputMethodIBus into
> >>> aura_shell,
> >>> because ibus doesn't depend on X library (though it depends on X key
> >>> event
> >>> definition). It's not possible for ui::InputMethodWin and
> >>> ui::InputMethodMac in
> >>> the future. So if we insist on integrating ui::InputMethodIBus into
> >>> aura_shell,
> >>> we must use different approaches for Windows and MacOSX, which is
> >>> obviously not
> >>> a good way to go.
> >>>
> >>
> >> The Mac side is a little nebulous so I am deferring thinking about it. As
> >> to InputMethodWin, what difficulties do you see? One that I can see is that
> >> the native system UI may not support the window transitions that we do, so
> >> that if we ever transform a textfield, the candidate window if anchored to
> >> that textfield may appear in the wrong position.
> >>
> > The difficulty is ui::InputMethodWin needs intercept a bunch of windows
> > messages (e.g. WM_IME_START_COMPOSITION) and call a bunch of windows
> > functions which are bound to HWND. We cannot simply do it in aura_shell.
> > I'm actually not afraid of the IME UI too much. As long as we are not
> > going to rotate a textfield to a strange degree, we can always position the
> > IME UI nicely.
> >
> >
> >>
> >> -Ben
> >>
> >
> >
> >
> > --
> > - James Su
> >

Ben Goodger (Google)

http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h File ui/aura/event.h (right): http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h#newcode190 ui/aura/event.h:190: class AURA_EXPORT TranslatedKeyEvent : public KeyEvent { Sorry, when ...

9 years ago (2011-12-14 15:30:21 UTC) #73

Ben Goodger (Google)

http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode697 content/browser/renderer_host/render_widget_host_view_aura.cc:697: #if defined(USE_X11) Is this specific to X11 or IBUS?

9 years ago (2011-12-14 18:20:25 UTC) #74

Yusuke Sato

http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode697 content/browser/renderer_host/render_widget_host_view_aura.cc:697: #if defined(USE_X11) On 2011/12/14 18:20:26, Ben Goodger (Google) wrote: ...

9 years ago (2011-12-15 06:29:59 UTC) #75

http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:697: #if
defined(USE_X11)
On 2011/12/14 18:20:26, Ben Goodger (Google) wrote:
> Is this specific to X11 or IBUS?

The 'if (!event->native_event())' part assumed that base::NativeEvent is
typedef'ed to a pointer which is not true on Windows. I slightly modified that
part to make it platform neutral, and removed the #if.

http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h
File ui/aura/event.h (right):

http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h#newcode190
ui/aura/event.h:190: class AURA_EXPORT TranslatedKeyEvent : public KeyEvent {
On 2011/12/14 15:30:21, Ben Goodger (Google) wrote:
> Sorry, when I mentioned "different event type" what I meant was a different
> value in the ui/base/event types enumeration... then you would just continue
to
> pass plain KeyEvent*s to everyone, avoiding the need to adjust the event
filter
> interface with knowledge of the translated type. You can feel free to keep
this
> class to help with constructing them (if the ctor has a substantially
different
> signature to KeyEvent's), but I think it probably better goes in
> ui/aura_shell/events.h/cc (you'd have to make a new file, I'm considering
moving
> DropTargetEvent there too).

oh I see. I've modified the code as follows:

- Revert all changes related to the EventFilter interface.
- Add two event type enum values to ui/base/events.h
- Move the TranslatedKeyEvent class to ui/aura_shell/event.h. 

Now classes under ui/aura/ don't know anything about the new event types and the
new event class. Only the new event filter for aura_shell,
aura_shell::InputMethodEventFIlter, uses the new types/class.

Yusuke Sato

interactive_ui_tests on linux_chromeos_aura trybot seems to show a real issue in the CL. I'm investigating ...

9 years ago (2011-12-16 03:32:00 UTC) #76

interactive_ui_tests on linux_chromeos_aura trybot seems to show a real issue in
the CL. I'm investigating it.

-Yusuke

On 2011/12/15 06:29:59, Yusuke Sato wrote:
>
http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_h...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_view_aura.cc:697: #if
> defined(USE_X11)
> On 2011/12/14 18:20:26, Ben Goodger (Google) wrote:
> > Is this specific to X11 or IBUS?
> 
> The 'if (!event->native_event())' part assumed that base::NativeEvent is
> typedef'ed to a pointer which is not true on Windows. I slightly modified that
> part to make it platform neutral, and removed the #if.
> 
> http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h
> File ui/aura/event.h (right):
> 
> http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h#newcode190
> ui/aura/event.h:190: class AURA_EXPORT TranslatedKeyEvent : public KeyEvent {
> On 2011/12/14 15:30:21, Ben Goodger (Google) wrote:
> > Sorry, when I mentioned "different event type" what I meant was a different
> > value in the ui/base/event types enumeration... then you would just continue
> to
> > pass plain KeyEvent*s to everyone, avoiding the need to adjust the event
> filter
> > interface with knowledge of the translated type. You can feel free to keep
> this
> > class to help with constructing them (if the ctor has a substantially
> different
> > signature to KeyEvent's), but I think it probably better goes in
> > ui/aura_shell/events.h/cc (you'd have to make a new file, I'm considering
> moving
> > DropTargetEvent there too).
> 
> oh I see. I've modified the code as follows:
> 
> - Revert all changes related to the EventFilter interface.
> - Add two event type enum values to ui/base/events.h
> - Move the TranslatedKeyEvent class to ui/aura_shell/event.h. 
> 
> Now classes under ui/aura/ don't know anything about the new event types and
the
> new event class. Only the new event filter for aura_shell,
> aura_shell::InputMethodEventFIlter, uses the new types/class.

Yusuke Sato

It turned out that the test failure was caused by my previous CL, not this ...

9 years ago (2011-12-16 07:38:08 UTC) #77

It turned out that the test failure was caused by my previous CL, not this one.
I'll fix it separately. http://crbug.com/107837

On 2011/12/16 03:32:00, Yusuke Sato wrote:
> interactive_ui_tests on linux_chromeos_aura trybot seems to show a real issue
in
> the CL. I'm investigating it.
> 
> -Yusuke
> 
> On 2011/12/15 06:29:59, Yusuke Sato wrote:
> >
>
http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_h...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
http://codereview.chromium.org/8576005/diff/102019/content/browser/renderer_h...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:697: #if
> > defined(USE_X11)
> > On 2011/12/14 18:20:26, Ben Goodger (Google) wrote:
> > > Is this specific to X11 or IBUS?
> > 
> > The 'if (!event->native_event())' part assumed that base::NativeEvent is
> > typedef'ed to a pointer which is not true on Windows. I slightly modified
that
> > part to make it platform neutral, and removed the #if.
> > 
> > http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h
> > File ui/aura/event.h (right):
> > 
> >
http://codereview.chromium.org/8576005/diff/102019/ui/aura/event.h#newcode190
> > ui/aura/event.h:190: class AURA_EXPORT TranslatedKeyEvent : public KeyEvent
{
> > On 2011/12/14 15:30:21, Ben Goodger (Google) wrote:
> > > Sorry, when I mentioned "different event type" what I meant was a
different
> > > value in the ui/base/event types enumeration... then you would just
continue
> > to
> > > pass plain KeyEvent*s to everyone, avoiding the need to adjust the event
> > filter
> > > interface with knowledge of the translated type. You can feel free to keep
> > this
> > > class to help with constructing them (if the ctor has a substantially
> > different
> > > signature to KeyEvent's), but I think it probably better goes in
> > > ui/aura_shell/events.h/cc (you'd have to make a new file, I'm considering
> > moving
> > > DropTargetEvent there too).
> > 
> > oh I see. I've modified the code as follows:
> > 
> > - Revert all changes related to the EventFilter interface.
> > - Add two event type enum values to ui/base/events.h
> > - Move the TranslatedKeyEvent class to ui/aura_shell/event.h. 
> > 
> > Now classes under ui/aura/ don't know anything about the new event types and
> the
> > new event class. Only the new event filter for aura_shell,
> > aura_shell::InputMethodEventFIlter, uses the new types/class.

Ben Goodger (Google)

Very good. I believe we're mostly down to stylistic nits now. http://codereview.chromium.org/8576005/diff/129002/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): ...

9 years ago (2011-12-20 21:02:02 UTC) #78

Yusuke Sato

http://codereview.chromium.org/8576005/diff/129002/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): http://codereview.chromium.org/8576005/diff/129002/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode858 content/browser/renderer_host/render_widget_host_view_aura.cc:858: ui::InputMethod* RenderWidgetHostViewAura::GetInputMethod() const { On 2011/12/20 21:02:02, Ben Goodger ...

9 years ago (2011-12-21 16:57:26 UTC) #79

Ben Goodger (Google)

> I added the checks for two reasons. One is for views_unittests. Since patch set ...

9 years ago (2011-12-21 21:40:00 UTC) #80

Yusuke Sato

On 2011/12/21 21:40:00, Ben Goodger (Google) wrote: > > I added the checks for two ...

9 years ago (2011-12-24 15:41:08 UTC) #81

Ben Goodger (Google)

Please create a new directory, ash/ime, and put event.* and input_method* in there. With that ...

9 years ago (2011-12-24 15:51:23 UTC) #82

Yusuke Sato

Done. Moved them to ash/ime/. Submitting.. On 2011/12/24 15:51:23, Ben Goodger (Google) wrote: > Please ...

8 years, 12 months ago (2011-12-26 05:15:47 UTC) #83

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusukes@chromium.org/8576005/145080

8 years, 12 months ago (2011-12-26 05:16:21 UTC) #84

Change committed as 115778

Issue 8576005: IME (input method editor) support for Aura, part 3 of 3: Use ui::InputMethod in ash. (Closed)

Description

Patch Set 1 : rebase, review #

Patch Set 2 : work in progress. removed most of USE_AURA from input_method_ibus.cc. #

Patch Set 3 : addressed comments #

Patch Set 4 : addressed Peng's comments #

Patch Set 5 : aura_win fix #

Patch Set 6 : address comments #

Patch Set 7 : address comments #

Patch Set 8 : RWHVA fix #

Patch Set 9 : fix RHWVA and ui::InputMethodBase #

Patch Set 10 : added two virtual functions to ui::InputMethodBase #

Patch Set 11 : rebase, remove part 1 and 2, add aura_unittests for DesktopHost #

Patch Set 12 : Handle mouse click in RWHVA #

Patch Set 13 : win_aura fix #

Patch Set 14 : git mv desktop_host_unittest.cc desktop_host_ime_unittest.cc #

Patch Set 15 : review fixes #

Patch Set 16 : review fix (see http://codereview.chromium.org/8659033/) #

Patch Set 17 : rebase #

Patch Set 18 : final rebase, really. #

Patch Set 19 : resolve conflict #

Patch Set 20 : moving IME code to aura_shell. not ready for review #

Patch Set 21 : moved IME code to aura_shell, not ready for review though #

Patch Set 22 : Add TranslatedKeyEvent to event.h and event_filter.h #

Patch Set 23 : rebase, review #

Patch Set 24 : review fixes #

Patch Set 25 : rebase, address review comments #

Patch Set 26 : Fix Windows support #

Patch Set 27 : rebase, fix views_unittests, aura_shell -> ash #

Patch Set 28 : fix win_shared #

Patch Set 29 : move to ash/ime/ #

Messages