IME for Mus: Make InputMethodMus use the IME Mojo API.

IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407
Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Committed: https://crrev.com/b26c35f30555178bfea1c07b47801fb58222fb0f
Cr-Original-Commit-Position: refs/heads/master@{#414748}
Cr-Commit-Position: refs/heads/master@{#415646}

[Hadi, 2016-08-12 20:20:29]

Description was changed from

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

BUG=548407
==========

to

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.

BUG=548407
==========

[Hadi, 2016-08-12 21:35:52]

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

[Hadi, 2016-08-12 21:35:53]

PTAL.

[Hadi, 2016-08-16 14:33:18]

sadrul@ how does this look?

[sadrul, 2016-08-16 20:01:52]

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:122:
ime_server_->StartSession(std::move(client_ptr), GetProxy(&input_method_));
Maybe this is simpler as:

  text_input_client_.reset(new ClientImpl(focused));
  ime_server_->StartSession(text_input_client_.CreateInterfacePtrAndBind(),
                            GetProxy(&input_method_));

where:

... TextInputClientImpl::CreateInterfacePtrAndBind() {
  return binding_.CreateInterfacePtrAndBind();
}

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.h (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.h:26: explicit
InputMethodMus(ui::internal::InputMethodDelegate* delegate,
Remove explicit

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus_unittest.cc (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus_unittest.cc:39: ui::KeyEvent* ReceieveKeyEvent() {
WaitUntilInputReceived()?

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus_unittest.cc:94: ASSERT_EQ(ui::ET_KEY_PRESSED,
received_event->type());
EXPECT_EQ

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus_unittest.cc:95:
ASSERT_EQ(key_event.GetCharacter(), received_event->GetCharacter());
ditto

Also check EXPECT_TRUE(received_event->is_char())

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.cc (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.h (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
No (c)

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.h:21: explicit
TextInputClientImpl(ui::mojom::TextInputClientRequest request,
No explicit

[Hadi, 2016-08-16 20:18:25]

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:122:
ime_server_->StartSession(std::move(client_ptr), GetProxy(&input_method_));
On 2016/08/16 20:01:51, sadrul wrote:
> Maybe this is simpler as:
> 
>   text_input_client_.reset(new ClientImpl(focused));
>   ime_server_->StartSession(text_input_client_.CreateInterfacePtrAndBind(),
>                             GetProxy(&input_method_));
> 
> where:
> 
> ... TextInputClientImpl::CreateInterfacePtrAndBind() {
>   return binding_.CreateInterfacePtrAndBind();
> }

Done.

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.h (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.h:26: explicit
InputMethodMus(ui::internal::InputMethodDelegate* delegate,
On 2016/08/16 20:01:51, sadrul wrote:
> Remove explicit

Done.

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus_unittest.cc (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus_unittest.cc:39: ui::KeyEvent* ReceieveKeyEvent() {
On 2016/08/16 20:01:51, sadrul wrote:
> WaitUntilInputReceived()?

Done.

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus_unittest.cc:94: ASSERT_EQ(ui::ET_KEY_PRESSED,
received_event->type());
On 2016/08/16 20:01:51, sadrul wrote:
> EXPECT_EQ

Done.

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/input_met...
ui/views/mus/input_method_mus_unittest.cc:95:
ASSERT_EQ(key_event.GetCharacter(), received_event->GetCharacter());
On 2016/08/16 20:01:51, sadrul wrote:
> ditto
> 
> Also check EXPECT_TRUE(received_event->is_char())

Done.

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.cc (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/08/16 20:01:51, sadrul wrote:
> no (c)

Done.

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.h (right):

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/08/16 20:01:51, sadrul wrote:
> No (c)

Done.

https://codereview.chromium.org/2230393002/diff/100001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.h:21: explicit
TextInputClientImpl(ui::mojom::TextInputClientRequest request,
On 2016/08/16 20:01:51, sadrul wrote:
> No explicit

Done.

[Hadi, 2016-08-16 20:18:49]

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

[commit-bot: I haz the power, 2016-08-16 20:19:09]

Dry run: 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, 2016-08-16 21:24:34]

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

[commit-bot: I haz the power, 2016-08-16 21:24:35]

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

[Hadi, 2016-08-16 21:54:17]

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

[commit-bot: I haz the power, 2016-08-16 21:54:54]

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

[sadrul, 2016-08-16 22:32:02]

https://codereview.chromium.org/2230393002/diff/140001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/140001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:67:
input_method_->ProcessKeyEvent(ui::Event::Clone(*event));
Wait, this seems backwards. We are supposed to receive non-char events from the
platform, and send those to the IME, to get back char events, right? For
example, if user presses arrow keys, then the IME should receive those events?

I was thinking we could remove the block of code in
https://cs.chromium.org/chromium/src/services/ui/ws/platform_display.cc?sq=pa...
once we have the IME working.

[commit-bot: I haz the power, 2016-08-16 23:19:21]

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

[commit-bot: I haz the power, 2016-08-16 23:19:23]

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

[Hadi, 2016-08-17 21:41:13]

PTAL.

Now InputMethodMus sends all char and non-char key events to IME driver.
test_ime_driver sends back non-char events using TextInputClient::
OnUnhandledEvent(). Other IME drivers may choose to handle non-char events
in a different way. TextInputClientImpl's implementation of OnHandledEvent()
calls InputMethodMus::DispatchKeyEventPostIME().

Also modified mus_ime_unittests to cover non-char events.

On 2016/08/16 22:32:02, sadrul wrote:
>
https://codereview.chromium.org/2230393002/diff/140001/ui/views/mus/input_met...
> File ui/views/mus/input_method_mus.cc (right):
> 
>
https://codereview.chromium.org/2230393002/diff/140001/ui/views/mus/input_met...
> ui/views/mus/input_method_mus.cc:67:
> input_method_->ProcessKeyEvent(ui::Event::Clone(*event));
> Wait, this seems backwards. We are supposed to receive non-char events from
the
> platform, and send those to the IME, to get back char events, right? For
> example, if user presses arrow keys, then the IME should receive those events?
> 
> I was thinking we could remove the block of code in
>
https://cs.chromium.org/chromium/src/services/ui/ws/platform_display.cc?sq=pa...
> once we have the IME working.

[Hadi, 2016-08-17 21:42:19]

Description was changed from

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.

BUG=548407
==========

to

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407
==========

[sadrul, 2016-08-18 03:57:58]

https://codereview.chromium.org/2230393002/diff/180001/services/ui/public/int...
File services/ui/public/interfaces/ime.mojom (right):

https://codereview.chromium.org/2230393002/diff/180001/services/ui/public/int...
services/ui/public/interfaces/ime.mojom:105: OnUnhandledEvent(ui.mojom.Event
key_event);
Document.

https://codereview.chromium.org/2230393002/diff/180001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.cc (right):

https://codereview.chromium.org/2230393002/diff/180001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.cc:29: if (key_event->is_char())
This should be a DCHECK()? Why would we get INSERT_CHAR with a non-char event?

https://codereview.chromium.org/2230393002/diff/180001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.cc:51:
input_method_->DispatchKeyEventPostIME(key_event->AsKeyEvent());
Ideally, ui::TextInputClient would know how to deal with unprocessed key-events,
and this function would just forward |key_event| to |text_input_client_|. But
the unprocessed key-events go through the ui::InputMethod in chrome, so this is
OK for now.

[Hadi, 2016-08-18 14:07:46]

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

[Hadi, 2016-08-18 14:07:56]

Addressed feedback. PTAL.

[commit-bot: I haz the power, 2016-08-18 14:07:59]

Dry run: 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, 2016-08-18 14:58:02]

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

[commit-bot: I haz the power, 2016-08-18 14:58:03]

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

[sadrul, 2016-08-18 20:31:52]

lgtm

[Hadi, 2016-08-18 22:36:21]

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

[Hadi, 2016-08-18 22:36:22]

sky@ can you please provide OWNERS for mash/* and services/ui/*?

tsepez@ can you please review  services/ui/public/interfaces/ime.mojom?

[sky, 2016-08-18 23:43:08]

https://codereview.chromium.org/2230393002/diff/220001/mash/session/session.cc
File mash/session/session.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/mash/session/session.c...
mash/session/session.cc:114: StartRestartableService(
Do we really need the Ime service to be restartable? It seems like the IME
service should be created the first time someone uses it and be allowed to close
when no one is connected to it.

https://codereview.chromium.org/2230393002/diff/220001/mash/session/session.c...
mash/session/session.cc:115: "mojo:test_ime_driver",
Why are we starting a test driver?

https://codereview.chromium.org/2230393002/diff/220001/services/ui/public/int...
File services/ui/public/interfaces/ime.mojom (right):

https://codereview.chromium.org/2230393002/diff/220001/services/ui/public/int...
services/ui/public/interfaces/ime.mojom:105: // Called when IME driver didn't
handle the event.
Please better document when this is called. Maybe the documentation should be in
InputMethod, e.g.:

// Called to process a key event. If the event is not handled by
// InputMethod OnUnhandledEvent is called on TextInputClient.

and then you could refer to ProcessKeyEvent here.

Also, not for this cl, but remind me why we went with the name of
TextInputClient instead of InputMethodClient? Wouldn't that better match who the
client is paired to?

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:67: event->StopPropagation();
Is the unconditional StopPropagation here going to be problematic? Will
accelerators be impacted? Can we only send the event if it's a char?

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:99: return "";
"" -> std::string()

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.cc:28: ui::KeyEvent* key_event =
(*event->key_event)->AsKeyEvent();
DCHECK event is a KeyEvent?

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.h (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.h:13: };
no ';'

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/window_ma...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/window_ma...
ui/views/mus/window_manager_connection.cc:101:
native_widget->window_tree_host()->InitInputMethod(connector_);
Not all creation of NativeWidgetMus flows through this function. You should move
initialization to NativeWidgetMus::Init, but make sure you verify there is a
WindowManagerConnection before doing that.

[Tom Sepez, 2016-08-19 17:36:42]

Mojom LGTM after updating comment per Sky's suggestion.

[Hadi, 2016-08-25 18:06:06]

sky@: Addressed feedback. PTAL.

https://codereview.chromium.org/2230393002/diff/220001/mash/session/session.cc
File mash/session/session.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/mash/session/session.c...
mash/session/session.cc:114: StartRestartableService(
On 2016/08/18 23:43:07, sky wrote:
> Do we really need the Ime service to be restartable? It seems like the IME
> service should be created the first time someone uses it and be allowed to
close
> when no one is connected to it.

Based on our internal discussion, moved starting of IME driver to IMEServerImpl.

https://codereview.chromium.org/2230393002/diff/220001/mash/session/session.c...
mash/session/session.cc:115: "mojo:test_ime_driver",
On 2016/08/18 23:43:07, sky wrote:
> Why are we starting a test driver?

Based on our internal discussion, InputMethodMus always assume that there is a
running driver. We only have the test driver for now, and we will switch to the
proper driver once we have it.

https://codereview.chromium.org/2230393002/diff/220001/services/ui/public/int...
File services/ui/public/interfaces/ime.mojom (right):

https://codereview.chromium.org/2230393002/diff/220001/services/ui/public/int...
services/ui/public/interfaces/ime.mojom:105: // Called when IME driver didn't
handle the event.
On 2016/08/18 23:43:07, sky wrote:
> Please better document when this is called. Maybe the documentation should be
in
> InputMethod, e.g.:
> 
> // Called to process a key event. If the event is not handled by
> // InputMethod OnUnhandledEvent is called on TextInputClient.
> 
> and then you could refer to ProcessKeyEvent here.

Done.

> 
> Also, not for this cl, but remind me why we went with the name of
> TextInputClient instead of InputMethodClient? Wouldn't that better match who
the
> client is paired to?

I agree that InputMethodClient is a better name. I chose to go with
TextInputClient so it matches with the corresponding ui::TextInputClient, which
has a similar functionality.

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:67: event->StopPropagation();
On 2016/08/18 23:43:07, sky wrote:
> Is the unconditional StopPropagation here going to be problematic? Will
> accelerators be impacted? Can we only send the event if it's a char?

On should we call event->StopPropagation() or not, the short answer is I don't
think it will make any difference with the current information. More details:

1. If the key event still needs handling post-ime, it is done by calling
DispatchKeyEventPostIME(). In this case we call this function at
TextInputClientImpl::OnUnhandledEvent().

2. In ChromeOS, ash::InputMethodEventHandler::OnKeyEvent() marks the event as
consumed right after sending it to InputMethod::DispatchKeyEvent(). The "if
(post_ime_)" check is used for bypassing InputMethodEventHandler in the
implementation of DispatchKeyEventPostIME().

3. In Mus+ash's case, WindowTreeHostMus::DispatchEvent() already stops
processing the event after calling InputMethod::DispatchKeyEvent(), so whether
we add StopPropagation() here or not shouldn't make any visible difference.

So, I can move the event->StopPropagation() to
WindowTreeHostMus::DispatchEvent() so it looks a bit more like the code for
ChromeOS (i.e. caller of InputMethod::DispatchKeyEvent() stops the propagation).

On should we send non-char key events to IME, I couldn't find a place in
ChromeOS where it stops the key events from being sent to the extension. In the
input method extension framework docs I couldn't find anything mentioning
chrome.input.ime.onKeyEvent() will only receive character events. By looking at
Google Input Tools source code, it seemed to me that there are places where left
and right arrows are handled (although I need to do more investigation to make
sure those are called via onKeyEvent()). So I think we should send key events to
the extension.

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:99: return "";
On 2016/08/18 23:43:07, sky wrote:
> "" -> std::string()

Done.

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.cc:28: ui::KeyEvent* key_event =
(*event->key_event)->AsKeyEvent();
On 2016/08/18 23:43:07, sky wrote:
> DCHECK event is a KeyEvent?

Done.

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
File ui/views/mus/text_input_client_impl.h (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/text_inpu...
ui/views/mus/text_input_client_impl.h:13: };
On 2016/08/18 23:43:08, sky wrote:
> no ';'

Done.

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/window_ma...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/window_ma...
ui/views/mus/window_manager_connection.cc:101:
native_widget->window_tree_host()->InitInputMethod(connector_);
On 2016/08/18 23:43:08, sky wrote:
> Not all creation of NativeWidgetMus flows through this function. You should
move
> initialization to NativeWidgetMus::Init, but make sure you verify there is a
> WindowManagerConnection before doing that.

Done.

[sky, 2016-08-25 20:17:53]

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/220001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.cc:67: event->StopPropagation();
On 2016/08/25 18:06:06, Hadi wrote:
> On 2016/08/18 23:43:07, sky wrote:
> > Is the unconditional StopPropagation here going to be problematic? Will
> > accelerators be impacted? Can we only send the event if it's a char?
> 
> On should we call event->StopPropagation() or not, the short answer is I don't
> think it will make any difference with the current information. More details:
> 
> 1. If the key event still needs handling post-ime, it is done by calling
> DispatchKeyEventPostIME(). In this case we call this function at
> TextInputClientImpl::OnUnhandledEvent().
> 
> 2. In ChromeOS, ash::InputMethodEventHandler::OnKeyEvent() marks the event as
> consumed right after sending it to InputMethod::DispatchKeyEvent(). The "if
> (post_ime_)" check is used for bypassing InputMethodEventHandler in the
> implementation of DispatchKeyEventPostIME().
> 
> 3. In Mus+ash's case, WindowTreeHostMus::DispatchEvent() already stops
> processing the event after calling InputMethod::DispatchKeyEvent(), so whether
> we add StopPropagation() here or not shouldn't make any visible difference.
> 
> So, I can move the event->StopPropagation() to
> WindowTreeHostMus::DispatchEvent() so it looks a bit more like the code for
> ChromeOS (i.e. caller of InputMethod::DispatchKeyEvent() stops the
propagation).
> 
> On should we send non-char key events to IME, I couldn't find a place in
> ChromeOS where it stops the key events from being sent to the extension. In
the
> input method extension framework docs I couldn't find anything mentioning
> chrome.input.ime.onKeyEvent() will only receive character events. By looking
at
> Google Input Tools source code, it seemed to me that there are places where
left
> and right arrows are handled (although I need to do more investigation to make
> sure those are called via onKeyEvent()). So I think we should send key events
to
> the extension.

Wow, thanks for the great writeup, and it all makes sense.

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.h (right):

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.h:54: // The toplevel window which is not owned by
this class.
Document this may be null for tests.

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:73: event->StopPropagation();
I tend to think this should be in InputMethodMus::DispatchKeyEvent. It seems
clearer that way as InputMethodMus can then document why it needs to stop
propagation.

[Hadi, 2016-08-26 15:57:07]

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/input_met...
File ui/views/mus/input_method_mus.h (right):

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/input_met...
ui/views/mus/input_method_mus.h:54: // The toplevel window which is not owned by
this class.
On 2016/08/25 20:17:53, sky wrote:
> Document this may be null for tests.

Done.

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2230393002/diff/280001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.cc:73: event->StopPropagation();
On 2016/08/25 20:17:53, sky wrote:
> I tend to think this should be in InputMethodMus::DispatchKeyEvent. It seems
> clearer that way as InputMethodMus can then document why it needs to stop
> propagation.

Move it. Also I added a temporary check so we only send char events to the IME
driver, so accelerators work for now. I'll investigate the options we have to
properly fix this, and then remove that check.

[Hadi, 2016-08-26 16:02:32]

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

[sky, 2016-08-26 16:02:38]

LGTM

[commit-bot: I haz the power, 2016-08-26 16:03:17]

Dry run: 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, 2016-08-26 17:12:37]

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

[commit-bot: I haz the power, 2016-08-26 17:12:38]

Dry run: This issue passed the CQ dry run.

[Hadi, 2016-08-26 17:35:18]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2016-08-26 17:35:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2230393002/#ps300001
(title: ".")

[commit-bot: I haz the power, 2016-08-26 17:35:40]

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, 2016-08-26 18:23:13]

Description was changed from

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407
==========

to

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407
==========

[commit-bot: I haz the power, 2016-08-26 18:23:14]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-08-26 18:25:30]

Description was changed from

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407
==========

to

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407

Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Cr-Commit-Position: refs/heads/master@{#414748}
==========

[commit-bot: I haz the power, 2016-08-26 18:25:32]

Patchset 16 (id:??) landed as
https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Cr-Commit-Position: refs/heads/master@{#414748}

[Elliot Glaysher, 2016-08-26 20:49:52]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2284003002/ by erg@chromium.org.

The reason for reverting is: All text input appears to be broken in chrome on
mash, and I bisected to this patch. Reverting locally appears to fix this..

[Hadi, 2016-08-30 19:27:30]

Description was changed from

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407

Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Cr-Commit-Position: refs/heads/master@{#414748}
==========

to

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407

Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Cr-Commit-Position: refs/heads/master@{#414748}
==========

[sadrul, 2016-08-30 19:29:31]

lgtm after the packaging change.

[Hadi, 2016-08-30 19:35:23]

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

[Hadi, 2016-08-30 19:35:24]

erg@ can you please try PS #17 on your side to make sure it fixes the issue
before I try to re-land the CL?

Thanks

[Hadi, 2016-08-30 20:28:07]

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

[commit-bot: I haz the power, 2016-08-30 20:28:46]

Dry run: 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, 2016-08-30 21:52:20]

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

[commit-bot: I haz the power, 2016-08-30 21:52:21]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2016-08-30 22:15:28]

On 2016/08/30 19:35:24, Hadi wrote:
> erg@ can you please try PS #17 on your side to make sure it fixes the issue
> before I try to re-land the CL?
> 
> Thanks

Elliot is running with --mash, which starts up everything in the mash
environment. You should be able to do the same.

You also need to update service_manifest of both chrome/app/mash/BUILD.gn and
chrome/test/BUILD.gn.

[Hadi, 2016-08-31 14:03:30]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2016-08-31 14:03:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sky@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2230393002/#ps340001
(title: "Update mash_browser_tests_manifest.")

[Hadi, 2016-08-31 14:03:32]

The CQ bit was unchecked by moshayedi@chromium.org

[Hadi, 2016-08-31 14:03:53]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2016-08-31 14:03:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sky@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2230393002/#ps340001
(title: "Update mash_browser_tests_manifest.")

[commit-bot: I haz the power, 2016-08-31 14:04:10]

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, 2016-08-31 16:17:14]

Description was changed from

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407

Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Cr-Commit-Position: refs/heads/master@{#414748}
==========

to

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407

Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Cr-Commit-Position: refs/heads/master@{#414748}
==========

[commit-bot: I haz the power, 2016-08-31 16:17:15]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-08-31 16:19:45]

Description was changed from

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407

Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Cr-Commit-Position: refs/heads/master@{#414748}
==========

to

==========
IME for Mus: Make InputMethodMus use the IME Mojo API.

This CL:
* Uses Mus' IMEServer to do the IME logic in InputMethodMus.
* Adds unittests to test InputMethodMus.
* Starts test_ime_driver on mus+ash session startup.
* Modifies test_ime_driver to not-handle non character events.

BUG=548407

Committed: https://crrev.com/aa4f90ab72b60fd90e048b1e18193c30f0462d77
Committed: https://crrev.com/b26c35f30555178bfea1c07b47801fb58222fb0f
Cr-Original-Commit-Position: refs/heads/master@{#414748}
Cr-Commit-Position: refs/heads/master@{#415646}
==========

[commit-bot: I haz the power, 2016-08-31 16:19:49]

Patchset 18 (id:??) landed as
https://crrev.com/b26c35f30555178bfea1c07b47801fb58222fb0f
Cr-Commit-Position: refs/heads/master@{#415646}