Introduce RemoteInputMethodWin

Introduce RemoteInputMethodWin

In order to enable IME support on Ash, we need a proxy
object which implements ui::InputMethod interface and works
as if it was the actual IME back-end running in the
metro_driver process.

This CL introduces RemoteInputMethodWin for this purpose.
RemoteInputMethodWin is not used in the production code yet.
So this CL never breaks existing functionary at the moment.

BUG=164964
TEST=ui_unittests --gtest_filter=RemoteInputMethodTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236239

[Seigo Nonaka, 2013-11-18 20:00:27]

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.cc (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:23: // Caveats: Currently we support only
one instance at the same time.
Could you describe why should we support multiple instances.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:24: class InstanceMapper {
Does this class is necessary?
Seems there is only static methods.
How about using anonymous-namespaced global variable and functions?

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:122: }
plz add one line break between functions.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:160: if (text_input_client_ == client) {
nit: I prefer early exit style but up to you.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:176: if (event.HasNativeEvent())
DCHECK?

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:263: // not supported.
If you will support this, please file a bug and leave NOTIMPLEMENTED with TODO
comment.
If this should not be reached, plz use NOTREACHED.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.h (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.h:32: //   someone needs to call
RemoteInputMethodPrivateWin::OnCandidatePopupChanged
Could you add example for this "someone"?

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.h:40: UI_EXPORT scoped_ptr<InputMethod>
CreateRemoteInputMethodWin(
How about moving this function into static class function as "Create"?

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.h:55: // Installs additional delegate that
is required by RemoteInputMethodWin.
Sorry, I'm bit confused with this comment.
An instance of this class only accept one delegate, right?
"Installs additional delegate" sounds like there are many delegates.
If this class can accept multiple delegates, please rename this class as
"AddRemoteDelegate".

[yukawa, 2013-11-19 07:17:50]

Thank you for the review. Could you take another look?
I've addressed your comment and added some new relevant test cases.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.cc (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:23: // Caveats: Currently we support only
one instance at the same time.
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> Could you describe why should we support multiple instances.

I don't think we need to support multiple instances at this moment.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:24: class InstanceMapper {
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> Does this class is necessary?
> Seems there is only static methods.
> How about using anonymous-namespaced global variable and functions?

Done.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:122: }
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> plz add one line break between functions.

Done.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:160: if (text_input_client_ == client) {
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> nit: I prefer early exit style but up to you.

Done. And simplified.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:176: if (event.HasNativeEvent())
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> DCHECK?

Um, I changed my mind and added WM_CHAR handler because it looks natural to me
that RemoteInputMethodWin::DispatchKeyEvent can handles native WM_CHAR events
even though it is not dispatched at the moment.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:263: // not supported.
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> If you will support this, please file a bug and leave NOTIMPLEMENTED with TODO
> comment.
> If this should not be reached, plz use NOTREACHED.

I don't want to support this.  IMHO, I'd like to deprecate InputMethodObserver
because it is used only in unit test and not well maintained. Added
NOTIMPLEMENTED anyway.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.h (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.h:32: //   someone needs to call
RemoteInputMethodPrivateWin::OnCandidatePopupChanged
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> Could you add example for this "someone"?

Done.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.h:40: UI_EXPORT scoped_ptr<InputMethod>
CreateRemoteInputMethodWin(
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> How about moving this function into static class function as "Create"?

Sorry, to which class?

1)
class UI_EXPORT RemoteInputMethodWin {
 public:
  static scoped_ptr<InputMethod> Create(internal::InputMethodDelegate*
delegate);

 private:
  DISALLOW_IMPLICIT_CONSTRUCTORS(RemoteInputMethodWin);
};

2)
class UI_EXPORT RemoteInputMethodPrivateWin {
 public:
  static scoped_ptr<InputMethod> Create(internal::InputMethodDelegate*
delegate);
  ...
};

1) is contradict to your another comment that we should not use static-method
only class.

2) does not make sense because RemoteInputMethodPrivateWin::Create does not
create an instance of RemoteInputMethodPrivateWin despite its method name.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.h:55: // Installs additional delegate that
is required by RemoteInputMethodWin.
On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> Sorry, I'm bit confused with this comment.
> An instance of this class only accept one delegate, right?
> "Installs additional delegate" sounds like there are many delegates.
> If this class can accept multiple delegates, please rename this class as
> "AddRemoteDelegate".

No, multiple delegates are not supported.  Updated the comment.

[Seigo Nonaka, 2013-11-20 04:14:29]

almost l-gtm. Please let me ask a question in DispatchKeyEvent.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.cc (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:176: if (event.HasNativeEvent())
Let me confirm again, will RemoteInputMethodWin dispatch native key event? If
this condition is never reached, NOTREACHED or DCHECK is appropriate I think.
On 2013/11/19 07:17:51, yukawa wrote:
> On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> > DCHECK?
> 
> Um, I changed my mind and added WM_CHAR handler because it looks natural to me
> that RemoteInputMethodWin::DispatchKeyEvent can handles native WM_CHAR events
> even though it is not dispatched at the moment.

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.h (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.h:40: UI_EXPORT scoped_ptr<InputMethod>
CreateRemoteInputMethodWin(
Sorry I miss understood. Current comment is fine.
On 2013/11/19 07:17:51, yukawa wrote:
> On 2013/11/18 20:00:27, Seigo Nonaka wrote:
> > How about moving this function into static class function as "Create"?
> 
> Sorry, to which class?
> 
> 1)
> class UI_EXPORT RemoteInputMethodWin {
>  public:
>   static scoped_ptr<InputMethod> Create(internal::InputMethodDelegate*
> delegate);
> 
>  private:
>   DISALLOW_IMPLICIT_CONSTRUCTORS(RemoteInputMethodWin);
> };
> 
> 2)
> class UI_EXPORT RemoteInputMethodPrivateWin {
>  public:
>   static scoped_ptr<InputMethod> Create(internal::InputMethodDelegate*
> delegate);
>   ...
> };
> 
> 1) is contradict to your another comment that we should not use static-method
> only class.
> 
> 2) does not make sense because RemoteInputMethodPrivateWin::Create does not
> create an instance of RemoteInputMethodPrivateWin despite its method name.

[yukawa, 2013-11-20 05:06:06]

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.cc (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:176: if (event.HasNativeEvent())
On 2013/11/20 04:14:29, Seigo Nonaka wrote:
> Let me confirm again, will RemoteInputMethodWin dispatch native key event? If
> this condition is never reached, NOTREACHED or DCHECK is appropriate I think.

Do you mean we should do as follows?

if (event.HasNativeEvent()) {
  NOTREACHED();
  return false;
}

If we should never call ui::InputMethod::DispatchKeyEvent with native key
events, we should assert it as a comment in input_method.h.  Please note that
RemoteInputMethodWin will be instantiated via CreateInputMethod function and
most of Aura code does not know RemoteInputMethodWin's implementation details.

[Hiro Komatsu, 2013-11-20 05:19:06]

https://codereview.chromium.org/67503004/diff/360003/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_delegate_win.h (right):

https://codereview.chromium.org/67503004/diff/360003/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_delegate_win.h:30: const std::vector<int32>&
input_scopes,
Is it possible to use enum or typedef?  If no, please leave a comment why.

[Seigo Nonaka, 2013-11-20 05:40:40]

lgtm

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_win.cc (right):

https://codereview.chromium.org/67503004/diff/190001/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_win.cc:176: if (event.HasNativeEvent())
Okay, as we talked offline, it seems good to implement also for fabricated key
event cases.
On 2013/11/20 05:06:07, yukawa wrote:
> On 2013/11/20 04:14:29, Seigo Nonaka wrote:
> > Let me confirm again, will RemoteInputMethodWin dispatch native key event?
If
> > this condition is never reached, NOTREACHED or DCHECK is appropriate I
think.
> 
> Do you mean we should do as follows?
> 
> if (event.HasNativeEvent()) {
>   NOTREACHED();
>   return false;
> }
> 
> If we should never call ui::InputMethod::DispatchKeyEvent with native key
> events, we should assert it as a comment in input_method.h.  Please note that
> RemoteInputMethodWin will be instantiated via CreateInputMethod function and
> most of Aura code does not know RemoteInputMethodWin's implementation details.

[yukawa, 2013-11-20 05:53:47]

Thanks.

Komatsu-san, could you take a look?

[yukawa, 2013-11-20 06:26:53]

https://codereview.chromium.org/67503004/diff/360003/ui/base/ime/remote_input...
File ui/base/ime/remote_input_method_delegate_win.h (right):

https://codereview.chromium.org/67503004/diff/360003/ui/base/ime/remote_input...
ui/base/ime/remote_input_method_delegate_win.h:30: const std::vector<int32>&
input_scopes,
On 2013/11/20 05:19:07, Hiro Komatsu wrote:
> Is it possible to use enum or typedef?  If no, please leave a comment why.

It's not plausible because this data will be packed into an IPC message and we
don't want to use Win32 specific enum in the IPC layer.  Added a comment.

[Hiro Komatsu, 2013-11-20 06:36:28]

lgtm

[commit-bot: I haz the power, 2013-11-20 07:29:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukawa@chromium.org/67503004/520001

[commit-bot: I haz the power, 2013-11-20 07:49:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukawa@chromium.org/67503004/710001

[commit-bot: I haz the power, 2013-11-20 08:19:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukawa@chromium.org/67503004/790001

[commit-bot: I haz the power, 2013-11-20 09:10:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukawa@chromium.org/67503004/930001

[commit-bot: I haz the power, 2013-11-20 11:09:54]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-11-20 11:24:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukawa@chromium.org/67503004/800002

[commit-bot: I haz the power, 2013-11-20 13:29:40]

Change committed as 236239