Sync Ime and Keyboard events.

Sync Ime and Keyboard events.

Adding code to sync ime and keyboard events to make sure they arrive to
renderer in correct order. Ime events will be queued in the input_event_filter
if the message_ queue is not empty (Compositor is handling those events). If
the queue is empty, then the message will not be accepted and just forwared
directly to renderer.

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

[aurimas (slooooooooow), 2012-12-10 23:20:17]

Hello Min,

Could you take a look at this CL please?

Thanks,
Aurimas

[aurimas (slooooooooow), 2012-12-10 23:39:55]

Hello James,

I have created a CL for syncing Ime and Keyboard events on InputEventFilter as
discussed in the bug thread. Could you review this change?

Thanks,
Aurimas

[aurimas (slooooooooow), 2012-12-11 02:08:10]

Hello James (Zhe),

Could you take a look at this CL?

Aurimas

[James Su, 2012-12-11 02:54:49]

https://codereview.chromium.org/11490017/diff/2001/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/11490017/diff/2001/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:49: forwardPendingImeEvents();
Should pending ime events be forwarded after forwarding the input event?

https://codereview.chromium.org/11490017/diff/2001/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:176: while
(isImeEvent(messages_.front())) {
Is it necessary to check if messages_ is empty?

[aurimas (slooooooooow), 2012-12-11 03:41:22]

See comments below.

https://codereview.chromium.org/11490017/diff/2001/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/11490017/diff/2001/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:49: forwardPendingImeEvents();
On 2012/12/11 02:54:50, James Su wrote:
> Should pending ime events be forwarded after forwarding the input event?

Good catch. Fixed.

https://codereview.chromium.org/11490017/diff/2001/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:176: while
(isImeEvent(messages_.front())) {
On 2012/12/11 02:54:50, James Su wrote:
> Is it necessary to check if messages_ is empty?

There could be a case were the queue is:
keyboard | ime | ime | keyboard

In this case we want to only forward ime and ime and keep the last keyboard
event as it is yet to be processed by the compositor.

Do that make sense?

[jamesr, 2012-12-11 03:48:48]

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:170: return false;
indent by 2 spaces

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.h (right):

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.h:77: // Returns whether given message
is an Ime Event.
separate this from the previous line with a blank line

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.h:78: bool isImeEvent(const
IPC::Message& message);
functions in chromium that aren't trivial setters/getters start with an
Uppercase letter.

this function doesn't depend on any member state, so it should either be a
static member function or just a free function in the .cc

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.h:79: // Queues the given Ime Event
message to the message_ queue if the queue is
"Ime Event" is awkward in prose. Maybe IME event?

[jamesr, 2012-12-11 03:54:07]

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:176: while
(isImeEvent(messages_.front())) {
you're calling stl::queue::front() without verifying that you satisfy its
precondition - that the queue isn't empty

[James Su, 2012-12-11 03:57:04]

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:176: while
(isImeEvent(messages_.front())) {
I think we should check if messages_ is empty here. Because if an input method
is followed by one or more ime events, the queue may look like:
front: input, ime, ime, ...

The input event will be popped first, thus the queue might be empty if all
remained events are ime events.

[aurimas (slooooooooow), 2012-12-11 04:07:25]

Addressed all the comments.

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:170: return false;
On 2012/12/11 03:48:49, jamesr wrote:
> indent by 2 spaces

Done.

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:176: while
(isImeEvent(messages_.front())) {
On 2012/12/11 03:57:04, James Su wrote:
> I think we should check if messages_ is empty here. Because if an input method
> is followed by one or more ime events, the queue may look like:
> front: input, ime, ime, ...
> 
> The input event will be popped first, thus the queue might be empty if all
> remained events are ime events.

Done.

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.h (right):

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.h:77: // Returns whether given message
is an Ime Event.
On 2012/12/11 03:48:49, jamesr wrote:
> separate this from the previous line with a blank line

Done.

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.h:78: bool isImeEvent(const
IPC::Message& message);
On 2012/12/11 03:48:49, jamesr wrote:
> functions in chromium that aren't trivial setters/getters start with an
> Uppercase letter.
> 
> this function doesn't depend on any member state, so it should either be a
> static member function or just a free function in the .cc

Done.

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.h:79: // Queues the given Ime Event
message to the message_ queue if the queue is
On 2012/12/11 03:48:49, jamesr wrote:
> "Ime Event" is awkward in prose. Maybe IME event?

Done.

https://codereview.chromium.org/11490017/diff/9006/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.h:79: // Queues the given Ime Event
message to the message_ queue if the queue is
On 2012/12/11 03:48:49, jamesr wrote:
> "Ime Event" is awkward in prose. Maybe IME event?

Done.

[jamesr, 2012-12-11 04:25:43]

Inline diffs appear to be broken on this upload

- put IsImeEvent in an anonymous namespace
(http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces)

- put a blank line above the function comments for ForwardPendingImeEvents()

otherwise lgtm

[James Su, 2012-12-11 04:26:41]

LGTM.

Weird that side-by-side diffs are empty, but unified diffs are ok.

[qinmin, 2012-12-11 05:15:48]

lgtm with one nit

https://codereview.chromium.org/11490017/diff/8004/content/renderer/gpu/input...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/11490017/diff/8004/content/renderer/gpu/input...
content/renderer/gpu/input_event_filter.cc:16: 
does this belongs to content namespace or an anonymous one?

[aurimas (slooooooooow), 2012-12-11 05:50:31]

Fixed the nits.

[commit-bot: I haz the power, 2012-12-11 05:50:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/11490017/8

[commit-bot: I haz the power, 2012-12-11 09:54:04]

Change committed as 172287