Remove MenuMessagePumpDispatcher

Remove MenuMessagePumpDispatcher

MenuMessagePumpDispatcher is used on Windows to handle keyboard events
and some other events in menu message loops. In order to have fully
async menus, we need to get rid of it. Instead, we can use
MenuKeyEventHandler which is used on Chrome OS and Linux with some
modifications. Following are messages that are handled in
MenuMessagePumpDispatcher and how they can be handled after removing
this class:

 - WM_KEYDOWN: This will be handled in MenuKeyEventHandler::OnKeyEvent.

 - WM_CHAR: This is used to handle "translated" characters for mnemonics
   according to the active keyboard layout. We can achieve this in
   MenuKeyEventHandler by using event->GetCharacter() instead of
   ui::DomCodeToUsLayoutCharacter().

 - WM_SYSKEYDOWN: This happens when Alt or F10 keys are pressed
   (according to MSDN). We can handle these keys in MenuKeyEventHandler.

 - WM_CANCELMODE: This is already handled in
   ActivationChangeObserverImpl::OnCancelMode() which is used in
   MenuMessageLoopAura.

BUG=564255, 566019
Committed: https://crrev.com/0df5837131a55ff6560a80b181ef90e620ea8092
Cr-Commit-Position: refs/heads/master@{#371894}

[mohsen, 2016-01-25 04:59:51]

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

[mohsen, 2016-01-25 04:59:51]

Please take a look...

[sky, 2016-01-25 17:23:12]

I'm pretty sure we do, but just to make sure you verified it. Do we have test
coverage of this code? Most likely in
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test?

[mohsen, 2016-01-26 04:38:08]

There are a few tests testing basic keyboard functionality for menus including
arrow keys and also Escape/Return/Menu keys. However, I couldn't find anything
for system keys (Alt/F10), cancel mode, and non-US mnemonics. Adding test for
Alt/F10 seems easy, but I'm not sure how to test cancel mode and non-US
mnemonics. Any idea?

[sky, 2016-01-26 04:54:11]

I can only think to use alt-tab, but I suspect that would introduce
flake. So, don't worry about it.

  -Scott

On Mon, Jan 25, 2016 at 8:38 PM,  <mohsen@chromium.org> wrote:
> There are a few tests testing basic keyboard functionality for menus
> including
> arrow keys and also Escape/Return/Menu keys. However, I couldn't find
> anything
> for system keys (Alt/F10), cancel mode, and non-US mnemonics. Adding test
> for
> Alt/F10 seems easy, but I'm not sure how to test cancel mode and non-US
> mnemonics. Any idea?
>
> https://codereview.chromium.org/1625313002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2016-01-26 05:37:32]

In Alt-Tab, as soon as you press down Alt key, menu gets hidden, so it doesn't
end up generating WM_CANCELMODE for the menu.

[mohsen, 2016-01-26 05:47:48]

Can we send WM_CANCELMODE to the HWND explicitly in a test?

[sky, 2016-01-26 14:19:45]

You can probably send WM_CANCELMODE via PostMessage/SendMessage.

On Mon, Jan 25, 2016 at 9:47 PM,  <mohsen@chromium.org> wrote:
> Can we send WM_CANCELMODE to the HWND explicitly in a test?
>
> https://codereview.chromium.org/1625313002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2016-01-27 00:01:31]

Patchset #4 (id:60001) has been deleted

[mohsen, 2016-01-27 02:01:57]

OK. I've added tests for Alt/F10 and WM_CANCELMODE. Please take a look...

[mohsen, 2016-01-27 16:19:29]

Description was changed from

==========
Remove MenuMessagePumpDispatcher

MenuMessagePumpDispatcher is used on Windows to handle keyboard events
and some other events in menu message loops. In order to have fully
async menus, we need to get rid of it. Instead, we can use
MenuKeyEventHandler which is used on Chrome OS and Linux with some
modifications. Following are messages that are handled in
MenuMessagePumpDispatcher and how they can be handled after removing
this class:

 - WM_KEYDOWN: This will be handled in MenuKeyEventHandler::OnKeyEvent.

 - WM_CHAR: This is used to handle "translated" characters for mnemonics
   according to the active keyboard layout. We can achieve this in
   MenuKeyEventHandler by using event->GetCharacter() instead of
   ui::DomCodeToUsLayoutCharacter().

 - WM_SYSKEYDOWN: This happens when Alt or F10 keys are pressed
   (according to MSDN). We can handle these keys in MenuKeyEventHandler.

 - WM_CANCELMODE: This is already handled in
   ActivationChangeObserverImpl::OnCancelMode() which is used in
   MenuMessageLoopAura.

BUG=564255
==========

to

==========
Remove MenuMessagePumpDispatcher

MenuMessagePumpDispatcher is used on Windows to handle keyboard events
and some other events in menu message loops. In order to have fully
async menus, we need to get rid of it. Instead, we can use
MenuKeyEventHandler which is used on Chrome OS and Linux with some
modifications. Following are messages that are handled in
MenuMessagePumpDispatcher and how they can be handled after removing
this class:

 - WM_KEYDOWN: This will be handled in MenuKeyEventHandler::OnKeyEvent.

 - WM_CHAR: This is used to handle "translated" characters for mnemonics
   according to the active keyboard layout. We can achieve this in
   MenuKeyEventHandler by using event->GetCharacter() instead of
   ui::DomCodeToUsLayoutCharacter().

 - WM_SYSKEYDOWN: This happens when Alt or F10 keys are pressed
   (according to MSDN). We can handle these keys in MenuKeyEventHandler.

 - WM_CANCELMODE: This is already handled in
   ActivationChangeObserverImpl::OnCancelMode() which is used in
   MenuMessageLoopAura.

BUG=564255,566019
==========

[sky, 2016-01-27 17:53:41]

LGTM

https://codereview.chromium.org/1625313002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right):

https://codereview.chromium.org/1625313002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:2288: typedef
BookmarkBarViewTest25<ui::VKEY_F10> BookmarkBarViewTest25F10;
nit: using

https://codereview.chromium.org/1625313002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:2292: typedef
BookmarkBarViewTest25<ui::VKEY_MENU> BookmarkBarViewTest25Alt;
nit: using.

[mohsen, 2016-01-27 22:14:14]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-01-27 22:14:15]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1625313002/#ps100001
(title: "Replaced typedef with using")

[commit-bot: I haz the power, 2016-01-27 22:15:11]

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

[commit-bot: I haz the power, 2016-01-27 22:22:54]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-01-27 22:23:45]

Description was changed from

==========
Remove MenuMessagePumpDispatcher

MenuMessagePumpDispatcher is used on Windows to handle keyboard events
and some other events in menu message loops. In order to have fully
async menus, we need to get rid of it. Instead, we can use
MenuKeyEventHandler which is used on Chrome OS and Linux with some
modifications. Following are messages that are handled in
MenuMessagePumpDispatcher and how they can be handled after removing
this class:

 - WM_KEYDOWN: This will be handled in MenuKeyEventHandler::OnKeyEvent.

 - WM_CHAR: This is used to handle "translated" characters for mnemonics
   according to the active keyboard layout. We can achieve this in
   MenuKeyEventHandler by using event->GetCharacter() instead of
   ui::DomCodeToUsLayoutCharacter().

 - WM_SYSKEYDOWN: This happens when Alt or F10 keys are pressed
   (according to MSDN). We can handle these keys in MenuKeyEventHandler.

 - WM_CANCELMODE: This is already handled in
   ActivationChangeObserverImpl::OnCancelMode() which is used in
   MenuMessageLoopAura.

BUG=564255,566019
==========

to

==========
Remove MenuMessagePumpDispatcher

MenuMessagePumpDispatcher is used on Windows to handle keyboard events
and some other events in menu message loops. In order to have fully
async menus, we need to get rid of it. Instead, we can use
MenuKeyEventHandler which is used on Chrome OS and Linux with some
modifications. Following are messages that are handled in
MenuMessagePumpDispatcher and how they can be handled after removing
this class:

 - WM_KEYDOWN: This will be handled in MenuKeyEventHandler::OnKeyEvent.

 - WM_CHAR: This is used to handle "translated" characters for mnemonics
   according to the active keyboard layout. We can achieve this in
   MenuKeyEventHandler by using event->GetCharacter() instead of
   ui::DomCodeToUsLayoutCharacter().

 - WM_SYSKEYDOWN: This happens when Alt or F10 keys are pressed
   (according to MSDN). We can handle these keys in MenuKeyEventHandler.

 - WM_CANCELMODE: This is already handled in
   ActivationChangeObserverImpl::OnCancelMode() which is used in
   MenuMessageLoopAura.

BUG=564255,566019

Committed: https://crrev.com/0df5837131a55ff6560a80b181ef90e620ea8092
Cr-Commit-Position: refs/heads/master@{#371894}
==========

[commit-bot: I haz the power, 2016-01-27 22:23:46]

Patchset 5 (id:??) landed as
https://crrev.com/0df5837131a55ff6560a80b181ef90e620ea8092
Cr-Commit-Position: refs/heads/master@{#371894}