Added a11y alert on entering overview mode

Added a11y alert on entering window overview mode.

The patch alerts a11y support that the overview
mode has been activated. If chromevox is enabled, it 
will speak aloud "Alert, entered window overview mode".

BUG=332992
TEST=WindowSelectorTest.A11yAlertOnOverviewMode

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267246

[Nina, 2014-04-25 19:30:51]

Hi Terry, Rob,

Can you take a look into this? The patch alerts a11y support that the overview
mode has been activated. If chromevox is enabled, it will speak aloud "Alert,
window overview activated". Once I get the LGTMs I'll add jamescook, who is
owner of the delegate interface.

Thanks!

[tdanderson, 2014-04-28 15:34:38]

Nico,

Looks good! I have left some comments inline below. Also, please include a brief
issue description (similar to the text of the comment you sent to Rob and I on
this issue.)

It would also be nice to add a test as well. Would it be feasible to do
something simple like in AcceleratorControllerTest.DisallowedWithNoWindow?

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/accessibility_del...
File ash/accessibility_delegate.h (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/accessibility_del...
ash/accessibility_delegate.h:22: A11Y_ALERT_WINDOW_OVERVIEW_STARTING
I'd prefer the naming of this to be consistent with how we are going to word the
voice announcement (see comment below). Let's wait for Laura's input on the best
wording.

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
File ash/wm/overview/window_overview.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
ash/wm/overview/window_overview.cc:13: #include "ash/shell_delegate.h"
I don't think you need this #include.

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
ash/wm/overview/window_overview.cc:153:
Shell::GetInstance()->accessibility_delegate()->TriggerAccessibilityAlert(
Is accessibility_delegate() guaranteed to be non-NULL here?

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
ash/wm/overview/window_overview.cc:154: A11Y_ALERT_WINDOW_OVERVIEW_STARTING);
Indent should be length 4 here.

https://chromiumcodereview.appspot.com/257763008/diff/1/chrome/app/generated_...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/chrome/app/generated_...
chrome/app/generated_resources.grd:14570: +      Window overview activated
I wonder what the best terminology is here. I am going to ask Laura on issue
332992.

[tdanderson, 2014-04-28 16:47:00]

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/accessibility_del...
File ash/accessibility_delegate.h (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/accessibility_del...
ash/accessibility_delegate.h:22: A11Y_ALERT_WINDOW_OVERVIEW_STARTING
On 2014/04/28 15:34:39, tdanderson wrote:
> I'd prefer the naming of this to be consistent with how we are going to word
the
> voice announcement (see comment below). Let's wait for Laura's input on the
best
> wording.

Let's call this A11Y_ALERT_WINDOW_OVERVIEW_MODE_ENTERED

https://chromiumcodereview.appspot.com/257763008/diff/1/chrome/app/generated_...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/chrome/app/generated_...
chrome/app/generated_resources.grd:14570: +      Window overview activated
On 2014/04/28 15:34:39, tdanderson wrote:
> I wonder what the best terminology is here. I am going to ask Laura on issue
> 332992.

Let's go with "Entered window overview mode."

[Nina, 2014-04-28 17:24:43]

Changes applied! Can you take a look into it again?

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/accessibility_del...
File ash/accessibility_delegate.h (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/accessibility_del...
ash/accessibility_delegate.h:22: A11Y_ALERT_WINDOW_OVERVIEW_STARTING
On 2014/04/28 16:47:00, tdanderson wrote:
> On 2014/04/28 15:34:39, tdanderson wrote:
> > I'd prefer the naming of this to be consistent with how we are going to word
> the
> > voice announcement (see comment below). Let's wait for Laura's input on the
> best
> > wording.
> 
> Let's call this A11Y_ALERT_WINDOW_OVERVIEW_MODE_ENTERED

Done.

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/accessibility_del...
ash/accessibility_delegate.h:22: A11Y_ALERT_WINDOW_OVERVIEW_STARTING
On 2014/04/28 15:34:39, tdanderson wrote:
> I'd prefer the naming of this to be consistent with how we are going to word
the
> voice announcement (see comment below). Let's wait for Laura's input on the
best
> wording.

Done.

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
File ash/wm/overview/window_overview.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
ash/wm/overview/window_overview.cc:13: #include "ash/shell_delegate.h"
On 2014/04/28 15:34:39, tdanderson wrote:
> I don't think you need this #include.

Done.

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
ash/wm/overview/window_overview.cc:153:
Shell::GetInstance()->accessibility_delegate()->TriggerAccessibilityAlert(
On 2014/04/28 15:34:39, tdanderson wrote:
> Is accessibility_delegate() guaranteed to be non-NULL here?

Apparently yes

https://chromiumcodereview.appspot.com/257763008/diff/1/ash/wm/overview/windo...
ash/wm/overview/window_overview.cc:154: A11Y_ALERT_WINDOW_OVERVIEW_STARTING);
On 2014/04/28 15:34:39, tdanderson wrote:
> Indent should be length 4 here.

Done.

https://chromiumcodereview.appspot.com/257763008/diff/1/chrome/app/generated_...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/257763008/diff/1/chrome/app/generated_...
chrome/app/generated_resources.grd:14570: +      Window overview activated
On 2014/04/28 16:47:00, tdanderson wrote:
> On 2014/04/28 15:34:39, tdanderson wrote:
> > I wonder what the best terminology is here. I am going to ask Laura on issue
> > 332992.
> 
> Let's go with "Entered window overview mode."

Done.

[Nina, 2014-04-28 19:33:24]

Added unittest!

[tdanderson, 2014-04-28 21:59:54]

In the issue description:

* TEST=WindowSelectorTest.A11yAlertOnOverviewMode
* Change "Alert, window overview activated" to the new text that we're speaking
* Change the title from "Added a11y alert on entering overview" to "Added a11y
alert on entering overview mode"

Otherwise LGTM with the comments addressed below!

Adding flackr@ to take a second look.

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
File ash/wm/overview/window_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
ash/wm/overview/window_selector_unittest.cc:261: scoped_ptr<aura::Window>
window1(CreateWindow(bounds));
Perhaps add an EXPECT_NE here to check that the alert was not sent before
ToggleOverview() was called.

https://chromiumcodereview.appspot.com/257763008/diff/50001/chrome/app/genera...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/257763008/diff/50001/chrome/app/genera...
chrome/app/generated_resources.grd:14525: +    <message
name="IDS_A11Y_ALERT_WINDOW_OVERVIEW_MODE_ENTERED" desc="The accessible text
read when starting the Overview.">
I would add to the description here, something like "The accessible text read
when the user enters window overview mode in Ash."

[flackr, 2014-04-28 23:34:42]

LGTM with the changes that Terry suggested and a couple nits.

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
File ash/wm/overview/window_overview.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
ash/wm/overview/window_overview.cc:154:
Shell::GetInstance()->accessibility_delegate()->TriggerAccessibilityAlert(
nit: Re-use local "shell" variable.

Also, while overview from alt tab is likely to go away, I'd prefer if we only
deliver this notification if window_selector_->mode() ==
WindowSelector::OVERVIEW since I don't think this notification makes sense when
we auto enter overview from alt-tab cycling.

[Nina, 2014-04-29 13:12:17]

sky@chromium.org: Please review changes in 

chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc
ash/accessibility_delegate.h

Thanks!

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
File ash/wm/overview/window_overview.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
ash/wm/overview/window_overview.cc:154:
Shell::GetInstance()->accessibility_delegate()->TriggerAccessibilityAlert(
On 2014/04/28 23:34:42, flackr wrote:
> nit: Re-use local "shell" variable.
> 
> Also, while overview from alt tab is likely to go away, I'd prefer if we only
> deliver this notification if window_selector_->mode() ==
> WindowSelector::OVERVIEW since I don't think this notification makes sense
when
> we auto enter overview from alt-tab cycling.

Done.

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
File ash/wm/overview/window_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/50001/ash/wm/overview/w...
ash/wm/overview/window_selector_unittest.cc:261: scoped_ptr<aura::Window>
window1(CreateWindow(bounds));
On 2014/04/28 21:59:55, tdanderson wrote:
> Perhaps add an EXPECT_NE here to check that the alert was not sent before
> ToggleOverview() was called.

Done.

https://chromiumcodereview.appspot.com/257763008/diff/50001/chrome/app/genera...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/257763008/diff/50001/chrome/app/genera...
chrome/app/generated_resources.grd:14525: +    <message
name="IDS_A11Y_ALERT_WINDOW_OVERVIEW_MODE_ENTERED" desc="The accessible text
read when starting the Overview.">
On 2014/04/28 21:59:55, tdanderson wrote:
> I would add to the description here, something like "The accessible text read
> when the user enters window overview mode in Ash."

Done.

[flackr, 2014-04-29 14:21:16]

https://chromiumcodereview.appspot.com/257763008/diff/70001/ash/wm/overview/w...
File ash/wm/overview/window_overview.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/70001/ash/wm/overview/w...
ash/wm/overview/window_overview.cc:155: if (window_selector_->mode() ==
WindowSelector::OVERVIEW)
nit: need braces { } since the code block is multiple lines.

[Nina, 2014-04-29 14:24:33]

Fixed nit

https://chromiumcodereview.appspot.com/257763008/diff/70001/ash/wm/overview/w...
File ash/wm/overview/window_overview.cc (right):

https://chromiumcodereview.appspot.com/257763008/diff/70001/ash/wm/overview/w...
ash/wm/overview/window_overview.cc:155: if (window_selector_->mode() ==
WindowSelector::OVERVIEW)
On 2014/04/29 14:21:16, flackr wrote:
> nit: need braces { } since the code block is multiple lines.

Done.

[sky, 2014-04-29 15:33:31]

LGTM

[tdanderson, 2014-04-30 14:35:14]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-04-30 14:35:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nsatragno@chromium.org/257763008/90001

[commit-bot: I haz the power, 2014-04-30 17:01:12]

Change committed as 267246