Allows ash accelerators in wrench menu

Translates events so that accelerators which require ET_TRANSLATED_KEY_RELEASE and ET_TRANSLATED_KEY_PRESS
are triggered.

Translation is always done, so nested dispatcher should not be used for any widgets which have editable content.

BUG=113247
TEST=NestedDispatcherTest.AcceleratorHandled

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

[oshima, 2012-02-29 19:28:41]

lgtm with a few nits

http://codereview.chromium.org/9538015/diff/4001/ash/accelerators/accelerator...
File ash/accelerators/accelerator_dispatcher_win.cc (right):

http://codereview.chromium.org/9538015/diff/4001/ash/accelerators/accelerator...
ash/accelerators/accelerator_dispatcher_win.cc:25: bool
AcceleratorDispatcher::Dispatch(const MSG& msg) {
can you file a bug and add TODO to consolidate win and linux? You can assign
that to me.

http://codereview.chromium.org/9538015/diff/4001/ash/accelerators/nested_disp...
File ash/accelerators/nested_dispatcher_controller_unittest.cc (right):

http://codereview.chromium.org/9538015/diff/4001/ash/accelerators/nested_disp...
ash/accelerators/nested_dispatcher_controller_unittest.cc:67: 
private:

http://codereview.chromium.org/9538015/diff/4001/ash/accelerators/nested_disp...
ash/accelerators/nested_dispatcher_controller_unittest.cc:163: EXPECT_EQ(0,
inner_dispatcher.num_key_events_dispatched());
please check if the accelerator is pressed/handled as well.

[pkotwicz, 2012-02-29 20:35:05]

Sky for any additional nits

[sky, 2012-02-29 22:04:17]

Do we really have accelerators that require translation?

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
File ash/accelerators/accelerator_dispatcher.h (right):

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
ash/accelerators/accelerator_dispatcher.h:19: // Inner dispatcher must represent
widget with no editable content.
This comment makes no sense to me.

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
File ash/accelerators/accelerator_dispatcher_win.cc (right):

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
ash/accelerators/accelerator_dispatcher_win.cc:32: if(msg.message == WM_KEYDOWN
|| msg.message == WM_SYSKEYDOWN ||
nit: 'if(' -> 'if ('

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
File ash/accelerators/nested_dispatcher_controller_unittest.cc (right):

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
ash/accelerators/nested_dispatcher_controller_unittest.cc:57: TestTarget() :
accelerator_pressed_count_(0) {};
nit: no ; here and next line.

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
ash/accelerators/nested_dispatcher_controller_unittest.cc:166:
base::TimeDelta::FromMilliseconds(100));
Why does this need to delay 100ms? Can't it use PostDelayedTask with no time?

[oshima, 2012-02-29 22:19:50]

On 2012/02/29 22:04:17, sky wrote:
> Do we really have accelerators that require translation?

Yes, see ash/accelerator/accelerator_controller.cc. This is necessary
to distinguish accelerator that should happen before and after IME
processes key event.

> 
>
http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
> File ash/accelerators/accelerator_dispatcher.h (right):
> 
>
http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
> ash/accelerators/accelerator_dispatcher.h:19: // Inner dispatcher must
represent
> widget with no editable content.
> This comment makes no sense to me.
> 
>
http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
> File ash/accelerators/accelerator_dispatcher_win.cc (right):
> 
>
http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/accelerator...
> ash/accelerators/accelerator_dispatcher_win.cc:32: if(msg.message ==
WM_KEYDOWN
> || msg.message == WM_SYSKEYDOWN ||
> nit: 'if(' -> 'if ('
> 
>
http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
> File ash/accelerators/nested_dispatcher_controller_unittest.cc (right):
> 
>
http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
> ash/accelerators/nested_dispatcher_controller_unittest.cc:57: TestTarget() :
> accelerator_pressed_count_(0) {};
> nit: no ; here and next line.
> 
>
http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
> ash/accelerators/nested_dispatcher_controller_unittest.cc:166:
> base::TimeDelta::FromMilliseconds(100));
> Why does this need to delay 100ms? Can't it use PostDelayedTask with no time?

[pkotwicz, 2012-03-01 23:56:37]

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
File ash/accelerators/nested_dispatcher_controller_unittest.cc (right):

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
ash/accelerators/nested_dispatcher_controller_unittest.cc:166:
base::TimeDelta::FromMilliseconds(100));
The delay is arbitrary. However, I beleive the delay is useful to avoid
flakiness. DispatchKeyReleaseA cannot occur before RunWithDispatch for the test
to be successful.
I don't know if posting a non delayed task guarantees this.

[oshima, 2012-03-02 00:12:20]

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
File ash/accelerators/nested_dispatcher_controller_unittest.cc (right):

http://codereview.chromium.org/9538015/diff/1015/ash/accelerators/nested_disp...
ash/accelerators/nested_dispatcher_controller_unittest.cc:166:
base::TimeDelta::FromMilliseconds(100));
Sorry I missed this in first review.

On 2012/03/01 23:56:37, pkotwicz wrote:
> The delay is arbitrary. However, I beleive the delay is useful to avoid
> flakiness. DispatchKeyReleaseA cannot occur before RunWithDispatch for the
test
> to be successful.
> I don't know if posting a non delayed task guarantees this.

 DispatchKeyReleaseA is already using PostNativeEvent so you should be able to
just call DispatchKeyReleaseA.
You don't even need to post task here. Or does this fail without it?

[oshima, 2012-03-02 18:28:27]

lgtm

[pkotwicz, 2012-03-02 19:20:47]

sky for rubberstamp

[sky, 2012-03-02 20:39:37]

LGTM

[commit-bot: I haz the power, 2012-03-05 14:59:25]

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

[commit-bot: I haz the power, 2012-03-05 18:05:12]

Change committed as 124963