Fixes issue with accelerators when a menu is open

Implements accelerator handling for menus on aura.

BUG=105964
TEST=Manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120777

[oshima, 2012-01-19 18:29:48]

sky should review this too.

http://codereview.chromium.org/9224001/diff/2001/ui/views/controls/menu/menu_...
File ui/views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/9224001/diff/2001/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_controller.cc:919: flags & kModifierFlagMask);
just accelerator(key_code, flags & kModifierFlagMask);

btw, why do you need this mask?

http://codereview.chromium.org/9224001/diff/2001/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_controller.cc:921: GetFocusManager();
menu_button_->GetFocusManager()

http://codereview.chromium.org/9224001/diff/2001/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_controller.cc:923:
ash::Shell::GetInstance()->accelerator_controller();
have you looked into handling this on aura/ash side? If so, what's the
difficulty?

http://codereview.chromium.org/9224001/diff/2001/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_controller.cc:925: return
base::MessagePumpDispatcher::EVENT_QUIT;
You're executing accelerator inside nested loop while menu is open and I'm
worrying that there may be an accelerator that shouldn't be executed in this
situation. Scott, WDYT?

http://codereview.chromium.org/9224001/diff/2001/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_controller.cc:949: gtk_main_do_eSvent(event);
revert this change?

[sky, 2012-01-19 21:34:00]

This doesn't feel like the right place for this code. I like Sadrul's approach
better (http://codereview.chromium.org/9264002/), although I still feel there
should be some central place we're registering these. Maybe that's just a
separate file instead of making ChromeBrowserMainExtraPartsAura provide the
functionality, which doesn't feel right.

[pkotwicz, 2012-01-21 03:04:34]

This is a trial for using the approach of using a nested dispatcher.
For menu:
1) send events to ash::AcceleratorHandler
2) If ash::AcceleratorHandler does not consume event, forward event to
MenuController

Only implemented for linux, will implement for Win if this approach is to be
used

[oshima, 2012-01-23 18:06:39]

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
File ash/accelerators/accelerator_handler.cc (right):

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.cc:7: #include "base/message_loop.h"
i believe you dont need this.

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.cc:16: } // namespace ash
two spaces before //

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
File ash/accelerators/accelerator_handler.h (right):

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.h:17: class ASH_EXPORT AcceleratorHandler :
public MessageLoop::Dispatcher {
There is an "AcceleratorHandler" class in views/focus/accelerator_handler.h, so
it'd be better to use different name.

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.h:20:
AcceleratorHandler(MessageLoop::Dispatcher* nested_dispatcher);
explicit

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.h:26: protected:
can this be private?

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.h:27: MessageLoop::Dispatcher*
nested_dispatcher_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
File ash/accelerators/accelerator_handler_linux.cc (right):

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler_linux.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler_linux.cc:14: #include <X11/Xlib.h>
move this before chrome includes.

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler_linux.cc:22: ui::EF_ALT_DOWN);
If this is necessary, won't it make more sense to do this
in accelerator's constructor?

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler_linux.cc:23: } //namespace
}  // namespace

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler_linux.cc:29: ash::Shell::GetInstance()
->accelerator_controller();
no space before ->

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler_linux.cc:32: if (accelerator_controller &&
accelerator_controller->
(a &&
 a->Process())

would be more readable.

http://codereview.chromium.org/9224001/diff/7002/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/9224001/diff/7002/ash/ash.gyp#newcode192
ash/ash.gyp:192: }]
did you need this? I thought _linux.cc is included only on linux.

http://codereview.chromium.org/9224001/diff/7002/ui/views/controls/menu/menu_...
File ui/views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/9224001/diff/7002/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_controller.cc:323: loop->RunWithDispatcher(new
ash::AcceleratorHandler(this));
this leaks memory. You can do:

ash::AcceleratorHandler handler(this);
loop->RunWithDispatcher(&handler);

calling ash function from Dispatcher() is another approach (as we discussed),
but I'll leave the final decision to sky.

[pkotwicz, 2012-01-23 19:33:02]

I fixed most of the issues as requested. The others I left comments about.

Sky, which approach do you advocate?

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
File ash/accelerators/accelerator_handler.h (right):

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.h:17: class ASH_EXPORT AcceleratorHandler :
public MessageLoop::Dispatcher {
Oshima, do you have any suggestions for names?

http://codereview.chromium.org/9224001/diff/7002/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/9224001/diff/7002/ash/ash.gyp#newcode192
ash/ash.gyp:192: }]
I think it is necessary. I believe gyp files for major targets have similar
logic which is why _linux.cc ends up being included only on linux. Please
correct me if I am wrong

[pkotwicz, 2012-01-23 19:34:56]

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
File ash/accelerators/accelerator_handler_linux.cc (right):

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler_linux.cc:22: ui::EF_ALT_DOWN);
I put it here to mimic chromium/src/ash/accelerators/accelerator_filter.cc

[oshima, 2012-01-23 21:40:51]

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
File ash/accelerators/accelerator_handler.h (right):

http://codereview.chromium.org/9224001/diff/7002/ash/accelerators/accelerator...
ash/accelerators/accelerator_handler.h:17: class ASH_EXPORT AcceleratorHandler :
public MessageLoop::Dispatcher {
On 2012/01/23 19:33:02, pkotwicz wrote:
> Oshima, do you have any suggestions for names?


AcceleratorDispatcher? Hmm, I'm not good at coming up good name. Scott, WDYT?

http://codereview.chromium.org/9224001/diff/7002/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/9224001/diff/7002/ash/ash.gyp#newcode192
ash/ash.gyp:192: }]
On 2012/01/23 19:33:02, pkotwicz wrote:
> I think it is necessary. I believe gyp files for major targets have similar
> logic which is why _linux.cc ends up being included only on linux. Please
> correct me if I am wrong

http://code.google.com/codesearch#OAMlx_jo-ck/src/build/common.gypi&exact_pac...

common.gypi does exclude _linux for non linux, although we may be overriding it
somewhere. Can you find where?

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
File ash/accelerators/accelerator_handler.cc (right):

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler.cc:11: nested_dispatcher_ =
nested_dispatcher;
: nested_dispatcher_(nested_dispatcher) {
 DCHECK(nested_dispatcher);
}

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler.cc:14: } // namespace ash
two spaces before //

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
File ash/accelerators/accelerator_handler_linux.cc (right):

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler_linux.cc:5: #include <X11/Xlib.h>
move this after accelerator_handler.h but before accelerator_controller.h. Sorry
if it wasn't clear.

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler_linux.cc:23: } // namespace
two spaces before //

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler_linux.cc:38: } // namespace ash
ditto

[sky, 2012-01-23 23:14:34]

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
File ash/accelerators/accelerator_handler.cc (right):

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler.cc:11: nested_dispatcher_ =
nested_dispatcher;
Use member initializer list.

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
File ash/accelerators/accelerator_handler.h (right):

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler.h:22: #if defined(OS_LINUX)
What about windows?

http://codereview.chromium.org/9224001/diff/10006/ash/accelerators/accelerato...
ash/accelerators/accelerator_handler.h:26: private:
newline between 25 and 26.

http://codereview.chromium.org/9224001/diff/10006/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/9224001/diff/10006/ash/ash.gyp#newcode182
ash/ash.gyp:182: 'conditions' : [
Aren't these rules defined in common.gypi.

[pkotwicz, 2012-01-25 18:28:29]

Added windows implementation.
Renamed classes to AcceleratorDispatcher

[oshima, 2012-01-25 18:38:56]

thanks, looks better.

http://codereview.chromium.org/9224001/diff/23007/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher.h (right):

http://codereview.chromium.org/9224001/diff/23007/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher.h:13: 
Please add description what this is for.

http://codereview.chromium.org/9224001/diff/23007/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher.h:14: #if defined(OS_MACOSX)
do you need this now?

http://codereview.chromium.org/9224001/diff/23007/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher.h:18: #endif // defined(OS_MACOSX)
two spaces before //

http://codereview.chromium.org/9224001/diff/23007/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher.h:35: } // namespace ash
two spaces before //

http://codereview.chromium.org/9224001/diff/23007/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_win.cc (right):

http://codereview.chromium.org/9224001/diff/23007/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_win.cc:26: case WM_SYSKEYDOWN:
use extra {} if case block has local variable.

[pkotwicz, 2012-01-25 21:05:23]

Changes as requested.

Will deal with Mac part in separate patch

[oshima, 2012-01-25 21:24:37]

http://codereview.chromium.org/9224001/diff/22007/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher.h (right):

http://codereview.chromium.org/9224001/diff/22007/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher.h:28: XEvent* xev) OVERRIDE;
This doesn't make sense for mac (OS_POSIX is defined for mac as well). I'd
recommend to change this to USE_X11 and remove OS_MACOSX for now.
Just Add TODO for mac port.

http://codereview.chromium.org/9224001/diff/22007/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_win.cc (right):

http://codereview.chromium.org/9224001/diff/22007/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_win.cc:38: DispatchMessage(&msg);
Doesn't this have to be

nested_dispatcer_->Dispatch(msg);

?

[oshima, 2012-01-26 17:02:55]

lgtm with nits. Please ask sky for owners review.

http://codereview.chromium.org/9224001/diff/26002/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher.h (right):

http://codereview.chromium.org/9224001/diff/26002/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher.h:18: 
remove empty line

http://codereview.chromium.org/9224001/diff/26002/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_win.cc (right):

http://codereview.chromium.org/9224001/diff/26002/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_win.cc:24: switch(msg.message) {
I believe we prefer "if" for such case.

[sky, 2012-01-26 18:26:14]

LGTM

http://codereview.chromium.org/9224001/diff/24015/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher.h (right):

http://codereview.chromium.org/9224001/diff/24015/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher.h:17: // TODO(pkotwicz): Port
AcceleratorDispatcher to mac.
Document why and when you use this.

[commit-bot: I haz the power, 2012-01-26 18:34:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/9224001/24015

[commit-bot: I haz the power, 2012-01-26 20:15:44]

Try job failure for 9224001-24015 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[oshima, 2012-01-27 21:41:34]

http://codereview.chromium.org/9224001/diff/2003/ui/aura/client/accelerator_c...
File ui/aura/client/accelerator_client.h (right):

http://codereview.chromium.org/9224001/diff/2003/ui/aura/client/accelerator_c...
ui/aura/client/accelerator_client.h:26: virtual bool Process(const
ui::Accelerator& accelerator) = 0;
Can you pass base::NativeEvent so that menu_controller do not have to deal with
ui::Accelerator?

http://codereview.chromium.org/9224001/diff/2003/ui/views/controls/menu/menu_...
File ui/views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/9224001/diff/2003/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_controller.cc:941: return
base::MessagePumpDispatcher::EVENT_PROCESSED;
I think WM accelerator should be handled before chrome process event. once you
change to use base::NativeEvent, you can simply call

if (accelerator_client && accelerator_client->Process(event))
  return EVENT_PROCESSED;

at the beginning of Dispatch.

[pkotwicz, 2012-01-27 23:43:49]

I think this is what you wanted.

I did not put wm accelerator handling as the first line of Dispatch because:
1) I am not implementing a proper dispatcher.
2) The beginning of the Dispatch method deals with what happens when the menu is
being closed. It makes most sense to handle the wm accelerators as the first
line of processing keyboard events. (Thus we still redirect to the root window
dispatcher if the window is being closed on linux)

[oshima, 2012-01-30 23:04:11]

On 2012/01/27 23:43:49, pkotwicz wrote:
> I think this is what you wanted.
> 
> I did not put wm accelerator handling as the first line of Dispatch because:
> 1) I am not implementing a proper dispatcher.
> 2) The beginning of the Dispatch method deals with what happens when the menu
is
> being closed. It makes most sense to handle the wm accelerators as the first
> line of processing keyboard events. (Thus we still redirect to the root window
> dispatcher if the window is being closed on linux)

Hi,

I found one issue with current scheme, which is that because RunWithDispatcher
can bypass
ash's event processing, menu_controller can see/process event even when screen
is locked.
This makes it mandatory to move the call at the top of Dispatch, but I chatted
with Scott and
we agreed that nested dispatcher client API, like this

aura::client::DispatcherClient::GetInstance()->RunWithDispatcher(this);

would be better and safe. RunWithDispatcher does all pre processing that ash
needs to do first
and dispatch events to this only if ash didn't handle it. Scott, please let us
know if
I got it wrong.

[pkotwicz, 2012-01-31 14:55:39]

Changes as requested.

accelerator_dispatcher*.* is mostly same as used to be except that they handle
the case the screen is locked now.

[oshima, 2012-01-31 18:19:52]

http://codereview.chromium.org/9224001/diff/35001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_linux.cc (right):

http://codereview.chromium.org/9224001/diff/35001/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_linux.cc:29: return EVENT_PROCESSED;
I believe you need to call

return RootWindow::GetDispatcher()->Dispatch(event);

otherwise it will drop all events.

http://codereview.chromium.org/9224001/diff/35001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_win.cc (right):

http://codereview.chromium.org/9224001/diff/35001/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_win.cc:26: return true;
ditto

http://codereview.chromium.org/9224001/diff/35001/ash/accelerators/nested_dis...
File ash/accelerators/nested_dispatcher_controller.cc (right):

http://codereview.chromium.org/9224001/diff/35001/ash/accelerators/nested_dis...
ash/accelerators/nested_dispatcher_controller.cc:16: MessageLoop::Dispatcher*
nested_dispatcher, bool nestable_tasks_allowed) {
one argument per line. (This style has been banned last year)

http://codereview.chromium.org/9224001/diff/35001/ui/aura/client/dispatcher_c...
File ui/aura/client/dispatcher_client.h (right):

http://codereview.chromium.org/9224001/diff/35001/ui/aura/client/dispatcher_c...
ui/aura/client/dispatcher_client.h:18: AURA_EXPORT extern const char
kDispatcherClient[];
I think you don't need to export this. I know some of other header has one, but
they're replaced with Set/Get methods like below and should be removed.

http://codereview.chromium.org/9224001/diff/35001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/9224001/diff/35001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:325: DCHECK(dispatcher_client);
no need for DCHECK as next line fill fail immediately.
DCHECK is useful when NULL reference fails a lot later.

[pkotwicz, 2012-01-31 20:01:07]

http://codereview.chromium.org/9224001/diff/35001/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/9224001/diff/35001/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:325: DCHECK(dispatcher_client);
On 2012/01/31 18:19:52, oshima wrote:
> no need for DCHECK as next line fill fail immediately.
> DCHECK is useful when NULL reference fails a lot later.

Done.

[oshima, 2012-01-31 20:42:00]

lgtm with nits.

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_linux.cc (right):

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_linux.cc:17: #include
"base/message_pump_x.h"
are these two necessary?

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_linux.cc:34: if
(shell->IsScreenLocked())
one liner too. same for win.

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_win.cc (right):

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_win.cc:10: #include
"base/message_pump_win.h"
aren't these two necessary?

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/nested_dis...
File ash/accelerators/nested_dispatcher_controller.h (right):

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/nested_dis...
ash/accelerators/nested_dispatcher_controller.h:21: 
remove empty line.

http://codereview.chromium.org/9224001/diff/38002/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/9224001/diff/38002/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:325:
dispatcher_client->RunWithDispatcher(this, true);
can this be one liner?

[pkotwicz, 2012-02-01 19:11:31]

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/accelerato...
File ash/accelerators/accelerator_dispatcher_linux.cc (right):

http://codereview.chromium.org/9224001/diff/38002/ash/accelerators/accelerato...
ash/accelerators/accelerator_dispatcher_linux.cc:34: if
(shell->IsScreenLocked())
I need to use shell later, so it makes sense to keep it as a two liner here

[pkotwicz, 2012-02-02 15:43:37]

sky for OWNERS

[sky, 2012-02-03 17:23:12]

LGTM

http://codereview.chromium.org/9224001/diff/38004/ash/accelerators/nested_dis...
File ash/accelerators/nested_dispatcher_controller.h (right):

http://codereview.chromium.org/9224001/diff/38004/ash/accelerators/nested_dis...
ash/accelerators/nested_dispatcher_controller.h:27:
DISALLOW_COPY_AND_ASSIGN(NestedDispatcherController);
private: