Windows accessibility improvements: 1. All WebKit roles are now passed to the...

Windows accessibility improvements: 1. All WebKit roles are now passed to the
browser and then converted to MSAA roles - this is both because the
logic needs to be more complicated, and so we can support Mac accessibility,
too. 2. The serializable object used to pass accessibility info from the
renderer to the browser now uses a map for uncommon attributes, rather than
having a bunch of fields that are almost always empty. 3. Handles the
accSelect method with TAKE_FOCUS, allowing assistive technology to set focus
to a particular control. 4. Implements several other IAccessible2 interfaces.

BUG=25564
BUG=13291
TEST=None


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

[tfarina, 2010-05-17 17:06:54]

http://codereview.chromium.org/2121004/diff/2001/3001
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3001#newcode255
chrome/browser/browser_accessibility.cc:255: // Return false if the string is
empty.
I'd get rid of this comment, the code is self explanatory.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode256
chrome/browser/browser_accessibility.cc:256: if (action.size() == 0)
if (action.empty()) ?

http://codereview.chromium.org/2121004/diff/2001/3001#newcode284
chrome/browser/browser_accessibility.cc:284: if (description.size() == 0)
if (description.empty()) ?

Same as above. And for the other occurrences below.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode803
chrome/browser/browser_accessibility.cc:803: role_ = ROLE_SYSTEM_ALERT;
I see this pattern for all below.
Would be a case to use a macro?

http://codereview.chromium.org/2121004/diff/2001/3002
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/2121004/diff/2001/3002#newcode246
chrome/browser/browser_accessibility.h:246: STDMETHODIMP get_nCharacters(long
*n_characters);
nit: long* foo

Also, could you fix the other occurrences below?

http://codereview.chromium.org/2121004/diff/2001/3004
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/2121004/diff/2001/3004#newcode36
chrome/browser/browser_accessibility_manager.h:36: };
maybe, would be good to add a virtual destructor?

http://codereview.chromium.org/2121004/diff/2001/3004#newcode68
chrome/browser/browser_accessibility_manager.h:68: void
SetFocus(BrowserAccessibility* node);
Could we pass |node| as const reference?

const BrowserAccessibility& node

http://codereview.chromium.org/2121004/diff/2001/3010
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3010#newcode1521
chrome/browser/renderer_host/render_widget_host_view_win.cc:1521: ||
!render_widget_host_
the || should be in the end of the above line. And in the end for the others
too.

[dmazzoni, 2010-05-17 18:28:07]

Thanks for the quick review! Style comments all addressed.

http://codereview.chromium.org/2121004/diff/2001/3001
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3001#newcode255
chrome/browser/browser_accessibility.cc:255: // Return false if the string is
empty.
On 2010/05/17 17:06:54, tfarina wrote:
> I'd get rid of this comment, the code is self explanatory.

Done.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode256
chrome/browser/browser_accessibility.cc:256: if (action.size() == 0)
On 2010/05/17 17:06:54, tfarina wrote:
> if (action.empty()) ?

Changed throughout.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode803
chrome/browser/browser_accessibility.cc:803: role_ = ROLE_SYSTEM_ALERT;
On 2010/05/17 17:06:54, tfarina wrote:
> I see this pattern for all below.
> Would be a case to use a macro? 

They're actually not always the same.

I changed it so that the case where ia2_role_ is the same as role_ is now
handled automatically; the switch only specifies both if they're different.
Clearer?

http://codereview.chromium.org/2121004/diff/2001/3002
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/2121004/diff/2001/3002#newcode246
chrome/browser/browser_accessibility.h:246: STDMETHODIMP get_nCharacters(long
*n_characters);
On 2010/05/17 17:06:54, tfarina wrote:
> nit: long* foo
> 
> Also, could you fix the other occurrences below?

Done.

http://codereview.chromium.org/2121004/diff/2001/3004
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/2121004/diff/2001/3004#newcode36
chrome/browser/browser_accessibility_manager.h:36: };
On 2010/05/17 17:06:54, tfarina wrote:
> maybe, would be good to add a virtual destructor?

Done.

http://codereview.chromium.org/2121004/diff/2001/3004#newcode68
chrome/browser/browser_accessibility_manager.h:68: void
SetFocus(BrowserAccessibility* node);
On 2010/05/17 17:06:54, tfarina wrote:
> Could we pass |node| as const reference?
> 
> const BrowserAccessibility& node
> 
> 

Done.

http://codereview.chromium.org/2121004/diff/2001/3010
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3010#newcode1521
chrome/browser/renderer_host/render_widget_host_view_win.cc:1521: ||
!render_widget_host_
On 2010/05/17 17:06:54, tfarina wrote:
> the || should be in the end of the above line. And in the end for the others
> too.

Done.

[Chris Guillory, 2010-05-17 18:34:31]

http://codereview.chromium.org/2121004/diff/2001/3001
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3001#newcode39
chrome/browser/browser_accessibility.cc:39: attributes_ = src.attributes;
Perhaps we can use swap instead of assignment for the map and other allocated
fields.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode43
chrome/browser/browser_accessibility.cc:43: if (name_.empty() &&
HasAttribute(WebAccessibility::ATTR_DESCRIPTION)) {
Comment here please.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode255
chrome/browser/browser_accessibility.cc:255: // Return false if the string is
empty.
On 2010/05/17 17:06:54, tfarina wrote:
> I'd get rid of this comment, the code is self explanatory.

Let's at least point out that S_FALSE is returned when there is no default
action.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode329
chrome/browser/browser_accessibility.cc:329: string16 help_str;
This code block appears often. Perhaps make a bool GetAttribute(BSTR*) helper
method?

http://codereview.chromium.org/2121004/diff/2001/3001#newcode428
chrome/browser/browser_accessibility.cc:428: if (target->role_name_.size() > 0)
{
!empty()

http://codereview.chromium.org/2121004/diff/2001/3010
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3010#newcode1520
chrome/browser/renderer_host/render_widget_host_view_win.cc:1520: if
(!browser_accessibility_manager_.get()
Do we really need these checks?

http://codereview.chromium.org/2121004/diff/2001/3015
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3015#newcode3941
chrome/renderer/render_view.cc:3941: webview()->clearFocusedNode();
Comment here.

[dmazzoni, 2010-05-17 21:20:27]

http://codereview.chromium.org/2121004/diff/2001/3001
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3001#newcode39
chrome/browser/browser_accessibility.cc:39: attributes_ = src.attributes;
On 2010/05/17 18:34:31, Chris Guillory wrote:
> Perhaps we can use swap instead of assignment for the map and other allocated
> fields.

I like the idea, but I'm not worried unless we profile the code and see that
this is actually slow.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode43
chrome/browser/browser_accessibility.cc:43: if (name_.empty() &&
HasAttribute(WebAccessibility::ATTR_DESCRIPTION)) {
On 2010/05/17 18:34:31, Chris Guillory wrote:
> Comment here please.

Done.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode255
chrome/browser/browser_accessibility.cc:255: // Return false if the string is
empty.
On 2010/05/17 18:34:31, Chris Guillory wrote:
> On 2010/05/17 17:06:54, tfarina wrote:
> > I'd get rid of this comment, the code is self explanatory.
> 
> Let's at least point out that S_FALSE is returned when there is no default
> action.

I added a new comment up above.

http://codereview.chromium.org/2121004/diff/2001/3001#newcode329
chrome/browser/browser_accessibility.cc:329: string16 help_str;
On 2010/05/17 18:34:31, Chris Guillory wrote:
> This code block appears often. Perhaps make a bool GetAttribute(BSTR*) helper
> method?

I'm not sure how the number of lines of code could be reduced, can you clarify?

http://codereview.chromium.org/2121004/diff/2001/3010
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3010#newcode1520
chrome/browser/renderer_host/render_widget_host_view_win.cc:1520: if
(!browser_accessibility_manager_.get()
On 2010/05/17 18:34:31, Chris Guillory wrote:
> Do we really need these checks?

When the renderer process dies, browser_accessibility_manager_ and
render_widget_host_ can become NULL while this object sticks around...so I don't
think it's too paranoid to check for that case.

http://codereview.chromium.org/2121004/diff/2001/3015
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3015#newcode3941
chrome/renderer/render_view.cc:3941: webview()->clearFocusedNode();
On 2010/05/17 18:34:31, Chris Guillory wrote:
> Comment here.

Done.

[Chris Guillory, 2010-05-17 21:45:42]

http://codereview.chromium.org/2121004/diff/2001/3001
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/2121004/diff/2001/3001#newcode329
chrome/browser/browser_accessibility.cc:329: string16 help_str;
On 2010/05/17 21:20:28, Dominic Mazzoni wrote:
> On 2010/05/17 18:34:31, Chris Guillory wrote:
> > This code block appears often. Perhaps make a bool GetAttribute(BSTR*)
helper
> > method?
> 
> I'm not sure how the number of lines of code could be reduced, can you
clarify?
> 

I was thinking that here you would make a call to something like:
if(!GetTargetAttribute(WebAccessibility::ATTR_HELP, &help))
  return S_FALSE;

[Chris Guillory, 2010-05-17 21:57:51]

LGTM. Great work!