Enable keyboard accelerators while a menu is open

Enable keyboard accelerators while a menu is open

We're now using a pre-target event handler (the MenuEventFilter) to
handle key events when a menu is present.

BUG=476893, 401690
TEST=views_unittests --gtest_filter=MenuControllerTest.*

Committed: https://crrev.com/692024d9d32cfe3ddc01143243c4b99e43261954
Cr-Commit-Position: refs/heads/master@{#348632}

[afakhry, 2015-05-14 23:30:44]

afakhry@chromium.org changed reviewers:
+ oshima@chromium.org, sadrul@chromium.org

[afakhry, 2015-05-14 23:30:44]

sadrul@ and oshima@: Could you please review this CL?

For simplicity, I haven't removed the old files and classes that are no longer
used. I will run the try bots to see if this solution creates any issues.

Thanks!

[afakhry, 2015-05-15 00:34:07]

On 2015/05/14 23:30:44, afakhry wrote:
> sadrul@ and oshima@: Could you please review this CL?
> 
> For simplicity, I haven't removed the old files and classes that are no longer
> used. I will run the try bots to see if this solution creates any issues.
> 
> Thanks!

The failures are because the MenuEventFilter is created in Shell::Init() and
then it's set for aura::Env also in Shell::Init(). Anything that doesn't use
ash::Shell will fail. Not sure where to put and create MenuEventFilter.

[afakhry, 2015-05-20 18:56:03]

afakhry@chromium.org changed reviewers:
+ sky@chromium.org

[afakhry, 2015-05-20 18:56:04]

Looping in +sky@ for advice and opinion.

Here's a solution that works well, but is different from the current approach:
1 - We no longer use overriden dispatchers or nested message loops.
2 - We use a 'MenuEventFilter' as a pre-target handler to intercept the events
and forward them to the MenuController and handle the accelerators accordingly.
3 - This way, events have been already rewritten and are in their final state.
4 - Currently, MenuControllerTest is failing because the expectations are quite
different now. I can spend some time rewriting these tests but first I need to
make sure this is an acceptable and reasonable solution.

[sky, 2015-05-20 20:01:02]

sky@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[sky, 2015-05-20 20:01:03]

I thought Peter had all this working at one point. What changed?

[pkotwicz, 2015-05-20 20:35:00]

sky@: The "accelerator while a menu is open" logic got broken via many many
things. I don't think it ever worked very well in the first place unfortunately.

afakhry@ I think that sadrul@ has talked to you about using EventTargeter
instead
It should be possible to come up with a solution which:
- Properly does event rewriting
- Sometimes does not redispatch the key event if the accelerator should be
dispatched after the menu is closed as occurs today
- Removes NestedAcceleratorController / NestedAcceleratorDispatcher (though not
NestedAcceleratorDelegate)

[pkotwicz, 2015-05-20 20:35:29]

afakhry@: Feel free to reach out to me if you have any questions

[oshima, 2015-05-20 21:43:07]

On 2015/05/20 20:35:00, pkotwicz wrote:
> sky@: The "accelerator while a menu is open" logic got broken via many many
> things. I don't think it ever worked very well in the first place
unfortunately.
> 
> afakhry@ I think that sadrul@ has talked to you about using EventTargeter
> instead

We feel EventHandler may be cleaner because we just want to steal key events,
and other
events such as mouse/touch should flow as normal case. There is no need to
forward events.

sadrul@, please let us know if you disagree. It shouldn't be hard to switch to
Targeter.

> It should be possible to come up with a solution which:
> - Properly does event rewriting

EventHanders should be getting rewritten keys already.

> - Sometimes does not redispatch the key event if the accelerator should be
> dispatched after the menu is closed as occurs today

AFAIK, this CL eliminates nested message loop, so there is no need to
redispatch. We can simply
close menu (and some may not) and execute accelerators.

> - Removes NestedAcceleratorController / NestedAcceleratorDispatcher (though
not
> NestedAcceleratorDelegate)

These classes should be gone with this approach (or Targeter).

[sadrul, 2015-05-21 18:36:26]

On 2015/05/20 18:56:04, afakhry wrote:
> Looping in +sky@ for advice and opinion.
> 
> Here's a solution that works well, but is different from the current approach:
> 1 - We no longer use overriden dispatchers or nested message loops.

Can you clarify this a bit? I see a nested message-loop running in
MenuMessageLoopAura::Run(), so it's unclear what you mean when you say we no
longer use nested message loops.

[ I am hoping to give more concrete feedback on this later today, but very
briefly: I am thinking it might make sense to add explicit support for
accelerators farther up in the events layer, e.g. at EventSource, EventTarget
etc. instead of trying to use pre-target handlers for this. ]

[afakhry, 2015-05-21 19:01:08]

On 2015/05/21 18:36:26, sadrul wrote:
> On 2015/05/20 18:56:04, afakhry wrote:
> > Looping in +sky@ for advice and opinion.
> > 
> > Here's a solution that works well, but is different from the current
approach:
> > 1 - We no longer use overriden dispatchers or nested message loops.
> 
> Can you clarify this a bit? I see a nested message-loop running in
> MenuMessageLoopAura::Run(), so it's unclear what you mean when you say we no
> longer use nested message loops.
> 
> [ I am hoping to give more concrete feedback on this later today, but very
> briefly: I am thinking it might make sense to add explicit support for
> accelerators farther up in the events layer, e.g. at EventSource, EventTarget
> etc. instead of trying to use pre-target handlers for this. ]

Yes, I meant we're not using the nested dispatcher runloop that we used to have
in order to inject the NestedAcceleratorController and the
NestedAcceleratorDispatcher.

[afakhry, 2015-05-22 18:29:46]

@sadrul, friendly ping. Could you please provide more feedback about this
solution so that I can know if there's hope to make it more appealing to you, or
I should start with a different solution that you will suggest avoiding the
points you didn't like about this one?

[oshima, 2015-05-22 19:24:49]

On 2015/05/21 19:01:08, afakhry wrote:
> On 2015/05/21 18:36:26, sadrul wrote:
> > On 2015/05/20 18:56:04, afakhry wrote:
> > > Looping in +sky@ for advice and opinion.
> > > 
> > > Here's a solution that works well, but is different from the current
> approach:
> > > 1 - We no longer use overriden dispatchers or nested message loops.
> > 
> > Can you clarify this a bit? I see a nested message-loop running in
> > MenuMessageLoopAura::Run(), so it's unclear what you mean when you say we no
> > longer use nested message loops.
> > 
> > [ I am hoping to give more concrete feedback on this later today, but very
> > briefly: I am thinking it might make sense to add explicit support for
> > accelerators farther up in the events layer, e.g. at EventSource,
EventTarget
> > etc. instead of trying to use pre-target handlers for this. ]
> 
> Yes, I meant we're not using the nested dispatcher runloop that we used to
have
> in order to inject the NestedAcceleratorController and the
> NestedAcceleratorDispatcher.

Ok, your comment 1 confused me because "runloop" in menu_message_loop_aura.cc is
the nested message loop.
I wonder if we still need nested loop though, given that we now have event
targeter/handlers. I'm probably missing something important? 

sadrul@, can you explain why you don't like pre-target handlers? I should be
possible to use targeter,
but it'd requires more plumbing than just using pre-target handlers.

Regarding where we should handle accelerators, one caveat is that there are
accelerators that are handled only when content didn't consume, which you
probably have to take into consideration.

[sky, 2015-05-22 19:41:32]

On Fri, May 22, 2015 at 12:24 PM,  <oshima@chromium.org> wrote:
> On 2015/05/21 19:01:08, afakhry wrote:
>>
>> On 2015/05/21 18:36:26, sadrul wrote:
>> > On 2015/05/20 18:56:04, afakhry wrote:
>> > > Looping in +sky@ for advice and opinion.
>> > >
>> > > Here's a solution that works well, but is different from the current
>> approach:
>> > > 1 - We no longer use overriden dispatchers or nested message loops.
>> >
>> > Can you clarify this a bit? I see a nested message-loop running in
>> > MenuMessageLoopAura::Run(), so it's unclear what you mean when you say
>> > we no
>> > longer use nested message loops.
>> >
>> > [ I am hoping to give more concrete feedback on this later today, but
>> > very
>> > briefly: I am thinking it might make sense to add explicit support for
>> > accelerators farther up in the events layer, e.g. at EventSource,
>
> EventTarget
>>
>> > etc. instead of trying to use pre-target handlers for this. ]
>
>
>> Yes, I meant we're not using the nested dispatcher runloop that we used to
>
> have
>>
>> in order to inject the NestedAcceleratorController and the
>> NestedAcceleratorDispatcher.
>
>
> Ok, your comment 1 confused me because "runloop" in
> menu_message_loop_aura.cc is
> the nested message loop.
> I wonder if we still need nested loop though, given that we now have event
> targeter/handlers. I'm probably missing something important?

All call sites are currently expecting the nested message loop.
Converting away from that would be nice, but it'll be a lot of work.

  -Scott

>
> sadrul@, can you explain why you don't like pre-target handlers? I should be
> possible to use targeter,
> but it'd requires more plumbing than just using pre-target handlers.
>
> Regarding where we should handle accelerators, one caveat is that there are
> accelerators that are handled only when content didn't consume, which you
> probably have to take into consideration.
>
>
> https://codereview.chromium.org/1138523006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-05-22 19:42:47]

On Fri, May 22, 2015 at 12:41 PM, Scott Violet <sky@chromium.org> wrote:
> On Fri, May 22, 2015 at 12:24 PM,  <oshima@chromium.org> wrote:
>> On 2015/05/21 19:01:08, afakhry wrote:
>>>
>>> On 2015/05/21 18:36:26, sadrul wrote:
>>> > On 2015/05/20 18:56:04, afakhry wrote:
>>> > > Looping in +sky@ for advice and opinion.
>>> > >
>>> > > Here's a solution that works well, but is different from the current
>>> approach:
>>> > > 1 - We no longer use overriden dispatchers or nested message loops.
>>> >
>>> > Can you clarify this a bit? I see a nested message-loop running in
>>> > MenuMessageLoopAura::Run(), so it's unclear what you mean when you say
>>> > we no
>>> > longer use nested message loops.
>>> >
>>> > [ I am hoping to give more concrete feedback on this later today, but
>>> > very
>>> > briefly: I am thinking it might make sense to add explicit support for
>>> > accelerators farther up in the events layer, e.g. at EventSource,
>>
>> EventTarget
>>>
>>> > etc. instead of trying to use pre-target handlers for this. ]
>>
>>
>>> Yes, I meant we're not using the nested dispatcher runloop that we used to
>>
>> have
>>>
>>> in order to inject the NestedAcceleratorController and the
>>> NestedAcceleratorDispatcher.
>>
>>
>> Ok, your comment 1 confused me because "runloop" in
>> menu_message_loop_aura.cc is
>> the nested message loop.
>> I wonder if we still need nested loop though, given that we now have event
>> targeter/handlers. I'm probably missing something important?
>
> All call sites are currently expecting the nested message loop.
> Converting away from that would be nice, but it'll be a lot of work.

Also note that I don't think event handlers are the like are currently
rich enough for all that the nested message loop does. I believe we
would need to change around MessageLoop (probably the pump) to expose
the functionality menus need.

  -Scott
>
>   -Scott
>
>>
>> sadrul@, can you explain why you don't like pre-target handlers? I should be
>> possible to use targeter,
>> but it'd requires more plumbing than just using pre-target handlers.
>>
>> Regarding where we should handle accelerators, one caveat is that there are
>> accelerators that are handled only when content didn't consume, which you
>> probably have to take into consideration.
>>
>>
>> https://codereview.chromium.org/1138523006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2015-05-22 23:02:33]

I tried the CL. It seems as if loses some of the functionality of
menu_message_pump_dispatcher_win.cc

For instance: on Windows pressing <F10> while the menu is open
- Closes the menu when the CL is not applied
- Does not do anything when the CL is applied

[pkotwicz, 2015-05-22 23:13:01]

I think that it should be possible to come up with a solution which uses both
MenuMessagePumpDispatcher (menu_message_pump_dispatcher_win.*) and
MenuEventFilter (I do not know how you are going to handle WM_CONTEXTMENU
without MenuMessagePumpDispatcher)

[afakhry, 2015-05-22 23:36:58]

On 2015/05/22 23:13:01, pkotwicz wrote:
> I think that it should be possible to come up with a solution which uses both
> MenuMessagePumpDispatcher (menu_message_pump_dispatcher_win.*) and
> MenuEventFilter (I do not know how you are going to handle WM_CONTEXTMENU
> without MenuMessagePumpDispatcher)

@pkotwicz: Thanks for trying this on Windows as I'm doing my testing here on
ChromeOS and rely on the bots to tell me if something went wrong on other
platforms.
I can't see a ui::EventType that represents WM_CONTEXTMENU, is there one that
I'm not seeing?

[pkotwicz, 2015-05-23 00:02:48]

There isn't a ui::EventType for WM_CONTEXTMENU as far as I know.

[pkotwicz, 2015-05-23 00:12:36]

There is unfortunately very little test coverage for menu stuff

[sky, 2015-05-23 16:10:21]

That isn't quite true. There is a slew of tests covering menu
functionality in chrome under the bookmarks code. Unfortunately they
are interactive ui tests.

  -Scott

On Fri, May 22, 2015 at 5:12 PM,  <pkotwicz@chromium.org> wrote:
> There is unfortunately very little test coverage for menu stuff
>
> https://codereview.chromium.org/1138523006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2015-05-26 16:53:45]

[snip]
> 
> sadrul@, can you explain why you don't like pre-target handlers? I should be
> possible to use targeter,
> but it'd requires more plumbing than just using pre-target handlers.

I thought about this a bit more, and I think the proposal I had for using
event-targeters wouldn't be particularly useful (the targeter can redirect the
events to the right widget, but in the process of the Widget receiving that
event, the pre-target handlers on Shell, including the accelerator-handler, will
have triggered, and so the Widget wouldn't be the first to handle the event). So
for the short-term, I think using a pre-target handler would be reasonable.
(although as I suggested to afakhry@ offline, we shouldn't still need to add API
on aura::Env for this).

For the long term, I still think we need to find a good solution for
accelerators. We use pre-target handlers for too many things, but it would make
more sense to have more explicit API for some of these functionalities (IME
being another example of this).

> 
> Regarding where we should handle accelerators, one caveat is that there are
> accelerators that are handled only when content didn't consume, which you
> probably have to take into consideration.

That shouldn't necessarily be a problem. We have some support for async handling
of events (e.g. touch events can also be used for gesture-event generation after
async response from a renderer).

[afakhry, 2015-06-11 21:28:19]

This is a fully working solution (finally!) :)

sadrul: Please review the following files:-
  ui/views/controls/menu/menu_controller.h
  ui/views/controls/menu/menu_controller_unittest.cc
  ui/views/controls/menu/menu_event_filter.cc
  ui/views/controls/menu/menu_event_filter.h
  ui/views/controls/menu/menu_message_loop_aura.cc
  ui/views/controls/menu/menu_message_loop_aura.h
  ui/views/controls/menu/menu_message_pump_dispatcher_win.cc
  ui/views/views.gyp
oshima: Please review the following files:-
  ash/accelerators/accelerator_controller.cc
  ash/accelerators/accelerator_controller.h
  ash/accelerators/accelerator_table.cc
  ash/accelerators/accelerator_table.h
  ash/accelerators/ash_menu_event_filter_delegate.cc
  ash/accelerators/ash_menu_event_filter_delegate.h
  ash/shell.cc
  ash/shell.h

[oshima, 2015-06-12 19:59:19]

m

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:69: menu_item_(),
you can skip these

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:83:
menu_controller_->showing_ = false;
do you need these?

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:85: menu_controller_ =
nullptr;
any reason why we shouldn't use scoped_ptr for this as well?

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:100: // With the NULL
targeter instantiated and assigned we expect the menu to
nullptr

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:39: // be done properly.
can you be more specific about the reason? (If it'll contain too much ash
details, then I'm fine)

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:43: // it, it will either pass it to
the accelerator controller to be processed
"accelerator controller" is ash specific. you can just say "it will either be
processed now, or"

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:44: // now (and returns true - the
event's propagation should be stopped), or
s/should/will/ ?

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:52:
DISALLOW_COPY_AND_ASSIGN(Delegate);
you don't need this for pure interface.

[afakhry, 2015-06-12 23:50:47]

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:69: menu_item_(),
On 2015/06/12 19:59:18, oshima wrote:
> you can skip these

I know but I always like to be explicit .. however I removed them in this test.

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:83:
menu_controller_->showing_ = false;
On 2015/06/12 19:59:18, oshima wrote:
> do you need these?

Yes the destructor will crash on a DCHECK() if |showing| is not set to false
previously.

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:85: menu_controller_ =
nullptr;
On 2015/06/12 19:59:18, oshima wrote:
> any reason why we shouldn't use scoped_ptr for this as well?

Done. Using a scoped_ptr with a specialized Deleter as discussed offline.

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:100: // With the NULL
targeter instantiated and assigned we expect the menu to
On 2015/06/12 19:59:18, oshima wrote:
> nullptr

Actually NULL here refers to the ui::NullEventTargeter.
I updated the comment.

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:39: // be done properly.
On 2015/06/12 19:59:18, oshima wrote:
> can you be more specific about the reason? (If it'll contain too much ash
> details, then I'm fine)

Actually this is specific to ash::AcceleratorController which needs to know the
previous accelerator in some cases to be able to process the current
accelerator.

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:43: // it, it will either pass it to
the accelerator controller to be processed
On 2015/06/12 19:59:18, oshima wrote:
> "accelerator controller" is ash specific. you can just say "it will either be
> processed now, or"

Done.

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:44: // now (and returns true - the
event's propagation should be stopped), or
On 2015/06/12 19:59:19, oshima wrote:
> s/should/will/ ?

It "will". Because the MenuEventFilter will check the return value of
Delegate::ProcessAccelerator(), if true, it will stop the even't propagation.

Done.

https://codereview.chromium.org/1138523006/diff/220001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:52:
DISALLOW_COPY_AND_ASSIGN(Delegate);
On 2015/06/12 19:59:19, oshima wrote:
> you don't need this for pure interface.

Done.

[pkotwicz, 2015-06-16 16:04:59]

A few drive by comments

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:89: if (event->type() !=
ui::ET_KEY_PRESSED || (flags & kKeyFlagsMask) != 0) {
There's a bunch of subtle differences between
MenuEventDispatcher::DispatchEvent() and MenuEventFilter::OnKeyEvent().
For instance:
- MenuEventFilter::OnKeyEvent() only calls MenuController::SelectByChar() if
there are no ui::EventFlags
- MenuEventDispatcher::DispatchEvent() calls MenuController::OnKeyDown() even if
Ctrl is pressed but MenuEventFilter does not.
- The return values of MenuController::OnKeyDown() and
MenuController::SelectByChar() affect whether
MenuController::TerminateNestedMessageLoop() gets called in
MenuEventDispatcher::DispatchEvent() but not in
MenuEventDispatcher::DispatchEvent()

I think that all of the subtle changes that you are making are correct. However,
it would be great if you can put these changes in a different CL which you land
before you land this CL.
I think (but am not sure) that MenuController::OnKeyDown() does not need a
return value. (This would be an awesome simplification)

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:106: event->StopPropagation();
Mnemonics are only run when base::RunLoop has finished. I wonder whether we
should do the same for accelerators.

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
It would be nice if MenuEventFilter handled as many events as possible (all of
the events except WM_CONTEXTMENU).
This would make the code more readable in my opinion because the event handling
in MenuMessagePumpDispatcher and MenuEventFilter would be mutually exclusive
(Yes, this would require #ifdefs but that's ok in my opinion)

[afakhry, 2015-06-16 23:56:40]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:89: if (event->type() !=
ui::ET_KEY_PRESSED || (flags & kKeyFlagsMask) != 0) {
On 2015/06/16 16:04:59, pkotwicz wrote:
> There's a bunch of subtle differences between
> MenuEventDispatcher::DispatchEvent() and MenuEventFilter::OnKeyEvent().
> For instance:
> - MenuEventFilter::OnKeyEvent() only calls MenuController::SelectByChar() if
> there are no ui::EventFlags

Right, in the old code, we call MenuController::SelectByChar only if the flags
show that Alt and Ctrl are not pressed. I added the shift and command flags too
because when any is pressed we need to handle a potential accelerator and
otherwise handle MenuController::SelectByChar.


> - MenuEventDispatcher::DispatchEvent() calls MenuController::OnKeyDown() even
if
> Ctrl is pressed but MenuEventFilter does not.

Right, MenuController::OnKeyDown() expect certain key codes that it will handle
and no flags are expected. For example, why should pressing the RIGHT key be
equivalent to pressing Ctrl+RIGHT. It doesn't make sense.

> - The return values of MenuController::OnKeyDown() and
> MenuController::SelectByChar() affect whether
> MenuController::TerminateNestedMessageLoop() gets called in
> MenuEventDispatcher::DispatchEvent() but not in
> MenuEventDispatcher::DispatchEvent()

Calling MenuController::OnKeyDown() or MenuController::SelectByChar() will
result in changing the value of MenuController::exit_type() (if the menu needs
to exit in correspondence with the return values of these methods). This will
have same effect since we already have the following code below:

if (menu_controller->exit_type() != MenuController::EXIT_NONE)
  menu_controller->TerminateNestedMessageLoop();

> 
> I think that all of the subtle changes that you are making are correct.
However,
> it would be great if you can put these changes in a different CL which you
land
> before you land this CL.

I'm not sure why that would be more preferred here, but I'm open to whatever is
necessary. Let's see what other reviewers will say.

> I think (but am not sure) that MenuController::OnKeyDown() does not need a
> return value. (This would be an awesome simplification)

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:106: event->StopPropagation();
On 2015/06/16 16:04:59, pkotwicz wrote:
> Mnemonics are only run when base::RunLoop has finished. I wonder whether we
> should do the same for accelerators.

That's exactly what we're doing here, right? Take a look at
AshMenuEventEventFilterDelegate::ProcessAccelerator(). If the accelerator is not
one of those that are allowed to keep the menu open while being processed, the
menu will be closed (which will result in the termination of the nested message
loop) and then the event will be allowed to continue its propagation, and the
accelerator will be processed normally by the AcceleratorController outside of
the menu message loop.

To test this you can apply the patch, add some logging statements, build and
run, open a menu and then press Ctrl+Alt+/ to open the keyboard overlay, which
is not one of the accelerators that will keep the menu open. See that it will be
processed only after the menu has been closed.

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
On 2015/06/16 16:04:59, pkotwicz wrote:
> It would be nice if MenuEventFilter handled as many events as possible (all of
> the events except WM_CONTEXTMENU).
> This would make the code more readable in my opinion because the event
handling
> in MenuMessagePumpDispatcher and MenuEventFilter would be mutually exclusive
> (Yes, this would require #ifdefs but that's ok in my opinion)

That would be my preference too but we can't do that unfortunately.
1- We must handle WM_KEYDOWN here explicitly so that we can call
DispatchMessage() only (Which will make the event reach the
MenuEventFilter::OnKeyEvent() without calling TranslateMessage(). Otherwise
TranslateMessage() will be called by MessagePumpForUI::ProcessMessageHelper()
here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
Which will result in the generation of WM_CHAR. WM_CHAR events take a very
different path than normally-dispatched WM_KEYDOWN events. See 
HWNDMessageHandler::OnImeMessages() at:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/win/hwnd_...

This path will never hit the MenuEventFilter::OnKeyEvent() so handling these
events there will never happen.
If we allow the genration of WM_CHAR, the above path will lead to inserting a
character in the omnibox or any enabled textfiled.

2- We must handle WM_CHAR explicitly here so that we can set
should_perform_default = false for the same above reason.

3- As for WM_CANCELMODE, It's similar to WM_CONTEXTMENU, it seems that it's not
represented in the ui::Event stack.

[pkotwicz, 2015-06-17 14:26:24]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:89: if (event->type() !=
ui::ET_KEY_PRESSED || (flags & kKeyFlagsMask) != 0) {
I prefer making these subtle changes in a different CL because:
- It makes the job of reviewing this CL much easier. It is much simpler if
MenuEventFilter can be assumed to be a trivial refactor rather one with really
subtle differences. (It took me a while to notice them)
- It is possible to take the subtle changes that you are making in this CL to
completion in a separate CL. For instance, it does not make sense for
MenuController::OnKeyDown() to have a return value if the return value is
redundant and can be inferred from MenuController::exit_type().

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:106: event->StopPropagation();
Sorry that I was unclear.
In your CL, if a user presses an accelerator which closes the wrench menu:
- The menu closes
- The accelerator action is run
- The base::RunLoop ends sometime in the future.
MenuController::TerminateNestedMessageLoop() asynchronously ends the
base::RunLoop. If a user presses an accelerator while dragging a bookmark from
the wrench menu, it is possible that the base::RunLoop keeps on running for
minutes after MenuController::TerminateNestedMessageLoop() has been called.

If a user presses a mnemonic while the wrench menu is open:
- The menu closes
- The base::RunLoop ends sometime in the future
- The mnemonic action is run only once the base::RunLoop has ended (It is run in
MenuRunnerImpl::MenuDone())

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
I was unaware of the special handling for WM_CHAR. Thanks for pointing it out.

Pardon my ignorance. Why is calling TranslateMessage() as a result of WM_KEYDOWN
bad? The old code was calling TranslateMessage() as a result of WM_KEYDOWN
without any negative effects. I also applied your CL locally and made the change
and did not see negative effects. (I do understand that you need to return
POST_DISPATCH_NONE as a result of WM_CHAR events)

The ET_CANCEL_MODE event is dispatched as a result of WM_CANCELMODE

[afakhry, 2015-06-17 18:48:00]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:106: event->StopPropagation();
On 2015/06/17 14:26:24, pkotwicz wrote:
> Sorry that I was unclear.
> In your CL, if a user presses an accelerator which closes the wrench menu:
> - The menu closes
> - The accelerator action is run
> - The base::RunLoop ends sometime in the future.
> MenuController::TerminateNestedMessageLoop() asynchronously ends the
> base::RunLoop. If a user presses an accelerator while dragging a bookmark from
> the wrench menu, it is possible that the base::RunLoop keeps on running for
> minutes after MenuController::TerminateNestedMessageLoop() has been called.
> 
> If a user presses a mnemonic while the wrench menu is open:
> - The menu closes
> - The base::RunLoop ends sometime in the future
> - The mnemonic action is run only once the base::RunLoop has ended (It is run
in
> MenuRunnerImpl::MenuDone())

Thanks for clarifying. I understand what you mean now. In both situations
(mnemonics and accelerators) The

-> RunLoop::Quit();
   |
   --> MessageLopp::QuitNow();

are both executed before we handle either the mnemonic or accelerator. However,
in the case of mnemonics, it is executed as you mentioned in
MenuRunnerImpl::MenuDone() after:

-> RunLoop::AfterRun();

Which of course pops the previous run loop from the stack.

I tried the example you gave earlier (dragging a bookmark) and didn't see any
issues and validated the the runloops quit normally.

Whether processing the accelerators after RunLoop::AfterRun() has completed is
the right thing to do, I'm not sure, since I'm not seeing any issues, but
whether it's doable, yes it's very easy. So if you could explain this a bit more
so that I can understand why we should do it, that's be great.

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
On 2015/06/17 14:26:24, pkotwicz wrote:
> I was unaware of the special handling for WM_CHAR. Thanks for pointing it out.
> 
> Pardon my ignorance. Why is calling TranslateMessage() as a result of
WM_KEYDOWN
> bad? The old code was calling TranslateMessage() as a result of WM_KEYDOWN
> without any negative effects. I also applied your CL locally and made the
change
> and did not see negative effects. (I do understand that you need to return
> POST_DISPATCH_NONE as a result of WM_CHAR events)
> 
> The ET_CANCEL_MODE event is dispatched as a result of WM_CANCELMODE

If you want to see the negative effect I was talking about, try
TranslateMessage() before DispatchMessage() in WM_KEYDOWN. Open the wrench menu,
press the 'T' key (for new tab) see what happens. A new tab is opened, and the
letter 't' shows up in the omnibox. Clearly that's not what we want.

The old code needed to call TranslateMessage() as it was handling the selection
by mnemonics (MenuController::SelectByChar()) at WM_CHAR. In this CL however, at
WM_KEYDOWN we instead just DispatchMessage() which will lead to the event being
handled by MenuEventFilter::OnKeyEvent() which handles both
MenuController::OnKeyDown() and MenuController::SelectByChar().

[pkotwicz, 2015-06-18 19:45:57]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:106: event->StopPropagation();
I am unsure what the right thing to do is. It would be useful to understand why
the mnemonics processing is the way it is. The scary prospect is than an
accelerator starts a base::RunLoop (I do not know of any which do). Accelerators
do pretty complex things (log out of Chrome OS, open a new window)

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
I tried it again. I get the same behavior that you do but only if I call
TranslateMessage() more than once as a result of WM_KEYDOWN (e.g. 2, 5, 10
times). I suspect that you were calling TranslateMessage() more than once.

I do not know why the behavior is different when TranslateMessage() is called
multiple times. It would be good to find out.

[afakhry, 2015-07-08 20:06:18]

I have renewed my commitment to get this solution done. I'm now handling
accelerators that should close the menu only after the nested message loop
terminates and exits. 

Please take a look! Thanks!

[pkotwicz, 2015-07-09 14:49:09]

I will take  a look tonight

[pkotwicz, 2015-07-10 17:33:21]

sadrul@ and sky@ what do you think about putting subtle changes w.r.t to
MenuEventDispatcher in a separate CL? For instance, this CL relies on
MenuController::SelectByChar() and MenuController::OnKeyDown() to close the menu
instead of having MenuEventDispatcher/MenuEventFilter explicitly call
MenuController::TerminateNestedMessageLoop(). I think that it is possible to
remove the return values of MenuController::SelectByChar() and
MenuController::OnKeyDown() completely. I think that this would be an awesome
simplification.

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:89: if (event->type() !=
ui::ET_KEY_PRESSED || (flags & kKeyFlagsMask) != 0) {
I still think that making these changes in a separate CL is useful.

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
Have you addressed this comment?

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:34: bool ProcessAccelerator(const
ui::Accelerator& accelerator) override {
A boolean return value is confusing. I think that we should have an enum return
value similar to ui::wm::NestedAcceleratorDelegate::Result

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:46: MenuEventFilter::Delegate*
delegate) {
The task might be run prior to the message loop finishing. I tried pressing an
accelerator while dragging a bookmark from the "Bookmarks" submenu

Example of weird behavior while pressing an accelerator while a drag and drop is
in progress: Start a drag and drop operation from the bookmark menu. Hover over
the bookmarks menu as if you would reorder the bookmark. Press <Ctrl>+<N>
accelerator to open a new window. The wrench menu does not close. A new window
is opened in between the wrench menu's host browser window and the wrench menu.

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:108: if (event->type() !=
ui::ET_KEY_PRESSED || (flags & kKeyFlagsMask) != 0) {
This causes a change in behavior. If I have a bookmark whose name starts with
"+", pressing <Shift>+<-> will open the bookmark. MenuController::SelectByChar()
does this. This CL make this not work anymore

[afakhry, 2015-07-10 23:49:47]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
On 2015/07/10 17:33:21, pkotwicz wrote:
> Have you addressed this comment?

Yes, I spent the whole day yesterday testing on Windows. As I described above,
in this patch, if we call TranslateMessage() before DispatchMessage() in
WM_KEYDOWN this will result in the insertion of mnemonic characters in the
omnibox if it is in focus. I involved oshima@ in the tests yesterday and we made
an experiment that led to filing this bug
https://code.google.com/p/chromium/issues/detail?id=508668

We also made another experiment (hack actually) if you insist on calling
TranslateMessage(). We will keep a MSG msg_; as a member here. Then the below
cases for WM_KEYDOWN and WM_CHAR become as the following:

case WM_KEYDOWN: {                                                      
  ui::KeyboardCode key_code = ui::KeyboardCodeFromNative(msg);
  if (key_code >= ui::VKEY_0 && key_code <= ui::VKEY_Z) {
    // Alpha-numeric key (mnemonic) should be handled at WM_CHAR.
    // Cache the msg for later and translate.
    msg_ = msg;
    TranslateMessage(&msg);
  } else {
    // Dispatch now to be handled now by MenuEventFilter.
    DispatchMessage(&msg);
  }
       
  should_perform_default = false;                                       
  break;                                                                
}                                                                       
case WM_CHAR: {                                                         
  // Dispatch the cached msg_.
  DispatchMessage(&msg_); 
                                     
  should_perform_default = false;                                       
  break;                                                                
}         

The above is a hack as you can see but it works. I'm not saying we should commit
such code, all I'm trying to do is to make you aware of the issue.

Let me know what you think. We can also request review from someone who's an
expert on Windows events handling.

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:34: bool ProcessAccelerator(const
ui::Accelerator& accelerator) override {
On 2015/07/10 17:33:21, pkotwicz wrote:
> A boolean return value is confusing. I think that we should have an enum
return
> value similar to ui::wm::NestedAcceleratorDelegate::Result

Done.

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:46: MenuEventFilter::Delegate*
delegate) {
On 2015/07/10 17:33:21, pkotwicz wrote:
> The task might be run prior to the message loop finishing. I tried pressing an
> accelerator while dragging a bookmark from the "Bookmarks" submenu
> 
> Example of weird behavior while pressing an accelerator while a drag and drop
is
> in progress: Start a drag and drop operation from the bookmark menu. Hover
over
> the bookmarks menu as if you would reorder the bookmark. Press <Ctrl>+<N>
> accelerator to open a new window. The wrench menu does not close. A new window
> is opened in between the wrench menu's host browser window and the wrench
menu.

Try it again now. I actually forgot to call menu_controller->CancelAll(); when
the accelerator is determined to be for processing later. This happened after I
changed the meaning of the return value of Delegate::ProcessAccelerator() from
patch set 13 to patch set 15.

I fixed now. Thanks for checking.

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:108: if (event->type() !=
ui::ET_KEY_PRESSED || (flags & kKeyFlagsMask) != 0) {
On 2015/07/10 17:33:21, pkotwicz wrote:
> This causes a change in behavior. If I have a bookmark whose name starts with
> "+", pressing <Shift>+<-> will open the bookmark.
MenuController::SelectByChar()
> does this. This CL make this not work anymore

Thanks for pointing that out! I excluded the SHIFT_DOWN from the flags mask.

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:113: if
(menu_controller->OnKeyDown(event->key_code())) {
By the way the return value of MenuController::OnKeyDown() is convenient to know
whether or not to check for mnemonics. Otherwise I will have to do something
like this without it.

menu_controller->OnKeyDown(event->key_code());
if (menu_controller->exit_type() != MenuController::EXIT_NONE) {
  // Do the mnemonic handling here.
}

So if there's a consensus that we want to get rid of the return value, I'm fine
with creating a separate CL for this.

[pkotwicz, 2015-07-13 23:29:18]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
Thank you for pointing me to the bug and for the explanation. I now understand
your point.

However, this CL causes a regression. Currently, on Windows and Windows only, if
I switch to using a French keyboard, bookmark www.google.com, set the bookmark's
name to "étrange", if I type "é" while the "Bookmarks" menu is open, Chrome
navigates to the bookmarked URL. This CL breaks this feature. It would be nice
to know if this feature is intentional or not. (I suspect that it is)

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:46: MenuEventFilter::Delegate*
delegate) {
I tried it again. It is still possible for the accelerator to be run prior to
the message loop finishing (MenuRunnerImpl::MenuDone()) if the user is dragging
a bookmark when they press the accelerator. My understanding is that the purpose
of reposting the accelerator was to execute the accelerator AFTER the
base::RunLoop ends.

[afakhry, 2015-07-14 20:50:44]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
On 2015/07/13 23:29:17, pkotwicz wrote:
> Thank you for pointing me to the bug and for the explanation. I now understand
> your point.
> 
> However, this CL causes a regression. Currently, on Windows and Windows only,
if
> I switch to using a French keyboard, bookmark http://www.google.com, set the
bookmark's
> name to "étrange", if I type "é" while the "Bookmarks" menu is open, Chrome
> navigates to the bookmarked URL. This CL breaks this feature. It would be nice
> to know if this feature is intentional or not. (I suspect that it is)

I will try to investigate this later on Windows. I don't have a french keyboard
nor a Windows workstation and the only way for me to test this on Windows is on
my personal laptop, and you can imagine how frustrating and time consuming this
is. My guess is that we need to handle mnemnoics at WM_CHAR (rather than
WM_KEYDOWN) so that the character is in its final state (i.e. 'e' with an accent
rather than just 'e'). Maybe the hacky solution that I sent you above that
caches the MSG for later could fix this issue. Could you please try it if you
can? Thanks!

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:46: MenuEventFilter::Delegate*
delegate) {
On 2015/07/13 23:29:18, pkotwicz wrote:
> I tried it again. It is still possible for the accelerator to be run prior to
> the message loop finishing (MenuRunnerImpl::MenuDone()) if the user is
dragging
> a bookmark when they press the accelerator. My understanding is that the
purpose
> of reposting the accelerator was to execute the accelerator AFTER the
> base::RunLoop ends.

I did some further investigations and found out that while drag and drop is in
progress, the runloop that kept running the menu is not the nested message loop
in the menu controller, but rather the one in the DragDropClient (i.e.
ash::DragDropController). So I added code to cancel the drag and drop (if any)
whenever we determine that we need to repost the accelerator for processing
later. Please take a look. Now
ash::AshMenuEventFilterDelegate::ProcessAcceleratorNow() will always run after
MenuRunnerImpl::MenuDone(). If you have alternative solutions, please specify.

[pkotwicz, 2015-07-15 00:37:15]

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_pump_dispatcher_win.cc (right):

https://codereview.chromium.org/1138523006/diff/240001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_pump_dispatcher_win.cc:23: uint32_t
MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
The hacky solution does not work. The "é" key does not have a keycode between
ui::VKEY_0 and ui::VKEY_Z. This is what a French Canadian Keyboard looks like if
you are interested
http://ergocanada.com/products/keyboards/sk595_cdn_french_layout.gif

Caching the MSG does not sound like a good idea. The wParam attribute of the msg
struct is different in the WM_KEYDOWN and WM_CHAR messages.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646280(v=vs.85).aspx
vs
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646276(v=vs.85).aspx

Using your personal laptop to build Chrome sounds less than ideal. I do not have
a Windows machine either. The Waterloo office has an off-corp shared z600
Windows machine. It has served me well.

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:46: MenuEventFilter::Delegate*
delegate) {
I am not much of a fan of cancelling the drag. It is possible that there will be
some new functionality in the future which starts a base::RunLoop while the menu
is open. I would suggest checking whether MenuController::GetActiveInstance() is
null and either:
(1) Not running the accelerator if MenuController::GetActiveInstance() is
non-null
(2) Posting a delayed task if MenuController::GetActiveInstance() is non-null
and not run the accelerator if MenuController::GetActiveInstance() is still
non-null at that time.
I got approval from oshima@ to use this technique in
https://docs.google.com/document/d/1u_YnDf4cgNXl5BcSITiXuVP-iOM-Cy7-_yAypDjCq...

Apparently some people want to be able to use accelerators while doing a drag
and drop operation so we cannot drop all keys events during all drag and drop
operations https://crbug.com/376761

[afakhry, 2015-07-16 00:29:27]

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:46: MenuEventFilter::Delegate*
delegate) {
On 2015/07/15 00:37:14, pkotwicz wrote:
> I am not much of a fan of cancelling the drag. It is possible that there will
be
> some new functionality in the future which starts a base::RunLoop while the
menu
> is open. I would suggest checking whether MenuController::GetActiveInstance()
is
> null and either:
> (1) Not running the accelerator if MenuController::GetActiveInstance() is
> non-null
> (2) Posting a delayed task if MenuController::GetActiveInstance() is non-null
> and not run the accelerator if MenuController::GetActiveInstance() is still
> non-null at that time.
> I got approval from oshima@ to use this technique in
>
https://docs.google.com/document/d/1u_YnDf4cgNXl5BcSITiXuVP-iOM-Cy7-_yAypDjCq...
> 
> Apparently some people want to be able to use accelerators while doing a drag
> and drop operation so we cannot drop all keys events during all drag and drop
> operations https://crbug.com/376761

Ok, so there are two conflicting goals here:
1- Allowing accelerators while dragging a menu item.
2- Executing the allowed accelerators only after MenuRunnerImpl::MenuDone()
while drag and drop is in progress.

MenuRunnerImpl::MenuDone() will never be called as long as drag-n-drop is still
in progress (since its Runloop is still running
https://code.google.com/p/chromium/codesearch#chromium/src/ash/drag_drop/drag...).
This is actually the current behavior with or without this solution, the
accelerator will be executed before MenuRunnerImpl::MenuDone() is called. This
patch doesn't change this behavior, in both cases the menu closes, the
accelerator is executed, then MenuRunnerImpl::MenuDone() is called only when the
user let go of the mouse button (i.e. the drag-drop is finished).

If we want to guarantee that the accelerators are never executed until
MenuDone() is called, then we have to cancel the drag-n-drop as I did in this
patch, otherwise, if we want to keep drag and drop running until the user
actually cancels it and allow accelerators at the same time, then we have to
accept Accelerator execution before MenuDone().

WDYT? and oshima@ too, WDYT?

[pkotwicz, 2015-07-16 01:10:57]

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:46: MenuEventFilter::Delegate*
delegate) {
It would be interesting to find out whether "Executing the allowed accelerators
only after MenuRunnerImpl::MenuDone()" matters. sky@ would be the best person to
answer this question. The implementation that we have for mnemonics seems to
indicate that it might matter

Personally, I would be hesitant to assume that "the only base::RunLoop which can
be started when a menu is open is for a drag and drop operation." We use
base::RunLoops for counter-intuitive things like getting data from the system
clipboard on Desktop Linux (I don't think that it is possible for a paste
operation to start before the menu closes)

[afakhry, 2015-08-24 19:11:41]

Hey pkotwicz, Could you please take another look at this CL? I updated it taking
into account your recent changes.

https://codereview.chromium.org/1138523006/diff/340001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/340001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:147: client->DragCancel();
Please let me know if I should keep this code to cancel any in-progress drag and
drop when the menu should exit.

Remember, I added this previously when I determined that the reason why (while
drag-n-drop is in progress) the accelerator is handled prior to
MenuRunnerImpl::MenuDone() is not because the nested message loop of the menu
hasn't terminated, but rather because it's run from within the drag-n-drop
nested message loop, which hasn't exited.

Also note that this is the current behavior without this patch.

[afakhry, 2015-08-25 00:18:35]

Patchset #19 (id:360001) has been deleted

[pkotwicz, 2015-08-25 01:14:47]

The patch to menu_message_pump_dispatcher_win.cc seems to be gone

[afakhry, 2015-08-25 15:54:08]

On 2015/08/25 01:14:47, pkotwicz wrote:
> The patch to menu_message_pump_dispatcher_win.cc seems to be gone

Yes, I have decided to leave the Windows implementation as is for now.

[pkotwicz, 2015-08-26 01:26:54]

Sorry for the delay. Thank you for updating the CL. Hopefully we can put it to
bed soon

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.h:120: bool
ShouldMenuBeKeptOpenForAccelerator(
I think that it would be clearer if this method was named
ShouldCloseMenuAndRepostAccelerator(). The return value would have to be the
opposite of what it is now.
I think that ShouldMenuBeKeptOpenForAccelerator() returning true for
accelerators which are never handled by AcceleratorController is confusing.

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
File ash/accelerators/ash_menu_event_filter_delegate.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
ash/accelerators/ash_menu_event_filter_delegate.cc:55: if
(focus_manager->ProcessAccelerator(accelerator))
What's the thinking behind calling FocusManager::ProcessAccelerator() ?
With the CL applied pressing Ctrl+W closes the browser window when the menu was
open when it did not do so before. It is also possible to trigger any extension
browser actions which have keyboard shortcuts assigned. I do not know whether
this change is a good change. However, this change should not be part of this CL

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:117: result =
delegate->ProcessAccelerator(accelerator);
With this CL applied, it is no longer possible to use the arrow keys while the
Ctrl key is pressed. I am unsure whether this is a good change, but it should
not be in this CL. This CL is pretty big as is.

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:133: 
I think you can move the calling of
MenuEventFilter::Delegate::ProcessAccelerator() here (line 133)

if (menu_controller->exit_type() == MenuController::EXIT_NONE) {
  close_menu_and_repost_accelerator =
delegate->ShouldCloseMenuAndRepostAccelerator();
  if (!close_menu_and_repost_accelerator) {
    delegate->ProcessAcceleratorNow(accelerator);
  }
}

I split up MenuEventFilter::Delegate::ProcessAccelerator() into two because I
think that having MenuEventFilter::Delegate::ProcessAccelerator() return
Delegate::RESULT_PROCESSED when it did not do anything is confusing

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:140: if (result ==
Delegate::RESULT_PROCESS_LATER) {
I would remove the drag and drop specific logic

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (left):

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:149:
aura::client::GetDispatcherClient(root), &nested_dispatcher);
Don't we still need DispatcherRunLoop on Windows?

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:159: menu_event_filter_.get());
Shouldn't we only be attaching the MenuEventFilter on non-Windows? I understand
that the Windows code will never call into MenuEventFilter::OnKeyEvent() but
this is super subtle.

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:209:
menu_event_filter_->ClearDelegate();
With this CL applied, if I:
1) Open the wrench menu
2) Right click on one of the wrench menu's bookmarks to bring up the context
menu
3) Press escape to close the context menu
I am unable to use the arrow keys for navigating the menu anymore

[afakhry, 2015-08-26 18:39:21]

Thanks for the review. PTAL.

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.h:120: bool
ShouldMenuBeKeptOpenForAccelerator(
On 2015/08/26 01:26:54, pkotwicz wrote:
> I think that it would be clearer if this method was named
> ShouldCloseMenuAndRepostAccelerator(). The return value would have to be the
> opposite of what it is now.
> I think that ShouldMenuBeKeptOpenForAccelerator() returning true for
> accelerators which are never handled by AcceleratorController is confusing.

Done.

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
File ash/accelerators/ash_menu_event_filter_delegate.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
ash/accelerators/ash_menu_event_filter_delegate.cc:55: if
(focus_manager->ProcessAccelerator(accelerator))
On 2015/08/26 01:26:54, pkotwicz wrote:
> What's the thinking behind calling FocusManager::ProcessAccelerator() ?
> With the CL applied pressing Ctrl+W closes the browser window when the menu
was
> open when it did not do so before. It is also possible to trigger any
extension
> browser actions which have keyboard shortcuts assigned. I do not know whether
> this change is a good change. However, this change should not be part of this
CL

Exactly. I did this after a discussion with Oshima to make it possible to
performed accelerators handled by the BrowserView like Ctrl+W. Not sure if
everyone will agree to enable this. Unfortunately he's not here now to ask him
for some input. :(

I think this changes the current behavior so I removed it from this CL for now.
I think it can be easy to add it back later on if needed.

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:117: result =
delegate->ProcessAccelerator(accelerator);
On 2015/08/26 01:26:54, pkotwicz wrote:
> With this CL applied, it is no longer possible to use the arrow keys while the
> Ctrl key is pressed. I am unsure whether this is a good change, but it should
> not be in this CL. This CL is pretty big as is.

Done. I modified this code so now it looks almost similar to the one in 
MenuEventDispatcher::DispatchEvent().

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:133: 
On 2015/08/26 01:26:54, pkotwicz wrote:
> I think you can move the calling of
> MenuEventFilter::Delegate::ProcessAccelerator() here (line 133)
> 
> if (menu_controller->exit_type() == MenuController::EXIT_NONE) {
>   close_menu_and_repost_accelerator =
> delegate->ShouldCloseMenuAndRepostAccelerator();
>   if (!close_menu_and_repost_accelerator) {
>     delegate->ProcessAcceleratorNow(accelerator);
>   }
> }
> 
> I split up MenuEventFilter::Delegate::ProcessAccelerator() into two because I
> think that having MenuEventFilter::Delegate::ProcessAccelerator() return
> Delegate::RESULT_PROCESSED when it did not do anything is confusing

I agree, it used to be confusing. Done!

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:140: if (result ==
Delegate::RESULT_PROCESS_LATER) {
On 2015/08/26 01:26:54, pkotwicz wrote:
> I would remove the drag and drop specific logic

Done.

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (left):

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:149:
aura::client::GetDispatcherClient(root), &nested_dispatcher);
On 2015/08/26 01:26:54, pkotwicz wrote:
> Don't we still need DispatcherRunLoop on Windows?

Done.

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:159: menu_event_filter_.get());
On 2015/08/26 01:26:54, pkotwicz wrote:
> Shouldn't we only be attaching the MenuEventFilter on non-Windows? I
understand
> that the Windows code will never call into MenuEventFilter::OnKeyEvent() but
> this is super subtle.

Done. I kept the windows code exactly as is.

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:209:
menu_event_filter_->ClearDelegate();
On 2015/08/26 01:26:54, pkotwicz wrote:
> With this CL applied, if I:
> 1) Open the wrench menu
> 2) Right click on one of the wrench menu's bookmarks to bring up the context
> menu
> 3) Press escape to close the context menu
> I am unable to use the arrow keys for navigating the menu anymore

That's weird. I can't repro this at all. In fact the above code has been added
to prevent this bug, since MenuController::TerminateNestedMessageLoop() can be
called multiple times before the RunLoop has actually exited, which results in
calling MenuMessageLoopAura::QuitNow() multiple times before the runloop exits. 

There's event an interactive_ui_test I believe that was written by you,
BookmarkBarViewTest24.ContextMenusKeyboardEscape, which I made sure it passes
after I added the above code.

[pkotwicz, 2015-08-26 20:23:25]

On 2015/08/26 18:39:21, afakhry wrote:
> Thanks for the review. PTAL.
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
> File ash/accelerators/accelerator_controller.h (right):
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
> ash/accelerators/accelerator_controller.h:120: bool
> ShouldMenuBeKeptOpenForAccelerator(
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > I think that it would be clearer if this method was named
> > ShouldCloseMenuAndRepostAccelerator(). The return value would have to be the
> > opposite of what it is now.
> > I think that ShouldMenuBeKeptOpenForAccelerator() returning true for
> > accelerators which are never handled by AcceleratorController is confusing.
> 
> Done.
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
> File ash/accelerators/ash_menu_event_filter_delegate.cc (right):
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
> ash/accelerators/ash_menu_event_filter_delegate.cc:55: if
> (focus_manager->ProcessAccelerator(accelerator))
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > What's the thinking behind calling FocusManager::ProcessAccelerator() ?
> > With the CL applied pressing Ctrl+W closes the browser window when the menu
> was
> > open when it did not do so before. It is also possible to trigger any
> extension
> > browser actions which have keyboard shortcuts assigned. I do not know
whether
> > this change is a good change. However, this change should not be part of
this
> CL
> 
> Exactly. I did this after a discussion with Oshima to make it possible to
> performed accelerators handled by the BrowserView like Ctrl+W. Not sure if
> everyone will agree to enable this. Unfortunately he's not here now to ask him
> for some input. :(
> 
> I think this changes the current behavior so I removed it from this CL for
now.
> I think it can be easy to add it back later on if needed.
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> File ui/views/controls/menu/menu_event_filter.cc (right):
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> ui/views/controls/menu/menu_event_filter.cc:117: result =
> delegate->ProcessAccelerator(accelerator);
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > With this CL applied, it is no longer possible to use the arrow keys while
the
> > Ctrl key is pressed. I am unsure whether this is a good change, but it
should
> > not be in this CL. This CL is pretty big as is.
> 
> Done. I modified this code so now it looks almost similar to the one in 
> MenuEventDispatcher::DispatchEvent().
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> ui/views/controls/menu/menu_event_filter.cc:133: 
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > I think you can move the calling of
> > MenuEventFilter::Delegate::ProcessAccelerator() here (line 133)
> > 
> > if (menu_controller->exit_type() == MenuController::EXIT_NONE) {
> >   close_menu_and_repost_accelerator =
> > delegate->ShouldCloseMenuAndRepostAccelerator();
> >   if (!close_menu_and_repost_accelerator) {
> >     delegate->ProcessAcceleratorNow(accelerator);
> >   }
> > }
> > 
> > I split up MenuEventFilter::Delegate::ProcessAccelerator() into two because
I
> > think that having MenuEventFilter::Delegate::ProcessAccelerator() return
> > Delegate::RESULT_PROCESSED when it did not do anything is confusing
> 
> I agree, it used to be confusing. Done!
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> ui/views/controls/menu/menu_event_filter.cc:140: if (result ==
> Delegate::RESULT_PROCESS_LATER) {
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > I would remove the drag and drop specific logic
> 
> Done.
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> File ui/views/controls/menu/menu_message_loop_aura.cc (left):
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> ui/views/controls/menu/menu_message_loop_aura.cc:149:
> aura::client::GetDispatcherClient(root), &nested_dispatcher);
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > Don't we still need DispatcherRunLoop on Windows?
> 
> Done.
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> File ui/views/controls/menu/menu_message_loop_aura.cc (right):
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> ui/views/controls/menu/menu_message_loop_aura.cc:159:
menu_event_filter_.get());
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > Shouldn't we only be attaching the MenuEventFilter on non-Windows? I
> understand
> > that the Windows code will never call into MenuEventFilter::OnKeyEvent() but
> > this is super subtle.
> 
> Done. I kept the windows code exactly as is.
> 
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> ui/views/controls/menu/menu_message_loop_aura.cc:209:
> menu_event_filter_->ClearDelegate();
> On 2015/08/26 01:26:54, pkotwicz wrote:
> > With this CL applied, if I:
> > 1) Open the wrench menu
> > 2) Right click on one of the wrench menu's bookmarks to bring up the context
> > menu
> > 3) Press escape to close the context menu
> > I am unable to use the arrow keys for navigating the menu anymore
> 
> That's weird. I can't repro this at all. In fact the above code has been added
> to prevent this bug, since MenuController::TerminateNestedMessageLoop() can be
> called multiple times before the RunLoop has actually exited, which results in
> calling MenuMessageLoopAura::QuitNow() multiple times before the runloop
exits. 
> 
> There's event an interactive_ui_test I believe that was written by you,
> BookmarkBarViewTest24.ContextMenusKeyboardEscape, which I made sure it passes
> after I added the above code.

Sorry I wrote down the wrong steps:
1) Open the wrench menu
2) Right click on one of the wrench menu's bookmarks to bring up the context
menu
3) Press escape to close the context menu
I am no longer able to use <Alt>+<+> to maximize/restore the window anymore

[afakhry, 2015-08-26 21:02:40]

On 2015/08/26 20:23:25, pkotwicz wrote:
> On 2015/08/26 18:39:21, afakhry wrote:
> > Thanks for the review. PTAL.
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
> > File ash/accelerators/accelerator_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/accel...
> > ash/accelerators/accelerator_controller.h:120: bool
> > ShouldMenuBeKeptOpenForAccelerator(
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > I think that it would be clearer if this method was named
> > > ShouldCloseMenuAndRepostAccelerator(). The return value would have to be
the
> > > opposite of what it is now.
> > > I think that ShouldMenuBeKeptOpenForAccelerator() returning true for
> > > accelerators which are never handled by AcceleratorController is
confusing.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
> > File ash/accelerators/ash_menu_event_filter_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
> > ash/accelerators/ash_menu_event_filter_delegate.cc:55: if
> > (focus_manager->ProcessAccelerator(accelerator))
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > What's the thinking behind calling FocusManager::ProcessAccelerator() ?
> > > With the CL applied pressing Ctrl+W closes the browser window when the
menu
> > was
> > > open when it did not do so before. It is also possible to trigger any
> > extension
> > > browser actions which have keyboard shortcuts assigned. I do not know
> whether
> > > this change is a good change. However, this change should not be part of
> this
> > CL
> > 
> > Exactly. I did this after a discussion with Oshima to make it possible to
> > performed accelerators handled by the BrowserView like Ctrl+W. Not sure if
> > everyone will agree to enable this. Unfortunately he's not here now to ask
him
> > for some input. :(
> > 
> > I think this changes the current behavior so I removed it from this CL for
> now.
> > I think it can be easy to add it back later on if needed.
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > File ui/views/controls/menu/menu_event_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > ui/views/controls/menu/menu_event_filter.cc:117: result =
> > delegate->ProcessAccelerator(accelerator);
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > With this CL applied, it is no longer possible to use the arrow keys while
> the
> > > Ctrl key is pressed. I am unsure whether this is a good change, but it
> should
> > > not be in this CL. This CL is pretty big as is.
> > 
> > Done. I modified this code so now it looks almost similar to the one in 
> > MenuEventDispatcher::DispatchEvent().
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > ui/views/controls/menu/menu_event_filter.cc:133: 
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > I think you can move the calling of
> > > MenuEventFilter::Delegate::ProcessAccelerator() here (line 133)
> > > 
> > > if (menu_controller->exit_type() == MenuController::EXIT_NONE) {
> > >   close_menu_and_repost_accelerator =
> > > delegate->ShouldCloseMenuAndRepostAccelerator();
> > >   if (!close_menu_and_repost_accelerator) {
> > >     delegate->ProcessAcceleratorNow(accelerator);
> > >   }
> > > }
> > > 
> > > I split up MenuEventFilter::Delegate::ProcessAccelerator() into two
because
> I
> > > think that having MenuEventFilter::Delegate::ProcessAccelerator() return
> > > Delegate::RESULT_PROCESSED when it did not do anything is confusing
> > 
> > I agree, it used to be confusing. Done!
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > ui/views/controls/menu/menu_event_filter.cc:140: if (result ==
> > Delegate::RESULT_PROCESS_LATER) {
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > I would remove the drag and drop specific logic
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > File ui/views/controls/menu/menu_message_loop_aura.cc (left):
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > ui/views/controls/menu/menu_message_loop_aura.cc:149:
> > aura::client::GetDispatcherClient(root), &nested_dispatcher);
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > Don't we still need DispatcherRunLoop on Windows?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > File ui/views/controls/menu/menu_message_loop_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > ui/views/controls/menu/menu_message_loop_aura.cc:159:
> menu_event_filter_.get());
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > Shouldn't we only be attaching the MenuEventFilter on non-Windows? I
> > understand
> > > that the Windows code will never call into MenuEventFilter::OnKeyEvent()
but
> > > this is super subtle.
> > 
> > Done. I kept the windows code exactly as is.
> > 
> >
>
https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
> > ui/views/controls/menu/menu_message_loop_aura.cc:209:
> > menu_event_filter_->ClearDelegate();
> > On 2015/08/26 01:26:54, pkotwicz wrote:
> > > With this CL applied, if I:
> > > 1) Open the wrench menu
> > > 2) Right click on one of the wrench menu's bookmarks to bring up the
context
> > > menu
> > > 3) Press escape to close the context menu
> > > I am unable to use the arrow keys for navigating the menu anymore
> > 
> > That's weird. I can't repro this at all. In fact the above code has been
added
> > to prevent this bug, since MenuController::TerminateNestedMessageLoop() can
be
> > called multiple times before the RunLoop has actually exited, which results
in
> > calling MenuMessageLoopAura::QuitNow() multiple times before the runloop
> exits. 
> > 
> > There's event an interactive_ui_test I believe that was written by you,
> > BookmarkBarViewTest24.ContextMenusKeyboardEscape, which I made sure it
passes
> > after I added the above code.
> 
> Sorry I wrote down the wrong steps:
> 1) Open the wrench menu
> 2) Right click on one of the wrench menu's bookmarks to bring up the context
> menu
> 3) Press escape to close the context menu
> I am no longer able to use <Alt>+<+> to maximize/restore the window anymore

Thank you so much for catching that. It should be fixed now. Please take another
look! :)

[pkotwicz, 2015-08-27 17:19:55]

A few more comments. The CL looks much cleaner now!

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:117: result =
delegate->ProcessAccelerator(accelerator);
The code looks much cleaner now!

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:730: if
(actions_keeping_menu_open_.count(action))
How about:
return actions_keeping_menu_open_.count(action) == 0;

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:430: const AcceleratorAction
kActionsKeepingMenuOpen[] = {
You should double check which accelerators should not close the menu with UX. My
preference would be to make this list as short as possible. Potentially have
just: { TOGGLE_CAPS_LOCK, DISABLE_CAPS_LOCK, PREVIOUS_IME, NEXT_IME, SWITCH_IME,
TAKE_SCREENSHOT, SILENCE_SPOKEN_FEEDBACK }

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:438: TAKE_PARTIAL_SCREENSHOT,
There are bugs with taking a partial screenshot while a menu is open. e.g. I can
use the arrow keys to navigate through the menu because MenuEventFilter gets key
events before PartialScreenshotController does. Perhaps the partial screenshot
functionality should be fixed to work better with menus but that should be done
in a separate CL

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:449: TOGGLE_MIRROR_MODE,
The TOGGLE_MIRROR_MODE accelerator causes a crash if it is processed while the
menu is still open. Removing it from this list seems to fix the problem

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:451: TOGGLE_TOUCH_VIEW_TESTING,
This accelerator maximizes the window (if you are running with
--ash-enable-touch-view-testing). The menu does not reposition itself to take
into account the fact that the window got maximized. We should remove the
accelerator from this list to get rid of the weird behavior

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:96: MenuController* menu_controller
= MenuController::GetActiveInstance();
Nit: Can you pass in the active menu controller in the constructor?

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:106:
should_close_and_repost_accelerator = true;
Can we just drop the accelerator in this case? Note that the behavior in this
scenario with the CL applied is different than the behavior without the CL
applied.

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:113: (flags & kKeyFlagsMask) == 0) {
menu_event_dispatcher.cc still runs MenuController::SelectByChar() even if the
EF_COMMAND_DOWN modifier is present. Can we get the same behavior here too?

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:196:
menu_event_filter_->SetDelegate(previous_delegate);
Can we remove the MenuEventFilter pretarget handler here? I don't think that
this will change the behavior from what you currently have except in the case
that we get a key event in between the time that MenuMessageLoopAura::QuitNow()
is called and the time that base::RunLoop quits. I think this is ok.

My understanding based on reading https://codereview.chromium.org/1664001 is
that the interesting behavior in between the time that
MenuMessageLoopAura::QuitNow() is called and the time that base::RunLoop quits
is to make tests pass on Windows

[afakhry, 2015-08-28 01:24:41]

Thanks for your suggestions! It's much simpler and cleaner now. PTAL!

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:730: if
(actions_keeping_menu_open_.count(action))
On 2015/08/27 17:19:55, pkotwicz wrote:
> How about:
> return actions_keeping_menu_open_.count(action) == 0;

Much better! Done. Thanks! :)

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:430: const AcceleratorAction
kActionsKeepingMenuOpen[] = {
On 2015/08/27 17:19:55, pkotwicz wrote:
> You should double check which accelerators should not close the menu with UX.
My
> preference would be to make this list as short as possible. Potentially have
> just: { TOGGLE_CAPS_LOCK, DISABLE_CAPS_LOCK, PREVIOUS_IME, NEXT_IME,
SWITCH_IME,
> TAKE_SCREENSHOT, SILENCE_SPOKEN_FEEDBACK }

I agree, however, I also believe that one should be able to control the
brightness, the volume, and the spoken feedback while the menu is open.
Especially the volume and brightness because I remember seeing those in one of
your design docs. In any case, I will try to get feedback on this list as you
suggested.

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:438: TAKE_PARTIAL_SCREENSHOT,
On 2015/08/27 17:19:55, pkotwicz wrote:
> There are bugs with taking a partial screenshot while a menu is open. e.g. I
can
> use the arrow keys to navigate through the menu because MenuEventFilter gets
key
> events before PartialScreenshotController does. Perhaps the partial screenshot
> functionality should be fixed to work better with menus but that should be
done
> in a separate CL

I see. That can be easily fixed later by maybe adding a null-pre-target handler
while the partial screenshot taking is in progress.

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:449: TOGGLE_MIRROR_MODE,
On 2015/08/27 17:19:55, pkotwicz wrote:
> The TOGGLE_MIRROR_MODE accelerator causes a crash if it is processed while the
> menu is still open. Removing it from this list seems to fix the problem

That's weird! I tried the mirror mode with two displays and the menu open. No
crash occurred. I removed this accelerator from this list. I'm not sure why I
added it here in the first place.

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:451: TOGGLE_TOUCH_VIEW_TESTING,
On 2015/08/27 17:19:55, pkotwicz wrote:
> This accelerator maximizes the window (if you are running with
> --ash-enable-touch-view-testing). The menu does not reposition itself to take
> into account the fact that the window got maximized. We should remove the
> accelerator from this list to get rid of the weird behavior

Done.

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:96: MenuController* menu_controller
= MenuController::GetActiveInstance();
On 2015/08/27 17:19:55, pkotwicz wrote:
> Nit: Can you pass in the active menu controller in the constructor?

Great idea! And it opened the path to create the event filter on the stack
inside MenuMessageLoopAura::Run() which is perfect for nested run loops with
nested menus. Now I don't event need MenuEventFilter::GetCurrentDelegate() so I
removed it.

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:106:
should_close_and_repost_accelerator = true;
On 2015/08/27 17:19:55, pkotwicz wrote:
> Can we just drop the accelerator in this case? Note that the behavior in this
> scenario with the CL applied is different than the behavior without the CL
> applied.

Done.

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:113: (flags & kKeyFlagsMask) == 0) {
On 2015/08/27 17:19:55, pkotwicz wrote:
> menu_event_dispatcher.cc still runs MenuController::SelectByChar() even if the
> EF_COMMAND_DOWN modifier is present. Can we get the same behavior here too?

I know but isn't that something we want to prevent? See
https://code.google.com/p/chromium/issues/detail?id=471488#c12 It's a different
bug but it looks like we want to treat the Search key on CrOs as a modifier like
Ctrl & Alt.

That being said, I removed this change in this CL. We can add it later if we
need to.
Done.

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:196:
menu_event_filter_->SetDelegate(previous_delegate);
On 2015/08/27 17:19:55, pkotwicz wrote:
> Can we remove the MenuEventFilter pretarget handler here? I don't think that
> this will change the behavior from what you currently have except in the case
> that we get a key event in between the time that
MenuMessageLoopAura::QuitNow()
> is called and the time that base::RunLoop quits. I think this is ok.
> 
> My understanding based on reading https://codereview.chromium.org/1664001 is
> that the interesting behavior in between the time that
> MenuMessageLoopAura::QuitNow() is called and the time that base::RunLoop quits
> is to make tests pass on Windows

This is an awesome idea! I don't even need to care if QuitNow() is called
multiple times before the run_loop exits, so I removed all the needed code to
achieve that. I don't know why I didn't think of that in the first place.
Done. It's much cleaner and simpler now.

[afakhry, 2015-08-28 19:14:57]

Patchset #24 (id:480001) has been deleted

[pkotwicz, 2015-08-28 19:48:27]

https://codereview.chromium.org/1138523006/diff/440001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/440001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:105: event->StopPropagation();
I checked and removing the call to ui::Event::StopPropagation() fixes
BookmarkBarViewTest12.*
I believe that doing this makes the behavior in the time between
MenuMessageLoopAura::QuitNow() is called and the time that the event filter is
applied the same as without the patch applied

Thanks for telling me about the test failure. I did not think about this
scenario.

[afakhry, 2015-08-28 19:56:40]

https://codereview.chromium.org/1138523006/diff/440001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/440001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:105: event->StopPropagation();
On 2015/08/28 19:48:27, pkotwicz wrote:
> I checked and removing the call to ui::Event::StopPropagation() fixes
> BookmarkBarViewTest12.*
> I believe that doing this makes the behavior in the time between
> MenuMessageLoopAura::QuitNow() is called and the time that the event filter is
> applied the same as without the patch applied
> 
> Thanks for telling me about the test failure. I did not think about this
> scenario.

Yes exactly. However I did things a bit differently in the most recent patch.
PTAL and let me know if you prefer a different solution. The test is not failing
now.

[afakhry, 2015-08-28 20:10:21]

Don't stop the event's propagation when the menu has exited.

[pkotwicz, 2015-08-28 20:41:50]

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:172:
menu_event_filter->SetDelegate(filter_delegate);
Can you pass in the MenuEventFilter::Delegate to the constructor?

[afakhry, 2015-08-28 21:42:20]

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:172:
menu_event_filter->SetDelegate(filter_delegate);
On 2015/08/28 20:41:50, pkotwicz wrote:
> Can you pass in the MenuEventFilter::Delegate to the constructor?

Done.

[pkotwicz, 2015-08-28 22:10:10]

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:105: // propagation.
Please include a comment as to why we do not stop propagation of the event. My
suggestion:
"If the user accepts a menu item in a nested menu, the menu item action is run
after the base::RunLoop for the innermost menu has quit but before the
base::RunLoop for the outermost menu has quit. If the menu item action starts a
base::RunLoop, the outermost menu's base::RunLoop will not quit till the
action's base::RunLoop ends. IDC_BOOKMARK_BAR_OPEN_ALL sometimes opens a modal
dialog. The modal dialog starts a base::RunLoop and keeps the base::RunLoop
running for the duration of its lifetime."

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:135: RepostAccelerator(accelerator,
delegate);
I have an idea to make this slightly clearer:
Change MenuController::Run() to return a MenuController::PostCloseAction struct.
I am thinking:
struct MenuController::PostCloseAction {
int command;
ui::Accelerator accelerator;
};

I would call MenuEventFilter::Delegate::ProcessAcceleratorNow() from
MenuRunnerImpl::MenuDone().
MenuEventFilter would call MenuController::set_post_close_action() if the
accelerator should be run once the menu is closed

I think I better understand why mnemonics actions are run in
MenuRunnerImpl::MenuDone() thanks to the
BookmarkBarViewTest12.CloseWithModalDialog test failure in the earlier CL

I will defer to sky@ as to what the correct design is.

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:147:
event->set_should_remove_native_touch_id_mapping(false);
I think that this is no longer necessary because we no longer call
ui::EventFromNative(). You should double check with tdresser@ though who added
this code. See https://codereview.chromium.org/886593004

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:155: void
MenuEventFilter::ClearDelegate() {
You no longer use this method. Can you remove it?

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:192: 
Nit: Remove new new line

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:25: // Defines a NULL-Object
delegate for non-ash platforms.
How about: "No-op delegate."

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:90: DCHECK(delegate);
This DCHECK is unnecessary

[afakhry, 2015-08-28 23:32:13]

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:105: // propagation.
On 2015/08/28 22:10:09, pkotwicz wrote:
> Please include a comment as to why we do not stop propagation of the event. My
> suggestion:
> "If the user accepts a menu item in a nested menu, the menu item action is run
> after the base::RunLoop for the innermost menu has quit but before the
> base::RunLoop for the outermost menu has quit. If the menu item action starts
a
> base::RunLoop, the outermost menu's base::RunLoop will not quit till the
> action's base::RunLoop ends. IDC_BOOKMARK_BAR_OPEN_ALL sometimes opens a modal
> dialog. The modal dialog starts a base::RunLoop and keeps the base::RunLoop
> running for the duration of its lifetime."

Copied your suggested comment as is.

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:135: RepostAccelerator(accelerator,
delegate);
On 2015/08/28 22:10:10, pkotwicz wrote:
> I have an idea to make this slightly clearer:
> Change MenuController::Run() to return a MenuController::PostCloseAction
struct.
> I am thinking:
> struct MenuController::PostCloseAction {
> int command;
> ui::Accelerator accelerator;
> };
> 
> I would call MenuEventFilter::Delegate::ProcessAcceleratorNow() from
> MenuRunnerImpl::MenuDone().
> MenuEventFilter would call MenuController::set_post_close_action() if the
> accelerator should be run once the menu is closed
> 
> I think I better understand why mnemonics actions are run in
> MenuRunnerImpl::MenuDone() thanks to the
> BookmarkBarViewTest12.CloseWithModalDialog test failure in the earlier CL
> 
> I will defer to sky@ as to what the correct design is.

Sure, let's defer to sky@.

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:147:
event->set_should_remove_native_touch_id_mapping(false);
On 2015/08/28 22:10:10, pkotwicz wrote:
> I think that this is no longer necessary because we no longer call
> ui::EventFromNative(). You should double check with tdresser@ though who added
> this code. See https://codereview.chromium.org/886593004

If it exists in MenuEventDispatcher, it should still remain here. Isn't that
your whole philosophy of not introducing too many changes in one CL? :) This is
a subject for another CL I think.

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:155: void
MenuEventFilter::ClearDelegate() {
On 2015/08/28 22:10:10, pkotwicz wrote:
> You no longer use this method. Can you remove it?

But I already did. You are reviewing an older patch!

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:192: 
On 2015/08/28 22:10:10, pkotwicz wrote:
> Nit: Remove new new line

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:25: // Defines a NULL-Object
delegate for non-ash platforms.
On 2015/08/28 22:10:10, pkotwicz wrote:
> How about: "No-op delegate."

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:90: DCHECK(delegate);
On 2015/08/28 22:10:10, pkotwicz wrote:
> This DCHECK is unnecessary

Done.

[pkotwicz, 2015-08-28 23:44:26]

I am down to nits. I think that this CL is ready to be reviewed by sky@ and
oshima@

I am going to be on vacation next week but will be around for part of Wednesday

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:438: TAKE_PARTIAL_SCREENSHOT,
Can you file a bug for fixing this?

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:449: TOGGLE_MIRROR_MODE,
To get the crash you must press the TOGGLE_MIRROR_MODE accelerator when a menu
is open for a browser window which is on the secondary display

https://codereview.chromium.org/1138523006/diff/540001/ash/accelerators/ash_m...
File ash/accelerators/ash_menu_event_filter_delegate.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ash/accelerators/ash_m...
ash/accelerators/ash_menu_event_filter_delegate.cc:14: ui::AcceleratorHistory*
history)
Idle wondering: Any reason that you pass ui::AcceleratorHistory to the
constructor instead of AcceleratorController?

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:71: DCHECK(root_window);
This DCHECK is unnecessary. Line 74 will crash if |root_window| is a nullptr

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:86: DCHECK(event);
Nit: This DCHECK() is unnecessary. This method will crash if |event| is null

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:96:
delegate->StoreInHistory(accelerator);
Shouldn't we do this only in the case where we call ui::Event::StopPropagation()
?

I think the comment should say that we do this because we stop propagation and
wm::AcceleratorFilter::OnKeyEvent() will not get the event.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:98: CHECK(menu_controller_);
Nit: Remove this CHECK(). You already have the DCHECK() in the constructor

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:117: // MenuController::OnKeyDown()
and no modifiers are pressed.
Nit: Remove the comment. It does not say anything that isn't obvious from the
code.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:131: // The event's propagation will
always be stopped.
Nit: This comment is no longer accurate

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:16: }  // namespace aura
Nit: We don't add comments for forward declaration namespaces.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:20: }  // namespace ui
Nit: remove namespace comment

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:27: // needed to handle the
interactions between accelerators and menus.
How about: "Handles key events when a menu is open."

I am assuming that the code in OnTouchEvent() is no longer necessary

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:34: enum Result {
Nit: Remove because it is no longer used

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:45: // be done properly.
How about: "Stores |accelerator| in AcceleratorHistory."

I have not seen the "vertical bar" character used for class names.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:50: // reposted after the menu exists
to be performed later.
How about: "Returns true if there is an action associated with |accelerator| and
the menu should be closed before performing the action."

I do not think it is worth mentioning what false means because it can either
mean:
- There is an action associated with the accelerator and it can be performed
while the menu is open
- There is no action associated with the accelerator

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:55: // associated with it.
How about: "Processes |accelerator| immediately."

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:61: // default delegate will be used.
Nit: "active menu in the currently running menu message loop" -> "active
MenuController"

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:162: 
Nit: kill the new line

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:172: 
Nit: kill the new line

[pkotwicz, 2015-08-28 23:45:49]

https://codereview.chromium.org/1138523006/diff/560001/ash/accelerators/ash_m...
File ash/accelerators/ash_menu_event_filter_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/560001/ash/accelerators/ash_m...
ash/accelerators/ash_menu_event_filter_delegate.h:12: }  // namespace ui
Nit: remove namespace comment

[pkotwicz, 2015-08-29 00:05:11]

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:147:
event->set_should_remove_native_touch_id_mapping(false);
I think that:

Correct:
scoped_ptr<ui::Event> copy(ui::EventFromNative(event));
copy->set_should_remove_native_touch_id_mapping(false);

Correct:
// No code

Incorrect:
scoped_ptr<ui::Event> copy(ui::EventFromNative(event));
copy->set_should_remove_native_touch_id_mapping(true);

Incorrect:
event->set_should_remove_native_touch_id_mapping(false)

You should double check with tdresser@ though

[afakhry, 2015-08-29 01:31:24]

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/420001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:438: TAKE_PARTIAL_SCREENSHOT,
On 2015/08/28 23:44:25, pkotwicz wrote:
> Can you file a bug for fixing this?

Will do once this CL lands. It doesn't make sense to file a bug saying hey we
can navigate the menu using the arrow keys while a partial screenshot is in
progress when we can't even do that now.

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:147:
event->set_should_remove_native_touch_id_mapping(false);
On 2015/08/29 00:05:11, pkotwicz wrote:
> I think that:
> 
> Correct:
> scoped_ptr<ui::Event> copy(ui::EventFromNative(event));
> copy->set_should_remove_native_touch_id_mapping(false);
> 
> Correct:
> // No code
> 
> Incorrect:
> scoped_ptr<ui::Event> copy(ui::EventFromNative(event));
> copy->set_should_remove_native_touch_id_mapping(true);
> 
> Incorrect:
> event->set_should_remove_native_touch_id_mapping(false)
> 
> You should double check with tdresser@ though
> 

|event| here is not a native event so we can't do ui::EventFromNative() here.
The copy also needs to be a TouchEvent.
But we should we create a copy?
Once sky@ and oshima@ review this CL, we can ask tdresser@ about this  line.

https://codereview.chromium.org/1138523006/diff/540001/ash/accelerators/ash_m...
File ash/accelerators/ash_menu_event_filter_delegate.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ash/accelerators/ash_m...
ash/accelerators/ash_menu_event_filter_delegate.cc:14: ui::AcceleratorHistory*
history)
On 2015/08/28 23:44:25, pkotwicz wrote:
> Idle wondering: Any reason that you pass ui::AcceleratorHistory to the
> constructor instead of AcceleratorController?

No need to pass either actually. This part is such an old code that I haven't
looked at for a very long while. My focus was on the views:: part.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:71: DCHECK(root_window);
On 2015/08/28 23:44:25, pkotwicz wrote:
> This DCHECK is unnecessary. Line 74 will crash if |root_window| is a nullptr

DCHECK()'s never hurt. It's better to crash on a DCHECK() than get a useless
segmentation fault. However, I removed it.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:86: DCHECK(event);
On 2015/08/28 23:44:25, pkotwicz wrote:
> Nit: This DCHECK() is unnecessary. This method will crash if |event| is null

Same reason as above. However removed.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:96:
delegate->StoreInHistory(accelerator);
On 2015/08/28 23:44:25, pkotwicz wrote:
> Shouldn't we do this only in the case where we call
ui::Event::StopPropagation()
> ?
> 
> I think the comment should say that we do this because we stop propagation and
> wm::AcceleratorFilter::OnKeyEvent() will not get the event.

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:98: CHECK(menu_controller_);
On 2015/08/28 23:44:25, pkotwicz wrote:
> Nit: Remove this CHECK(). You already have the DCHECK() in the constructor

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:117: // MenuController::OnKeyDown()
and no modifiers are pressed.
On 2015/08/28 23:44:25, pkotwicz wrote:
> Nit: Remove the comment. It does not say anything that isn't obvious from the
> code.

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:131: // The event's propagation will
always be stopped.
On 2015/08/28 23:44:25, pkotwicz wrote:
> Nit: This comment is no longer accurate

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:16: }  // namespace aura
On 2015/08/28 23:44:25, pkotwicz wrote:
> Nit: We don't add comments for forward declaration namespaces.

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:20: }  // namespace ui
On 2015/08/28 23:44:26, pkotwicz wrote:
> Nit: remove namespace comment

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:34: enum Result {
On 2015/08/28 23:44:25, pkotwicz wrote:
> Nit: Remove because it is no longer used

Oops forgot to do that. Done!

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:45: // be done properly.
On 2015/08/28 23:44:25, pkotwicz wrote:
> How about: "Stores |accelerator| in AcceleratorHistory."
> 
> I have not seen the "vertical bar" character used for class names.

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:50: // reposted after the menu exists
to be performed later.
On 2015/08/28 23:44:26, pkotwicz wrote:
> How about: "Returns true if there is an action associated with |accelerator|
and
> the menu should be closed before performing the action."
> 
> I do not think it is worth mentioning what false means because it can either
> mean:
> - There is an action associated with the accelerator and it can be performed
> while the menu is open
> - There is no action associated with the accelerator

Makes sense. Done!

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:55: // associated with it.
On 2015/08/28 23:44:26, pkotwicz wrote:
> How about: "Processes |accelerator| immediately."

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:61: // default delegate will be used.
On 2015/08/28 23:44:26, pkotwicz wrote:
> Nit: "active menu in the currently running menu message loop" -> "active
> MenuController"

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:162: 
On 2015/08/28 23:44:26, pkotwicz wrote:
> Nit: kill the new line

Done.

https://codereview.chromium.org/1138523006/diff/540001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:172: 
On 2015/08/28 23:44:26, pkotwicz wrote:
> Nit: kill the new line

Done.

https://codereview.chromium.org/1138523006/diff/560001/ash/accelerators/ash_m...
File ash/accelerators/ash_menu_event_filter_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/560001/ash/accelerators/ash_m...
ash/accelerators/ash_menu_event_filter_delegate.h:12: }  // namespace ui
On 2015/08/28 23:45:48, pkotwicz wrote:
> Nit: remove namespace comment

Done.

[pkotwicz, 2015-08-29 02:30:45]

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/520001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:147:
event->set_should_remove_native_touch_id_mapping(false);
Ok, let's ask tdresser@

[afakhry, 2015-08-31 21:05:59]

afakhry@chromium.org changed reviewers:
- sadrul@chromium.org

[afakhry, 2015-08-31 21:06:51]

sky@ and oshima@ Could you please take a look at this CL? Thanks!

[oshima, 2015-09-02 17:08:21]

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
File ash/accelerators/ash_menu_event_filter_delegate.cc (right):

https://codereview.chromium.org/1138523006/diff/380001/ash/accelerators/ash_m...
ash/accelerators/ash_menu_event_filter_delegate.cc:55: if
(focus_manager->ProcessAccelerator(accelerator))
On 2015/08/26 01:26:54, pkotwicz wrote:
> What's the thinking behind calling FocusManager::ProcessAccelerator() ?
> With the CL applied pressing Ctrl+W closes the browser window when the menu
was
> open when it did not do so before. It is also possible to trigger any
extension
> browser actions which have keyboard shortcuts assigned. I do not know whether
> this change is a good change. However, this change should not be part of this
CL

The whole point of this work is that an accelerator should work regardless of
the UI state as long as it makes sense. It's very confusing if ctrl-t can open a
new tab, while ctrl-w does not work, or ctrl-p does not work.

I'm ok to do this in a separate CL though.

https://codereview.chromium.org/1138523006/diff/580001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ash/shell.cc#newcode926
ash/shell.cc:926: menu_event_filter_delegate_.reset(new
AshMenuEventFilterDelegate);
can you reset this in reverse order?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:65: int outstanding_touches_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:102: 
// VIewsTestBase:

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:125:
scoped_ptr<aura::ScopedWindowTargeter> scoped_targeter(
do you need to allocate this in heap? can't you just create it on stack?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:234:
MenuControllerTestDeleter controller_deleter_;
Don't you need this, do you?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:246: FROM_HERE,
indent

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:72:
DCHECK_EQ(root_window->GetRootWindow(), root_window);
DCHECK(root_window->IsRootWindow()) ?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:79:
DCHECK_EQ(root_window->GetRootWindow(), root_window);
ditto

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:104:
menu_controller_->OnKeyDown(event->key_code());
can you add comment that this is for mnemonics?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:48: // |menu_controller| should be of
the active MenuController, it can't be
If this should be one that is always active, why can't this use
MenuController::GetActiveInstance() instead of passing?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:170: new
MenuEventFilter(controller, filter_delegate));
can't you just create this on stack?

[afakhry, 2015-09-02 18:18:38]

https://codereview.chromium.org/1138523006/diff/580001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ash/shell.cc#newcode926
ash/shell.cc:926: menu_event_filter_delegate_.reset(new
AshMenuEventFilterDelegate);
On 2015/09/02 17:08:21, oshima wrote:
> can you reset this in reverse order?

I used this order since AshMenuEventFilterDelegate uses AcceleratorController so
that's why it's created first. Do you want me to change the order of declaration
and construction?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:65: int outstanding_touches_;
On 2015/09/02 17:08:21, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:102: 
On 2015/09/02 17:08:21, oshima wrote:
> // VIewsTestBase:

Done.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:125:
scoped_ptr<aura::ScopedWindowTargeter> scoped_targeter(
On 2015/09/02 17:08:21, oshima wrote:
> do you need to allocate this in heap? can't you just create it on stack?

It's easier this way because I want to delete it in the middle of this function
by calling reset().

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:234:
MenuControllerTestDeleter controller_deleter_;
On 2015/09/02 17:08:21, oshima wrote:
> Don't you need this, do you?

What do you mean? I need this deleter to make sure certain values are cleared
before MenuController dtor is called or else we'll crash.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:246: FROM_HERE,
On 2015/09/02 17:08:21, oshima wrote:
> indent

Done.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:72:
DCHECK_EQ(root_window->GetRootWindow(), root_window);
On 2015/09/02 17:08:21, oshima wrote:
> DCHECK(root_window->IsRootWindow()) ?

Done.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:79:
DCHECK_EQ(root_window->GetRootWindow(), root_window);
On 2015/09/02 17:08:21, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.cc:104:
menu_controller_->OnKeyDown(event->key_code());
On 2015/09/02 17:08:21, oshima wrote:
> can you add comment that this is for mnemonics?

Done.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:48: // |menu_controller| should be of
the active MenuController, it can't be
On 2015/09/02 17:08:21, oshima wrote:
> If this should be one that is always active, why can't this use
> MenuController::GetActiveInstance() instead of passing?

Yes I used to do this in an earlier patch, but Peter suggested that we pass it
from MenuMessageLoopAura::Run() since we create and destroy the event filter
there. I can change this if you want?

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:170: new
MenuEventFilter(controller, filter_delegate));
On 2015/09/02 17:08:21, oshima wrote:
> can't you just create this on stack?

Done.

[oshima, 2015-09-02 20:58:54]

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:125:
scoped_ptr<aura::ScopedWindowTargeter> scoped_targeter(
On 2015/09/02 18:18:38, afakhry wrote:
> On 2015/09/02 17:08:21, oshima wrote:
> > do you need to allocate this in heap? can't you just create it on stack?
> 
> It's easier this way because I want to delete it in the middle of this
function
> by calling reset().

You can do that in a block, and I think the code will look simpler.
If not, then I'm fine.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:234:
MenuControllerTestDeleter controller_deleter_;
On 2015/09/02 18:18:38, afakhry wrote:
> On 2015/09/02 17:08:21, oshima wrote:
> > Don't you need this, do you?
> 
> What do you mean? I need this deleter to make sure certain values are cleared
> before MenuController dtor is called or else we'll crash.

You already specified the deleter class below, so I believe you don't have to
pass it, unless the deleter is stateful.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:48: // |menu_controller| should be of
the active MenuController, it can't be
On 2015/09/02 18:18:38, afakhry wrote:
> On 2015/09/02 17:08:21, oshima wrote:
> > If this should be one that is always active, why can't this use
> > MenuController::GetActiveInstance() instead of passing?
> 
> Yes I used to do this in an earlier patch, but Peter suggested that we pass it
> from MenuMessageLoopAura::Run() since we create and destroy the event filter
> there. I can change this if you want?

which patch #?

[afakhry, 2015-09-03 16:41:42]

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:125:
scoped_ptr<aura::ScopedWindowTargeter> scoped_targeter(
On 2015/09/02 20:58:54, oshima wrote:
> On 2015/09/02 18:18:38, afakhry wrote:
> > On 2015/09/02 17:08:21, oshima wrote:
> > > do you need to allocate this in heap? can't you just create it on stack?
> > 
> > It's easier this way because I want to delete it in the middle of this
> function
> > by calling reset().
> 
> You can do that in a block, and I think the code will look simpler.
> If not, then I'm fine.

Done.

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:234:
MenuControllerTestDeleter controller_deleter_;
On 2015/09/02 20:58:54, oshima wrote:
> On 2015/09/02 18:18:38, afakhry wrote:
> > On 2015/09/02 17:08:21, oshima wrote:
> > > Don't you need this, do you?
> > 
> > What do you mean? I need this deleter to make sure certain values are
cleared
> > before MenuController dtor is called or else we'll crash.
> 
> You already specified the deleter class below, so I believe you don't have to
> pass it, unless the deleter is stateful.

Yes you are right. The deleter is not stateful, so I removed it from above.
Done!

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/580001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:48: // |menu_controller| should be of
the active MenuController, it can't be
On 2015/09/02 20:58:54, oshima wrote:
> On 2015/09/02 18:18:38, afakhry wrote:
> > On 2015/09/02 17:08:21, oshima wrote:
> > > If this should be one that is always active, why can't this use
> > > MenuController::GetActiveInstance() instead of passing?
> > 
> > Yes I used to do this in an earlier patch, but Peter suggested that we pass
it
> > from MenuMessageLoopAura::Run() since we create and destroy the event filter
> > there. I can change this if you want?
> 
> which patch #?

Patch #21: first comment on this file:
https://codereview.chromium.org/1138523006/diff/420001/ui/views/controls/menu...

[sky, 2015-09-03 17:23:21]

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:71: // This a deleter to
enable us to use a scoped_ptr of a MenuController, since
Fix the grammar here. I can't parse what you're trying to say.

IMO what you have is overkill. Just use delete in your test.

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:144:
internal::MenuMessagePumpDispatcher nested_dispatcher(controller);
Why is the filtering specific to non-windows platforms?

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
menu_event_filter(controller, filter_delegate);
AFAICT the functionality you need is the ability to install an EventHandlerwhen
the menu is shown. If this is the case, why do you need all this logic and not a
more straightforward way to do that? Say a function on ViewsDelegate that is
called when the menu is shown allowing you to add an EventHandler?

[afakhry, 2015-09-04 01:49:43]

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:71: // This a deleter to
enable us to use a scoped_ptr of a MenuController, since
On 2015/09/03 17:23:21, sky wrote:
> Fix the grammar here. I can't parse what you're trying to say.
> 
> IMO what you have is overkill. Just use delete in your test.

I modified the comment. I admit it can be considered as overkill, but I thought
using scoped_ptr is less error prone. Anyway, please let me know if you still
don't like it, I'm fine with changing it.

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:144:
internal::MenuMessagePumpDispatcher nested_dispatcher(controller);
On 2015/09/03 17:23:21, sky wrote:
> Why is the filtering specific to non-windows platforms?

On Windows, we have the MenuMessagePumpDispatcher that has to handle certain
Windows specific key events, and the fact that Windows sends two event types for
each key press WM_KEYDOWN and WM_CHAR. 

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
menu_event_filter(controller, filter_delegate);
On 2015/09/03 17:23:21, sky wrote:
> AFAICT the functionality you need is the ability to install an
EventHandlerwhen
> the menu is shown. If this is the case, why do you need all this logic and not
a
> more straightforward way to do that? Say a function on ViewsDelegate that is
> called when the menu is shown allowing you to add an EventHandler?

I'm not sure I understand your suggestion. What's the problem with using what we
already have in aura::Env to add and remove the EventHandler? Currently
ViewsDelegate doesn't have this functionality, which will make this change an
even bigger refactor. Could you please explain a bit more?

[sky, 2015-09-04 14:38:54]

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
menu_event_filter(controller, filter_delegate);
On 2015/09/04 01:49:42, afakhry wrote:
> On 2015/09/03 17:23:21, sky wrote:
> > AFAICT the functionality you need is the ability to install an
> EventHandlerwhen
> > the menu is shown. If this is the case, why do you need all this logic and
not
> a
> > more straightforward way to do that? Say a function on ViewsDelegate that is
> > called when the menu is shown allowing you to add an EventHandler?
> 
> I'm not sure I understand your suggestion. What's the problem with using what
we
> already have in aura::Env to add and remove the EventHandler? Currently
> ViewsDelegate doesn't have this functionality, which will make this change an
> even bigger refactor. Could you please explain a bit more?

I'm trying to understand why you are doing the change you're doing.

I agree with you adding/removing an EventHandler is the way to go. What I don't
understand is why we need MenuEventFilter in views side of code. I'm suggesting
the logic of MenuEventFilter be moved into chrome/ash by way of ViewsDelegate.
ViewsDelegate gets a method that is called at the right time to allow you to
install the EventHandler you need to. Does that make sense?

[sky, 2015-09-04 14:40:23]

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:71: // This a deleter to
enable us to use a scoped_ptr of a MenuController, since
On 2015/09/04 01:49:42, afakhry wrote:
> On 2015/09/03 17:23:21, sky wrote:
> > Fix the grammar here. I can't parse what you're trying to say.
> > 
> > IMO what you have is overkill. Just use delete in your test.
> 
> I modified the comment. I admit it can be considered as overkill, but I
thought
> using scoped_ptr is less error prone. Anyway, please let me know if you still
> don't like it, I'm fine with changing it.

scoped_ptr is certainly a good thing, but when you have to write 10 lines of
cryptic code just to make it useful for one call site, that can easily be hidden
in a SetUp/TearDown, it's overkill. Use delete.

[oshima, 2015-09-04 18:27:01]

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
menu_event_filter(controller, filter_delegate);
On 2015/09/04 14:38:54, sky wrote:
> On 2015/09/04 01:49:42, afakhry wrote:
> > On 2015/09/03 17:23:21, sky wrote:
> > > AFAICT the functionality you need is the ability to install an
> > EventHandlerwhen
> > > the menu is shown. If this is the case, why do you need all this logic and
> not
> > a
> > > more straightforward way to do that? Say a function on ViewsDelegate that
is
> > > called when the menu is shown allowing you to add an EventHandler?
> > 
> > I'm not sure I understand your suggestion. What's the problem with using
what
> we
> > already have in aura::Env to add and remove the EventHandler? Currently
> > ViewsDelegate doesn't have this functionality, which will make this change
an
> > even bigger refactor. Could you please explain a bit more?
> 
> I'm trying to understand why you are doing the change you're doing.
> 
> I agree with you adding/removing an EventHandler is the way to go. What I
don't
> understand is why we need MenuEventFilter in views side of code. I'm
suggesting
> the logic of MenuEventFilter be moved into chrome/ash by way of ViewsDelegate.
> ViewsDelegate gets a method that is called at the right time to allow you to
> install the EventHandler you need to. Does that make sense?

My understanding of the role of ViewsDelegate is to customize views behavior for
views client such as ash/chrome, right?
I think the issue is that current MenuEventFilter has a mix of platform and
client behavior. How about
removing the accelerator handling part from MenuEventFilter and move it to the
implementation of MenuEventFilter::Delegate, then
let ViewsDelegate return the MenuEventFilter::Delegate instead of
MenuEventFilter::GetMenuEventFilterDelegate. What do you think?

I also think adding MenuEventFilter for every message loop is overkill though.
Can't this be moved to MenuController so that it gets created only once when
menu is open?

[oshima, 2015-09-04 18:32:10]

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
menu_event_filter(controller, filter_delegate);

> let ViewsDelegate return the MenuEventFilter::Delegate instead of
> MenuEventFilter::GetMenuEventFilterDelegate.

Or just have ViewsDelegate::HandleKeyEventOnMenu

(we probably don't need separate Delegate instance)

[afakhry, 2015-09-04 18:49:32]

On 2015/09/04 18:32:10, oshima wrote:
>
https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
> File ui/views/controls/menu/menu_message_loop_aura.cc (right):
> 
>
https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
> ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
> menu_event_filter(controller, filter_delegate);
> 
> > let ViewsDelegate return the MenuEventFilter::Delegate instead of
> > MenuEventFilter::GetMenuEventFilterDelegate.
> 
> Or just have ViewsDelegate::HandleKeyEventOnMenu
> 
> (we probably don't need separate Delegate instance)

sky@: Does this suggestion sound good to you?

[sky, 2015-09-08 15:24:45]

On Fri, Sep 4, 2015 at 11:49 AM,  <afakhry@chromium.org> wrote:
> On 2015/09/04 18:32:10, oshima wrote:
>
>
https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
>>
>> File ui/views/controls/menu/menu_message_loop_aura.cc (right):
>
>
>
>
https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
>>
>> ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
>> menu_event_filter(controller, filter_delegate);
>
>
>> > let ViewsDelegate return the MenuEventFilter::Delegate instead of
>> > MenuEventFilter::GetMenuEventFilterDelegate.
>
>
>> Or just have ViewsDelegate::HandleKeyEventOnMenu
>
>
>> (we probably don't need separate Delegate instance)

Yes. A single function as suggested by Oshima on ViewsDelegate SGTM.

  -Scott

>
>
> sky@: Does this suggestion sound good to you?
>
> https://codereview.chromium.org/1138523006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[afakhry, 2015-09-08 21:31:46]

sky@ & oshima@: Could you please take a look?
I'm now using ViewsDelegate and only creating the filter for non-nested menus.

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:71: // This a deleter to
enable us to use a scoped_ptr of a MenuController, since
On 2015/09/04 14:40:23, sky wrote:
> On 2015/09/04 01:49:42, afakhry wrote:
> > On 2015/09/03 17:23:21, sky wrote:
> > > Fix the grammar here. I can't parse what you're trying to say.
> > > 
> > > IMO what you have is overkill. Just use delete in your test.
> > 
> > I modified the comment. I admit it can be considered as overkill, but I
> thought
> > using scoped_ptr is less error prone. Anyway, please let me know if you
still
> > don't like it, I'm fine with changing it.
> 
> scoped_ptr is certainly a good thing, but when you have to write 10 lines of
> cryptic code just to make it useful for one call site, that can easily be
hidden
> in a SetUp/TearDown, it's overkill. Use delete.

Done.

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:169: MenuEventFilter
menu_event_filter(controller, filter_delegate);
On 2015/09/04 18:32:10, oshima wrote:
> 
> > let ViewsDelegate return the MenuEventFilter::Delegate instead of
> > MenuEventFilter::GetMenuEventFilterDelegate.
> 
> Or just have ViewsDelegate::HandleKeyEventOnMenu
> 
> (we probably don't need separate Delegate instance)

Done.

[oshima, 2015-09-08 21:43:08]

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:132: #if defined(USE_ASH)
entire ProcessAcceleratorNow function can be ifdef'ed right?

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.h:32: 
remove new line

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:98: if (menu_controller_) {
do you need this if?

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:19: class VIEWS_EXPORT
MenuEventFilter : public ui::EventHandler {
Is there any reason why this is called MenuEventFilter, not MenuEventHandler?
Many handlers have name XxxFilter because ui::EventHandler used to be called
EventFilter, but new classes should be called XxxHandler IMO.

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:172: menu_event_filter.get());
You can add/remove the filter in MenuEventFilter's ctor/dtor?

[afakhry, 2015-09-08 23:34:38]

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:132: #if defined(USE_ASH)
On 2015/09/08 21:43:08, oshima wrote:
> entire ProcessAcceleratorNow function can be ifdef'ed right?

Done.

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/680001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.h:32: 
On 2015/09/08 21:43:08, oshima wrote:
> remove new line

Done.

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:98: if (menu_controller_) {
On 2015/09/08 21:43:08, oshima wrote:
> do you need this if?

No. removed.

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_filter.h (right):

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_filter.h:19: class VIEWS_EXPORT
MenuEventFilter : public ui::EventHandler {
On 2015/09/08 21:43:08, oshima wrote:
> Is there any reason why this is called MenuEventFilter, not MenuEventHandler?
> Many handlers have name XxxFilter because ui::EventHandler used to be called
> EventFilter, but new classes should be called XxxHandler IMO.

Done.

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/680001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:172: menu_event_filter.get());
On 2015/09/08 21:43:08, oshima wrote:
> You can add/remove the filter in MenuEventFilter's ctor/dtor?

Done.

[oshima, 2015-09-08 23:51:59]

lgtm my bits

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.cc (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.cc:55: (flags & kKeyFlagsMask) == 0) {
You can look into this in a follow up CL, but I think this probably should check
against IsSystemKeyModifier & CONTROL_DOWN ?

[sky, 2015-09-09 00:01:59]

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.cc (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.cc:30: if (!menu_controller)
Can this really happen?
DCHECK?

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.h (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.h:19: class VIEWS_EXPORT
MenuEventHandler : public ui::EventHandler {
Why does this code need to be in views? Isn't it ash specific? That's the whole
reason I wanted a function on ViewsDelegate.

https://codereview.chromium.org/1138523006/diff/710001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/views_delegat...
ui/views/views_delegate.h:105: // Will perform accelerator handling when for
accelerators pressed while a
I think you mean:
Invoked when an accelerator is pressed and a menu is shown. Intended as a way
for accelerators to be processed while a menu is showing.
Also, this is aura and further linux specific, right?

[oshima, 2015-09-09 00:11:35]

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.h (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.h:19: class VIEWS_EXPORT
MenuEventHandler : public ui::EventHandler {
On 2015/09/09 00:01:59, sky wrote:
> Why does this code need to be in views? Isn't it ash specific? That's the
whole
> reason I wanted a function on ViewsDelegate.

This handles mnemonics. The mnemonics is a part of view's menu functionality, so
it should be in views, isn't it?

[afakhry, 2015-09-09 00:16:00]

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.h (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.h:19: class VIEWS_EXPORT
MenuEventHandler : public ui::EventHandler {
On 2015/09/09 00:11:35, oshima wrote:
> On 2015/09/09 00:01:59, sky wrote:
> > Why does this code need to be in views? Isn't it ash specific? That's the
> whole
> > reason I wanted a function on ViewsDelegate.
> 
> This handles mnemonics. The mnemonics is a part of view's menu functionality,
so
> it should be in views, isn't it?

Yes, as oshima mentioned, this has to handle mnemonics (by calling
MenuController::SelectByChar()) as well as menu key handling like navigation
using arrows and Esc to close and so on (by calling
MenuController::OnKeyDown()). These are not ash specific but for anything using
views menus. This handler should replace MenuEventDispatcher.

[afakhry, 2015-09-09 02:31:34]

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.cc (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.cc:30: if (!menu_controller)
On 2015/09/09 00:01:59, sky wrote:
> Can this really happen?
> DCHECK?

DCHECK()ed

https://codereview.chromium.org/1138523006/diff/710001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/710001/ui/views/views_delegat...
ui/views/views_delegate.h:105: // Will perform accelerator handling when for
accelerators pressed while a
On 2015/09/09 00:01:59, sky wrote:
> I think you mean:
> Invoked when an accelerator is pressed and a menu is shown. Intended as a way
> for accelerators to be processed while a menu is showing.
> Also, this is aura and further linux specific, right?

Done for the comment.
The implementation in ChromeViewsDelegate is currently ash specific.

[sky, 2015-09-09 15:44:52]

I'm still confused about why this is not applicable for windows. The code you
pointed at doesn't handle accelerators either.

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:144:
internal::MenuMessagePumpDispatcher nested_dispatcher(controller);
On 2015/09/04 01:49:42, afakhry wrote:
> On 2015/09/03 17:23:21, sky wrote:
> > Why is the filtering specific to non-windows platforms?
> 
> On Windows, we have the MenuMessagePumpDispatcher that has to handle certain
> Windows specific key events, and the fact that Windows sends two event types
for
> each key press WM_KEYDOWN and WM_CHAR. 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

I don't see how that handles the ash accelerators when run in ash mode though?

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.cc (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.cc:46: } else {
Style guide says no else after return.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.cc:50: // Below we'll handle the
mnemonics only if neither Alt nor Ctrl is
This comment is documenting the code and doesn't tell me why the processing is
doing what it is doing. Document the why. Why is it important to only handle
when alt nor ctrl is down?

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.h (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.h:19: class VIEWS_EXPORT
MenuEventHandler : public ui::EventHandler {
MenuEventHandler, and your description, are too generic. This class is primarily
about handling key events. A better name is MenuKeyEventHandler.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:20: #include
"ui/views/controls/menu/menu_event_handler.h"
Shouldn't this be in the not win section below? Say line 32?

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:168: // No need to recreate the
MenuEventHandler for every nested menu.
Again, this is documenting the code not the why.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views.gyp#new...
ui/views/views.gyp:377: 'controls/menu/menu_event_handler.cc',
Isn't this only appropriate for non-windows?

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views_delegat...
ui/views/views_delegate.h:107: virtual void HandleKeyEventOnMenu(ui::KeyEvent*
event);
In looking at how you use this function I think it would be better to have the
following:

enum class ProcessAcceleratorResult {
  // The accelerator was handled and the menu should close.
  HANDLED,

  // The accelerator was not handled.
  NOT_HANDLED
};
virtual AcceleratorResult ProcessAcceleratorWhileMenuShowing(const
ui::Accelerator& accelerator);

[afakhry, 2015-09-09 20:21:33]

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/620001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:144:
internal::MenuMessagePumpDispatcher nested_dispatcher(controller);
On 2015/09/09 15:44:52, sky wrote:
> On 2015/09/04 01:49:42, afakhry wrote:
> > On 2015/09/03 17:23:21, sky wrote:
> > > Why is the filtering specific to non-windows platforms?
> > 
> > On Windows, we have the MenuMessagePumpDispatcher that has to handle certain
> > Windows specific key events, and the fact that Windows sends two event types
> for
> > each key press WM_KEYDOWN and WM_CHAR. 
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> 
> I don't see how that handles the ash accelerators when run in ash mode though?

Exactly, because the current code is very unclear. That's why we want to get rid
of these nested dispatchers for Windows too, Probably in a follow up CL. This CL
is already too big.

To explain how Windows accelerators handling works. We create the
DispatcherRunLoop
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/public/dispa...
which will ask the DispatcherClient to prepare the runloop closures. This will
in this case create a NestedAcceleratorDispatcher
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/nested_...
Which will in turn create a NestedAcceleratorDispatcherWin
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/nested_...,
and that will handle the accelerator
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/nested_...

This is the same thing for Linux and chromeos rightnow, and the MenuEventHandler
is supposed to replace all of that.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.cc (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.cc:46: } else {
On 2015/09/09 15:44:52, sky wrote:
> Style guide says no else after return.

Done.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.cc:50: // Below we'll handle the
mnemonics only if neither Alt nor Ctrl is
On 2015/09/09 15:44:52, sky wrote:
> This comment is documenting the code and doesn't tell me why the processing is
> doing what it is doing. Document the why. Why is it important to only handle
> when alt nor ctrl is down?

The why here is not so clear to me. I think it was a design decision. See the
original code here
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

I think the reason is that for example Ctrl+t is an accelerator but 't' only is
a mnemonic. I improved the comment.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.h (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.h:19: class VIEWS_EXPORT
MenuEventHandler : public ui::EventHandler {
On 2015/09/09 15:44:52, sky wrote:
> MenuEventHandler, and your description, are too generic. This class is
primarily
> about handling key events. A better name is MenuKeyEventHandler.

Even with the OnTouchEvent() present?

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_message_loop_aura.cc (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:20: #include
"ui/views/controls/menu/menu_event_handler.h"
On 2015/09/09 15:44:52, sky wrote:
> Shouldn't this be in the not win section below? Say line 32?

Done.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_message_loop_aura.cc:168: // No need to recreate the
MenuEventHandler for every nested menu.
On 2015/09/09 15:44:52, sky wrote:
> Again, this is documenting the code not the why.

Done.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views.gyp#new...
ui/views/views.gyp:377: 'controls/menu/menu_event_handler.cc',
On 2015/09/09 15:44:52, sky wrote:
> Isn't this only appropriate for non-windows?

The intend is to use it for windows too in a follow up CL.

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/views_delegat...
ui/views/views_delegate.h:107: virtual void HandleKeyEventOnMenu(ui::KeyEvent*
event);
On 2015/09/09 15:44:52, sky wrote:
> In looking at how you use this function I think it would be better to have the
> following:
> 
> enum class ProcessAcceleratorResult {
>   // The accelerator was handled and the menu should close.
>   HANDLED,
> 
>   // The accelerator was not handled.
>   NOT_HANDLED
> };
> virtual AcceleratorResult ProcessAcceleratorWhileMenuShowing(const
> ui::Accelerator& accelerator);

Done.

[sky, 2015-09-09 21:30:55]

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.h (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.h:19: class VIEWS_EXPORT
MenuEventHandler : public ui::EventHandler {
On 2015/09/09 20:21:33, afakhry wrote:
> On 2015/09/09 15:44:52, sky wrote:
> > MenuEventHandler, and your description, are too generic. This class is
> primarily
> > about handling key events. A better name is MenuKeyEventHandler.
> 
> Even with the OnTouchEvent() present?

While not 100% accurate I feel that MenuKeyEventHandler is more descriptive.
Particularly because menu already handles many events, so to name it
MenuEventHandler implies it handles all events.

[afakhry, 2015-09-09 23:59:52]

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
File ui/views/controls/menu/menu_event_handler.h (right):

https://codereview.chromium.org/1138523006/diff/730001/ui/views/controls/menu...
ui/views/controls/menu/menu_event_handler.h:19: class VIEWS_EXPORT
MenuEventHandler : public ui::EventHandler {
On 2015/09/09 21:30:55, sky wrote:
> On 2015/09/09 20:21:33, afakhry wrote:
> > On 2015/09/09 15:44:52, sky wrote:
> > > MenuEventHandler, and your description, are too generic. This class is
> > primarily
> > > about handling key events. A better name is MenuKeyEventHandler.
> > 
> > Even with the OnTouchEvent() present?
> 
> While not 100% accurate I feel that MenuKeyEventHandler is more descriptive.
> Particularly because menu already handles many events, so to name it
> MenuEventHandler implies it handles all events.

Done.

[sky, 2015-09-10 16:20:33]

https://codereview.chromium.org/1138523006/diff/790001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/790001/ui/views/views_delegat...
ui/views/views_delegate.h:76: enum ProcessMenuAcceleratorResult {
enum class

https://codereview.chromium.org/1138523006/diff/790001/ui/views/views_delegat...
ui/views/views_delegate.h:79: ACCELERATOR_HANDLED,
The names and use is counter what I would have expected. How about renaming to
CLOSE_MENU and LEAVE_MENU_OPEN to better clarify what happens.

[afakhry, 2015-09-10 17:00:29]

https://codereview.chromium.org/1138523006/diff/790001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/790001/ui/views/views_delegat...
ui/views/views_delegate.h:76: enum ProcessMenuAcceleratorResult {
On 2015/09/10 16:20:33, sky wrote:
> enum class

Done.

https://codereview.chromium.org/1138523006/diff/790001/ui/views/views_delegat...
ui/views/views_delegate.h:79: ACCELERATOR_HANDLED,
On 2015/09/10 16:20:33, sky wrote:
> The names and use is counter what I would have expected. How about renaming to
> CLOSE_MENU and LEAVE_MENU_OPEN to better clarify what happens.

Done.

[sky, 2015-09-10 18:05:00]

LGTM

[pkotwicz, 2015-09-10 18:38:04]

pkotwicz@chromium.org changed reviewers:
+ tdresser@chromium.org

[pkotwicz, 2015-09-10 18:38:06]

tdresser@ Can you please take a look at MenuKeyEventHandler::OnTouchEvent()

My understanding is that
ui::TouchEvent::set_should_remove_native_touch_id_mapping() does not need to be
called because ui::EventFromNative(event) is not called the way it is in
MenuEventDispatcher::DispatchEvent()

[pkotwicz, 2015-09-10 19:13:00]

Just a few nits

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.h:123: // after the menu exits).
Nit: I think this would make for a simpler comment

How about:
"Returns true if the menu should close in order to perform the accelerator."

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:489: DISABLE_GPU_WATCHDOG,
Did you get UX feedback on the accelerators which should be on this list?
DISABLE_GPU_WATCHDOG is a weird accelerator to have in the list.

https://codereview.chromium.org/1138523006/diff/810001/ui/views/controls/menu...
File ui/views/controls/menu/menu_key_event_handler.cc (right):

https://codereview.chromium.org/1138523006/diff/810001/ui/views/controls/menu...
ui/views/controls/menu/menu_key_event_handler.cc:64: accelerator);
Nit: It would make me sleep easier if this block of code was only run when
menu_controller->exit_type() != MenuController::EXIT_NONE

https://codereview.chromium.org/1138523006/diff/810001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/810001/ui/views/views_delegat...
ui/views/views_delegate.h:115: virtual ProcessMenuAcceleratorResult
ProcessAcceleratorWhileMenuShowing(
Nit: Can you please add a comment to this function?

[afakhry, 2015-09-10 20:57:13]

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.h:123: // after the menu exits).
On 2015/09/10 19:13:00, pkotwicz wrote:
> Nit: I think this would make for a simpler comment
> 
> How about:
> "Returns true if the menu should close in order to perform the accelerator."

Done.

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/810001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:489: DISABLE_GPU_WATCHDOG,
On 2015/09/10 19:13:00, pkotwicz wrote:
> Did you get UX feedback on the accelerators which should be on this list?
> DISABLE_GPU_WATCHDOG is a weird accelerator to have in the list.

I just spoke to Albert, we can definitely take UX feedback, however, we think
that we can proceed without it for now, especially that the list is short and
adding or removing stuff from it later is not a complicated thing. 
I agree about the DISABLE_GPU_WATCHDOG being weird. I'm not sure why it ended up
being on this list. I removed it. Thanks for pointing it out!

https://codereview.chromium.org/1138523006/diff/810001/ui/views/controls/menu...
File ui/views/controls/menu/menu_key_event_handler.cc (right):

https://codereview.chromium.org/1138523006/diff/810001/ui/views/controls/menu...
ui/views/controls/menu/menu_key_event_handler.cc:64: accelerator);
On 2015/09/10 19:13:00, pkotwicz wrote:
> Nit: It would make me sleep easier if this block of code was only run when
> menu_controller->exit_type() != MenuController::EXIT_NONE

Makes sense. However you probably meant 
"menu_controller->exit_type() == MenuController::EXIT_NONE" i.e process
accelerators only if menu hadn't already exited.

Done!

https://codereview.chromium.org/1138523006/diff/810001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/1138523006/diff/810001/ui/views/views_delegat...
ui/views/views_delegate.h:115: virtual ProcessMenuAcceleratorResult
ProcessAcceleratorWhileMenuShowing(
On 2015/09/10 19:13:00, pkotwicz wrote:
> Nit: Can you please add a comment to this function?

Done.

[pkotwicz, 2015-09-10 21:43:13]

LGTM once you get tdresser@'s feedback

[pkotwicz, 2015-09-10 21:58:49]

https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:494: TOGGLE_WIFI,
TOGGLE_WIFI is a weird accelerator to include in the list too. All of the other
accelerators look reasonable. (I would vote to remove MEDIA_NEXT_TRACK,
MEDIA_PLAY_PAUSE and MEDIA_PREV_TRACK from this list too but I can see why
someone may want to include it)

[afakhry, 2015-09-10 23:31:02]

https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:494: TOGGLE_WIFI,
On 2015/09/10 21:58:49, pkotwicz wrote:
> TOGGLE_WIFI is a weird accelerator to include in the list too. All of the
other
> accelerators look reasonable. (I would vote to remove MEDIA_NEXT_TRACK,
> MEDIA_PLAY_PAUSE and MEDIA_PREV_TRACK from this list too but I can see why
> someone may want to include it)

Remember that this list contains actions that will be performed while the menu
is kept open. I don't see a reason to exclude TOGGLE_WIFI. Why should the
accelerator for toggling the wifi close the menu and be performed after that?

[pkotwicz, 2015-09-10 23:37:33]

https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:494: TOGGLE_WIFI,
The way I think of it is that this list should be kept as small as possible. We
need to make sure that none of these actions have bad interactions with the menu
being open. (I doubt that TOGGLE_WIFI has a bad interaction with the menu being
open) When I look at the list, I ask myself for each accelerator: "Why should
the menu stay open when the accelerator is pressed?"

[oshima, 2015-09-11 00:58:37]

On 2015/09/10 23:37:33, pkotwicz wrote:
>
https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
> File ash/accelerators/accelerator_table.cc (right):
> 
>
https://codereview.chromium.org/1138523006/diff/830001/ash/accelerators/accel...
> ash/accelerators/accelerator_table.cc:494: TOGGLE_WIFI,
> The way I think of it is that this list should be kept as small as possible.
We
> need to make sure that none of these actions have bad interactions with the
menu
> being open. (I doubt that TOGGLE_WIFI has a bad interaction with the menu
being
> open) When I look at the list, I ask myself for each accelerator: "Why should
> the menu stay open when the accelerator is pressed?"

I don't have strong opinion about whether or not this particular one should be
in the list or not, but I think what a user would ask is rather:

"why should this accelerator close the menu?"

in a same way that a user would ask

"why this accelerator doesn't work while menu is open?"

Sometimes, we have to close the menu even if it's not clear to user, but i think
minimizing such case is good thing in general.

[tdresser, 2015-09-11 12:24:18]

Re #94, ui::TouchEvent::set_should_remove_native_touch_id_mapping() only needs
to be called on copies on native events.
It doesn't look like you're copying the touch event, so this shouldn't be
called.

[pkotwicz, 2015-09-11 14:50:42]

oshima@: Fair enough

[afakhry, 2015-09-11 20:09:56]

Following tdresser@'s comment. I removed OnTouchEvent(). Thanks to pkotwicz@ who
pointed it out!

[sky, 2015-09-11 20:23:40]

And now the name I suggested is even better! :)

On Fri, Sep 11, 2015 at 1:09 PM,  <afakhry@chromium.org> wrote:
> Following tdresser@'s comment. I removed OnTouchEvent(). Thanks to pkotwicz@
> who
> pointed it out!
>
> https://codereview.chromium.org/1138523006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[afakhry, 2015-09-11 22:56:57]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-09-11 22:56:58]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
sky@chromium.org, pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/1138523006/#ps850001
(title: "Removed OnTouchEvent()")

[commit-bot: I haz the power, 2015-09-11 22:58:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138523006/850001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1138523006/850001

[commit-bot: I haz the power, 2015-09-11 23:20:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 23:20:36]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2015-09-14 16:07:59]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-09-14 16:08:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138523006/850001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1138523006/850001

[commit-bot: I haz the power, 2015-09-14 17:07:50]

Committed patchset #42 (id:850001)

[commit-bot: I haz the power, 2015-09-14 17:08:42]

Patchset 42 (id:??) landed as
https://crrev.com/692024d9d32cfe3ddc01143243c4b99e43261954
Cr-Commit-Position: refs/heads/master@{#348632}

[commit-bot: I haz the power, 2015-09-23 12:33:12]

Patchset 42 (id:??) landed as
https://crrev.com/692024d9d32cfe3ddc01143243c4b99e43261954
Cr-Commit-Position: refs/heads/master@{#348632}