Add accessible names to the launcher buttons

Add accessible names to the launcher buttons


BUG=104192
TEST=None


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

[Zachary Kuznia, 2012-01-28 02:22:05]

Hello sky,

Could you review this at: http://codereview.chromium.org/9121050


Add accessible names to the launcher buttons 

BUG=104192
TEST=None

[sky, 2012-01-30 04:53:15]

It would be better to create a grd file in ash so that we don't need to route
lookup through the delegate. I don't know how is there are any localization
issues when creating a new grd file. Maybe Tony knows.

Also, the title for tabbed and app item can and will change. It would be better
to override GetAccessibleState to call through to the delegate when you need the
title.

  -Scott

[Zachary Kuznia, 2012-01-30 09:32:03]

On 2012/01/30 04:53:15, sky wrote:
> It would be better to create a grd file in ash so that we don't need to route
> lookup through the delegate. I don't know how is there are any localization
> issues when creating a new grd file. Maybe Tony knows.

Done.

> 
> Also, the title for tabbed and app item can and will change. It would be
better
> to override GetAccessibleState to call through to the delegate when you need
the
> title.
> 

Done.
>   -Scott

[sky, 2012-01-30 15:33:45]

LGTM with the following changes.

http://codereview.chromium.org/9121050/diff/4002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/9121050/diff/4002/ash/launcher/launcher_view.c...
ash/launcher/launcher_view.cc:651: if (view == overflow_button_)
This should never be invoked for the overflow_button_. At best this should be a
DCHECK.

http://codereview.chromium.org/9121050/diff/4002/ash/test/test_shell_delegate.h
File ash/test/test_shell_delegate.h (right):

http://codereview.chromium.org/9121050/diff/4002/ash/test/test_shell_delegate...
ash/test/test_shell_delegate.h:35: 
remove this newline.

http://codereview.chromium.org/9121050/diff/4002/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/9121050/diff/4002/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/chrome_shell_delegate.cc:20: #include
"grit/generated_resources.h"
Can you remove these now?

[tony, 2012-01-30 18:28:37]

ash_strings.grd appears to be missing from this change.  Adding a new grd file
is fine.

[Zachary Kuznia, 2012-01-31 00:29:08]

http://codereview.chromium.org/9121050/diff/4002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/9121050/diff/4002/ash/launcher/launcher_view.c...
ash/launcher/launcher_view.cc:651: if (view == overflow_button_)
On 2012/01/30 15:33:46, sky wrote:
> This should never be invoked for the overflow_button_. At best this should be
a
> DCHECK.

Done.

http://codereview.chromium.org/9121050/diff/4002/ash/test/test_shell_delegate.h
File ash/test/test_shell_delegate.h (right):

http://codereview.chromium.org/9121050/diff/4002/ash/test/test_shell_delegate...
ash/test/test_shell_delegate.h:35: 
On 2012/01/30 15:33:46, sky wrote:
> remove this newline.

Done.

[commit-bot: I haz the power, 2012-01-31 00:30:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/9121050/11002

[tony, 2012-01-31 00:39:32]

http://codereview.chromium.org/9121050/diff/11002/ash/ash_strings.grd
File ash/ash_strings.grd (right):

http://codereview.chromium.org/9121050/diff/11002/ash/ash_strings.grd#newcode106
ash/ash_strings.grd:106: </translations>
If we're not going to include placeholder xtb files now, can you file a bug
about it and add a TODO(zork) comment here referencing the bug?

http://codereview.chromium.org/9121050/diff/11002/ash/ash_strings.grd#newcode113
ash/ash_strings.grd:113: <message name="IDS_AURA_APP_LIST_TITLE" desc="The title
used for the Aura app list in the launcer">
launcer -> launcher

[Zachary Kuznia, 2012-01-31 01:09:20]

http://codereview.chromium.org/9121050/diff/11002/ash/ash_strings.grd
File ash/ash_strings.grd (right):

http://codereview.chromium.org/9121050/diff/11002/ash/ash_strings.grd#newcode106
ash/ash_strings.grd:106: </translations>
On 2012/01/31 00:39:33, tony wrote:
> If we're not going to include placeholder xtb files now, can you file a bug
> about it and add a TODO(zork) comment here referencing the bug?

Done.

http://codereview.chromium.org/9121050/diff/11002/ash/ash_strings.grd#newcode113
ash/ash_strings.grd:113: <message name="IDS_AURA_APP_LIST_TITLE" desc="The title
used for the Aura app list in the launcer">
On 2012/01/31 00:39:33, tony wrote:
> launcer -> launcher

Done.

[tony, 2012-01-31 19:32:11]

LGTM.

Is ash only going to be used on cros?

[sky, 2012-01-31 19:38:16]

On 2012/01/31 19:32:11, tony wrote:
> LGTM.
> 
> Is ash only going to be used on cros?

Not sure. Parts of it may end up on windows.

[commit-bot: I haz the power, 2012-01-31 19:39:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/9121050/8003

[commit-bot: I haz the power, 2012-01-31 21:09:18]

Try job failure for 9121050-8003 (retry) on mac_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-01 06:02:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/9121050/15003

[commit-bot: I haz the power, 2012-02-01 08:13:56]

Change committed as 120044

[achuithb, 2012-02-01 11:32:10]

http://codereview.chromium.org/9121050/diff/15003/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/9121050/diff/15003/ash/launcher/launcher_view....
ash/launcher/launcher_view.cc:226:
l10n_util::GetStringUTF16(IDS_AURA_LAUNCHER_OVERFLOW_NAME));
I'm getting an assertion failure here. I see this is defined in ash_strings.grd;
not sure why it's not being picked up?

[Nikita (slow), 2012-02-01 15:28:40]

Seems that it not compiling with ninja

../../ash/launcher/launcher_view.cc:16:30: error: grit/ash_strings.h: No such
file or directory
../../ash/launcher/launcher_view.cc: In member function 'void
ash::internal::LauncherView::Init()':
../../ash/launcher/launcher_view.cc:226: error:
'IDS_AURA_LAUNCHER_OVERFLOW_NAME' was not declared in this scope
../../ash/launcher/launcher_view.cc: In member function 'virtual string16
ash::internal::LauncherView::GetAccessibleName(views::View*)':
../../ash/launcher/launcher_view.cc:665: error: 'IDS_AURA_APP_LIST_TITLE' was
not declared in this scope
../../ash/launcher/launcher_view.cc:668: error: 'IDS_AURA_CYCLER_TITLE' was not
declared in this scope
ninja: build stopped: subcommand failed.

[Nikita (slow), 2012-02-01 16:04:49]

Doing a clean build with use_aura=1 fails:


ash/launcher/launcher_view.cc:16:30: error: grit/ash_strings.h: No such file or
directory
ash/launcher/launcher_view.cc: In member function 'void
ash::internal::LauncherView::Init()':
ash/launcher/launcher_view.cc:226: error: 'IDS_AURA_LAUNCHER_OVERFLOW_NAME' was
not declared in this scope
ash/launcher/launcher_view.cc: In member function 'virtual string16
ash::internal::LauncherView::GetAccessibleName(views::View*)':
ash/launcher/launcher_view.cc:665: error: 'IDS_AURA_APP_LIST_TITLE' was not
declared in this scope
ash/launcher/launcher_view.cc:668: error: 'IDS_AURA_CYCLER_TITLE' was not
declared in this scope
make: *** [out/Release/obj.target/ash/ash/launcher/launcher_view.o] Error 1