Make accelerators not to work when the keyboard overlay is shown.

Make accelerators not to work when the keyboard overlay is shown.

- Refactor PartialScreenshotEventFiler so that it can be reused, and renamed it to OverlayEventFilter
- Use OverlayEventFilter for the keyboard overlay

BUG=129834
TEST=Manually check accelerators do not work when the keyboard overlay.


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

[mazda, 2012-07-26 00:21:29]

Hi guys,

Could you review this change?

derat: ash
mukai: partial screenshot code
satorux: chrome/browser/resources/chromeos

Thanks,

[satorux1, 2012-07-26 00:27:42]

chrome/browser/resources/chromeos  lgtm

[Daniel Erat, 2012-07-26 00:44:54]

partial review

http://codereview.chromium.org/10825026/diff/3001/ash/keyboard_overlay/keyboa...
File ash/keyboard_overlay/keyboard_overlay_view.cc (right):

http://codereview.chromium.org/10825026/diff/3001/ash/keyboard_overlay/keyboa...
ash/keyboard_overlay/keyboard_overlay_view.cc:50: bool
KeyboardOverlayView::IsCancelKeyEvent(aura::KeyEvent* event) {
nit: KeyEventShouldCancel()?  KeyEventCancels()?  IsCancellingKeyEvent()?

http://codereview.chromium.org/10825026/diff/3001/ash/keyboard_overlay/keyboa...
ash/keyboard_overlay/keyboard_overlay_view.cc:52: if ((event->type() ==
ui::ET_KEY_PRESSED) &&
just check this before the loop?

[mazda, 2012-07-26 01:17:06]

Thank you for the comments. Please have another look.

http://codereview.chromium.org/10825026/diff/3001/ash/keyboard_overlay/keyboa...
File ash/keyboard_overlay/keyboard_overlay_view.cc (right):

http://codereview.chromium.org/10825026/diff/3001/ash/keyboard_overlay/keyboa...
ash/keyboard_overlay/keyboard_overlay_view.cc:50: bool
KeyboardOverlayView::IsCancelKeyEvent(aura::KeyEvent* event) {
On 2012/07/26 00:44:54, Daniel Erat wrote:
> nit: KeyEventShouldCancel()?  KeyEventCancels()?  IsCancellingKeyEvent()?

Changed it to IsCancelingKeyEvent.

http://codereview.chromium.org/10825026/diff/3001/ash/keyboard_overlay/keyboa...
ash/keyboard_overlay/keyboard_overlay_view.cc:52: if ((event->type() ==
ui::ET_KEY_PRESSED) &&
On 2012/07/26 00:44:54, Daniel Erat wrote:
> just check this before the loop?

Done.

[Jun Mukai, 2012-07-26 01:19:26]

lgtm

http://codereview.chromium.org/10825026/diff/1013/ash/wm/partial_screenshot_v...
File ash/wm/partial_screenshot_view.cc (right):

http://codereview.chromium.org/10825026/diff/1013/ash/wm/partial_screenshot_v...
ash/wm/partial_screenshot_view.cc:74: return (event->key_code() ==
ui::VKEY_ESCAPE);
outer paren is not necessary

[Daniel Erat, 2012-07-26 01:52:51]

lgtm

http://codereview.chromium.org/10825026/diff/1013/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10825026/diff/1013/ash/shell.h#newcode85
ash/shell.h:85: class DisplayController;
nit: not your fault, but mind fixing the alphabetization here?

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter.cc
File ash/wm/overlay_event_filter.cc (right):

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.cc:30: // press or release before showing the
screenshot view. This is important not
nit: s/screenshot view/overlay/

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.cc:46: // taking a screenshot.
nit: s/taking a screenshot/displaying an overlay/

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter.h
File ash/wm/overlay_event_filter.h (right):

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:16: // EventFilter for "overlay window", which
intercepts events before they are
nit: s/for /for the /

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:17: // processed in usual paths (e.g. the partial
screenshot UI, the keyboard
nit: "processed by the usual path ..."

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:18: // overlay).  It does nothing for the first
time, but works when |Activate()|
nit: "It does nothing the first time..."

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:29: virtual void Cancel() = 0;
nit: mind adding blank lines between methods?

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:30: // Returns true if |event| is a key event to
cancel.
nit: "Returns true if the overlay should be canceled in response to |event|."

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:39: // Start the filtering of events.  It also
notifies the specified
nit: s/Start/Starts/

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:45: // End the filtering of events.
nit: s/End/Ends/

[mazda, 2012-07-26 02:09:08]

Thank you for the detailed review! I addressed all the comments.

http://codereview.chromium.org/10825026/diff/1013/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10825026/diff/1013/ash/shell.h#newcode85
ash/shell.h:85: class DisplayController;
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: not your fault, but mind fixing the alphabetization here?

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter.cc
File ash/wm/overlay_event_filter.cc (right):

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.cc:30: // press or release before showing the
screenshot view. This is important not
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: s/screenshot view/overlay/

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.cc:46: // taking a screenshot.
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: s/taking a screenshot/displaying an overlay/

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter.h
File ash/wm/overlay_event_filter.h (right):

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:16: // EventFilter for "overlay window", which
intercepts events before they are
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: s/for /for the /

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:17: // processed in usual paths (e.g. the partial
screenshot UI, the keyboard
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: "processed by the usual path ..."

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:18: // overlay).  It does nothing for the first
time, but works when |Activate()|
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: "It does nothing the first time..."

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:29: virtual void Cancel() = 0;
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: mind adding blank lines between methods?

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:30: // Returns true if |event| is a key event to
cancel.
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: "Returns true if the overlay should be canceled in response to |event|."

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:39: // Start the filtering of events.  It also
notifies the specified
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: s/Start/Starts/

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/overlay_event_filter...
ash/wm/overlay_event_filter.h:45: // End the filtering of events.
On 2012/07/26 01:52:51, Daniel Erat wrote:
> nit: s/End/Ends/

Done.

http://codereview.chromium.org/10825026/diff/1013/ash/wm/partial_screenshot_v...
File ash/wm/partial_screenshot_view.cc (right):

http://codereview.chromium.org/10825026/diff/1013/ash/wm/partial_screenshot_v...
ash/wm/partial_screenshot_view.cc:74: return (event->key_code() ==
ui::VKEY_ESCAPE);
On 2012/07/26 01:19:26, Jun Mukai wrote:
> outer paren is not necessary

Done.

[commit-bot: I haz the power, 2012-07-26 02:10:31]

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

[commit-bot: I haz the power, 2012-07-26 03:25:19]

Change committed as 148504