Improvements to accessibility extension api support for "views":...

Improvements to accessibility extension api support for "views":
1. Handles the new wrench menu.
2. Uses NotifyAccessibilityEvent to find out when focus changes, rather
     than installing focus change listeners that need to be cleaned up.

BUG=none
TEST=Updated AccessibilityEventRouterViewsTest.TestFocusNotification


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

[Jay Civelli, 2010-08-04 17:10:23]

http://codereview.chromium.org/3056045/diff/13001/14002
File chrome/browser/views/accessibility_event_router_views.cc (right):

http://codereview.chromium.org/3056045/diff/13001/14002#newcode140
chrome/browser/views/accessibility_event_router_views.cc:140: // window, we menu
events get routed to the profile of the most recent
Wording: "we menu events"

http://codereview.chromium.org/3056045/diff/13001/14002#newcode149
chrome/browser/views/accessibility_event_router_views.cc:149: if
(!is_accessible) {
Nit: the style preference is I believe not to put curly braces when the then
statement is 1 line.

http://codereview.chromium.org/3056045/diff/13001/14003
File chrome/browser/views/accessibility_event_router_views.h (right):

http://codereview.chromium.org/3056045/diff/13001/14003#newcode125
chrome/browser/views/accessibility_event_router_views.h:125: void
RecursiveGetMenuItemIndexAndCount(
Could you add a comment on what that function does?

http://codereview.chromium.org/3056045/diff/13001/14004
File chrome/browser/views/accessibility_event_router_views_unittest.cc (right):

http://codereview.chromium.org/3056045/diff/13001/14004#newcode19
chrome/browser/views/accessibility_event_router_views_unittest.cc:19: #include
"views/views_delegate.h"
I could not find this class from VisualStudio because it is not in views.gyp.
Could you please add it?

http://codereview.chromium.org/3056045/diff/13001/14004#newcode96
chrome/browser/views/accessibility_event_router_views_unittest.cc:96:
views::ViewsDelegate::views_delegate = new TestViewsDelegate();
May be this (and the associated delete) could be added to the SetUp/TearDown
methdods of the test class?

[dmazzoni, 2010-08-04 19:19:16]

On Wed, Aug 4, 2010 at 10:10 AM, <jcivelli@chromium.org> wrote:

>
> http://codereview.chromium.org/3056045/diff/13001/14002
> File chrome/browser/views/accessibility_event_router_views.cc (right):
>
> http://codereview.chromium.org/3056045/diff/13001/14002#newcode140
> chrome/browser/views/accessibility_event_router_views.cc:140: // window,
> we menu events get routed to the profile of the most recent
> Wording: "we menu events"
>

Fixed.


> http://codereview.chromium.org/3056045/diff/13001/14002#newcode149
> chrome/browser/views/accessibility_event_router_views.cc:149: if
> (!is_accessible) {
> Nit: the style preference is I believe not to put curly braces when the
> then statement is 1 line.
>

Fixed.


> http://codereview.chromium.org/3056045/diff/13001/14003
> File chrome/browser/views/accessibility_event_router_views.h (right):
>
> http://codereview.chromium.org/3056045/diff/13001/14003#newcode125
> chrome/browser/views/accessibility_event_router_views.h:125: void
> RecursiveGetMenuItemIndexAndCount(
> Could you add a comment on what that function does?
>

Done.


> http://codereview.chromium.org/3056045/diff/13001/14004
> File chrome/browser/views/accessibility_event_router_views_unittest.cc
> (right):
>
> http://codereview.chromium.org/3056045/diff/13001/14004#newcode19
> chrome/browser/views/accessibility_event_router_views_unittest.cc:19:
> #include "views/views_delegate.h"
> I could not find this class from VisualStudio because it is not in
> views.gyp. Could you please add it?
>

Sure, done.


> http://codereview.chromium.org/3056045/diff/13001/14004#newcode96
> chrome/browser/views/accessibility_event_router_views_unittest.cc:96:
> views::ViewsDelegate::views_delegate = new TestViewsDelegate();
> May be this (and the associated delete) could be added to the
> SetUp/TearDown methdods of the test class?


Good idea, done.

[Jay Civelli, 2010-08-04 21:58:11]

http://codereview.chromium.org/3056045/diff/26001/27002
File chrome/browser/views/accessibility_event_router_views.cc (right):

http://codereview.chromium.org/3056045/diff/26001/27002#newcode220
chrome/browser/views/accessibility_event_router_views.cc:220: int previous_count
= *count;
count an out param but it seems it must be set to 0 before this is called. Could
you document that?

[dmazzoni, 2010-08-04 22:15:16]

On Wed, Aug 4, 2010 at 2:58 PM, <jcivelli@chromium.org> wrote:

> http://codereview.chromium.org/3056045/diff/26001/27002
>
> File chrome/browser/views/accessibility_event_router_views.cc (right):
>
> http://codereview.chromium.org/3056045/diff/26001/27002#newcode220
> chrome/browser/views/accessibility_event_router_views.cc:220: int
> previous_count = *count;
> count an out param but it seems it must be set to 0 before this is
> called. Could you document that?
>

No problem. I documented both |index| and |count| to be more clear.

By the way: this function may not be necessary later once Scott finishes
refactoring some of the Wrench menu code so that Cut/Copy/Paste and
Zoom-In/Zoom-Out/Full-Screen are not quite so hardcoded. I'll revisit it
once he's done.

- Dominic

[Jay Civelli, 2010-08-04 22:22:49]

LGTM