Fix key identifier conversion.

Fix key identifier conversion.

Key identifiers are more expressive than simple key codes.  This patch
expands the target of the mapping to include modifier keys.

This also moves the key identifier conversion into the only directory from
which it is used.

BUG=none
TEST=unit test updated

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74227

[sky, 2011-01-27 23:19:23]

Don't you need to update chrome gyp files now for the new file?

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
File chrome/browser/extensions/key_identifier_conversion_views.cc (right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:287: static const
views::KeyEvent kUnknownKeyEvent(
statics can only be plain old data.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:301:
views::KeyEvent *event = new views::KeyEvent(
"KeyEvent *" -> "KeyEvent* "

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:306:
identifierMaps[j]->insert(IdentifierPair(key.identifiers[j], event));
Add a DCHECK to make sure someone didn't add the value twice.

[Erik does not do reviews, 2011-01-31 06:28:34]

(note: I'm out this week, so I'll be slow to respond)

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_input_api.cc (right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_input_api.cc:106: // Views does not have a
Meta event flag, so ignore that argument
nit: space above comment

Should this return an error if meta is present then?  Or are you going to remove
it from the API?

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
File chrome/browser/extensions/key_identifier_conversion_views.cc (right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:20: // In order:
key identifier, character and unicode codepoint.
It seems odd to stuff these different things into a single array (and without
constants to identify the different indices).  Add a comment explaining why you
did it this way (below it looks like they're always treated uniformly and I'm
assuming they're never explicitly referenced by their types).

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:294: for (int i =
0; i < kNumIdentifierTypes; ++i) {
nit: remove {}

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:323: if (iter !=
map.end()) {
nit: remove {}

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
File chrome/browser/extensions/key_identifier_conversion_views_unittest.cc
(right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views_unittest.cc:60:
EXPECT_EQ(views::Event::EF_SHIFT_DOWN,
isn't this exact case also tested in the next test?

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views_unittest.cc:64:
TEST_F(KeyEventFromKeyIdentifierTest, ShiftModifiersAreSet) {
should there be a similar control modifier test?

[bryeung, 2011-02-02 20:56:43]

RE: chrome_browser.gypi, I'm not sure why that change wasn't picked up by my
upload.py invocation.  (I'm using upload.py to ensure that the renames are
handled correctly.)

Looks like everything is there now.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_input_api.cc (right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_input_api.cc:106: // Views does not have a
Meta event flag, so ignore that argument
On 2011/01/31 06:28:35, Erik Kay wrote:
> nit: space above comment
> 
> Should this return an error if meta is present then?  Or are you going to
remove
> it from the API?

I've changed this to return an error.

My original intention was to ignore the meta argument until we decide we need
it, but I agree that returning an error will be more developer-friendly in the
interim.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
File chrome/browser/extensions/key_identifier_conversion_views.cc (right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:20: // In order:
key identifier, character and unicode codepoint.
On 2011/01/31 06:28:35, Erik Kay wrote:
> It seems odd to stuff these different things into a single array (and without
> constants to identify the different indices).  Add a comment explaining why
you
> did it this way (below it looks like they're always treated uniformly and I'm
> assuming they're never explicitly referenced by their types).

Done.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:287: static const
views::KeyEvent kUnknownKeyEvent(
On 2011/01/27 23:19:23, sky wrote:
> statics can only be plain old data.

Done, though I just made it a lazily initialized pointer.  Is there a better
way?

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:294: for (int i =
0; i < kNumIdentifierTypes; ++i) {
On 2011/01/31 06:28:35, Erik Kay wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:301:
views::KeyEvent *event = new views::KeyEvent(
On 2011/01/27 23:19:23, sky wrote:
> "KeyEvent *" -> "KeyEvent* "

Done.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:306:
identifierMaps[j]->insert(IdentifierPair(key.identifiers[j], event));
On 2011/01/27 23:19:23, sky wrote:
> Add a DCHECK to make sure someone didn't add the value twice.

Done.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:323: if (iter !=
map.end()) {
On 2011/01/31 06:28:35, Erik Kay wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
File chrome/browser/extensions/key_identifier_conversion_views_unittest.cc
(right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views_unittest.cc:60:
EXPECT_EQ(views::Event::EF_SHIFT_DOWN,
On 2011/01/31 06:28:35, Erik Kay wrote:
> isn't this exact case also tested in the next test?

This was testing both set and unset.  I've moved the unset case into the test
below.

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views_unittest.cc:64:
TEST_F(KeyEventFromKeyIdentifierTest, ShiftModifiersAreSet) {
On 2011/01/31 06:28:35, Erik Kay wrote:
> should there be a similar control modifier test?

Currently we don't have any identifiers returning Control-modified KeyEvents. 
When we sort through that, I'll add a test for them here.

[sky, 2011-02-02 21:14:49]

LGTM

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
File chrome/browser/extensions/key_identifier_conversion_views.cc (right):

http://codereview.chromium.org/6264020/diff/5001/chrome/browser/extensions/ke...
chrome/browser/extensions/key_identifier_conversion_views.cc:287: static const
views::KeyEvent kUnknownKeyEvent(
On 2011/02/02 20:56:44, bryeung wrote:
> On 2011/01/27 23:19:23, sky wrote:
> > statics can only be plain old data.
> 
> Done, though I just made it a lazily initialized pointer.  Is there a better
> way?

That's what folks typically do.

[Erik does not do reviews, 2011-02-08 18:34:42]

LGTM