Forward edit commands to be executed for shortcuts which aren't handled by...

Forward edit commands to be executed for shortcuts which aren't handled by
javascript.

As a result, cocoa keybindings work.

The approach:

We instantiate a vanilla NSResponder, call interpretKeyEvents on it, and
record all of the selectors passed into doCommandBySelector while
interpreting the key event.  The selectors are converted into edit commands
which can be passed to the render process.

Caveats:
- Shortcuts involving a sequence of key combinations (chords) don't work,
because we instantiate a new responder for each event.
- We ignore key combinations that don't include a modifier (ctrl, cmd, alt)
because this was causing strange behavior (e.g. tab always inserted a tab
rather than moving to the next field on the page).

BUG=12538
TEST=go to a page with a text field. make sure that ctrl-a moves cursor to beginning of line, ctrl-e goes to the end, ctrl-d deletes to the right, ctrl-h to the left, etc

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

[Nico, 2009-09-20 17:11:48]

LG with style comments addressed, but wait for what avi and hbono (the keyboard
experts) have to say too.

I'm very excited about this patch, both because of the functionality it adds and
because the code turned out to be pretty clean. I focus on style, as I'm not
terribly experienced with the keyboard code.

For my own understanding, rwhvm now sends each key down to a custom vanilla
nsresponder, which just records the selector that would be executed and converts
it into a string. This string is then sent to the renderer along with the
original key event. The renderer then executes this string if the original key
event is not handled by the web page.

the edits to rwhvm_editcommand_helper.h/mm are only to make a function public.

One limitation of this approach is that chorded shortcuts don't work (e.g. if
one would add a key binding for C-x C-s, that wouldn't work, because this only
executes a single keyboard event in the responder). I think that's fine, but
should be mentioned somewhere.

Perhaps update the CL description with both a short overview of the approach and
its limitations.

http://codereview.chromium.org/209046/diff/3001/3003
File chrome/browser/cocoa/rwhvm_editcommand_helper.h (right):

http://codereview.chromium.org/209046/diff/3001/3003#newcode69
Line 69: 
Remove extra newline.

http://codereview.chromium.org/209046/diff/3001/3004
File chrome/browser/cocoa/rwhvm_editcommand_helper.mm (right):

http://codereview.chromium.org/209046/diff/3001/3004#newcode131
Line 131: NSString* command_name_ns =
RWHVMEditCommandHelper::CommandNameForSelector(_cmd);
> 80 cols.

http://codereview.chromium.org/209046/diff/3001/3004#newcode173
Line 173: }
Add newline.

http://codereview.chromium.org/209046/diff/3001/3002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/209046/diff/3001/3002#newcode455
Line 455: NSString* selector_str =
In objc code, use objc variable naming (whichGoesLikeThis). Also, I think the
recommendation is to not use abbreviations like str but instead complete words
like string. Not completely sure, though.

http://codereview.chromium.org/209046/diff/3001/3002#newcode457
Line 457: edit_commands_->push_back(EditCommand([selector_str UTF8String], ""));
For consistency with the rest of the code, use
base::SysNSStringToUTF8(selector_str) (you need to include
base/sys_string_conversions.h for that).

http://codereview.chromium.org/209046/diff/3001/3002#newcode465
Line 465: forEvent:(NSEvent*)theEvent{
s/t{/t {/

http://codereview.chromium.org/209046/diff/3001/3002#newcode473
Line 473: [matcher release];
Use a scoped_nsobject instead (include base/scoped_nsobject.h , then do
scoped_nsobject<EditCommandMatcher> matcher(...alloc...);, and it's release
automatically when the scope is left. Use "matcher.get()" in the
interpretKeyEvents: line.

http://codereview.chromium.org/209046/diff/3001/3002#newcode548
Line 548: EditCommands edit_commands;
editCommands

http://codereview.chromium.org/209046/diff/3001/3002#newcode550
Line 550: if (!edit_commands.empty())
The block below checks `if (renderWidgetHostView_->render_widget_host_)` -- you
should probably do that too.

Or move this whole thing into the if below.

[jeremy, 2009-09-20 17:35:25]

Nice work!!! A few small remarks:
* Could you please add some more comments in the code (perhaps above the
NSResponder subclass) detailing exactly what's going on here.  Our keyboard
handling code is getting ever-more involved and some well placed comments can
really help.

* Is it possible to add a unit test, at least for parts of this CL?  I'm fine
with that being a follow-up to this one.

http://codereview.chromium.org/209046/diff/3001/3003
File chrome/browser/cocoa/rwhvm_editcommand_helper.h (right):

http://codereview.chromium.org/209046/diff/3001/3003#newcode69
Line 69: 
extra NL

http://codereview.chromium.org/209046/diff/3001/3004
File chrome/browser/cocoa/rwhvm_editcommand_helper.mm (right):

http://codereview.chromium.org/209046/diff/3001/3004#newcode131
Line 131: NSString* command_name_ns =
RWHVMEditCommandHelper::CommandNameForSelector(_cmd);
>80 chars.

[Matt Tolton, 2009-09-20 21:06:42]

Thanks for the quick review guys; sorry for all the little style issues.

I added prose above the nsresponder subclass and updated the cl description.

As far as testing, I'm willing but I wouldn't even know where to start in this
case.  Is there some framework already set up for testing keyboard handling
stuff?

http://codereview.chromium.org/209046/diff/3001/3003
File chrome/browser/cocoa/rwhvm_editcommand_helper.h (right):

http://codereview.chromium.org/209046/diff/3001/3003#newcode69
Line 69: 
On 2009/09/20 17:11:49, Nico wrote:
> Remove extra newline.

Done.

http://codereview.chromium.org/209046/diff/3001/3004
File chrome/browser/cocoa/rwhvm_editcommand_helper.mm (right):

http://codereview.chromium.org/209046/diff/3001/3004#newcode131
Line 131: NSString* command_name_ns =
RWHVMEditCommandHelper::CommandNameForSelector(_cmd);
On 2009/09/20 17:35:25, jeremy wrote:
> >80 chars.

Done.

http://codereview.chromium.org/209046/diff/3001/3004#newcode173
Line 173: }
On 2009/09/20 17:11:49, Nico wrote:
> Add newline.

Done.

http://codereview.chromium.org/209046/diff/3001/3002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/209046/diff/3001/3002#newcode455
Line 455: NSString* selector_str =
On 2009/09/20 17:11:49, Nico wrote:
> In objc code, use objc variable naming (whichGoesLikeThis). Also, I think the
> recommendation is to not use abbreviations like str but instead complete words
> like string. Not completely sure, though.

Done.

http://codereview.chromium.org/209046/diff/3001/3002#newcode457
Line 457: edit_commands_->push_back(EditCommand([selector_str UTF8String], ""));
On 2009/09/20 17:11:49, Nico wrote:
> For consistency with the rest of the code, use
> base::SysNSStringToUTF8(selector_str) (you need to include
> base/sys_string_conversions.h for that).

Done.

http://codereview.chromium.org/209046/diff/3001/3002#newcode465
Line 465: forEvent:(NSEvent*)theEvent{
On 2009/09/20 17:11:49, Nico wrote:
> s/t{/t {/

Done.

http://codereview.chromium.org/209046/diff/3001/3002#newcode473
Line 473: [matcher release];
On 2009/09/20 17:11:49, Nico wrote:
> Use a scoped_nsobject instead (include base/scoped_nsobject.h , then do
> scoped_nsobject<EditCommandMatcher> matcher(...alloc...);, and it's release
> automatically when the scope is left. Use "matcher.get()" in the
> interpretKeyEvents: line.

Done.

http://codereview.chromium.org/209046/diff/3001/3002#newcode548
Line 548: EditCommands edit_commands;
On 2009/09/20 17:11:49, Nico wrote:
> editCommands

Done.

http://codereview.chromium.org/209046/diff/3001/3002#newcode550
Line 550: if (!edit_commands.empty())
On 2009/09/20 17:11:49, Nico wrote:
> The block below checks `if (renderWidgetHostView_->render_widget_host_)` --
you
> should probably do that too.
> 
> Or move this whole thing into the if below.

Done.

[Avi (use Gerrit), 2009-09-21 16:45:55]

Very clever. LG.

http://codereview.chromium.org/209046/diff/6005/6006
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/209046/diff/6005/6006#newcode563
Line 563:
renderWidgetHostView_->render_widget_host_->ForwardEditCommandsForNextKeyEvent(
80 columns, please

[Nico, 2009-09-21 17:22:34]

I landed this. I will create one bug for "keyboard handling needs unit tests"
and one for "cocoa chorded keyboard stuff doesn't work" and close the original
bug.