Supports fake key events for setComposition/commitText by on-screen keyboards.

Supports fake key events for setComposition/commitText by on-screen keyboards.

IMF now assumes the IME composing happens with a physical key being pressed.
That assumption is not true for on-screen keyboard scenario.
For on-screen keyboard, when user touch a key button on the screen, the JS
code will call IME API like chrome.input.ime.setComposition(). IMF then should
generate events as:
  keydown(229), compositionstart, compositionupdate, input, keyup.
The events compositionstart, compositionupdate and input are generated by
RenderWidgetHostViewAura::SetCompositionText(). So we need to send fake key
events to generate keydown(229) and keyup.

The other part of this change is to make InputMethodEventFilter not depend on
physical key events. Originally, InputMethodEventFilter relies on key events
to find the root window and then get the EventProcessor. It will break
on-screen scenario, because without a physical key being pressed,
target_dispatcher_ will be NULL.
This cl is to let InputMethodEventFilter to always get the EventProcessor from
input_method_->GetTextInputClient()->GetAttachedWindow()->GetRootWindow()...

BUG=376183
TEST=Verified on Pixel device.

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

[Shu Chen, 2014-05-22 07:39:53]

nona@, pls review the changes under ui/base/ime/...

thakis@, pls review the changes under ui/wm/core/...

Thanks guys.

[Seigo Nonaka, 2014-05-26 04:59:28]

lgtm

https://codereview.chromium.org/298893003/diff/20001/ui/wm/core/input_method_...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/20001/ui/wm/core/input_method_...
ui/wm/core/input_method_event_filter.cc:66: if (target_dispatcher_) {
nit: I prefer early exit like:
if (!target_dispatcher_)
  return aura_event.handled();

[Shu Chen, 2014-05-26 05:04:48]

https://codereview.chromium.org/298893003/diff/20001/ui/wm/core/input_method_...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/20001/ui/wm/core/input_method_...
ui/wm/core/input_method_event_filter.cc:66: if (target_dispatcher_) {
On 2014/05/26 04:59:29, Seigo Nonaka wrote:
> nit: I prefer early exit like:
> if (!target_dispatcher_)
>   return aura_event.handled();

I don't think it's a good idea to have a dup code like this:

if (!target_dispatcher_)
  return aura_event.handled();
...
return aura_event.handled();

[Yuki, 2014-05-26 06:29:29]

https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.cc:476: SendFakeProcessKeyEvent(false);
Do we really want to send a PROCESSKEY event for keyup?
IIUC, DOM Level3 Events Sepc doesn't require it and no other browsers send such
events.

[Shu Chen, 2014-05-26 06:35:59]

https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.cc:476: SendFakeProcessKeyEvent(false);
On 2014/05/26 06:29:29, Yuki wrote:
> Do we really want to send a PROCESSKEY event for keyup?
> IIUC, DOM Level3 Events Sepc doesn't require it and no other browsers send
such
> events.

I think it would be better to keep the events sequence as the IME composing by
physical keyboard.
Otherwise, there is potential risks to break certain web apps. WDYT?

[Yuki, 2014-05-26 06:56:08]

https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.cc:476: SendFakeProcessKeyEvent(false);
On 2014/05/26 06:35:59, Shu Chen wrote:
> On 2014/05/26 06:29:29, Yuki wrote:
> > Do we really want to send a PROCESSKEY event for keyup?
> > IIUC, DOM Level3 Events Sepc doesn't require it and no other browsers send
> such
> > events.
> 
> I think it would be better to keep the events sequence as the IME composing by
> physical keyboard.
> Otherwise, there is potential risks to break certain web apps. WDYT?

http://www.w3.org/TR/DOM-Level-3-Events/
6.5.2 Legacy key models
If an Input Method Editor is processing key input and the event is keydown,
return 229.

Following the standard, you must not send 229 event for keyup.  The spec
explicitly says "If the event is keydown".

In addition, there are many Japanese articles that says "229 event is send only
for keydown, never for keyup", and web developers expect 229 keydown, and never
expect 229 keyup.

I didn't find an English article, but it should be because there is no (or very
minor) IME for English.  Probably you find many Chinese articles that says the
same thing.

I highly recommend that you don't break the existing behavior or custom.

[Shu Chen, 2014-05-26 12:28:35]

On 2014/05/26 06:56:08, Yuki wrote:
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> File ui/base/ime/input_method_chromeos.cc (right):
> 
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> ui/base/ime/input_method_chromeos.cc:476: SendFakeProcessKeyEvent(false);
> On 2014/05/26 06:35:59, Shu Chen wrote:
> > On 2014/05/26 06:29:29, Yuki wrote:
> > > Do we really want to send a PROCESSKEY event for keyup?
> > > IIUC, DOM Level3 Events Sepc doesn't require it and no other browsers send
> > such
> > > events.
> > 
> > I think it would be better to keep the events sequence as the IME composing
by
> > physical keyboard.
> > Otherwise, there is potential risks to break certain web apps. WDYT?
> 
> http://www.w3.org/TR/DOM-Level-3-Events/
> 6.5.2 Legacy key models
> If an Input Method Editor is processing key input and the event is keydown,
> return 229.
> 
> Following the standard, you must not send 229 event for keyup.  The spec
> explicitly says "If the event is keydown".
> 
> In addition, there are many Japanese articles that says "229 event is send
only
> for keydown, never for keyup", and web developers expect 229 keydown, and
never
> expect 229 keyup.
> 
> I didn't find an English article, but it should be because there is no (or
very
> minor) IME for English.  Probably you find many Chinese articles that says the
> same thing.
> 
> I highly recommend that you don't break the existing behavior or custom.

Can we send fake KeyA up event instead? I feel it would be weird to send keydown
but not keyup.
I recall GWS used to capture keyup to fetch suggestions for text changes in the
search input field.

[Yuki, 2014-05-26 12:53:42]

On 2014/05/26 12:28:35, Shu Chen wrote:
> On 2014/05/26 06:56:08, Yuki wrote:
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > File ui/base/ime/input_method_chromeos.cc (right):
> > 
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > ui/base/ime/input_method_chromeos.cc:476: SendFakeProcessKeyEvent(false);
> > On 2014/05/26 06:35:59, Shu Chen wrote:
> > > On 2014/05/26 06:29:29, Yuki wrote:
> > > > Do we really want to send a PROCESSKEY event for keyup?
> > > > IIUC, DOM Level3 Events Sepc doesn't require it and no other browsers
send
> > > such
> > > > events.
> > > 
> > > I think it would be better to keep the events sequence as the IME
composing
> by
> > > physical keyboard.
> > > Otherwise, there is potential risks to break certain web apps. WDYT?
> > 
> > http://www.w3.org/TR/DOM-Level-3-Events/
> > 6.5.2 Legacy key models
> > If an Input Method Editor is processing key input and the event is keydown,
> > return 229.
> > 
> > Following the standard, you must not send 229 event for keyup.  The spec
> > explicitly says "If the event is keydown".
> > 
> > In addition, there are many Japanese articles that says "229 event is send
> only
> > for keydown, never for keyup", and web developers expect 229 keydown, and
> never
> > expect 229 keyup.
> > 
> > I didn't find an English article, but it should be because there is no (or
> very
> > minor) IME for English.  Probably you find many Chinese articles that says
the
> > same thing.
> > 
> > I highly recommend that you don't break the existing behavior or custom.
> 
> Can we send fake KeyA up event instead? I feel it would be weird to send
keydown
> but not keyup.
> I recall GWS used to capture keyup to fetch suggestions for text changes in
the
> search input field.

What do you mean by "fake KeyA up"?

Reading the paragraph starting with "The keyCode for keydown or keyup events is
calculated as follows:" in 6.5.2, the 2nd rule applies to only keydown events. 
It doesn't applies to keyup events, but other rules apply to keyup events.  As a
result, usually keyup events hold a virtual keycode other than zero or 229 (see
the rest of rules).  Do not fire keyup events with VKEY_PROCESSKEY.

[Shu Chen, 2014-05-26 13:34:48]

On 2014/05/26 12:53:42, Yuki wrote:
> On 2014/05/26 12:28:35, Shu Chen wrote:
> > On 2014/05/26 06:56:08, Yuki wrote:
> > >
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > > File ui/base/ime/input_method_chromeos.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > > ui/base/ime/input_method_chromeos.cc:476: SendFakeProcessKeyEvent(false);
> > > On 2014/05/26 06:35:59, Shu Chen wrote:
> > > > On 2014/05/26 06:29:29, Yuki wrote:
> > > > > Do we really want to send a PROCESSKEY event for keyup?
> > > > > IIUC, DOM Level3 Events Sepc doesn't require it and no other browsers
> send
> > > > such
> > > > > events.
> > > > 
> > > > I think it would be better to keep the events sequence as the IME
> composing
> > by
> > > > physical keyboard.
> > > > Otherwise, there is potential risks to break certain web apps. WDYT?
> > > 
> > > http://www.w3.org/TR/DOM-Level-3-Events/
> > > 6.5.2 Legacy key models
> > > If an Input Method Editor is processing key input and the event is
keydown,
> > > return 229.
> > > 
> > > Following the standard, you must not send 229 event for keyup.  The spec
> > > explicitly says "If the event is keydown".
> > > 
> > > In addition, there are many Japanese articles that says "229 event is send
> > only
> > > for keydown, never for keyup", and web developers expect 229 keydown, and
> > never
> > > expect 229 keyup.
> > > 
> > > I didn't find an English article, but it should be because there is no (or
> > very
> > > minor) IME for English.  Probably you find many Chinese articles that says
> the
> > > same thing.
> > > 
> > > I highly recommend that you don't break the existing behavior or custom.
> > 
> > Can we send fake KeyA up event instead? I feel it would be weird to send
> keydown
> > but not keyup.
> > I recall GWS used to capture keyup to fetch suggestions for text changes in
> the
> > search input field.
> 
> What do you mean by "fake KeyA up"?
> 
> Reading the paragraph starting with "The keyCode for keydown or keyup events
is
> calculated as follows:" in 6.5.2, the 2nd rule applies to only keydown events.

> It doesn't applies to keyup events, but other rules apply to keyup events.  As
a
> result, usually keyup events hold a virtual keycode other than zero or 229
(see
> the rest of rules).  Do not fire keyup events with VKEY_PROCESSKEY.

I meant:
1) keyup must be sent to satisfy certain web app (e.g. GWS).
2) as you mentioned, VKEY_PROCESSKEY keyup must not be sent.
Then, we have to send a keyup event with some other key code. I propose a fixed
keyup of VKEY_A (or any other VKEY_??).

[Yuki, 2014-05-26 14:14:49]

On 2014/05/26 13:34:48, Shu Chen wrote:
> On 2014/05/26 12:53:42, Yuki wrote:
> > On 2014/05/26 12:28:35, Shu Chen wrote:
> > > On 2014/05/26 06:56:08, Yuki wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > > > File ui/base/ime/input_method_chromeos.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > > > ui/base/ime/input_method_chromeos.cc:476:
SendFakeProcessKeyEvent(false);
> > > > On 2014/05/26 06:35:59, Shu Chen wrote:
> > > > > On 2014/05/26 06:29:29, Yuki wrote:
> > > > > > Do we really want to send a PROCESSKEY event for keyup?
> > > > > > IIUC, DOM Level3 Events Sepc doesn't require it and no other
browsers
> > send
> > > > > such
> > > > > > events.
> > > > > 
> > > > > I think it would be better to keep the events sequence as the IME
> > composing
> > > by
> > > > > physical keyboard.
> > > > > Otherwise, there is potential risks to break certain web apps. WDYT?
> > > > 
> > > > http://www.w3.org/TR/DOM-Level-3-Events/
> > > > 6.5.2 Legacy key models
> > > > If an Input Method Editor is processing key input and the event is
> keydown,
> > > > return 229.
> > > > 
> > > > Following the standard, you must not send 229 event for keyup.  The spec
> > > > explicitly says "If the event is keydown".
> > > > 
> > > > In addition, there are many Japanese articles that says "229 event is
send
> > > only
> > > > for keydown, never for keyup", and web developers expect 229 keydown,
and
> > > never
> > > > expect 229 keyup.
> > > > 
> > > > I didn't find an English article, but it should be because there is no
(or
> > > very
> > > > minor) IME for English.  Probably you find many Chinese articles that
says
> > the
> > > > same thing.
> > > > 
> > > > I highly recommend that you don't break the existing behavior or custom.
> > > 
> > > Can we send fake KeyA up event instead? I feel it would be weird to send
> > keydown
> > > but not keyup.
> > > I recall GWS used to capture keyup to fetch suggestions for text changes
in
> > the
> > > search input field.
> > 
> > What do you mean by "fake KeyA up"?
> > 
> > Reading the paragraph starting with "The keyCode for keydown or keyup events
> is
> > calculated as follows:" in 6.5.2, the 2nd rule applies to only keydown
events.
> 
> > It doesn't applies to keyup events, but other rules apply to keyup events. 
As
> a
> > result, usually keyup events hold a virtual keycode other than zero or 229
> (see
> > the rest of rules).  Do not fire keyup events with VKEY_PROCESSKEY.
> 
> I meant:
> 1) keyup must be sent to satisfy certain web app (e.g. GWS).
> 2) as you mentioned, VKEY_PROCESSKEY keyup must not be sent.
> Then, we have to send a keyup event with some other key code. I propose a
fixed
> keyup of VKEY_A (or any other VKEY_??).

No, never send such key events.  It makes people confused.

IIUC, you need nothing for keyup events in addition to the existing code.  We're
already sending keyup events because most (if not all) of IMEs don't consume
keyup events.

For example, a user hit 'a' key with a IME turned on, then
1) ET_KEY_PRESSED with 'a' => IME consumes the key event => keydown with
keycode=229
2) ET_KEY_RELEASED with 'a' => IME doesn't consume the key event => keyup with
keycode=VKEY_A

Please test what key events are sent to a web page.  There are several web pages
for such purpose, or you can create your own test page, like nona@ and I did.

[Shu Chen, 2014-05-26 14:36:21]

On 2014/05/26 14:14:49, Yuki wrote:
> On 2014/05/26 13:34:48, Shu Chen wrote:
> > On 2014/05/26 12:53:42, Yuki wrote:
> > > On 2014/05/26 12:28:35, Shu Chen wrote:
> > > > On 2014/05/26 06:56:08, Yuki wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > > > > File ui/base/ime/input_method_chromeos.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/298893003/diff/20001/ui/base/ime/input_method...
> > > > > ui/base/ime/input_method_chromeos.cc:476:
> SendFakeProcessKeyEvent(false);
> > > > > On 2014/05/26 06:35:59, Shu Chen wrote:
> > > > > > On 2014/05/26 06:29:29, Yuki wrote:
> > > > > > > Do we really want to send a PROCESSKEY event for keyup?
> > > > > > > IIUC, DOM Level3 Events Sepc doesn't require it and no other
> browsers
> > > send
> > > > > > such
> > > > > > > events.
> > > > > > 
> > > > > > I think it would be better to keep the events sequence as the IME
> > > composing
> > > > by
> > > > > > physical keyboard.
> > > > > > Otherwise, there is potential risks to break certain web apps. WDYT?
> > > > > 
> > > > > http://www.w3.org/TR/DOM-Level-3-Events/
> > > > > 6.5.2 Legacy key models
> > > > > If an Input Method Editor is processing key input and the event is
> > keydown,
> > > > > return 229.
> > > > > 
> > > > > Following the standard, you must not send 229 event for keyup.  The
spec
> > > > > explicitly says "If the event is keydown".
> > > > > 
> > > > > In addition, there are many Japanese articles that says "229 event is
> send
> > > > only
> > > > > for keydown, never for keyup", and web developers expect 229 keydown,
> and
> > > > never
> > > > > expect 229 keyup.
> > > > > 
> > > > > I didn't find an English article, but it should be because there is no
> (or
> > > > very
> > > > > minor) IME for English.  Probably you find many Chinese articles that
> says
> > > the
> > > > > same thing.
> > > > > 
> > > > > I highly recommend that you don't break the existing behavior or
custom.
> > > > 
> > > > Can we send fake KeyA up event instead? I feel it would be weird to send
> > > keydown
> > > > but not keyup.
> > > > I recall GWS used to capture keyup to fetch suggestions for text changes
> in
> > > the
> > > > search input field.
> > > 
> > > What do you mean by "fake KeyA up"?
> > > 
> > > Reading the paragraph starting with "The keyCode for keydown or keyup
events
> > is
> > > calculated as follows:" in 6.5.2, the 2nd rule applies to only keydown
> events.
> > 
> > > It doesn't applies to keyup events, but other rules apply to keyup events.

> As
> > a
> > > result, usually keyup events hold a virtual keycode other than zero or 229
> > (see
> > > the rest of rules).  Do not fire keyup events with VKEY_PROCESSKEY.
> > 
> > I meant:
> > 1) keyup must be sent to satisfy certain web app (e.g. GWS).
> > 2) as you mentioned, VKEY_PROCESSKEY keyup must not be sent.
> > Then, we have to send a keyup event with some other key code. I propose a
> fixed
> > keyup of VKEY_A (or any other VKEY_??).
> 
> No, never send such key events.  It makes people confused.
> 
> IIUC, you need nothing for keyup events in addition to the existing code. 
We're
> already sending keyup events because most (if not all) of IMEs don't consume
> keyup events.
> 
> For example, a user hit 'a' key with a IME turned on, then
> 1) ET_KEY_PRESSED with 'a' => IME consumes the key event => keydown with
> keycode=229
> 2) ET_KEY_RELEASED with 'a' => IME doesn't consume the key event => keyup with
> keycode=VKEY_A
> 
> Please test what key events are sent to a web page.  There are several web
pages
> for such purpose, or you can create your own test page, like nona@ and I did.

Not sure whether you've noticed that this cl is related to on-screen keyboard
scenario.
Your case was referring to physical keyboard scenario.

[James Su, 2014-05-26 15:30:30]

What shuchen doing is to simulate physical keyboard input with on-screen
keyboard. There is no physical key event at all. The input method result
(composition and final text) is directly generated by the ime extension
providing on-screen keyboard.
As there is no physical key event at all, in order to avoid breaking
existing web apps which expect key down/up events around ime events, we
need to fake key down/up event pair for each ime result generated by the
ime keyboard extension. And because these key down/up events are completely
faked, using VK_PROCESSKEY as their key code is the only feasible solution.
And note that the on-screen keyboard may not be a virtual keyboard at all,
it can be a handwriting keyboard, or even a voice ime, so asking the
extension to fake those key down/up events doea not make sense.
 2014年5月26日 下午10:14于 <yukishiino@chromium.org>写道：

> On 2014/05/26 13:34:48, Shu Chen wrote:
>
>> On 2014/05/26 12:53:42, Yuki wrote:
>> > On 2014/05/26 12:28:35, Shu Chen wrote:
>> > > On 2014/05/26 06:56:08, Yuki wrote:
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/298893003/diff/20001/ui/
> base/ime/input_method_chromeos.cc
>
>> > > > File ui/base/ime/input_method_chromeos.cc (right):
>> > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/298893003/diff/20001/ui/
> base/ime/input_method_chromeos.cc#newcode476
>
>> > > > ui/base/ime/input_method_chromeos.cc:476:
>>
> SendFakeProcessKeyEvent(false);
>
>> > > > On 2014/05/26 06:35:59, Shu Chen wrote:
>> > > > > On 2014/05/26 06:29:29, Yuki wrote:
>> > > > > > Do we really want to send a PROCESSKEY event for keyup?
>> > > > > > IIUC, DOM Level3 Events Sepc doesn't require it and no other
>>
> browsers
>
>> > send
>> > > > > such
>> > > > > > events.
>> > > > >
>> > > > > I think it would be better to keep the events sequence as the IME
>> > composing
>> > > by
>> > > > > physical keyboard.
>> > > > > Otherwise, there is potential risks to break certain web apps.
>> WDYT?
>> > > >
>> > > > http://www.w3.org/TR/DOM-Level-3-Events/
>> > > > 6.5.2 Legacy key models
>> > > > If an Input Method Editor is processing key input and the event is
>> keydown,
>> > > > return 229.
>> > > >
>> > > > Following the standard, you must not send 229 event for keyup.  The
>> spec
>> > > > explicitly says "If the event is keydown".
>> > > >
>> > > > In addition, there are many Japanese articles that says "229 event
>> is
>>
> send
>
>> > > only
>> > > > for keydown, never for keyup", and web developers expect 229
>> keydown,
>>
> and
>
>> > > never
>> > > > expect 229 keyup.
>> > > >
>> > > > I didn't find an English article, but it should be because there is
>> no
>>
> (or
>
>> > > very
>> > > > minor) IME for English.  Probably you find many Chinese articles
>> that
>>
> says
>
>> > the
>> > > > same thing.
>> > > >
>> > > > I highly recommend that you don't break the existing behavior or
>> custom.
>> > >
>> > > Can we send fake KeyA up event instead? I feel it would be weird to
>> send
>> > keydown
>> > > but not keyup.
>> > > I recall GWS used to capture keyup to fetch suggestions for text
>> changes
>>
> in
>
>> > the
>> > > search input field.
>> >
>> > What do you mean by "fake KeyA up"?
>> >
>> > Reading the paragraph starting with "The keyCode for keydown or keyup
>> events
>> is
>> > calculated as follows:" in 6.5.2, the 2nd rule applies to only keydown
>>
> events.
>
>  > It doesn't applies to keyup events, but other rules apply to keyup
>> events.
>>
> As
>
>> a
>> > result, usually keyup events hold a virtual keycode other than zero or
>> 229
>> (see
>> > the rest of rules).  Do not fire keyup events with VKEY_PROCESSKEY.
>>
>
>  I meant:
>> 1) keyup must be sent to satisfy certain web app (e.g. GWS).
>> 2) as you mentioned, VKEY_PROCESSKEY keyup must not be sent.
>> Then, we have to send a keyup event with some other key code. I propose a
>>
> fixed
>
>> keyup of VKEY_A (or any other VKEY_??).
>>
>
> No, never send such key events.  It makes people confused.
>
> IIUC, you need nothing for keyup events in addition to the existing code.
>  We're
> already sending keyup events because most (if not all) of IMEs don't
> consume
> keyup events.
>
> For example, a user hit 'a' key with a IME turned on, then
> 1) ET_KEY_PRESSED with 'a' => IME consumes the key event => keydown with
> keycode=229
> 2) ET_KEY_RELEASED with 'a' => IME doesn't consume the key event => keyup
> with
> keycode=VKEY_A
>
> Please test what key events are sent to a web page.  There are several web
> pages
> for such purpose, or you can create your own test page, like nona@ and I
> did.
>
> https://codereview.chromium.org/298893003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yuki, 2014-05-26 16:06:25]

On 2014/05/26 15:30:30, James Su wrote:
> What shuchen doing is to simulate physical keyboard input with on-screen
> keyboard. There is no physical key event at all. The input method result
> (composition and final text) is directly generated by the ime extension
> providing on-screen keyboard.
> As there is no physical key event at all, in order to avoid breaking
> existing web apps which expect key down/up events around ime events, we
> need to fake key down/up event pair for each ime result generated by the
> ime keyboard extension. And because these key down/up events are completely
> faked, using VK_PROCESSKEY as their key code is the only feasible solution.
> And note that the on-screen keyboard may not be a virtual keyboard at all,
> it can be a handwriting keyboard, or even a voice ime, so asking the
> extension to fake those key down/up events doea not make sense.

Okay, then, I think you should use VKEY_UNKNOWN for fake keyup events because we
don't know it.  Neither of VKEY_PROCESSKEY or VKEY_A is appropriate.

Also could you make sure that you fire correct key events on both of physical
and virtual keyboards?  Not firing two successive PROCESSKEY or keyup, and
compositionupdate as well in the right timing.

[James Su, 2014-05-27 03:02:08]

2014-05-27 0:06 GMT+08:00 <yukishiino@chromium.org>:

> On 2014/05/26 15:30:30, James Su wrote:
>
>> What shuchen doing is to simulate physical keyboard input with on-screen
>> keyboard. There is no physical key event at all. The input method result
>> (composition and final text) is directly generated by the ime extension
>> providing on-screen keyboard.
>> As there is no physical key event at all, in order to avoid breaking
>> existing web apps which expect key down/up events around ime events, we
>> need to fake key down/up event pair for each ime result generated by the
>> ime keyboard extension. And because these key down/up events are
>> completely
>> faked, using VK_PROCESSKEY as their key code is the only feasible
>> solution.
>> And note that the on-screen keyboard may not be a virtual keyboard at all,
>> it can be a handwriting keyboard, or even a voice ime, so asking the
>> extension to fake those key down/up events doea not make sense.
>>
>
> Okay, then, I think you should use VKEY_UNKNOWN for fake keyup events
> because we
> don't know it.  Neither of VKEY_PROCESSKEY or VKEY_A is appropriate.
>
Actually the behavior of firing VK_PROCESSKEY key down/up was a de facto
standard borrowed from Windows system, which will always change a key
event's key code to VK_PROCESSKEY if it's processed by the IME, no matter
if it's a key down or up. In another word, on Windows, if the IME does
process key up events, then chrome will receive those key up events with
VK_PROCESSKEY key code. It's an intrinsic behavior of Windows system.
Although in the real world, most IMEs do not process key up events at all,
it doesn't mean that the IME cannot process them. As far as I remember,
there are some IMEs do need to process key up events.
So firing faked key up events with VK_PROCESSKEY key code shouldn't cause
problem at all. And chrome has been doing so for quite a long time before
nona's cl https://chromiumcodereview.appspot.com/24123006/. So I think in
such case, using VK_PROCESSKEY should be safer than VK_UNKNOWN.
And the W3 spec doesn't mention anything about how to calculate the key
code of a key up event at all. IMHO, it means, the key code value of key up
event is not important here.


> Also could you make sure that you fire correct key events on both of
> physical
> and virtual keyboards?  Not firing two successive PROCESSKEY or keyup, and
> compositionupdate as well in the right timing.
>
> https://codereview.chromium.org/298893003/
>



-- 
- James Su

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yuki, 2014-05-27 03:50:51]

On 2014/05/27 03:02:08, James Su wrote:
> Actually the behavior of firing VK_PROCESSKEY key down/up was a de facto
> standard borrowed from Windows system, which will always change a key
> event's key code to VK_PROCESSKEY if it's processed by the IME, no matter
> if it's a key down or up. In another word, on Windows, if the IME does
> process key up events, then chrome will receive those key up events with
> VK_PROCESSKEY key code. It's an intrinsic behavior of Windows system.
> Although in the real world, most IMEs do not process key up events at all,
> it doesn't mean that the IME cannot process them. As far as I remember,
> there are some IMEs do need to process key up events.
> So firing faked key up events with VK_PROCESSKEY key code shouldn't cause
> problem at all. And chrome has been doing so for quite a long time before
> nona's cl https://chromiumcodereview.appspot.com/24123006/. So I think in
> such case, using VK_PROCESSKEY should be safer than VK_UNKNOWN.
> And the W3 spec doesn't mention anything about how to calculate the key
> code of a key up event at all. IMHO, it means, the key code value of key up
> event is not important here.

Talking about de-facto standard, IE sends keydown events with keycode=229 and
keyup events with original keycode, and AFAIK Chrome has been trying to respect
IE's behaviors.  And many web articles say "you receieve keydown events with
keycode=229 and keyup events with original (not 229) keycode."  Following the
principle of least astonishment to web developers, I don't think we should fire
keyup events with keycode=229.  We'd better to use another keycode.

Talking about the standard, we should respect it as much as possible.  In this
case, rule 1 to 6 don't apply.  So rule 7 or 8 applies.  If we don't get the
virtual key code from the operating system, rule 7 doesn't apply.  Then, the
final rule 8 applies.

Talking about VKEY_UNKNOWN (keycode=0), there are not few keyboards (DE, IT,
etc.) that have a key which falls to rule 8.  Firefox sends key events with
keycode=0 for such keys, and Chrome, too.  (Sorry, I didn't test this with IE or
Safari.)  Considering keycode=0 is already used, it shouldn't be such dangerous.

So I'd recommend VKEY_UNKNOWN first, otherwise another keycode (not 229).  In
this case, it seems quite possible for me to follow the standard.

[James Su, 2014-05-27 04:12:48]

2014-05-27 11:50 GMT+08:00 <yukishiino@chromium.org>:

> On 2014/05/27 03:02:08, James Su wrote:
>
>> Actually the behavior of firing VK_PROCESSKEY key down/up was a de facto
>> standard borrowed from Windows system, which will always change a key
>> event's key code to VK_PROCESSKEY if it's processed by the IME, no matter
>> if it's a key down or up. In another word, on Windows, if the IME does
>> process key up events, then chrome will receive those key up events with
>> VK_PROCESSKEY key code. It's an intrinsic behavior of Windows system.
>> Although in the real world, most IMEs do not process key up events at all,
>> it doesn't mean that the IME cannot process them. As far as I remember,
>> there are some IMEs do need to process key up events.
>> So firing faked key up events with VK_PROCESSKEY key code shouldn't cause
>> problem at all. And chrome has been doing so for quite a long time before
>> nona's cl https://chromiumcodereview.appspot.com/24123006/. So I think in
>> such case, using VK_PROCESSKEY should be safer than VK_UNKNOWN.
>> And the W3 spec doesn't mention anything about how to calculate the key
>> code of a key up event at all. IMHO, it means, the key code value of key
>> up
>> event is not important here.
>>
>
> Talking about de-facto standard, IE sends keydown events with keycode=229
> and
> keyup events with original keycode, and AFAIK Chrome has been trying to
> respect
> IE's behaviors.  And many web articles say "you receieve keydown events
> with
> keycode=229 and keyup events with original (not 229) keycode."  Following
> the
> principle of least astonishment to web developers, I don't think we should
> fire
> keyup events with keycode=229.  We'd better to use another keycode.
>

> Talking about the standard, we should respect it as much as possible.  In
> this
> case, rule 1 to 6 don't apply.  So rule 7 or 8 applies.  If we don't get
> the
> virtual key code from the operating system, rule 7 doesn't apply.  Then,
> the
> final rule 8 applies.
>
> Talking about VKEY_UNKNOWN (keycode=0), there are not few keyboards (DE,
> IT,
> etc.) that have a key which falls to rule 8.  Firefox sends key events with
> keycode=0 for such keys, and Chrome, too.  (Sorry, I didn't test this with
> IE or
> Safari.)  Considering keycode=0 is already used, it shouldn't be such
> dangerous.
>
> So I'd recommend VKEY_UNKNOWN first, otherwise another keycode (not 229).
>  In
> this case, it seems quite possible for me to follow the standard.
>
> Correction: on Windows, the key codes are not generated by browsers (IE,
Chrome, Firefox), they are generated by the operating system. I can easily
get key up events with 229 key code in both IE and Chrome with an IME that
does handle key up events. On other platforms, we generate fake key events
with key code 229 to mimic Windows behavior.
So first of all, you need to admit that key up events with 229 key code are
perfectly legal.
Second, I can't say that using VK_UNKNOWN in such case would cause problem,
because I never tested it before. But as we have been using key code 229
for key up events for such cases in Chrome for several years, I can say
that it's been proved to be safe.
But anyway, I'm ok to use VK_UNKNOWN for faked key up events, if you insist
on it.


> https://codereview.chromium.org/298893003/
>



-- 
- James Su

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yuki, 2014-05-27 04:36:43]

Shuchen, if you're going to use VKEY_UNKNOWN, you'd better know
WindowTargeter::FindTargetForKeyEvent.  You may want to set
EF_IME_FABRICATED_KEY flag if necessary.  In that case, you may want to add a
comment in ui/events/event_constants.h close to the definition of
EF_IME_FABRICATED_KEY.

[Shu Chen, 2014-05-27 05:55:04]

On 2014/05/27 04:36:43, Yuki wrote:
> Shuchen, if you're going to use VKEY_UNKNOWN, you'd better know
> WindowTargeter::FindTargetForKeyEvent.  You may want to set
> EF_IME_FABRICATED_KEY flag if necessary.  In that case, you may want to add a
> comment in ui/events/event_constants.h close to the definition of
> EF_IME_FABRICATED_KEY.

VKEY_UNKNOWN vs. VKEY_PROCESSKEY is a minor issue. I've found the duplicated
events for physical keyboard scenario due to this cl.

With further tests, I found the existing key event sequence is wrong for
physical keyboard scenario (without this cl).
I made a very simple IME extension, which listens on
chrome.input.ime.onKeyEvent() and return true/false in the even the handler.
The actual event sequence is:
compositionstart, compositionupdate, input, keydown(229), keyup.
The expected event sequence is:
keydown(229), compositionstart, compositionupdate, input, keyup.

I recall nona@ used to ask me to workaround such issue by async key event
handling: chrome.input.ime.onKeyEvent.addListener(..., ['async']).
So I call chrome.input.ime.keyEventHandled(true) before calling
chrome.input.ime.setComposition(...).
Then chrome.input.ime.keyEventHandled(true) will trigger keydown(229),
chrome.input.ime.setComposition(...) will trigger compositionstart,
compositionupdate, input.
And IME doesn't handle keyup event, the last event is keyup.
Therefore, the workaround can make the correct event sequence. However, such
workaround is too tricky and suzhe@ confirmed the IMF behaviors are wrong for
key event processing.

I'm digging into the root cause of the wrong event sequence problem.

[Shu Chen, 2014-05-27 15:39:08]

Sorry, I mis-read the dup events. Actually there wasn't dup event.
What I saw was mis-ordered events, which were caused by race condition between
DispatchKeyEventPostIME() and GetTextInputClient()->SetCompositionText(). They
both generate RPC messages to renderer but calling DispatchKeyEventPostIME()
before SetCompositionText() cannot guarantee  the keydown event occurs before
compositionupdate.
That is a separated P2 bug need to be fixed.

This cl should have no problem for physical keyboard scenario, because for
physical keyboard, pending_key_events_ should never be empty. Therefore, "if
(pending_key_events_.empty()) { ... }" check should be just for on-screen
keyboard scenario.

Go back to VKEY_UNKNOWN vs. VKEY_PROCESSKEY issue. I am ok with both.
If you insist to use VKEY_UNKNOWN, I will make changes for that.

Thanks,
Shu

[Shu Chen, 2014-05-27 15:59:17]

Yuki, please take another look at the latest patchset. Thanks

[Shu Chen, 2014-05-28 04:01:04]

+sky@.

Scott, can you please help to review this cl? thakis@ seems OOO.

Thanks,
Shu

[Yuki, 2014-05-28 06:04:18]

https://codereview.chromium.org/298893003/diff/40001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.h (right):

https://codereview.chromium.org/298893003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.h:91: // Sends a fake key event with
VKEY_PROCESSKEY key code and dispatches it to
Could you update this comment?

https://codereview.chromium.org/298893003/diff/40001/ui/wm/core/input_method_...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/40001/ui/wm/core/input_method_...
ui/wm/core/input_method_event_filter.cc:91: if (target_dispatcher_) {
Do you really need this check?  In which cases will |target_dispatcher_| be
null?

[Shu Chen, 2014-05-28 06:59:50]

https://codereview.chromium.org/298893003/diff/40001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.h (right):

https://codereview.chromium.org/298893003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.h:91: // Sends a fake key event with
VKEY_PROCESSKEY key code and dispatches it to
On 2014/05/28 06:04:19, Yuki wrote:
> Could you update this comment?

Done.

https://codereview.chromium.org/298893003/diff/40001/ui/wm/core/input_method_...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/40001/ui/wm/core/input_method_...
ui/wm/core/input_method_event_filter.cc:91: if (target_dispatcher_) {
On 2014/05/28 06:04:19, Yuki wrote:
> Do you really need this check?  In which cases will |target_dispatcher_| be
> null?

When DispatchKeyEventPostIME() method is called without any physical key events,
it will be NULL.

I've refactored this part a little bit. PTAL.

[Yuki, 2014-05-28 07:59:28]

sadrul@, I think you have better and deeper understanding about
|target_dispatcher_| in InputMethodEventFilter and what design policy we should
follow.  Could you take a look?

[sadrul, 2014-05-28 18:07:15]

A couple of comments: (1) Please briefly explain the changes made in the CL in
the CL description. It's not at all clear what the problem currently is, and how
the change fixes the problem. (2) You should add tests to make sure the stream
of events that come out of IME for a define stream of incoming events is
correct.

[Shu Chen, 2014-05-29 08:18:24]

On 2014/05/28 18:07:15, sadrul wrote:
> A couple of comments: (1) Please briefly explain the changes made in the CL in
> the CL description. It's not at all clear what the problem currently is, and
how
> the change fixes the problem. (2) You should add tests to make sure the stream
> of events that come out of IME for a define stream of incoming events is
> correct.

Thanks for your review, Sadrul.

For #1, I've updated the CL description.
For #2, do you think it's ok to add an unit test in
input_method_event_filter_unittest.cc?
We cannot add end-to-end browser tests due to crbug.com/378140.
The browser test should be added along with the fix cl for crbug.com/378140.

Thanks,
Shu

[Shu Chen, 2014-05-30 08:12:47]

I've added an unit test. Please take another look. Thanks.

[Yuki, 2014-05-30 08:33:11]

https://codereview.chromium.org/298893003/diff/100001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/100001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:33: //typedef
aura::test::AuraTestBase InputMethodEventFilterTest;
nit: You forgot to revert these lines?

https://codereview.chromium.org/298893003/diff/100001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:114:
input_method_event_filter.UpdateTargetDispatcher(
I think we want to check that things work well without setting the target
dispatcher.  Please test the case that the target dispatcher is null.

[Shu Chen, 2014-05-30 08:46:35]

https://codereview.chromium.org/298893003/diff/100001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/100001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:33: //typedef
aura::test::AuraTestBase InputMethodEventFilterTest;
On 2014/05/30 08:33:11, Yuki wrote:
> nit: You forgot to revert these lines?

Done.

https://codereview.chromium.org/298893003/diff/100001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:114:
input_method_event_filter.UpdateTargetDispatcher(
On 2014/05/30 08:33:11, Yuki wrote:
> I think we want to check that things work well without setting the target
> dispatcher.  Please test the case that the target dispatcher is null.

Done.

[Yuki, 2014-05-30 08:54:06]

lgtm.  Please get a lgtm from sadrul, too.

https://codereview.chromium.org/298893003/diff/120001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/120001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:119: // Verifies no key event
happends because InputMethodEventFilter::
typo: s/happends/happened/

[Shu Chen, 2014-05-31 16:15:20]

https://codereview.chromium.org/298893003/diff/120001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/120001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:119: // Verifies no key event
happends because InputMethodEventFilter::
On 2014/05/30 08:54:06, Yuki wrote:
> typo: s/happends/happened/

Done.

[Shu Chen, 2014-05-31 16:17:50]

sky@/sadrul@, can you please help review this cl?

sky@, your approval is required for changes under ui/wm/core/... and ash/...

Thanks,
Shu

[sadrul, 2014-05-31 18:10:15]

https://codereview.chromium.org/298893003/diff/140001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/298893003/diff/140001/ash/shell.cc#newcode1160
ash/shell.cc:1160: : NULL);
The problem with this is that all non-ash users of InputMethodEventFilters will
have to do the same thing (e.g. desktop, athena, etc).

A better solution would be for the IMEFilter to install itself as the
ActivationChangeObserver to the ActivationClient of the root-window, and update
the |target_dispatcher_| itself when appropriate.

[Shu Chen, 2014-06-01 05:33:22]

PTAL.

https://codereview.chromium.org/298893003/diff/140001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/298893003/diff/140001/ash/shell.cc#newcode1160
ash/shell.cc:1160: : NULL);
On 2014/05/31 18:10:16, sadrul wrote:
> The problem with this is that all non-ash users of InputMethodEventFilters
will
> have to do the same thing (e.g. desktop, athena, etc).
> 
> A better solution would be for the IMEFilter to install itself as the
> ActivationChangeObserver to the ActivationClient of the root-window, and
update
> the |target_dispatcher_| itself when appropriate.

Done.

[sadrul, 2014-06-02 03:49:36]

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.cc:32: }
Should you RemoveObserver from activation_client_ here?

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.h (right):

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.h:32: explicit InputMethodEventFilter(
You don't need explicit anymore.

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.h:50: aura::Window* lost_active);
virtual, OVERRIDE

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:68: scoped_ptr<aura::Window>
test_window_;
DISALLOW_COPY_AND_ASSIGN

[Shu Chen, 2014-06-02 04:08:55]

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.cc:32: }
On 2014/06/02 03:49:37, sadrul wrote:
> Should you RemoveObserver from activation_client_ here?

Done.

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.h (right):

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.h:32: explicit InputMethodEventFilter(
On 2014/06/02 03:49:37, sadrul wrote:
> You don't need explicit anymore.

Done.

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.h:50: aura::Window* lost_active);
On 2014/06/02 03:49:37, sadrul wrote:
> virtual, OVERRIDE

Done.

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/190001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:68: scoped_ptr<aura::Window>
test_window_;
On 2014/06/02 03:49:37, sadrul wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Nico, 2014-06-04 00:25:38]

this lgtm as soon as sadrul is happy with it. Please make sure that the CL
description is up-to-date (looks like it isn't atm) before you land.

[Shu Chen, 2014-06-04 00:44:56]

On 2014/06/04 00:25:38, Nico wrote:
> this lgtm as soon as sadrul is happy with it. Please make sure that the CL
> description is up-to-date (looks like it isn't atm) before you land.

Done. Just updated the cl description.

[sadrul, 2014-06-04 02:31:56]

LGTM

[Shu Chen, 2014-06-04 06:18:11]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-06-04 06:19:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/298893003/230001

[Shu Chen, 2014-06-04 07:53:28]

The CQ bit was unchecked by shuchen@chromium.org

[Shu Chen, 2014-06-04 08:00:00]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-06-04 08:00:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/298893003/240001

[Shu Chen, 2014-06-04 08:08:47]

The CQ bit was unchecked by shuchen@chromium.org

[Shu Chen, 2014-06-04 08:23:49]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-06-04 08:24:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/298893003/260001

[commit-bot: I haz the power, 2014-06-04 10:16:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-04 10:17:20]

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

[commit-bot: I haz the power, 2014-06-04 10:17:21]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Shu Chen, 2014-06-04 15:18:03]

+oshima@ for changes of ash/shell.

[Shu Chen, 2014-06-04 15:23:02]

+jamescook@, for changes in apps/shell/browser/shell_desktop_controller.cc.

[oshima, 2014-06-04 16:43:40]

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.h (right):

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.h:33: aura::client::ActivationClient*
activation_client);
How about just pass a root window and get these from it?

[James Cook, 2014-06-04 16:50:30]

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.cc:33: // Because the |activation_client_|
pointer may be destroyed already.
Are you sure the right thing is going on with memory management here?  "may be
destroyed" worries me.

1) If activation_client_ is destroyed first, then we are holding an invalid
pointer to it. At the very least I wouldn't store this in a member variable.
2) If |this| is destroyed first, then the activation client observer list has an
invalid pointer to this.  That seems bad.

sadrul, do you know if this code can rely on the activation client always being
destroyed first?

[oshima, 2014-06-04 18:13:59]

On 2014/06/04 16:50:30, James Cook wrote:
>
https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
> File ui/wm/core/input_method_event_filter.cc (right):
> 
>
https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
> ui/wm/core/input_method_event_filter.cc:33: // Because the
|activation_client_|
> pointer may be destroyed already.
> Are you sure the right thing is going on with memory management here?  "may be
> destroyed" worries me.

+1 Thank you for catching it. Please make sure the this is created/destroyed in
the proper order so
that we don't have to do this kind of tricks.
 
> 1) If activation_client_ is destroyed first, then we are holding an invalid
> pointer to it. At the very least I wouldn't store this in a member variable.
> 2) If |this| is destroyed first, then the activation client observer list has
an
> invalid pointer to this.  That seems bad.
> 
> sadrul, do you know if this code can rely on the activation client always
being
> destroyed first?

[sadrul, 2014-06-04 18:21:06]

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.cc:33: // Because the |activation_client_|
pointer may be destroyed already.
On 2014/06/04 16:50:31, James Cook wrote:
> Are you sure the right thing is going on with memory management here?  "may be
> destroyed" worries me.
> 
> 1) If activation_client_ is destroyed first, then we are holding an invalid
> pointer to it. At the very least I wouldn't store this in a member variable.
> 2) If |this| is destroyed first, then the activation client observer list has
an
> invalid pointer to this.  That seems bad.
> 
> sadrul, do you know if this code can rely on the activation client always
being
> destroyed first?

I don't think so, no. Earlier patchset-12 I reviewed had RemoveObserver() here,
and I assume there were test failures that caused this change. But the proper
fix for that is to do RemoveObserver() here, and make sure that the
activation-client outlives the IME-filter.

[Shu Chen, 2014-06-05 07:00:18]

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.cc (right):

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.cc:33: // Because the |activation_client_|
pointer may be destroyed already.
I've let InputMethodEventFilter listens on OnHostCloseRequest() event and do
RemoveObserver() there.

On 2014/06/04 16:50:31, James Cook wrote:
> Are you sure the right thing is going on with memory management here?  "may be
> destroyed" worries me.
> 
> 1) If activation_client_ is destroyed first, then we are holding an invalid
> pointer to it. At the very least I wouldn't store this in a member variable.
> 2) If |this| is destroyed first, then the activation client observer list has
an
> invalid pointer to this.  That seems bad.
> 
> sadrul, do you know if this code can rely on the activation client always
being
> destroyed first?

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.cc:33: // Because the |activation_client_|
pointer may be destroyed already.
I've let InputMethodEventFilter listens on OnHostCloseRequest() event and do
RemoveObserver() there.

On 2014/06/04 18:21:07, sadrul wrote:
> On 2014/06/04 16:50:31, James Cook wrote:
> > Are you sure the right thing is going on with memory management here?  "may
be
> > destroyed" worries me.
> > 
> > 1) If activation_client_ is destroyed first, then we are holding an invalid
> > pointer to it. At the very least I wouldn't store this in a member variable.
> > 2) If |this| is destroyed first, then the activation client observer list
has
> an
> > invalid pointer to this.  That seems bad.
> > 
> > sadrul, do you know if this code can rely on the activation client always
> being
> > destroyed first?
> 
> I don't think so, no. Earlier patchset-12 I reviewed had RemoveObserver()
here,
> and I assume there were test failures that caused this change. But the proper
> fix for that is to do RemoveObserver() here, and make sure that the
> activation-client outlives the IME-filter.

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter.h (right):

https://codereview.chromium.org/298893003/diff/280001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter.h:33: aura::client::ActivationClient*
activation_client);
On 2014/06/04 16:43:40, oshima wrote:
> How about just pass a root window and get these from it?

Done.

[Shu Chen, 2014-06-05 14:07:25]

Offline IM'ed with oshima@. Thanks for his idea, I've changed the solution of
fixing the fake event issue: make sure the fake event carries correct
EventTarget.

Now the changes are minimized. Please take another look.

Thanks,
Shu

[oshima, 2014-06-05 15:03:13]

https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:450:
GetTextInputClient()->GetAttachedWindow()));
As I said, it is my understanding that DispatherApi is meant to be used by a
dispatcher. You should check with sadrul. Note that this reinterpret cast isn't
safe either.

My recommendations are one of:

a) pass TextInputClient to delegate's DispatchKeyEventPostIME, and
 then
 
TextInputClient::GetAttachedWindow()->GetHost()->dispatcher()->OnEventFromSource(event);

b) in InputMethodEventFilter, just use ActivationClient->GetActiveWindow()
  You can get it once from root window in SetInputMethodPropertyInRootWindow. 
No need to listen to activation change event.


I still recommend a), but b) should work too. [ b) requires assumption that the
dispatcher client for the input method event filter will never change over time,
but this is true]

[Shu Chen, 2014-06-05 15:27:02]

On 2014/06/05 15:03:13, oshima wrote:
>
https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
> File ui/base/ime/input_method_chromeos.cc (right):
> 
>
https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
> ui/base/ime/input_method_chromeos.cc:450:
> GetTextInputClient()->GetAttachedWindow()));
> As I said, it is my understanding that DispatherApi is meant to be used by a
> dispatcher. You should check with sadrul. Note that this reinterpret cast
isn't
> safe either.
I think the cast is safe because it's in chromeos specific code and
GetAttachedWindow() supposes to return aura::Window* if def USE_AURA.
> 
> My recommendations are one of:
> 
> a) pass TextInputClient to delegate's DispatchKeyEventPostIME, and
>  then
I think this would make things unclear, because DispatchKeyEventPostIME() method
will be called when both physical key event and soft key event occur.
For physical key event, ui::KeyEvent carries the target, so it's unnecessary to
pass in a TextInputClient instance.
And it would be ugly to create a method like DispatchFakeKeyEventPostIME(const
ui::KeyEvent&, TextInputClient*).
>  
>
TextInputClient::GetAttachedWindow()->GetHost()->dispatcher()->OnEventFromSource(event);
> 
> b) in InputMethodEventFilter, just use ActivationClient->GetActiveWindow()
>   You can get it once from root window in SetInputMethodPropertyInRootWindow. 
> No need to listen to activation change event.
> 
> 
> I still recommend a), but b) should work too. [ b) requires assumption that
the
> dispatcher client for the input method event filter will never change over
time,
> but this is true]
Sadrul, do you think I can use DispatherApi in such case?

Thanks,
Shu

[sadrul, 2014-06-05 15:46:27]

https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:450:
GetTextInputClient()->GetAttachedWindow()));
On 2014/06/05 15:03:13, oshima wrote:
> As I said, it is my understanding that DispatherApi is meant to be used by a
> dispatcher.

Indeed. You should not use this here.

> You should check with sadrul. Note that this reinterpret cast isn't
> safe either.
> 
> My recommendations are one of:
> 
> a) pass TextInputClient to delegate's DispatchKeyEventPostIME, and
>  then
>  
>
TextInputClient::GetAttachedWindow()->GetHost()->dispatcher()->OnEventFromSource(event);
> 
> b) in InputMethodEventFilter, just use ActivationClient->GetActiveWindow()
>   You can get it once from root window in SetInputMethodPropertyInRootWindow. 
> No need to listen to activation change event.
> 
> 
> I still recommend a), but b) should work too. [ b) requires assumption that
the
> dispatcher client for the input method event filter will never change over
time,
> but this is true]

Good suggestions. I too prefer (a) over (b).

[Shu Chen, 2014-06-05 16:13:17]

There is no need to pass in a TextInputClient instance to
InputMethodEventFilter.
InputMethodEventFilter owns the InputMethod instance which has
GetTextInputClient() method.

[oshima, 2014-06-05 16:16:58]

On 2014/06/05 16:13:17, Shu Chen wrote:
> There is no need to pass in a TextInputClient instance to
> InputMethodEventFilter.
> InputMethodEventFilter owns the InputMethod instance which has
> GetTextInputClient() method.

That's even better :) Note that you probably should always use the attached
window to dispatch because the target window on key event may be on the
different root window than one for TextInputClient.

[Shu Chen, 2014-06-05 16:26:53]

I've updated per your proposed solution. PTAL?

https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
File ui/base/ime/input_method_chromeos.cc (right):

https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:450:
GetTextInputClient()->GetAttachedWindow()));
On 2014/06/05 15:03:13, oshima wrote:
> As I said, it is my understanding that DispatherApi is meant to be used by a
> dispatcher. You should check with sadrul. Note that this reinterpret cast
isn't
> safe either.
> 
> My recommendations are one of:
> 
> a) pass TextInputClient to delegate's DispatchKeyEventPostIME, and
>  then
>  
>
TextInputClient::GetAttachedWindow()->GetHost()->dispatcher()->OnEventFromSource(event);
> 
> b) in InputMethodEventFilter, just use ActivationClient->GetActiveWindow()
>   You can get it once from root window in SetInputMethodPropertyInRootWindow. 
> No need to listen to activation change event.
> 
> 
> I still recommend a), but b) should work too. [ b) requires assumption that
the
> dispatcher client for the input method event filter will never change over
time,
> but this is true]

Done.

https://codereview.chromium.org/298893003/diff/370001/ui/base/ime/input_metho...
ui/base/ime/input_method_chromeos.cc:450:
GetTextInputClient()->GetAttachedWindow()));
On 2014/06/05 15:46:28, sadrul wrote:
> On 2014/06/05 15:03:13, oshima wrote:
> > As I said, it is my understanding that DispatherApi is meant to be used by a
> > dispatcher.
> 
> Indeed. You should not use this here.
> 
> > You should check with sadrul. Note that this reinterpret cast isn't
> > safe either.
> > 
> > My recommendations are one of:
> > 
> > a) pass TextInputClient to delegate's DispatchKeyEventPostIME, and
> >  then
> >  
> >
>
TextInputClient::GetAttachedWindow()->GetHost()->dispatcher()->OnEventFromSource(event);
> > 
> > b) in InputMethodEventFilter, just use ActivationClient->GetActiveWindow()
> >   You can get it once from root window in
SetInputMethodPropertyInRootWindow. 
> > No need to listen to activation change event.
> > 
> > 
> > I still recommend a), but b) should work too. [ b) requires assumption that
> the
> > dispatcher client for the input method event filter will never change over
> time,
> > but this is true]
> 
> Good suggestions. I too prefer (a) over (b).

Done.

[Shu Chen, 2014-06-05 16:30:12]

On 2014/06/05 16:16:58, oshima wrote:
> On 2014/06/05 16:13:17, Shu Chen wrote:
> > There is no need to pass in a TextInputClient instance to
> > InputMethodEventFilter.
> > InputMethodEventFilter owns the InputMethod instance which has
> > GetTextInputClient() method.
> 
> That's even better :) Note that you probably should always use the attached
> window to dispatch because the target window on key event may be on the
> different root window than one for TextInputClient.

I think we should respect the target window on key event, right?
If somehow a key event was dispatched from a unfocused window, we need to
dispatch to that unfocused window instead of the focused window, right?

[Shu Chen, 2014-06-05 16:41:38]

On 2014/06/05 16:30:12, Shu Chen wrote:
> On 2014/06/05 16:16:58, oshima wrote:
> > On 2014/06/05 16:13:17, Shu Chen wrote:
> > > There is no need to pass in a TextInputClient instance to
> > > InputMethodEventFilter.
> > > InputMethodEventFilter owns the InputMethod instance which has
> > > GetTextInputClient() method.
> > 
> > That's even better :) Note that you probably should always use the attached
> > window to dispatch because the target window on key event may be on the
> > different root window than one for TextInputClient.
> 
> I think we should respect the target window on key event, right?
> If somehow a key event was dispatched from a unfocused window, we need to
> dispatch to that unfocused window instead of the focused window, right?

With a second thought, I think you're right.
The key event can happen on window X, but due to asynchronous, window Y gets
focus and then DispatchKeyEventPostIME should happen on window Y, because other
operations on TextInputClient (such as SetCompositionText) were performed on
window Y.

[oshima, 2014-06-05 16:59:54]

On 2014/06/05 16:41:38, Shu Chen wrote:
> On 2014/06/05 16:30:12, Shu Chen wrote:
> > On 2014/06/05 16:16:58, oshima wrote:
> > > On 2014/06/05 16:13:17, Shu Chen wrote:
> > > > There is no need to pass in a TextInputClient instance to
> > > > InputMethodEventFilter.
> > > > InputMethodEventFilter owns the InputMethod instance which has
> > > > GetTextInputClient() method.
> > > 
> > > That's even better :) Note that you probably should always use the
attached
> > > window to dispatch because the target window on key event may be on the
> > > different root window than one for TextInputClient.
> > 
> > I think we should respect the target window on key event, right?
> > If somehow a key event was dispatched from a unfocused window, we need to
> > dispatch to that unfocused window instead of the focused window, right?
> 
> With a second thought, I think you're right.
> The key event can happen on window X, but due to asynchronous, window Y gets
> focus and then DispatchKeyEventPostIME should happen on window Y, because
other
> operations on TextInputClient (such as SetCompositionText) were performed on
> window Y.

Native (X) focus and aura/ash focus are completely independent. That is, X may
send key event to a root window A,
while the aura/ash's focused window may be on the root window B. Similar
situation can happen to mouse events,
and aura/ash need to be able to handle such cases. I'm not familiar with key
event handling path enough to tell
if this has been already been taken care of by the time InputMethodEventFilter
receives a key event, but i think
if you want to send a key event to the same root window as the text input
client, i think it's better to use the window
associated with the text input client.

[Shu Chen, 2014-06-05 17:04:01]

Done. Hopefully patchset #23 is the final patchset. :)

PTAL.

[oshima, 2014-06-05 17:14:49]

very nice :)

lgtm with nits.

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:37: aura::Window* window_;
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:49: new
wm::DefaultActivationClient(root_window());
I believe you don't need this

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:66: virtual void TearDown()
OVERRIDE {
new line between methods.

[sadrul, 2014-06-05 17:16:38]

LGTM

I am worried that we don't have sufficient test coverage for the cases you have
discussed in comments #66 #67. Can you please look into adding some tests for
those cases.

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:32:
TestTextInputClient(aura::Window* window) : window_(window) {}
explicit

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:37: aura::Window* window_;
DISALLOW_COPY_AND_ASSIGN

[Shu Chen, 2014-06-05 17:18:01]

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:37: aura::Window* window_;
On 2014/06/05 17:14:48, oshima wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:49: new
wm::DefaultActivationClient(root_window());
On 2014/06/05 17:14:48, oshima wrote:
> I believe you don't need this

Done.

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:66: virtual void TearDown()
OVERRIDE {
On 2014/06/05 17:14:48, oshima wrote:
> new line between methods.

Done.

[Shu Chen, 2014-06-05 17:27:49]

For tests of #66 #67, I will add a new unit test in InputMethodEventFilterTest.

Thanks,
Shu

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
File ui/wm/core/input_method_event_filter_unittest.cc (right):

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:32:
TestTextInputClient(aura::Window* window) : window_(window) {}
On 2014/06/05 17:16:37, sadrul wrote:
> explicit

Done.

https://codereview.chromium.org/298893003/diff/410001/ui/wm/core/input_method...
ui/wm/core/input_method_event_filter_unittest.cc:37: aura::Window* window_;
On 2014/06/05 17:16:37, sadrul wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Shu Chen, 2014-06-05 17:29:09]

Hi jamescook@, do you have more comments?

Thanks,
Shu

[James Cook, 2014-06-05 19:36:36]

Ah, I didn't think you wanted me since you aren't touching apps/shell/ anymore.
:-)

LGTM

[Shu Chen, 2014-06-06 07:32:43]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-06-06 07:34:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/298893003/480001

[Shu Chen, 2014-06-06 08:04:13]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-06-06 08:05:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/298893003/500001

[commit-bot: I haz the power, 2014-06-06 11:07:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-06 12:07:21]

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

[commit-bot: I haz the power, 2014-06-06 12:07:23]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Shu Chen, 2014-06-06 12:08:47]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-06-06 12:09:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/298893003/500001

[commit-bot: I haz the power, 2014-06-06 13:22:06]

Change committed as 275422