IME (input method editor) support for Aura, part 1 of 3: Add ui/base/ime/ classes

IME (input method editor) support for Aura, part 1 of 3: Add ui/base/ime/ classes.

ui::InputMethod: 
- An interface similar to views::InputMethod, but does not have Has neither Views nor Aura dependencies. 

ui::InputMethodIBus 
- Port of views::InputMethodIBus. Has neither Views nor Aura dependencies. 

Original review: http://codereview.chromium.org/8576005/ (PatchSet #1-8, Comment #1-24)

BUG=97261
TEST=see part 3 of 3.


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

[Yusuke Sato, 2011-11-29 10:41:01]

Hi James,
To make it easier for non-IME folks to review the huge CL
(http://codereview.chromium.org/8576005/), I'd like to split it into 3 parts,
ui/base/ime/, ui/views/ime/, and the rest, and submit the first two in advance.

This CL is the first one out of three, and is exactly the same as PatchSet #8 if
the huge CL.  Can you take a brief look?

[James Su, 2011-11-30 05:16:29]

LGTM.

[Yusuke Sato, 2011-11-30 05:21:24]

+brettw
Brett, could you take a look at base/ changes?

I'm adding platform neutral (i.e. depend on neither Views nor Aura) IME classes
to ui/base/ime/, and need typedefs for native windows.

[Yusuke Sato, 2011-12-02 02:35:59]

Brett,
Could you take a brief look at this? It's 5-line change. Your review is
necessary to finish Aura IME work (http://crbug.com/97261#c20).

Thanks,
Yusuke

On 2011/11/30 05:21:24, Yusuke Sato wrote:
> +brettw
> Brett, could you take a look at base/ changes?
> 
> I'm adding platform neutral (i.e. depend on neither Views nor Aura) IME
classes
> to ui/base/ime/, and need typedefs for native windows.

[brettw, 2011-12-02 03:49:17]

http://codereview.chromium.org/8659033/diff/3001/base/event_types.h
File base/event_types.h (right):

http://codereview.chromium.org/8659033/diff/3001/base/event_types.h#newcode29
base/event_types.h:29: typedef HWND NativeWindow;
Why not use ui/gfx/native_widget_types.h instead of modifying this?

[Yusuke Sato, 2011-12-02 04:02:24]

http://codereview.chromium.org/8659033/diff/3001/base/event_types.h
File base/event_types.h (right):

http://codereview.chromium.org/8659033/diff/3001/base/event_types.h#newcode29
base/event_types.h:29: typedef HWND NativeWindow;
On 2011/12/02 03:49:17, brettw wrote:
> Why not use ui/gfx/native_widget_types.h instead of modifying this?

This is because ui::InputMethod, which is designed to depend on neither Views
nor Aura, requires a real system window handle.

If I understand correctly, using gfx::NativeWindow introduces Aura dependency
since gfx::NativeWindow is typedefed to aura::Window* when USE_AURA is #defined
(on both Windows and Linux).

[Ben Goodger (Google), 2011-12-02 04:06:53]

To work on Aura, you're going to have to use an aura::Window, which means an
Aura dependency somewhere. We can't have you using X directly. We wrap X pretty
well and it is deeply buried, since we may eventually remove it.

So you may as well use gfx::NativeView.

[Yusuke Sato, 2011-12-02 05:20:31]

Sure, I've removed the base::NativeWindow typedefs (and moved X related code
from ui/base/ime/input_method_base.cc to ui/aura/desktop_host_linux.cc where
native X events are handled.)

http://codereview.chromium.org/8659033/diff2/3001:19002/ui/base/ime/input_met...
http://codereview.chromium.org/8576005/diff2/71002:73001/ui/aura/desktop_host...
(in a separate CL; http://codereview.chromium.org/8576005/)

On 2011/12/02 04:06:53, Ben Goodger (Google) wrote:
> To work on Aura, you're going to have to use an aura::Window, which means an
> Aura dependency somewhere. We can't have you using X directly. We wrap X
pretty
> well and it is deeply buried, since we may eventually remove it.
> 
> So you may as well use gfx::NativeView.

[James Su, 2011-12-02 05:23:40]

On 2011/12/02 04:06:53, Ben Goodger (Google) wrote:
> To work on Aura, you're going to have to use an aura::Window, which means an
> Aura dependency somewhere. We can't have you using X directly. We wrap X
pretty
> well and it is deeply buried, since we may eventually remove it.
> 
> So you may as well use gfx::NativeView.

ui::InputMethod layer is designed to talk to the operating system's native input
method layer, which is usually bound to the real native window system, for
example on Windows IME related messages are tightly bound to the top level HWND,
on Linux XWindow's native input method protocol is XIM which is bound to the top
level XWindow as well, though we don't support XIM for now. In the future when
we replace XWindow with another window system (e.g. Wayland), we still need to
expose that system's native input method layer to ui::InputMethod, which is
again not the aura layer.

[James Su, 2011-12-02 05:31:00]

This change LGTM. 

On 2011/12/02 05:20:31, Yusuke Sato wrote:
> Sure, I've removed the base::NativeWindow typedefs (and moved X related code
> from ui/base/ime/input_method_base.cc to ui/aura/desktop_host_linux.cc where
> native X events are handled.)
> 
>
http://codereview.chromium.org/8659033/diff2/3001:19002/ui/base/ime/input_met...
>
http://codereview.chromium.org/8576005/diff2/71002:73001/ui/aura/desktop_host...
> (in a separate CL; http://codereview.chromium.org/8576005/)
> 
> On 2011/12/02 04:06:53, Ben Goodger (Google) wrote:
> > To work on Aura, you're going to have to use an aura::Window, which means an
> > Aura dependency somewhere. We can't have you using X directly. We wrap X
> pretty
> > well and it is deeply buried, since we may eventually remove it.
> > 
> > So you may as well use gfx::NativeView.

[brettw, 2011-12-02 18:18:58]

For future reference, I think adding NativeWindow to base/event_types.h is the
wrong thing, so if we need to revisit this to make it use the "real" native
window, we should find a better solution or place for this to live.

[brettw, 2011-12-02 18:19:10]

(you don't need my review any more so don't block on me)

[commit-bot: I haz the power, 2011-12-03 08:17:02]

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

[commit-bot: I haz the power, 2011-12-03 08:17:07]

Presubmit check for 8659033-20001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: ui/ui.gyp

Presubmit checks took 1.7s to calculate.

[Yusuke Sato, 2011-12-03 08:20:30]

+sky (OWNER of ui/ui.gyp)

[sky, 2011-12-05 15:15:28]

LGTM

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method.cc
File ui/base/ime/input_method.cc (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method.cc...
ui/base/ime/input_method.cc:9: InputMethod::~InputMethod() {
you could inline this if you want.

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

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method.h#...
ui/base/ime/input_method.h:54: virtual void
set_delegate(internal::InputMethodDelegate* delegate) = 0;
virtual methods shouldn't use unix_hacker_style syntax.

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
File ui/base/ime/input_method_base.cc (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
ui/base/ime/input_method_base.cc:24: DCHECK(delegate);
Why does the delegate have to be non-NULL?

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
File ui/base/ime/input_method_base.h (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
ui/base/ime/input_method_base.h:49: TextInputClient* focused_before,
TextInputClient* focused) {}
nit: when you wrap, each param on its own line.

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_de...
File ui/base/ime/input_method_delegate.h (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_de...
ui/base/ime/input_method_delegate.h:26: ui::EventType type, ui::KeyboardCode
key_code, int flags) = 0;
each param on its own line.

[Yusuke Sato, 2011-12-05 22:55:14]

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method.cc
File ui/base/ime/input_method.cc (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method.cc...
ui/base/ime/input_method.cc:9: InputMethod::~InputMethod() {
On 2011/12/05 15:15:29, sky wrote:
> you could inline this if you want.

Done.

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

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method.h#...
ui/base/ime/input_method.h:54: virtual void
set_delegate(internal::InputMethodDelegate* delegate) = 0;
On 2011/12/05 15:15:29, sky wrote:
> virtual methods shouldn't use unix_hacker_style syntax.

Done.

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
File ui/base/ime/input_method_base.cc (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
ui/base/ime/input_method_base.cc:24: DCHECK(delegate);
On 2011/12/05 15:15:29, sky wrote:
> Why does the delegate have to be non-NULL?

Removed.

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
File ui/base/ime/input_method_base.h (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_ba...
ui/base/ime/input_method_base.h:49: TextInputClient* focused_before,
TextInputClient* focused) {}
On 2011/12/05 15:15:29, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_de...
File ui/base/ime/input_method_delegate.h (right):

http://codereview.chromium.org/8659033/diff/20001/ui/base/ime/input_method_de...
ui/base/ime/input_method_delegate.h:26: ui::EventType type, ui::KeyboardCode
key_code, int flags) = 0;
On 2011/12/05 15:15:29, sky wrote:
> each param on its own line.

Done.

[commit-bot: I haz the power, 2011-12-05 22:55:35]

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

[commit-bot: I haz the power, 2011-12-06 02:41:44]

Change committed as 113095