Do not send some keyboard shortcuts to the renderers

Do not send some keyboard shortcuts to the renderers

Walking the whole menu on every keypress seems ridiculous. Linux does this too :-/ (Caching is made hard because the user can change key equivalents in system preferences at every point in time, and we're not notified of that. And people only hit max 5 keys/second, so it's not all that ridiculous).

There's a UI test for this, but the interactive UI tests are not enabled on OS X, so it's not executed.

Menu walking code based on code from CocoatechCore.

BUG=5496, 15090, 24877
TEST=Go to http://unixpapa.com/js/testkey.html , check "keydown", focus the textbox, and make sure that cmd-t still opens tabs, cmd-shift-[ still switches tabs, cmd-w still closes tabs. however, cmd-L should not focus url bar and cmd-1 should not go to the first tab.

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

[James Su, 2009-10-13 09:12:33]

I'm not familiar with this part of code. However I noticed that both Linux and
Mac use a loop to find the command id for a key down event. I'm wondering the
performance(battery life) impact of this piece of code, as it will be run for
every key down events.

Regards
James Su

[Nico, 2009-10-13 14:08:42]

On 2009/10/13 09:12:33, James Su wrote:
> I'm not familiar with this part of code. However I noticed that both Linux and
> Mac use a loop to find the command id for a key down event. I'm wondering the
> performance(battery life) impact of this piece of code, as it will be run for
> every key down events.
> 
> Regards
> James Su

Yes, at least on OS X I want to change this to use a cache…the menu doesn't
change at runtime, so I don't have to worry about eviction, so it should be
easy. A map lookup per keypress still sounds expensive, but slightly better.

[pink (ping after 24hrs), 2009-10-13 14:38:35]

drive-by: note that we do sometimes change the command keys for menu items at
runtime (cmd-w/cmd-shift-w are one example). Not sure if that impacts your
caching idea.

Also, what's the license on the CocoatechCore code?

[James Su, 2009-10-14 01:01:14]

I'll look into this piece of code to see if it's possible to use another
mechanism to work around this lookup code. I'm just wondering how does Cocoa
lookup a key in the menu, as a key event will eventually be dispatched to
Cocoa's menu key event handler.
On Linux, gtk uses a hash map for this purpose.

Regards
James Su

On 2009/10/13 14:08:42, Nico wrote:
> On 2009/10/13 09:12:33, James Su wrote:
> > I'm not familiar with this part of code. However I noticed that both Linux
and
> > Mac use a loop to find the command id for a key down event. I'm wondering
the
> > performance(battery life) impact of this piece of code, as it will be run
for
> > every key down events.
> > 
> > Regards
> > James Su
> 
> Yes, at least on OS X I want to change this to use a cache…the menu doesn't
> change at runtime, so I don't have to worry about eviction, so it should be
> easy. A map lookup per keypress still sounds expensive, but slightly better.

[Nico, 2009-10-14 01:04:19]

It's a secret how Cocoa does it. You say [[NSApp mainMenu]
performKeyEquivalent:keyEvent], and cocoa checks if there's a menu
entry with that shortcut and if so calls the callback associated with
that menu entry. It's not really possible to do this manually; check
the link I put into this CL description.

(but the approach in this CL gets us there in most cases)

On Tue, Oct 13, 2009 at 6:01 PM,  <suzhe@chromium.org> wrote:
> I'll look into this piece of code to see if it's possible to use another
> mechanism to work around this lookup code. I'm just wondering how does Co=
coa
> lookup a key in the menu, as a key event will eventually be dispatched to
> Cocoa's menu key event handler.
> On Linux, gtk uses a hash map for this purpose.
>
> Regards
> James Su
>
> On 2009/10/13 14:08:42, Nico wrote:
>>
>> On 2009/10/13 09:12:33, James Su wrote:
>> > I'm not familiar with this part of code. However I noticed that both
>> > Linux
>
> and
>>
>> > Mac use a loop to find the command id for a key down event. I'm
>> > wondering
>
> the
>>
>> > performance(battery life) impact of this piece of code, as it will be
>> > run
>
> for
>>
>> > every key down events.
>> >
>> > Regards
>> > James Su
>
>> Yes, at least on OS X I want to change this to use a cache=85the menu
>> doesn't
>> change at runtime, so I don't have to worry about eviction, so it should
>> be
>> easy. A map lookup per keypress still sounds expensive, but slightly
>> better.
>
>
>
> http://codereview.chromium.org/273032
>

[Nico, 2009-10-14 21:46:02]

Re license: I contacted the cocoatech guy, and he responded """The license is
"none"  use as you want, no restrictions.""". Since my function is only based on
his, and not a carbon copy, I guess that's fine?

(but this CL is still on hold until suzhe is done with his change)

[James Su, 2009-10-14 22:38:35]

You can go ahead and submit this CL. I may need several days to prepare the
change.

Regards
James Su

On 2009/10/14 21:46:02, Nico wrote:
> Re license: I contacted the cocoatech guy, and he responded """The license is
> "none"  use as you want, no restrictions.""". Since my function is only based
on
> his, and not a carbon copy, I guess that's fine?
> 
> (but this CL is still on hold until suzhe is done with his change)

[Nico, 2009-10-26 04:17:52]

This is now ready to go in. LG?

[James Su, 2009-10-26 04:36:12]

LGTM. But Mac trybot seems doesn't like it.

On 2009/10/26 04:17:52, Nico wrote:
> This is now ready to go in. LG?

[Nico, 2009-10-26 04:39:18]

That's because LKGR is still at r30029 but this patch depends on
r30030. I'll probably try to just submit this. It works fine locally.

On Sun, Oct 25, 2009 at 9:36 PM,  <suzhe@chromium.org> wrote:
> LGTM. But Mac trybot seems doesn't like it.
>
> On 2009/10/26 04:17:52, Nico wrote:
>>
>> This is now ready to go in. LG?
>
>
>
> http://codereview.chromium.org/273032
>

[jam, 2009-10-26 15:45:46]

Can BrowserInteractiveTest, ReserveKeyboardAccelerators be enabled for Mac now?

[Nico, 2009-10-26 15:58:09]

No, interactive_tests don't run on OS X yet (
http://code.google.com/p/chromium/issues/detail?id=21276 , and this
excerpt from chrome.gyp:

        # TODO(port): enable on mac.
        {
          'includes': ['test/interactive_ui/interactive_ui_tests.gypi']
        },

).

On Mon, Oct 26, 2009 at 8:45 AM,  <jam@chromium.org> wrote:
> Can BrowserInteractiveTest, ReserveKeyboardAccelerators be enabled for Mac
> now?
>
> http://codereview.chromium.org/273032
>