Move KeyIdentifier->KeyEvent conversion to src/ui/.

Move KeyIdentifier->KeyEvent conversion to src/ui/.

This shouldn't depend on anything in src/chrome and will be useful to
use from src/ui/keyboard.

BUG=none

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

[bryeung, 2013-04-14 15:40:16]

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
File ui/base/events/key_identifier_conversion.cc (right):

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.cc:328: // FIXME: what about races?
Sadrul: do you know what the Right Way (TM) is to do this initialization?

[sadrul, 2013-04-15 02:05:12]

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
File ui/base/events/key_identifier_conversion.cc (right):

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.cc:17: static const int
kNumIdentifierTypes = 3;
We typically don't mark things 'static' in anonymous namespace.

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.cc:328: // FIXME: what about races?
On 2013/04/14 15:40:16, bryeung wrote:
> Sadrul: do you know what the Right Way (TM) is to do this initialization?

You can keep a static base::PlatformThreadId in InitializeMaps() and initialize
it properly in the first call, and then check for each subsequent call to it.
But is that necessary? Most of the code in ui/ is meant to be called from the UI
thread anyway, so perhaps the caller needs to verify that it is calling this
from the right thread.

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
File ui/base/events/key_identifier_conversion.h (right):

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.h:10: #include
"ui/base/keycodes/keyboard_codes.h"
You probably don't need this include here?

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.h:20: UI_EXPORT const ui::KeyEvent&
KeyEventFromKeyIdentifier(
I wonder if there could be a KeyEvent constructor instead that takes in the
string identifier.

[bryeung, 2013-04-15 15:34:13]

Thanks Sadrul: ptal.

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
File ui/base/events/key_identifier_conversion.cc (right):

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.cc:17: static const int
kNumIdentifierTypes = 3;
On 2013/04/15 02:05:12, sadrul wrote:
> We typically don't mark things 'static' in anonymous namespace.

Fixed (throughout).

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.cc:328: // FIXME: what about races?
On 2013/04/15 02:05:12, sadrul wrote:
> On 2013/04/14 15:40:16, bryeung wrote:
> > Sadrul: do you know what the Right Way (TM) is to do this initialization?
> 
> You can keep a static base::PlatformThreadId in InitializeMaps() and
initialize
> it properly in the first call, and then check for each subsequent call to it.
> But is that necessary? Most of the code in ui/ is meant to be called from the
UI
> thread anyway, so perhaps the caller needs to verify that it is calling this
> from the right thread.

Works for me: I'll add a comment about that to the header.

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
File ui/base/events/key_identifier_conversion.h (right):

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.h:10: #include
"ui/base/keycodes/keyboard_codes.h"
On 2013/04/15 02:05:12, sadrul wrote:
> You probably don't need this include here?

Good catch.  Done.

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.h:20: UI_EXPORT const ui::KeyEvent&
KeyEventFromKeyIdentifier(
On 2013/04/15 02:05:12, sadrul wrote:
> I wonder if there could be a KeyEvent constructor instead that takes in the
> string identifier.

It could, but I didn't for two reasons:
- this is a pretty big, hairy chunk of code that I'd prefer to keep separate
from the core events.* files.
- it doesn't create a "real" KeyEvent, just a "prototype" key event that then
requires the other setup.

That said, if we think this belongs in the core KeyEvent API (via the
constructor), I could easily work around both of those issues.  Would you prefer
I moved it there?

[sadrul, 2013-04-15 16:53:28]

ui/base/events/ LGTM

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
File ui/base/events/key_identifier_conversion.h (right):

https://codereview.chromium.org/13957005/diff/1/ui/base/events/key_identifier...
ui/base/events/key_identifier_conversion.h:20: UI_EXPORT const ui::KeyEvent&
KeyEventFromKeyIdentifier(
On 2013/04/15 15:34:13, bryeung wrote:
> On 2013/04/15 02:05:12, sadrul wrote:
> > I wonder if there could be a KeyEvent constructor instead that takes in the
> > string identifier.
> 
> It could, but I didn't for two reasons:
> - this is a pretty big, hairy chunk of code that I'd prefer to keep separate
> from the core events.* files.
> - it doesn't create a "real" KeyEvent, just a "prototype" key event that then
> requires the other setup.

Thanks for explaining. I now agree with you that having this separate sounds
better.

[bryeung, 2013-04-15 17:43:40]

Thanks Sadrul!

+ben: can you look for OWNERS please?

[Ben Goodger (Google), 2013-04-16 16:32:35]

lgtm

[bryeung, 2013-04-16 19:34:35]

Committed patchset #3 manually as r194416 (presubmit successful).