Do not toggle Applist with Search key press when accessibility is enabled.

Do not toggle Applist with Search key press when accessibility is enabled.

BUG=132296
TEST=manual + aura_shell_unittests


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

[Yusuke Sato, 2012-06-14 09:18:50]

Zach, could you review this?

[Zachary Kuznia, 2012-06-14 10:02:22]

http://codereview.chromium.org/10543158/diff/1/ash/accelerators/accelerator_c...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10543158/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:365: if
(Shell::GetInstance()->delegate()->IsSpokenFeedbackEnabled())
Perhaps we should DCHECK that the accelerator being pressed is the search key? 
We don't want this to mask other accelerators.

http://codereview.chromium.org/10543158/diff/1/ash/shell/shell_delegate_impl.h
File ash/shell/shell_delegate_impl.h (right):

http://codereview.chromium.org/10543158/diff/1/ash/shell/shell_delegate_impl....
ash/shell/shell_delegate_impl.h:29: virtual bool IsSpokenFeedbackEnabled() const
OVERRIDE;
This duplicates "IsInAccessibilityMode()" in
ash/system/tray/system_tray_delegate.h, is there a way to combine them easily? 
If not, perhaps we should add a TODO

[Yusuke Sato, 2012-06-14 11:41:40]

Please take another look.

http://codereview.chromium.org/10543158/diff/1/ash/accelerators/accelerator_c...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10543158/diff/1/ash/accelerators/accelerator_c...
ash/accelerators/accelerator_controller.cc:365: if
(Shell::GetInstance()->delegate()->IsSpokenFeedbackEnabled())
On 2012/06/14 10:02:22, Zachary Kuznia wrote:
> Perhaps we should DCHECK that the accelerator being pressed is the search key?

> We don't want this to mask other accelerators.

Done.

http://codereview.chromium.org/10543158/diff/1/ash/shell/shell_delegate_impl.h
File ash/shell/shell_delegate_impl.h (right):

http://codereview.chromium.org/10543158/diff/1/ash/shell/shell_delegate_impl....
ash/shell/shell_delegate_impl.h:29: virtual bool IsSpokenFeedbackEnabled() const
OVERRIDE;
On 2012/06/14 10:02:22, Zachary Kuznia wrote:
> This duplicates "IsInAccessibilityMode()" in
> ash/system/tray/system_tray_delegate.h, is there a way to combine them easily?

> If not, perhaps we should add a TODO

Thanks! Done.

[dmazzoni, 2012-06-14 16:45:25]

http://codereview.chromium.org/10543158/diff/7001/ash/shell/shell_delegate_im...
File ash/shell/shell_delegate_impl.cc (right):

http://codereview.chromium.org/10543158/diff/7001/ash/shell/shell_delegate_im...
ash/shell/shell_delegate_impl.cc:97: spoken_feedback_enabled_ =
!spoken_feedback_enabled_;
This worries me slightly, I'm afraid it could get out of sync.

Spoken feedback is turned on in a couple of different places - not only via the
global shortcut but also by toggling the checkbox in the Options. It's also
persistent - i.e. it will turn on automatically on boot if the preference is
set.

The main toggle function is in
src/chrome/browser/chromeos/accessibility/accessibility_util.cc - I think you
need to hook into there, or read the preference directly in order to be 100%
sure that this state will always be the same.

[Yusuke Sato, 2012-06-14 16:57:15]

http://codereview.chromium.org/10543158/diff/7001/ash/shell/shell_delegate_im...
File ash/shell/shell_delegate_impl.cc (right):

http://codereview.chromium.org/10543158/diff/7001/ash/shell/shell_delegate_im...
ash/shell/shell_delegate_impl.cc:97: spoken_feedback_enabled_ =
!spoken_feedback_enabled_;
If I understand correctly, ash/shell/* files are just for building test
environment for Ash developers, and are not included in a production Chrome OS
image.

The real implementation of ShellDelegate::ToggleSpokenFeedback() and
ShellDelegate::IsSpokenFeedbackEnabled() are in
chrome/browser/ui/views/ash/chrome_shell_delegate.cc. As you can see in
https://chromiumcodereview.appspot.com/10543158/diff/7001/chrome/browser/ui/v...
, the ShellDelegate functions in the file correctly use
chromeos::accessibility:: utility functions as you suggested.

On 2012/06/14 16:45:25, Dominic Mazzoni wrote:
> This worries me slightly, I'm afraid it could get out of sync.
> 
> Spoken feedback is turned on in a couple of different places - not only via
the
> global shortcut but also by toggling the checkbox in the Options. It's also
> persistent - i.e. it will turn on automatically on boot if the preference is
> set.
> 
> The main toggle function is in
> src/chrome/browser/chromeos/accessibility/accessibility_util.cc - I think you
> need to hook into there, or read the preference directly in order to be 100%
> sure that this state will always be the same.

[dmazzoni, 2012-06-14 17:05:00]

http://codereview.chromium.org/10543158/diff/7001/ash/shell/shell_delegate_im...
File ash/shell/shell_delegate_impl.cc (right):

http://codereview.chromium.org/10543158/diff/7001/ash/shell/shell_delegate_im...
ash/shell/shell_delegate_impl.cc:97: spoken_feedback_enabled_ =
!spoken_feedback_enabled_;
On 2012/06/14 16:57:15, Yusuke Sato wrote:
> If I understand correctly, ash/shell/* files are just for building test
> environment for Ash developers, and are not included in a production Chrome OS
> image.

Ah! Thanks, I didn't realize that. Looks good.

- Dominic

[Zachary Kuznia, 2012-06-15 02:30:12]

lgtm

[Yusuke Sato, 2012-06-15 02:32:59]

+sky

[sky, 2012-06-15 14:54:27]

LGTM

[commit-bot: I haz the power, 2012-06-16 09:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10543158/2004

[commit-bot: I haz the power, 2012-06-16 11:29:14]

Change committed as 142590