Synthetic KeyEvent delivery, part I.

Synthetic KeyEvent delivery, part I.

This delivers synthetic key events to the views hierarchy.  This
currently does nothing, as nothing is listening for the event in
TOUCH_UI (or elsewhere).  That part will come later.

BUG=none
TEST=unit test for key identifier conversion + manual for extension api

[bryeung, 2010-08-12 18:16:25]

Here's my initial draft of this CL.  Please have a look before I send it off to
Scott and Erik.

[rjkroege, 2010-08-12 18:35:32]

and lint it so you don't have a bunch of silly errors like I did. :-)

http://codereview.chromium.org/3153008/diff/1/8
File chrome/browser/extensions/extension_input_api.cc (right):

http://codereview.chromium.org/3153008/diff/1/8#newcode5
chrome/browser/extensions/extension_input_api.cc:5: #include
"chrome/browser/extensions/extension_input_api.h"
Does this break the mac or Linux builds because of the dependency on root_view?

http://codereview.chromium.org/3153008/diff/1/10
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/3153008/diff/1/10#newcode85
chrome/browser/views/tab_contents/tab_contents_container.cc:85: #if
defined(TOUCH_UI)
this is missing the secret sauce of sending the key strokes to the RenderView.
You could add a // TODO or FIXME in this regard?

http://codereview.chromium.org/3153008/diff/1/12
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3153008/diff/1/12#newcode3004
chrome/chrome_browser.gypi:3004: 'browser/extensions/extension_input_api.cc',
Aah. you don't break the Mac build I think. Maybe have a comment about this in
the extension_input_api.* file? (And have a story for supporting the other
platforms at some point?)

[bryeung, 2010-08-12 21:47:59]

Linted.  Thanks for taking a look.  I'll send it to Erik/Scott now.

http://codereview.chromium.org/3153008/diff/1/10
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/3153008/diff/1/10#newcode85
chrome/browser/views/tab_contents/tab_contents_container.cc:85: #if
defined(TOUCH_UI)
On 2010/08/12 18:35:32, rjkroege wrote:
> this is missing the secret sauce of sending the key strokes to the RenderView.
> You could add a // TODO or FIXME in this regard?

Actually I should just remove this.  RE: my recent conversation with Alex, the
path to the RenderViewHost is not going to be this class.

http://codereview.chromium.org/3153008/diff/1/12
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3153008/diff/1/12#newcode3004
chrome/chrome_browser.gypi:3004: 'browser/extensions/extension_input_api.cc',
On 2010/08/12 18:35:32, rjkroege wrote:
> Aah. you don't break the Mac build I think. Maybe have a comment about this in
> the extension_input_api.* file? (And have a story for supporting the other
> platforms at some point?)

comment added

[bryeung, 2010-08-12 21:53:03]

Hello,

Scott: I'm hoping you're willing to have a look at the views/base plumbing.

Erik: not surprisingly, the extensions stuff is intended for you.

Thanks!

[Erik does not do reviews, 2010-08-14 20:27:45]

http://codereview.chromium.org/3153008/diff/8001/9007
File chrome/browser/extensions/extension_input_api.cc (right):

http://codereview.chromium.org/3153008/diff/8001/9007#newcode25
chrome/browser/extensions/extension_input_api.cc:25: const wchar_t kType[] =
L"type";
can you change these to char? (there's an effort underway to purge usage of
wchar)

http://codereview.chromium.org/3153008/diff/8001/9007#newcode51
chrome/browser/extensions/extension_input_api.cc:51: if (!browser) {
remove unneeded {} from one line if blocks (several places below as well)

http://codereview.chromium.org/3153008/diff/8001/9007#newcode66
chrome/browser/extensions/extension_input_api.cc:66: views::RootView* root_view
= browser_view->GetRootView();
just return this directly.  you don't need the variable.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode79
chrome/browser/extensions/extension_input_api.cc:79: return false;
also set error_ to something reasonable

http://codereview.chromium.org/3153008/diff/8001/9007#newcode86
chrome/browser/extensions/extension_input_api.cc:86: return false;
same here

http://codereview.chromium.org/3153008/diff/8001/9007#newcode107
chrome/browser/extensions/extension_input_api.cc:107: views::KeyEvent
event(type, code, flags, 0, 0);
move this next to the call to ProcessKeyEvent

http://codereview.chromium.org/3153008/diff/8001/9007#newcode111
chrome/browser/extensions/extension_input_api.cc:111: return false;
set error_ here too

http://codereview.chromium.org/3153008/diff/8001/9007#newcode114
chrome/browser/extensions/extension_input_api.cc:114: return
root_view->ProcessKeyEvent(event);
Is this just going to wind up sending a key event to whatever has focus?  The
docs in the json say that it sends to the current tab, but this looks like it
should work anywhere in the view hierarchy, right?

http://codereview.chromium.org/3153008/diff/8001/9007#newcode114
chrome/browser/extensions/extension_input_api.cc:114: return
root_view->ProcessKeyEvent(event);
also, check the return value and set error_ if false

http://codereview.chromium.org/3153008/diff/8001/9008
File chrome/browser/extensions/extension_input_api.h (right):

http://codereview.chromium.org/3153008/diff/8001/9008#newcode25
chrome/browser/extensions/extension_input_api.h:25: // We may eventually support
other platforms by adding the necessary
There doesn't appear to be anything here that's specific to TOUCH_UI, just
VIEWS.  Why not make this dependent on VIEWS instead?

http://codereview.chromium.org/3153008/diff/8001/9009
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3153008/diff/8001/9009#newcode3616
chrome/chrome_browser.gypi:3616: ['OS=="linux" and toolkit_views==1 and
touchui==0',{
this doesn't need to be touch or linux specific as far as I can tell

http://codereview.chromium.org/3153008/diff/8001/9010
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3153008/diff/8001/9010#newcode2266
chrome/common/extensions/api/extension_api.json:2266: "description": "Whether or
not the ALT key was pressed."
change "was" to "is" (others too)

[bryeung, 2010-08-16 15:31:34]

Thanks Erik!  I think I've addressed everything.

http://codereview.chromium.org/3153008/diff/8001/9007
File chrome/browser/extensions/extension_input_api.cc (right):

http://codereview.chromium.org/3153008/diff/8001/9007#newcode25
chrome/browser/extensions/extension_input_api.cc:25: const wchar_t kType[] =
L"type";
On 2010/08/14 20:27:45, Erik Kay wrote:
> can you change these to char? (there's an effort underway to purge usage of
> wchar)

Done.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode51
chrome/browser/extensions/extension_input_api.cc:51: if (!browser) {
On 2010/08/14 20:27:45, Erik Kay wrote:
> remove unneeded {} from one line if blocks (several places below as well)

Done.  Strange that the linter didn't catch it for me though.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode66
chrome/browser/extensions/extension_input_api.cc:66: views::RootView* root_view
= browser_view->GetRootView();
On 2010/08/14 20:27:45, Erik Kay wrote:
> just return this directly.  you don't need the variable.

Done.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode79
chrome/browser/extensions/extension_input_api.cc:79: return false;
On 2010/08/14 20:27:45, Erik Kay wrote:
> also set error_ to something reasonable

Done.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode86
chrome/browser/extensions/extension_input_api.cc:86: return false;
On 2010/08/14 20:27:45, Erik Kay wrote:
> same here

Done.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode107
chrome/browser/extensions/extension_input_api.cc:107: views::KeyEvent
event(type, code, flags, 0, 0);
On 2010/08/14 20:27:45, Erik Kay wrote:
> move this next to the call to ProcessKeyEvent

Done.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode111
chrome/browser/extensions/extension_input_api.cc:111: return false;
On 2010/08/14 20:27:45, Erik Kay wrote:
> set error_ here too

Done.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode114
chrome/browser/extensions/extension_input_api.cc:114: return
root_view->ProcessKeyEvent(event);
On 2010/08/14 20:27:45, Erik Kay wrote:
> Is this just going to wind up sending a key event to whatever has focus?  The
> docs in the json say that it sends to the current tab, but this looks like it
> should work anywhere in the view hierarchy, right?

Good point.  The json doc is out of date.  Updated.

http://codereview.chromium.org/3153008/diff/8001/9007#newcode114
chrome/browser/extensions/extension_input_api.cc:114: return
root_view->ProcessKeyEvent(event);
On 2010/08/14 20:27:45, Erik Kay wrote:
> also, check the return value and set error_ if false

Done.

http://codereview.chromium.org/3153008/diff/8001/9008
File chrome/browser/extensions/extension_input_api.h (right):

http://codereview.chromium.org/3153008/diff/8001/9008#newcode25
chrome/browser/extensions/extension_input_api.h:25: // We may eventually support
other platforms by adding the necessary
On 2010/08/14 20:27:45, Erik Kay wrote:
> There doesn't appear to be anything here that's specific to TOUCH_UI, just
> VIEWS.  Why not make this dependent on VIEWS instead?

Done.

http://codereview.chromium.org/3153008/diff/8001/9009
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3153008/diff/8001/9009#newcode3616
chrome/chrome_browser.gypi:3616: ['OS=="linux" and toolkit_views==1 and
touchui==0',{
On 2010/08/14 20:27:45, Erik Kay wrote:
> this doesn't need to be touch or linux specific as far as I can tell

Good point.  Changed to exclude only if toolkit_views=0.

http://codereview.chromium.org/3153008/diff/8001/9010
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3153008/diff/8001/9010#newcode2266
chrome/common/extensions/api/extension_api.json:2266: "description": "Whether or
not the ALT key was pressed."
On 2010/08/14 20:27:45, Erik Kay wrote:
> change "was" to "is" (others too)

Done.

[Erik does not do reviews, 2010-08-16 16:00:09]

Changes look good.

One more thing I left out of my last review is that you need an API test.  See
chrome/browser/*apitest.cc (and their matching files in
test/data/extensions/api_test/).

http://codereview.chromium.org/3153008/diff/8001/9007
File chrome/browser/extensions/extension_input_api.cc (right):

http://codereview.chromium.org/3153008/diff/8001/9007#newcode51
chrome/browser/extensions/extension_input_api.cc:51: if (!browser) {
On 2010/08/16 15:31:34, bryeung wrote:
> On 2010/08/14 20:27:45, Erik Kay wrote:
> > remove unneeded {} from one line if blocks (several places below as well)
> 
> Done.  Strange that the linter didn't catch it for me though.

Google style allows either form.  Chrome style prefers no {}, but I don't think
we've modified the default Google linter for this kind of thing.

[bryeung, 2010-08-16 19:13:00]

On 2010/08/16 16:00:09, Erik Kay wrote:
> One more thing I left out of my last review is that you need an API test.  See
> chrome/browser/*apitest.cc (and their matching files in
> test/data/extensions/api_test/).

Added a test.  Hopefully I've got it wired in correctly.

This code is a bit hard to test from the API level right now, as it isn't
supposed to have any effect on the browser.  But at least the framework is in
place, so it will be easy to add more detailed tests when the views code starts
listening to the events.

> Google style allows either form.  Chrome style prefers no {}, but I don't
think
> we've modified the default Google linter for this kind of thing.

Got it.  Good to know.

Thanks!

[Erik does not do reviews, 2010-08-16 20:59:10]

http://codereview.chromium.org/3153008/diff/28001/29012
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3153008/diff/28001/29012#newcode2251
chrome/common/extensions/api/extension_api.json:2251: "parameters": [
sorry, I missed this the first time, but you're missing a callback argument
(you'll see this in my comment about your test).  The last argument should be
type function name callback (see other examples).

http://codereview.chromium.org/3153008/diff/28001/29015
File chrome/test/data/extensions/api_test/input/test.js (right):

http://codereview.chromium.org/3153008/diff/28001/29015#newcode7
chrome/test/data/extensions/api_test/input/test.js:7: var result =
chrome.experimental.input.sendKeyboardEvent(e);
this function is async and has no return value, so the pattern should be:
chrome.experimental.input.sendKeyboardEvent(e, function() {
  if (chrome.extension.lastError) {
    // This is currently expected to fail.
    // chrome.test.fail();
  }
  chrome.test.succeed();
}

[bryeung, 2010-08-16 21:25:59]

On 2010/08/16 20:59:10, Erik Kay wrote:
> sorry, I missed this the first time, but you're missing a callback argument
> (you'll see this in my comment about your test).  The last argument should be
> type function name callback (see other examples).

Oops!  Sorry.

I had this nagging concern about the return value: I guess I should have paid
more attention to it (and studied the other api tests more carefully).

I think I have it cleaned up now.

Bryan

[Erik does not do reviews, 2010-08-16 21:28:07]

assuming it passes the trybots, LGTM

[Erik does not do reviews, 2010-08-16 21:28:56]

http://codereview.chromium.org/3153008/diff/33001/34011
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/3153008/diff/33001/34011#newcode1592
chrome/chrome_tests.gypi:1592: 'browser/extensions/extension_input_apitest.cc',
oops.  one more thing - this should be VIEWS only right?

[bryeung, 2010-08-16 21:43:27]

On 2010/08/16 21:28:56, Erik Kay wrote:
> http://codereview.chromium.org/3153008/diff/33001/34011
> File chrome/chrome_tests.gypi (right):
> 
> http://codereview.chromium.org/3153008/diff/33001/34011#newcode1592
> chrome/chrome_tests.gypi:1592:
'browser/extensions/extension_input_apitest.cc',
> oops.  one more thing - this should be VIEWS only right?

Gah!  Good catch.

Fixed.

[Erik does not do reviews, 2010-08-16 21:46:50]

LGTM


On Mon, Aug 16, 2010 at 2:43 PM, <bryeung@chromium.org> wrote:

> On 2010/08/16 21:28:56, Erik Kay wrote:
>
>> http://codereview.chromium.org/3153008/diff/33001/34011
>> File chrome/chrome_tests.gypi (right):
>>
>
>  http://codereview.chromium.org/3153008/diff/33001/34011#newcode1592
>> chrome/chrome_tests.gypi:1592:
>>
> 'browser/extensions/extension_input_apitest.cc',
>
>> oops.  one more thing - this should be VIEWS only right?
>>
>
> Gah!  Good catch.
>
> Fixed.
>
>
> http://codereview.chromium.org/3153008/show
>

[brettw, 2010-08-17 05:49:52]

http://codereview.chromium.org/3153008/diff/37001/38004
File base/keyboard_code_conversion.h (right):

http://codereview.chromium.org/3153008/diff/37001/38004#newcode5
base/keyboard_code_conversion.h:5: #ifndef BASE_KEYBOARD_CODE_CONVERSION_H_
If this file can be moved out of base, I think that's a good idea.
keyboard_codes.h itself should probably be in app which is supposed to be
specifically for "high level gui apps" (while base is for everything ever, like
command line utils).

http://codereview.chromium.org/3153008/diff/37001/38004#newcode9
base/keyboard_code_conversion.h:9: #include "base/keyboard_codes_posix.h"
Looks like this should be including plain "keyboard_codes.h" rather than the
posix-specific one.

http://codereview.chromium.org/3153008/diff/37001/38004#newcode15
base/keyboard_code_conversion.h:15: KeyboardCode KeyCodeFromKeyIdentifier(const
std::string& key_identifier);
Can you comment this? In particular, I have no idea what a "key identifier" is.

[bryeung, 2010-08-17 15:01:47]

On 2010/08/17 05:49:52, brettw wrote:
> http://codereview.chromium.org/3153008/diff/37001/38004
> File base/keyboard_code_conversion.h (right):
> 
> http://codereview.chromium.org/3153008/diff/37001/38004#newcode5
> base/keyboard_code_conversion.h:5: #ifndef BASE_KEYBOARD_CODE_CONVERSION_H_
> If this file can be moved out of base, I think that's a good idea.
> keyboard_codes.h itself should probably be in app which is supposed to be
> specifically for "high level gui apps" (while base is for everything ever,
like
> command line utils).

Do you mind if I submit this code initially in base/* and then make a separate
CL to move all of the keyboard code conversion utilities into app/?

It seems to make more sense to move them all at once, and I'd prefer not to add
the other files to this CL.

> http://codereview.chromium.org/3153008/diff/37001/38004#newcode9
> base/keyboard_code_conversion.h:9: #include "base/keyboard_codes_posix.h"
> Looks like this should be including plain "keyboard_codes.h" rather than the
> posix-specific one.

Thanks.  Fixed.

> http://codereview.chromium.org/3153008/diff/37001/38004#newcode15
> base/keyboard_code_conversion.h:15: KeyboardCode
KeyCodeFromKeyIdentifier(const
> std::string& key_identifier);
> Can you comment this? In particular, I have no idea what a "key identifier"
is.

Added.  Thanks.

[brettw, 2010-08-17 16:31:29]

On 2010/08/17 15:01:47, bryeung wrote:
> On 2010/08/17 05:49:52, brettw wrote:
> > http://codereview.chromium.org/3153008/diff/37001/38004
> > File base/keyboard_code_conversion.h (right):
> > 
> > http://codereview.chromium.org/3153008/diff/37001/38004#newcode5
> > base/keyboard_code_conversion.h:5: #ifndef BASE_KEYBOARD_CODE_CONVERSION_H_
> > If this file can be moved out of base, I think that's a good idea.
> > keyboard_codes.h itself should probably be in app which is supposed to be
> > specifically for "high level gui apps" (while base is for everything ever,
> like
> > command line utils).
> 
> Do you mind if I submit this code initially in base/* and then make a separate
> CL to move all of the keyboard code conversion utilities into app/?
> 
> It seems to make more sense to move them all at once, and I'd prefer not to
add
> the other files to this CL.

Only because you'll also move the existing ones :) Please do it ASAP after
checking this in; this kind of thing has a habit of getting put off and then
forgotten.

[bryeung, 2010-08-17 17:01:47]

On 2010/08/17 16:31:29, brettw wrote:
> Only because you'll also move the existing ones :) Please do it ASAP after
> checking this in; this kind of thing has a habit of getting put off and then
> forgotten.

For sure.  I'll start on the CL now while I wait for a final LGTM and someone
willing to submit for me (I'm not a committer yet).

Thanks.

[brettw, 2010-08-17 18:26:46]

My concerns are LGTM (I didn't review the whole thing).