Aura: Support non-global keyboard shortcuts.

Aura: Support non-global keyboard shortcuts.

BUG=100232
TEST=Manual


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

[mazda, 2011-10-26 12:41:56]

Hi Ben,

I enabled non-global shortcuts on Linux first.
It worked with relatively small change, but please advise me if you have any
better ideas.

Thanks!

[Ben Goodger (Google), 2011-10-26 19:06:04]

http://codereview.chromium.org/8341050/diff/1005/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/8341050/diff/1005/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:139: #if defined(OS_LINUX) &&
defined(USE_AURA)
Will this build & run on Windows too? If so, let's just have this be #if
defined(USE_AURA)

http://codereview.chromium.org/8341050/diff/1005/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:2306: #if defined(OS_WIN) &&
defined(USE_AURA)
same comment here.

I'd like to share as much in common as possible between the win & linux paths
for aura.

http://codereview.chromium.org/8341050/diff/1005/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/8341050/diff/1005/views/view.cc#newcode45
views/view.cc:45: #include "ui/aura/focus_manager.h"
you won't need these items after you do the item in my note below:

http://codereview.chromium.org/8341050/diff/1005/views/view.cc#newcode1234
views/view.cc:1234: aura::Window* window = GetWidget()->GetNativeWindow();
This should be done by implementing NativeWidgetAura::ClearNativeFocus().

http://codereview.chromium.org/8341050/diff/1005/views/widget/native_widget_a...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8341050/diff/1005/views/widget/native_widget_a...
views/widget/native_widget_aura.cc:451: if (key.type() == ui::ET_KEY_PRESSED &&
GetWidget()->GetFocusManager())
Is it standard practice to dispatch accelerators/tab traversal etc _after_ IME?

[mazda, 2011-10-27 13:05:04]

Please take another look at the change?

http://codereview.chromium.org/8341050/diff/1005/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/8341050/diff/1005/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:139: #if defined(OS_LINUX) &&
defined(USE_AURA)
On 2011/10/26 19:06:04, Ben Goodger (Google) wrote:
> Will this build & run on Windows too? If so, let's just have this be #if
> defined(USE_AURA)

Done.

http://codereview.chromium.org/8341050/diff/1005/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:2306: #if defined(OS_WIN) &&
defined(USE_AURA)
On 2011/10/26 19:06:04, Ben Goodger (Google) wrote:
> same comment here.
> 
> I'd like to share as much in common as possible between the win & linux paths
> for aura.

Done.

http://codereview.chromium.org/8341050/diff/1005/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/8341050/diff/1005/views/view.cc#newcode45
views/view.cc:45: #include "ui/aura/focus_manager.h"
On 2011/10/26 19:06:04, Ben Goodger (Google) wrote:
> you won't need these items after you do the item in my note below:

Done.

http://codereview.chromium.org/8341050/diff/1005/views/view.cc#newcode1234
views/view.cc:1234: aura::Window* window = GetWidget()->GetNativeWindow();
On 2011/10/26 19:06:04, Ben Goodger (Google) wrote:
> This should be done by implementing NativeWidgetAura::ClearNativeFocus().

Done.

http://codereview.chromium.org/8341050/diff/1005/views/widget/native_widget_a...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8341050/diff/1005/views/widget/native_widget_a...
views/widget/native_widget_aura.cc:451: if (key.type() == ui::ET_KEY_PRESSED &&
GetWidget()->GetFocusManager())
On 2011/10/26 19:06:04, Ben Goodger (Google) wrote:
> Is it standard practice to dispatch accelerators/tab traversal etc _after_
IME?
I think so.
While users compose text with IME, they would not expect accelerators are
handled by Chrome, so IME needs to handle key events first.
NativeWidget{Gtk,Views} are also implemented in that way.

[Ben Goodger (Google), 2011-10-27 15:33:01]

http://codereview.chromium.org/8341050/diff/7001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/8341050/diff/7001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:136: #include
"views/widget/native_widget_win.h"
This is wrong. We don't build this file on OS_WIN && USE_AURA... you need to
move this back to the previous section.

http://codereview.chromium.org/8341050/diff/7001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:2306: #if defined(OS_WIN)
What I meant here was can we use the Linux accelerator logic for USE_AURA &&
OS_WIN... it looked pretty portable.

http://codereview.chromium.org/8341050/diff/7001/views/widget/native_widget_a...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8341050/diff/7001/views/widget/native_widget_a...
views/widget/native_widget_aura.cc:441:
GetWidget()->GetFocusManager()->OnKeyEvent(key);
OK... so does this handle browser key events (e.g. Ctrl+T, L, etc) when the
renderer has focus?

[mazda, 2011-10-28 14:44:33]

Please take another look.

http://codereview.chromium.org/8341050/diff/7001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/8341050/diff/7001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:109: #include
"chrome/browser/ui/views/accelerator_table_linux.h"
Now that this header is used for win_aura as well, so I renamed it to
accelerator_table.h

http://codereview.chromium.org/8341050/diff/7001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:136: #include
"views/widget/native_widget_win.h"
On 2011/10/27 15:33:01, Ben Goodger (Google) wrote:
> This is wrong. We don't build this file on OS_WIN && USE_AURA... you need to
> move this back to the previous section.

Done.

http://codereview.chromium.org/8341050/diff/7001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:2306: #if defined(OS_WIN)
On 2011/10/27 15:33:01, Ben Goodger (Google) wrote:
> What I meant here was can we use the Linux accelerator logic for USE_AURA &&
> OS_WIN... it looked pretty portable.

Ah, I understood.
I fixed it and checked that shortcut keys worked on Windows as well.

http://codereview.chromium.org/8341050/diff/7001/views/widget/native_widget_a...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8341050/diff/7001/views/widget/native_widget_a...
views/widget/native_widget_aura.cc:441:
GetWidget()->GetFocusManager()->OnKeyEvent(key);
On 2011/10/27 15:33:01, Ben Goodger (Google) wrote:
> OK... so does this handle browser key events (e.g. Ctrl+T, L, etc) when the
> renderer has focus?

Yes, shortcut keys are handled when web content area has the focus.

[Ben Goodger (Google), 2011-10-28 14:55:49]

LGTM. Don't you still need the nativewidgetaura change?

[mazda, 2011-10-28 15:13:31]

Oops, somehow the change of native_widget_aura.cc had been omitted from the
changelist.
I restored the change.

Thanks!

[commit-bot: I haz the power, 2011-10-28 16:15:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/8341050/13001

[commit-bot: I haz the power, 2011-10-28 17:32:45]

Change committed as 107747