AppShell: Create input method and handle key events

AppShell: Create input method and handle key events

Create an input method and an event handler to dispatch key events on the input
method.

Aura gives us raw key up/down events, but text input requires an input method
("IME"). This also enables the keypress event to be sent to the renderer.

BUG=675413
R=steel@chromium.org

Committed: https://crrev.com/81c91f8fcdd5f0321d80de3795e97e62e12eced8
Cr-Commit-Position: refs/heads/master@{#439363}

[michaelpg, 2016-12-15 05:57:26]

PTAL. Not a ton of code, but it took me all day to find which event was missing
>_<

[Rahul Chaturvedi, 2016-12-15 22:42:46]

(lgtm % nits) but please have an aura reviewer look at it before you land. Maybe
jamescook@?

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/in...
File extensions/shell/browser/input_method_event_handler.h (right):

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/in...
extensions/shell/browser/input_method_event_handler.h:24: // True when an event
is dispatched within DispatchKeyEventPostIME(), meaning
nit: This comment should be on the field declaration. This method is a
self-documenting setter.

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/in...
extensions/shell/browser/input_method_event_handler.h:26: void SetPostIME(bool
post_ime);
nit: Since this is a setter, set_post_ime(bool post_ime)?

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
File extensions/shell/browser/shell_desktop_controller_aura.cc (right):

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
extensions/shell/browser/shell_desktop_controller_aura.cc:348:
host_->SetSharedInputMethod(input_method_.get());
When we're destructing, and input_method_ will no longer be valid, do we need to
unset this?

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
File extensions/shell/browser/shell_desktop_controller_aura.h (right):

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
extensions/shell/browser/shell_desktop_controller_aura.h:129: 
nit: Add a comment here that input_method_ needs to outlive
input_method_event_handler_, hence the order of these declarations should not be
changed.

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
extensions/shell/browser/shell_desktop_controller_aura.h:131: 
nit: remove line, so these two are grouped together.

[michaelpg, 2016-12-15 23:33:45]

michaelpg@chromium.org changed reviewers:
+ jamescook@chromium.org

[michaelpg, 2016-12-15 23:33:46]

James, do you mind taking a look?

[michaelpg, 2016-12-15 23:37:21]

michaelpg@chromium.org changed reviewers:
+ shuchen@chromium.org

[michaelpg, 2016-12-15 23:37:21]

Actually, shuchen, you may as well look at this too, just to make sure I'm not
doing anything crazy.

[michaelpg, 2016-12-15 23:55:53]

Updated for rkc's feedback.

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/in...
File extensions/shell/browser/input_method_event_handler.h (right):

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/in...
extensions/shell/browser/input_method_event_handler.h:24: // True when an event
is dispatched within DispatchKeyEventPostIME(), meaning
On 2016/12/15 22:42:46, Rahul Chaturvedi wrote:
> nit: This comment should be on the field declaration. This method is a
> self-documenting setter.

Done.

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/in...
extensions/shell/browser/input_method_event_handler.h:26: void SetPostIME(bool
post_ime);
On 2016/12/15 22:42:46, Rahul Chaturvedi wrote:
> nit: Since this is a setter, set_post_ime(bool post_ime)?

Done.

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
File extensions/shell/browser/shell_desktop_controller_aura.cc (right):

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
extensions/shell/browser/shell_desktop_controller_aura.cc:348:
host_->SetSharedInputMethod(input_method_.get());
On 2016/12/15 22:42:46, Rahul Chaturvedi wrote:
> When we're destructing, and input_method_ will no longer be valid, do we need
to
> unset this?

Ugh, good point, this isn't quite safe. shuchen, am I abusing
SetSharedInputMethod?

We can achieve the same thing by triggering the host to create the input method
instead, then overriding the input method's delegate to point to us. Then the
host owns the input method. See next patch.

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
File extensions/shell/browser/shell_desktop_controller_aura.h (right):

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
extensions/shell/browser/shell_desktop_controller_aura.h:129: 
On 2016/12/15 22:42:46, Rahul Chaturvedi wrote:
> nit: Add a comment here that input_method_ needs to outlive
> input_method_event_handler_, hence the order of these declarations should not
be
> changed.

Removed input_method_.

https://codereview.chromium.org/2580733002/diff/1/extensions/shell/browser/sh...
extensions/shell/browser/shell_desktop_controller_aura.h:131: 
On 2016/12/15 22:42:46, Rahul Chaturvedi wrote:
> nit: remove line, so these two are grouped together.

Acknowledged.

[James Cook, 2016-12-16 00:24:38]

I can take a look at this tomorrow morning, but I'm not super-familiar with
event dispatching at that level. Consider asking sadrul@ if you want better
feedback. I'm happy to look, tho.

[James Cook, 2016-12-16 19:00:50]

Code looks fine. Is it possible to add some test coverage for keyboard events to
shell_desktop_controller_aura_unittest.cc?

https://codereview.chromium.org/2580733002/diff/40001/extensions/shell/browse...
File extensions/shell/browser/input_method_event_handler.cc (right):

https://codereview.chromium.org/2580733002/diff/40001/extensions/shell/browse...
extensions/shell/browser/input_method_event_handler.cc:13: :
input_method_(input_method), post_ime_(false) {}
nit: DCHECK(input_method_) so it's self documenting that input_method can't be
null

(I'm trying to encourage people to do this sort of thing. I love preconditions.
:-)

https://codereview.chromium.org/2580733002/diff/40001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller_aura.cc (right):

https://codereview.chromium.org/2580733002/diff/40001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller_aura.cc:371:
host_->window()->RemovePreTargetHandler(root_window_event_filter_.get());
good catch

[michaelpg, 2016-12-17 02:51:39]

Patchset #4 (id:60001) has been deleted

[michaelpg, 2016-12-17 02:56:54]

Patchset #4 (id:80001) has been deleted

[michaelpg, 2016-12-17 03:07:46]

On 2016/12/16 19:00:50, James Cook wrote:
> Code looks fine. Is it possible to add some test coverage for keyboard events
to
> shell_desktop_controller_aura_unittest.cc?

Done... took me all day to figure out that the current test class creates two
hosts and displays (it was segfaulting when adding a second, even empty, test
case)

https://codereview.chromium.org/2580733002/diff/40001/extensions/shell/browse...
File extensions/shell/browser/input_method_event_handler.cc (right):

https://codereview.chromium.org/2580733002/diff/40001/extensions/shell/browse...
extensions/shell/browser/input_method_event_handler.cc:13: :
input_method_(input_method), post_ime_(false) {}
On 2016/12/16 19:00:50, James Cook wrote:
> nit: DCHECK(input_method_) so it's self documenting that input_method can't be
> null
> 
> (I'm trying to encourage people to do this sort of thing. I love
preconditions.
> :-)

Done.

[michaelpg, 2016-12-17 03:09:31]

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

[commit-bot: I haz the power, 2016-12-17 03:10:04]

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-12-17 04:02:20]

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

[commit-bot: I haz the power, 2016-12-17 04:02:21]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-17 18:24:42]

LGTM with nits. Thanks for adding the test.

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
File extensions/shell/browser/shell_desktop_controller_aura.cc (right):

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
extensions/shell/browser/shell_desktop_controller_aura.cc:383:
display::Screen::SetScreenInstance(nullptr);
nit: do this before screen_.reset() -- if someone adds side effects to the
screen destructor you probably don't want to allow it to reach into the
singleton instance

Also, I suggest adding a comment somewhere that app_shell only supports a single
screen/display. (I'm pretty sure that's true -- it looks like there's only one
ShellDesktopController, so one screen_.)

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
File extensions/shell/browser/shell_desktop_controller_aura_unittest.cc (right):

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
extensions/shell/browser/shell_desktop_controller_aura_unittest.cc:95:
EXPECT_TRUE(input_method);
ASSERT_TRUE since line 99 will crash if input_method is null

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
extensions/shell/browser/shell_desktop_controller_aura_unittest.cc:104:
ignore_result(controller_->host_->dispatcher()->DispatchEvent(
Do you know what this will return? If so, EXPECT it instead of ignore_result().

[michaelpg, 2016-12-18 01:46:10]

Thanks!

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
File extensions/shell/browser/shell_desktop_controller_aura.cc (right):

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
extensions/shell/browser/shell_desktop_controller_aura.cc:383:
display::Screen::SetScreenInstance(nullptr);
On 2016/12/17 18:24:41, James Cook wrote:
> nit: do this before screen_.reset() -- if someone adds side effects to the
> screen destructor you probably don't want to allow it to reach into the
> singleton instance
> 
> Also, I suggest adding a comment somewhere that app_shell only supports a
single
> screen/display. (I'm pretty sure that's true -- it looks like there's only one
> ShellDesktopController, so one screen_.)

Done.

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
File extensions/shell/browser/shell_desktop_controller_aura_unittest.cc (right):

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
extensions/shell/browser/shell_desktop_controller_aura_unittest.cc:95:
EXPECT_TRUE(input_method);
On 2016/12/17 18:24:41, James Cook wrote:
> ASSERT_TRUE since line 99 will crash if input_method is null

Done.

https://codereview.chromium.org/2580733002/diff/120001/extensions/shell/brows...
extensions/shell/browser/shell_desktop_controller_aura_unittest.cc:104:
ignore_result(controller_->host_->dispatcher()->DispatchEvent(
On 2016/12/17 18:24:41, James Cook wrote:
> Do you know what this will return? If so, EXPECT it instead of
ignore_result().

Done.

[michaelpg, 2016-12-18 01:46:24]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-12-18 01:46:24]

The patchset sent to the CQ was uploaded after l-g-t-m from steel@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2580733002/#ps140001
(title: "nits")

[commit-bot: I haz the power, 2016-12-18 01:46:27]

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

[michaelpg, 2016-12-18 01:46:45]

Description was changed from

==========
AppShell: Create input method and handle key events

Create an input method and an event handler to dispatch key events on the input
method.

Aura gives us raw key up/down events, but text input requires an input method
("IME"). This also enables the keypress event to be sent to the renderer.

BUG=none
R=steel@chromium.org
==========

to

==========
AppShell: Create input method and handle key events

Create an input method and an event handler to dispatch key events on the input
method.

Aura gives us raw key up/down events, but text input requires an input method
("IME"). This also enables the keypress event to be sent to the renderer.

BUG=675413
R=steel@chromium.org
==========

[michaelpg, 2016-12-18 01:46:51]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2016-12-18 01:46:55]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-12-18 01:46:57]

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-12-18 02:50:16]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482025615234020,
"parent_rev": "225f697d4ba322a852ce6a4c596383e290e595fc", "commit_rev":
"e8f61119947847dcd3301d3c19223d23db3d9a5e"}

[commit-bot: I haz the power, 2016-12-18 02:50:53]

Description was changed from

==========
AppShell: Create input method and handle key events

Create an input method and an event handler to dispatch key events on the input
method.

Aura gives us raw key up/down events, but text input requires an input method
("IME"). This also enables the keypress event to be sent to the renderer.

BUG=675413
R=steel@chromium.org
==========

to

==========
AppShell: Create input method and handle key events

Create an input method and an event handler to dispatch key events on the input
method.

Aura gives us raw key up/down events, but text input requires an input method
("IME"). This also enables the keypress event to be sent to the renderer.

BUG=675413
R=steel@chromium.org

Review-Url: https://codereview.chromium.org/2580733002
==========

[commit-bot: I haz the power, 2016-12-18 02:50:54]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-12-18 02:53:09]

Description was changed from

==========
AppShell: Create input method and handle key events

Create an input method and an event handler to dispatch key events on the input
method.

Aura gives us raw key up/down events, but text input requires an input method
("IME"). This also enables the keypress event to be sent to the renderer.

BUG=675413
R=steel@chromium.org

Review-Url: https://codereview.chromium.org/2580733002
==========

to

==========
AppShell: Create input method and handle key events

Create an input method and an event handler to dispatch key events on the input
method.

Aura gives us raw key up/down events, but text input requires an input method
("IME"). This also enables the keypress event to be sent to the renderer.

BUG=675413
R=steel@chromium.org

Committed: https://crrev.com/81c91f8fcdd5f0321d80de3795e97e62e12eced8
Cr-Commit-Position: refs/heads/master@{#439363}
==========

[commit-bot: I haz the power, 2016-12-18 02:53:10]

Patchset 6 (id:??) landed as
https://crrev.com/81c91f8fcdd5f0321d80de3795e97e62e12eced8
Cr-Commit-Position: refs/heads/master@{#439363}