Add a context field to the accessibility extension API.

Add a context field to the accessibility extension API.
This makes it possible to provide more helpful spoken feedback when focusing
a toolbar button or infobar button - e.g. it can now read the full text of
the infobar when you move focus to any of its buttons.

BUG=106724
TEST=Adds new unit tests; will need new ChromeVox push to test manually.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114640

[dmazzoni, 2011-12-07 18:47:37]

hashimoto: primary reviewer
sky: chrome/browser/ui/views/
aa: rubberstamp for private extension api change

[sky, 2011-12-07 22:01:54]

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
File chrome/browser/ui/views/accessibility_event_router_views.cc (right):

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.cc:299: void
AccessibilityEventRouterViews::AddContextInfo(
Order should match header.

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.cc:323: if
(static_text_child) {
I don't know this code well enough, so this might not matter. But it seem if you
have a string of ROLE_ALERT ancestors you're going to revisit the same views
many times by way of FindDescendantWithAccessibleRole.

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.cc:335: views::View*
AccessibilityEventRouterViews::FindDescendantWithAccessibleRole(
Neither of these methods look like they need any state from
AccessibilityEventRouterViews. I would be inclined to make them static.

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
File chrome/browser/ui/views/accessibility_event_router_views.h (right):

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.h:106: void
AddContextInfo(views::View* view, AccessibilityControlInfo* info);
Should this return a status indicating whether it was found?

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.h:125: views::View*
view, ui::AccessibilityTypes::Role role);
nit: each param on its own line.

[hashimoto, 2011-12-07 23:07:35]

http://codereview.chromium.org/8850004/diff/1/chrome/browser/accessibility/ac...
File chrome/browser/accessibility/accessibility_events.h (right):

http://codereview.chromium.org/8850004/diff/1/chrome/browser/accessibility/ac...
chrome/browser/accessibility/accessibility_events.h:63: void set_context(const
std::string& context) { context_ = context; }
Instead of exposing the setter, how about setting |context_| by ctors?
It will be more consistent with |name_|

http://codereview.chromium.org/8850004/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8850004/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:283: "description": "The
localized name of the context for the object, like the name of the surrounding
toolbar or group of controls."
Add "optional": true

[dmazzoni, 2011-12-07 23:13:49]

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
File chrome/browser/ui/views/accessibility_event_router_views.cc (right):

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.cc:299: void
AccessibilityEventRouterViews::AddContextInfo(
On 2011/12/07 22:01:55, sky wrote:
> Order should match header.

Done.

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.cc:323: if
(static_text_child) {
On 2011/12/07 22:01:55, sky wrote:
> I don't know this code well enough, so this might not matter. But it seem if
you
> have a string of ROLE_ALERT ancestors you're going to revisit the same views
> many times by way of FindDescendantWithAccessibleRole.

That should never happen (multiple ALERTs in the parent chain). I moved the
return up one level so that only the first ALERT it hits matters.

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.cc:335: views::View*
AccessibilityEventRouterViews::FindDescendantWithAccessibleRole(
On 2011/12/07 22:01:55, sky wrote:
> Neither of these methods look like they need any state from
> AccessibilityEventRouterViews. I would be inclined to make them static.

Done - made a few more methods static too.

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
File chrome/browser/ui/views/accessibility_event_router_views.h (right):

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.h:106: void
AddContextInfo(views::View* view, AccessibilityControlInfo* info);
On 2011/12/07 22:01:55, sky wrote:
> Should this return a status indicating whether it was found?

Not currently needed but probably a good design for this helper function.

http://codereview.chromium.org/8850004/diff/1/chrome/browser/ui/views/accessi...
chrome/browser/ui/views/accessibility_event_router_views.h:125: views::View*
view, ui::AccessibilityTypes::Role role);
On 2011/12/07 22:01:55, sky wrote:
> nit: each param on its own line.

Done.

[dmazzoni, 2011-12-07 23:34:12]

http://codereview.chromium.org/8850004/diff/1/chrome/browser/accessibility/ac...
File chrome/browser/accessibility/accessibility_events.h (right):

http://codereview.chromium.org/8850004/diff/1/chrome/browser/accessibility/ac...
chrome/browser/accessibility/accessibility_events.h:63: void set_context(const
std::string& context) { context_ = context; }
On 2011/12/07 23:07:35, hashimoto wrote:
> Instead of exposing the setter, how about setting |context_| by ctors?
> It will be more consistent with |name_|

Sure, I changed it to be in the constructor.

Now there are a couple of subclasses where a context probably isn't needed, so
the subclass passes std::string(), does that seem reasonable?

http://codereview.chromium.org/8850004/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8850004/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:283: "description": "The
localized name of the context for the object, like the name of the surrounding
toolbar or group of controls."
On 2011/12/07 23:07:35, hashimoto wrote:
> Add "optional": true

Done.

[hashimoto, 2011-12-08 00:04:12]

http://codereview.chromium.org/8850004/diff/1/chrome/browser/accessibility/ac...
File chrome/browser/accessibility/accessibility_events.h (right):

http://codereview.chromium.org/8850004/diff/1/chrome/browser/accessibility/ac...
chrome/browser/accessibility/accessibility_events.h:63: void set_context(const
std::string& context) { context_ = context; }
On 2011/12/07 23:34:12, Dominic Mazzoni wrote:
> On 2011/12/07 23:07:35, hashimoto wrote:
> > Instead of exposing the setter, how about setting |context_| by ctors?
> > It will be more consistent with |name_|
> 
> Sure, I changed it to be in the constructor.
> 
> Now there are a couple of subclasses where a context probably isn't needed, so
> the subclass passes std::string(), does that seem reasonable?

How about having two ctors (two argument version and three argument version) ?
Or having single two-argument ctor while having set_context as a protected
method and using it to set |context| in subclasses' ctors.

[dmazzoni, 2011-12-08 03:37:27]

On Wed, Dec 7, 2011 at 4:04 PM, <hashimoto@chromium.org> wrote:

> How about having two ctors (two argument version and three argument
> version) ?
> Or having single two-argument ctor while having set_context as a
> protected method and using it to set |context| in subclasses' ctors.
>

The second idea sounds good, will do.

- Dominic

[dmazzoni, 2011-12-12 06:57:21]

On Wed, Dec 7, 2011 at 7:37 PM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Wed, Dec 7, 2011 at 4:04 PM, <hashimoto@chromium.org> wrote:
>
>> How about having two ctors (two argument version and three argument
>> version) ?
>> Or having single two-argument ctor while having set_context as a
>> protected method and using it to set |context| in subclasses' ctors.
>>
>
> The second idea sounds good, will do.


Ready for another look.

[sky, 2011-12-12 16:20:56]

LGTM

http://codereview.chromium.org/8850004/diff/9003/chrome/browser/ui/views/acce...
File chrome/browser/ui/views/accessibility_event_router_views.h (right):

http://codereview.chromium.org/8850004/diff/9003/chrome/browser/ui/views/acce...
chrome/browser/ui/views/accessibility_event_router_views.h:111:
ui::AccessibilityTypes::Role role);
This is the preferred wrapping style when spanning multiple lines. Make 85-99
match this.

[dmazzoni, 2011-12-12 17:12:03]

http://codereview.chromium.org/8850004/diff/9003/chrome/browser/ui/views/acce...
File chrome/browser/ui/views/accessibility_event_router_views.h (right):

http://codereview.chromium.org/8850004/diff/9003/chrome/browser/ui/views/acce...
chrome/browser/ui/views/accessibility_event_router_views.h:111:
ui::AccessibilityTypes::Role role);
On 2011/12/12 16:20:58, sky wrote:
> This is the preferred wrapping style when spanning multiple lines. Make 85-99
> match this.

Done.

[hashimoto, 2011-12-13 01:07:05]

lgtm