Enable Config::MUS to use classic IME instead of servicified IME.

Enable Config::MUS to use classic IME instead of servicified IME.

Adds |WindowTreeHostMusInitParams.use_classic_ime| to enable using
InputMethodChromeOS instead of InputMethodMus. Later we will wire this up
with a command line argument so we can use classic IME in --mus.

BUG=706913
Review-Url: https://codereview.chromium.org/2831583005
Cr-Commit-Position: refs/heads/master@{#469442}
Committed: https://chromium.googlesource.com/chromium/src/+/13d5c661b83dd941fbc49c63a079ffad1ae17e44

[Hadi, 2017-04-20 17:35:52]

The CQ bit was checked by moshayedi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 17:36:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-04-20 17:39:34]

Description was changed from

==========
Enable WindowTreeHostMus to use classic IME instead of servicified IME.

BUG=706913
==========

to

==========
Enable WindowTreeHostMus to use classic IME instead of servicified IME.

Adds |WindowTreeHostMusInitParams.use_classic_ime| to enable using
InputMethodChromeOS instead of InputMethodMus. Later we will wire this up
with a command line argument so we can use classic IME in --mus.

BUG=706913
==========

[commit-bot: I haz the power, 2017-04-20 18:00:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 18:00:56]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Hadi, 2017-04-20 18:02:39]

moshayedi@chromium.org changed reviewers:
+ sky@chromium.org

[Hadi, 2017-04-20 18:02:39]

PTAL.

Since this is just a transitional change, I tried to keep the effects of this CL
minimal. If I wanted to have a single input_method_ variable in
WindowTreeHostMus to make this class cleaner, I thought I have to do either of
the following:

(1) Move DispatchKeyEvent() with the callback arg to ui::InputMethod, which
would require lots of changes to other sub-classes of ui::InputMethod.
(2) Remove the callback arg from InputMethodMus::DispatchKeyEvent() and handle
IME request queuing, etc. in WindowTreeHostMus.

What are your thoughts?

[Hadi, 2017-04-20 19:05:23]

The CQ bit was checked by moshayedi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 19:05:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-04-20 20:18:49]

https://codereview.chromium.org/2831583005/diff/20001/ui/aura/mus/input_metho...
File ui/aura/mus/input_method_mus.h (left):

https://codereview.chromium.org/2831583005/diff/20001/ui/aura/mus/input_metho...
ui/aura/mus/input_method_mus.h:31: using EventResultCallback =
base::Callback<void(ui::mojom::EventResult)>;
Is there a reason to remove the type? Having an explicit type makes for less
confusing as to what true and false mean.

https://codereview.chromium.org/2831583005/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2831583005/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:230: input_method_chromeos_ =
base::MakeUnique<ui::InputMethodChromeOS>(this);
Are you sure we need to set the input method at all in classic? We will soon be
using this code:
https://chromium.googlesource.com/chromium/src/+/master/ash/display/window_tr...
which sets the SharedInputMethod.

[commit-bot: I haz the power, 2017-04-20 20:21:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 20:21:01]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-05-02 19:05:03]

The CQ bit was checked by moshayedi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 19:05:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-02 19:07:44]

Addressed feedback. PTAL.

[Hadi, 2017-05-02 19:09:12]

Description was changed from

==========
Enable WindowTreeHostMus to use classic IME instead of servicified IME.

Adds |WindowTreeHostMusInitParams.use_classic_ime| to enable using
InputMethodChromeOS instead of InputMethodMus. Later we will wire this up
with a command line argument so we can use classic IME in --mus.

BUG=706913
==========

to

==========
Enable Config::MUS to use classic IME instead of servicified IME.

Adds |WindowTreeHostMusInitParams.use_classic_ime| to enable using
InputMethodChromeOS instead of InputMethodMus. Later we will wire this up
with a command line argument so we can use classic IME in --mus.

BUG=706913
==========

[commit-bot: I haz the power, 2017-05-02 20:08:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 20:08:30]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-03 15:25:27]

https://codereview.chromium.org/2831583005/diff/50001/ash/display/window_tree...
File ash/display/window_tree_host_manager.cc (right):

https://codereview.chromium.org/2831583005/diff/50001/ash/display/window_tree...
ash/display/window_tree_host_manager.cc:826: DCHECK(!host->has_input_method());
The plan is to use this code for both mash and mus. For the mash case IME will
be hosted by chrome, for the mus case we want what you have here. So, doesn't
this code need to know the ash::Config so that it conditionally set the
InputMethod?

[Hadi, 2017-05-03 19:32:16]

The CQ bit was checked by moshayedi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 19:33:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-03 19:33:49]

The CQ bit was checked by moshayedi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 19:34:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-03 19:35:42]

https://codereview.chromium.org/2831583005/diff/50001/ash/display/window_tree...
File ash/display/window_tree_host_manager.cc (right):

https://codereview.chromium.org/2831583005/diff/50001/ash/display/window_tree...
ash/display/window_tree_host_manager.cc:826: DCHECK(!host->has_input_method());
On 2017/05/03 15:25:27, sky wrote:
> The plan is to use this code for both mash and mus. For the mash case IME will
> be hosted by chrome, for the mus case we want what you have here. So, doesn't
> this code need to know the ash::Config so that it conditionally set the
> InputMethod?

Done.

[commit-bot: I haz the power, 2017-05-03 20:53:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 20:53:53]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-04 03:09:18]

Nice! Just a couple of questions.

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
File ash/mus/ash_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
ash/mus/ash_window_tree_host_mus.cc:89: ui::EventDispatchDetails
AshWindowTreeHostMus::DispatchKeyEventPostIME(
Can you elaborate on why this is the right fix? Why doesn't
SendEventToSink(event) work in this case?

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
File ash/mus/ash_window_tree_host_mus.h (right):

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
ash/mus/ash_window_tree_host_mus.h:40: // ui::internal::InputMethodDelegate:
As this override comes through via AshWindowTreeHostMus prefixing the override
with InputMethodDelegate is confusing. Please group with the other
WindowTreeHostMus overrides to make it clear where the overrides comes from.

https://codereview.chromium.org/2831583005/diff/90001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus_init_params.h (right):

https://codereview.chromium.org/2831583005/diff/90001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus_init_params.h:69: // Use classic IME instead of
servicified IME.
Please document what 'classic' means in this context.

[Hadi, 2017-05-04 12:12:10]

The CQ bit was checked by moshayedi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 12:12:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hadi, 2017-05-04 12:15:17]

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
File ash/mus/ash_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
ash/mus/ash_window_tree_host_mus.cc:89: ui::EventDispatchDetails
AshWindowTreeHostMus::DispatchKeyEventPostIME(
On 2017/05/04 03:09:18, sky wrote:
> Can you elaborate on why this is the right fix? Why doesn't
> SendEventToSink(event) work in this case?

The implementation here was based on what AshWindowTreeHostPlatform does in this
case.

We could use SendEventToSink() instead of event_sink()->OnEventFromSource(). I
made this change.

The reason for SetPostIME(true)/(false): When execution reaches here, we are
still executing InputMethodEventHandler::OnKeyEvent(). If we don't disable it
for SendEventToSink() here, we get an infinite recursion since
InputMethodEventHandler will be tried again. When code reaches here it means
that the input method logic couldn't handle the event and wants to see if there
is anything other than IME that can handle the key event.

(Actually I am thinking what are the consequences of removing
DispatchKeyEventPostIME(), and letting ui::EventDispatcher try the post IME
handlers and targets; But for the sake of this CL, this implementation of
DispatchKeyEventPostIME is to avoid infinite recursion).

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
File ash/mus/ash_window_tree_host_mus.h (right):

https://codereview.chromium.org/2831583005/diff/90001/ash/mus/ash_window_tree...
ash/mus/ash_window_tree_host_mus.h:40: // ui::internal::InputMethodDelegate:
On 2017/05/04 03:09:18, sky wrote:
> As this override comes through via AshWindowTreeHostMus prefixing the override
> with InputMethodDelegate is confusing. Please group with the other
> WindowTreeHostMus overrides to make it clear where the overrides comes from.

Done.

https://codereview.chromium.org/2831583005/diff/90001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus_init_params.h (right):

https://codereview.chromium.org/2831583005/diff/90001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus_init_params.h:69: // Use classic IME instead of
servicified IME.
On 2017/05/04 03:09:18, sky wrote:
> Please document what 'classic' means in this context.

Done.

[commit-bot: I haz the power, 2017-05-04 13:07:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 13:07:53]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-04 19:48:53]

Thanks for the details. LGTM

[Hadi, 2017-05-04 20:01:51]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-05-04 20:02:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 20:08:50]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1493928111430160,
"parent_rev": "c945299c463a8f795a59698927a2b541c0f25414", "commit_rev":
"13d5c661b83dd941fbc49c63a079ffad1ae17e44"}

[commit-bot: I haz the power, 2017-05-04 20:09:02]

Description was changed from

==========
Enable Config::MUS to use classic IME instead of servicified IME.

Adds |WindowTreeHostMusInitParams.use_classic_ime| to enable using
InputMethodChromeOS instead of InputMethodMus. Later we will wire this up
with a command line argument so we can use classic IME in --mus.

BUG=706913
==========

to

==========
Enable Config::MUS to use classic IME instead of servicified IME.

Adds |WindowTreeHostMusInitParams.use_classic_ime| to enable using
InputMethodChromeOS instead of InputMethodMus. Later we will wire this up
with a command line argument so we can use classic IME in --mus.

BUG=706913

Review-Url: https://codereview.chromium.org/2831583005
Cr-Commit-Position: refs/heads/master@{#469442}
Committed:
https://chromium.googlesource.com/chromium/src/+/13d5c661b83dd941fbc49c63a079...
==========

[commit-bot: I haz the power, 2017-05-04 20:09:03]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/13d5c661b83dd941fbc49c63a079...