Retires ui::TranslatedKeyEvent and replaces it with KeyEvent.

Retires ui::TranslatedKeyEvent and replaces it with KeyEvent.

BUG=339355
TEST=Run unittests.
TBR=jamesr@chromium.org, pfeldman@chromium.org

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

[Yuki, 2014-05-02 08:04:55]

Could you review this CL?

[sadrul, 2014-05-05 06:43:46]

https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc#newco...
ui/events/event.cc:600: case ET_TRANSLATED_KEY_RELEASE:
Can we remove the _TRANSLATED_ types, and use a KeyEventFlags instead?

[Yuki, 2014-05-08 05:28:26]

https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc#newco...
ui/events/event.cc:600: case ET_TRANSLATED_KEY_RELEASE:
On 2014/05/05 06:43:47, sadrul wrote:
> Can we remove the _TRANSLATED_ types, and use a KeyEventFlags instead?

ui::Accelerator seems distinguishing a translated key event from a regular key
event.  I don't yet fully understand the accelerator system, but we cannot
simply remove _TRANSLATED_ types.  I think it's technically possible to remove
_TRANSLATED_ types, but some changes may be needed in accelerator handling.

I think this is one of the reasons why we needed _TRANSLATED_ types.

It may be a big change, and I don't think we need such a big change now.

[sadrul, 2014-05-08 14:19:03]

On 2014/05/08 05:28:26, Yuki wrote:
> https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc
> File ui/events/event.cc (right):
> 
>
https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc#newco...
> ui/events/event.cc:600: case ET_TRANSLATED_KEY_RELEASE:
> On 2014/05/05 06:43:47, sadrul wrote:
> > Can we remove the _TRANSLATED_ types, and use a KeyEventFlags instead?
> 
> ui::Accelerator seems distinguishing a translated key event from a regular key
> event.  I don't yet fully understand the accelerator system, but we cannot
> simply remove _TRANSLATED_ types.  I think it's technically possible to remove
> _TRANSLATED_ types, but some changes may be needed in accelerator handling.

ui::Accelerator also has modifiers, and since we will add a KeyEventFlags for
the translated keys, there should still be the necessary information available
to it? What difficulties did you run into?

> 
> I think this is one of the reasons why we needed _TRANSLATED_ types.
> 
> It may be a big change, and I don't think we need such a big change now.

[Yuki, 2014-05-08 16:01:44]

On 2014/05/08 14:19:03, sadrul wrote:
> On 2014/05/08 05:28:26, Yuki wrote:
> > https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc
> > File ui/events/event.cc (right):
> > 
> >
>
https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc#newco...
> > ui/events/event.cc:600: case ET_TRANSLATED_KEY_RELEASE:
> > On 2014/05/05 06:43:47, sadrul wrote:
> > > Can we remove the _TRANSLATED_ types, and use a KeyEventFlags instead?
> > 
> > ui::Accelerator seems distinguishing a translated key event from a regular
key
> > event.  I don't yet fully understand the accelerator system, but we cannot
> > simply remove _TRANSLATED_ types.  I think it's technically possible to
remove
> > _TRANSLATED_ types, but some changes may be needed in accelerator handling.
> 
> ui::Accelerator also has modifiers, and since we will add a KeyEventFlags for
> the translated keys, there should still be the necessary information available
> to it? What difficulties did you run into?

Hmm, modifiers are meant for Shift, Ctrl, Alt, etc. and we're passing flags with
a mask as follows.
  mask = EF_SHIFT_DOWN | EF_CONTROL_DOWN | ...
  Accelerator a(keycode, flags & mask);
I feel uneasy to add EF_TRANSLATED_KEY to |mask|, it looks a little weird to me.

I'm now thinking about the difference between translated keys and original keys.
 If the difference is fundamental, it's natural to have ET_TRANSLATED_KEY_*
separately.  If not, it's natural to have only ET_KEY_*.  I'm on the fence and
not convinced.  Probably you're right, but not sure yet.  As a thought
experiment, what about having only one event type ET_KEY and a flag for
press/release like EF_KEY_PRESSED?

And I'm just afraid that I would break something.  I'd rather prefer just
removing TranslatedKeyEvent class until I get better understanding.
If we had the following code snippet somewhere, this code will no longer work as
expected.
  if (event.type() == ET_KEY_PRESSED)
    // this is not a translated key event.
We need to rewrite it as follows.
  if (event.type() == ET_KEY_PRESSED && event.flags & EF_TRANSLATED_KEY == 0)
    // this is not a translated key event.

It seems a very tough job and it should be hard to prove if we rewrote all
related code or not.

[sadrul, 2014-05-14 01:20:15]

On 2014/05/08 16:01:44, Yuki wrote:
> On 2014/05/08 14:19:03, sadrul wrote:
> > On 2014/05/08 05:28:26, Yuki wrote:
> > > https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc
> > > File ui/events/event.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/267723008/diff/40001/ui/events/event.cc#newco...
> > > ui/events/event.cc:600: case ET_TRANSLATED_KEY_RELEASE:
> > > On 2014/05/05 06:43:47, sadrul wrote:
> > > > Can we remove the _TRANSLATED_ types, and use a KeyEventFlags instead?
> > > 
> > > ui::Accelerator seems distinguishing a translated key event from a regular
> key
> > > event.  I don't yet fully understand the accelerator system, but we cannot
> > > simply remove _TRANSLATED_ types.  I think it's technically possible to
> remove
> > > _TRANSLATED_ types, but some changes may be needed in accelerator
handling.
> > 
> > ui::Accelerator also has modifiers, and since we will add a KeyEventFlags
for
> > the translated keys, there should still be the necessary information
available
> > to it? What difficulties did you run into?
> 
> Hmm, modifiers are meant for Shift, Ctrl, Alt, etc. and we're passing flags
with
> a mask as follows.
>   mask = EF_SHIFT_DOWN | EF_CONTROL_DOWN | ...
>   Accelerator a(keycode, flags & mask);
> I feel uneasy to add EF_TRANSLATED_KEY to |mask|, it looks a little weird to
me.
> 
> I'm now thinking about the difference between translated keys and original
keys.
>  If the difference is fundamental, it's natural to have ET_TRANSLATED_KEY_*
> separately.  If not, it's natural to have only ET_KEY_*.  I'm on the fence and
> not convinced.  Probably you're right, but not sure yet.  As a thought
> experiment, what about having only one event type ET_KEY and a flag for
> press/release like EF_KEY_PRESSED?
> 
> And I'm just afraid that I would break something.  I'd rather prefer just
> removing TranslatedKeyEvent class until I get better understanding.

Yes. I don't think anyone is arguing to have just one event-type and using flags
to represent the different types.

> If we had the following code snippet somewhere, this code will no longer work
as
> expected.
>   if (event.type() == ET_KEY_PRESSED)
>     // this is not a translated key event.
> We need to rewrite it as follows.
>   if (event.type() == ET_KEY_PRESSED && event.flags & EF_TRANSLATED_KEY == 0)
>     // this is not a translated key event.

Hm. This is a good point. Do you know if we do this in a lot of places?

> 
> It seems a very tough job and it should be hard to prove if we rewrote all
> related code or not.

[Yuki, 2014-05-14 08:01:22]

On 2014/05/14 01:20:15, sadrul wrote:
> On 2014/05/08 16:01:44, Yuki wrote:
> > If we had the following code snippet somewhere, this code will no longer
work
> as
> > expected.
> >   if (event.type() == ET_KEY_PRESSED)
> >     // this is not a translated key event.
> > We need to rewrite it as follows.
> >   if (event.type() == ET_KEY_PRESSED && event.flags & EF_TRANSLATED_KEY ==
0)
> >     // this is not a translated key event.
> 
> Hm. This is a good point. Do you know if we do this in a lot of places?

https://code.google.com/p/chromium/codesearch#search/&q=ET_KEY_PRESSED%20(%5C...

Code search shows 76 hits (96 hits just for ET_KEY_PRESSED).
We need to check it one by one.

[Yuki, 2014-05-16 05:54:44]

On 2014/05/14 08:01:22, Yuki wrote:
> On 2014/05/14 01:20:15, sadrul wrote:
> > On 2014/05/08 16:01:44, Yuki wrote:
> > > If we had the following code snippet somewhere, this code will no longer
> work
> > as
> > > expected.
> > >   if (event.type() == ET_KEY_PRESSED)
> > >     // this is not a translated key event.
> > > We need to rewrite it as follows.
> > >   if (event.type() == ET_KEY_PRESSED && event.flags & EF_TRANSLATED_KEY ==
> 0)
> > >     // this is not a translated key event.
> > 
> > Hm. This is a good point. Do you know if we do this in a lot of places?
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=ET_KEY_PRESSED%20(%5C...
> 
> Code search shows 76 hits (96 hits just for ET_KEY_PRESSED).
> We need to check it one by one.

Ping?

I'd like to go with this CL without unifying ET_TRANSLATED_KEY_* into ET_KEY_*.
If we really want to unify them, we'd better do it in a separate CL, I think.

[sadrul, 2014-05-16 05:59:11]

On 2014/05/16 05:54:44, Yuki wrote:
> On 2014/05/14 08:01:22, Yuki wrote:
> > On 2014/05/14 01:20:15, sadrul wrote:
> > > On 2014/05/08 16:01:44, Yuki wrote:
> > > > If we had the following code snippet somewhere, this code will no longer
> > work
> > > as
> > > > expected.
> > > >   if (event.type() == ET_KEY_PRESSED)
> > > >     // this is not a translated key event.
> > > > We need to rewrite it as follows.
> > > >   if (event.type() == ET_KEY_PRESSED && event.flags & EF_TRANSLATED_KEY
==
> > 0)
> > > >     // this is not a translated key event.
> > > 
> > > Hm. This is a good point. Do you know if we do this in a lot of places?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=ET_KEY_PRESSED%20(%5C...
> > 
> > Code search shows 76 hits (96 hits just for ET_KEY_PRESSED).
> > We need to check it one by one.
> 
> Ping?
> 
> I'd like to go with this CL without unifying ET_TRANSLATED_KEY_* into
ET_KEY_*.
> If we really want to unify them, we'd better do it in a separate CL, I think.

OH, sorry. I misunderstood; I thought we were still undecided how to proceed. I
now remember that your proposal is to remove the TranslatedKeyEvent type, and
replace that with KeyEvent, but keep the ET_TRANSLATED_KEY* event types. This
sounds reasonable to me. I will do a proper review tomorrow morning. Sorry about
the delay.

[sadrul, 2014-05-16 14:09:47]

LGTM

[Yuki, 2014-05-16 14:43:06]

Could you guys review this CL as follows?
sadrul@ already reviewed this CL overall.

tengs@
  ash/sticky_keys/sticky_keys_controller.cc

pkotwicz@
  ash/wm/overlay_event_filter.cc

pfeldman@
  content/shell/browser/shell_platform_data_aura.cc

jamesr@
  mojo/examples/launcher/launcher.cc

sky@
  ui/wm/core/input_method_event_filter.cc

[pkotwicz, 2014-05-16 14:58:39]

ash/wm LGTM

[Tim Song, 2014-05-16 16:52:39]

lgtm

[sky, 2014-05-16 17:32:47]

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

https://codereview.chromium.org/267723008/diff/60001/ui/wm/core/input_method_...
ui/wm/core/input_method_event_filter.cc:41: // The |event| is already handled by
this object, change the type of the
This comment doesn't make sense anymore, nor do I get what this does. Could you
better describe (in the comment) why we're doing this.

[Yuki, 2014-05-19 10:04:30]

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

https://codereview.chromium.org/267723008/diff/60001/ui/wm/core/input_method_...
ui/wm/core/input_method_event_filter.cc:41: // The |event| is already handled by
this object, change the type of the
On 2014/05/16 17:32:47, sky wrote:
> This comment doesn't make sense anymore, nor do I get what this does. Could
you
> better describe (in the comment) why we're doing this.

Done.

[sky, 2014-05-19 16:08:01]

Thanks! LGTM

[Yuki, 2014-05-20 09:49:07]

Let me TBR for content/shell/browser/shell_platform_data_aura.cc and
mojo/examples/launcher/launcher.cc because the change in these files are quite
same as ui/wm/core/input_method_event_filter.cc .

[Yuki, 2014-05-20 09:49:19]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-05-20 09:49:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/267723008/100001

[commit-bot: I haz the power, 2014-05-20 13:09:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-20 14:01:33]

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

[commit-bot: I haz the power, 2014-05-20 14:01:33]

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

[Yuki, 2014-05-20 14:08:51]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-05-20 14:09:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/267723008/100001

[commit-bot: I haz the power, 2014-05-20 19:23:02]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-20 20:16:15]

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

[commit-bot: I haz the power, 2014-05-20 20:16:15]

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

[Yuki, 2014-05-21 02:58:09]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-05-21 03:00:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/267723008/100001

[commit-bot: I haz the power, 2014-05-21 06:16:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-21 07:14:37]

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

[commit-bot: I haz the power, 2014-05-21 07:14:38]

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

[Yuki, 2014-05-21 08:10:16]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-05-21 08:12:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/267723008/150009

[commit-bot: I haz the power, 2014-05-21 20:02:44]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-22 00:29:22]

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

[commit-bot: I haz the power, 2014-05-22 00:29:23]

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

[Yuki, 2014-05-22 09:22:30]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-05-22 09:24:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/267723008/170001

[commit-bot: I haz the power, 2014-05-22 17:52:48]

Change committed as 272228