Make web content accessibility tree a descendant of main window's tree again

Make the web content accessibility tree a descendant of the main window's
accessibility tree again, which makes it possible to debug Chrome
accessibility on Windows using tools like AccExplorer32 or AccProbe.

This change refactors the code that retrieves the Windows IAccessible
interface from a View that wraps a native HWND. The previous code was
brittle, using ViewProps and hardcoded View classname tests. This
change moves that logic directly to the classes that wrap native
views.

BUG=54220
BUG=85673
TEST=Updated AccessibilityWinBrowserTest.ContainsRendererAccessibilityTree

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

[dmazzoni, 2011-06-10 18:30:10]

ctguil to review accessibility logic.
ben to review refactoring of Views methods.

[Chris Guillory, 2011-06-10 19:10:09]

LGTM. Some comments about naming.

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc
(right):

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc:140:
NativeTabContentsContainerWin::GetNativeAccessibleView() {
Nit: I don't think you need to indent the function.

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.h
(right):

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.h:34:
virtual gfx::NativeAccessibleView GetNativeAccessibleView();
Perhaps the type should be NativeViewAccessible and the function should be named
GetNativeViewAccessible. Do you think that sounds better? We refer to the
accessibility implementation as an accessible in other parts of the code.

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/tab_contents_container_native.h
(right):

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container_native.h:14:
#include "ui/base/accessibility/accessibility_types.h"
Nit: Extra line here and this include isn't in sort order.

http://codereview.chromium.org/6995126/diff/5005/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/6995126/diff/5005/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:42: struct IAccessible;
Nit: We don't want to include oleacc.h for this definition?

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
File views/controls/native/native_view_host.cc (right):

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
views/controls/native/native_view_host.cc:204: gfx::NativeAccessibleView
accessible_view =
Do we need to verify native_wrapper_.get()?

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
File views/controls/native/native_view_host_wrapper.h (right):

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
views/controls/native/native_view_host_wrapper.h:61: // Return the native
accessible view corresponding to this view.
If you decide to rename the type:
accessible view -> view accessible

[tfarina, 2011-06-10 20:09:01]

http://codereview.chromium.org/6995126/diff/5005/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6995126/diff/5005/views/view.h#newcode1444
views/view.h:1444: #ifdef OS_WIN
the old code is corrected and the preferred way.

[dmazzoni, 2011-06-10 22:38:40]

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc
(right):

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc:140:
NativeTabContentsContainerWin::GetNativeAccessibleView() {
On 2011/06/10 19:10:09, Chris Guillory wrote:
> Nit: I don't think you need to indent the function.

Done.

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.h
(right):

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.h:34:
virtual gfx::NativeAccessibleView GetNativeAccessibleView();
On 2011/06/10 19:10:09, Chris Guillory wrote:
> Perhaps the type should be NativeViewAccessible and the function should be
named
> GetNativeViewAccessible. Do you think that sounds better? We refer to the
> accessibility implementation as an accessible in other parts of the code.

I don't have a strong preference. Changed.

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/tab_contents_container_native.h
(right):

http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container_native.h:14:
#include "ui/base/accessibility/accessibility_types.h"
On 2011/06/10 19:10:09, Chris Guillory wrote:
> Nit: Extra line here and this include isn't in sort order.

Done.

http://codereview.chromium.org/6995126/diff/5005/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/6995126/diff/5005/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:42: struct IAccessible;
On 2011/06/10 19:10:09, Chris Guillory wrote:
> Nit: We don't want to include oleacc.h for this definition?

I think this is better, because hundreds of files include native_widget_types.h
but only a tiny fraction of them actually need oleacc.h. I think I only needed
to add #include <oleacc.h> to 1 or 2 cc files.

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
File views/controls/native/native_view_host.cc (right):

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
views/controls/native/native_view_host.cc:204: gfx::NativeAccessibleView
accessible_view =
On 2011/06/10 19:10:09, Chris Guillory wrote:
> Do we need to verify native_wrapper_.get()?

Done.

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
File views/controls/native/native_view_host_wrapper.h (right):

http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
views/controls/native/native_view_host_wrapper.h:61: // Return the native
accessible view corresponding to this view.
On 2011/06/10 19:10:09, Chris Guillory wrote:
> If you decide to rename the type:
> accessible view -> view accessible

Done.

http://codereview.chromium.org/6995126/diff/5005/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6995126/diff/5005/views/view.h#newcode1444
views/view.h:1444: #ifdef OS_WIN
On 2011/06/10 20:09:01, tfarina wrote:
> the old code is corrected and the preferred way.

Done.

[Chris Guillory, 2011-06-10 22:40:25]

On 2011/06/10 22:38:40, Dominic Mazzoni wrote:
>
http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
> File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc
> (right):
> 
>
http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
> chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc:140:
> NativeTabContentsContainerWin::GetNativeAccessibleView() {
> On 2011/06/10 19:10:09, Chris Guillory wrote:
> > Nit: I don't think you need to indent the function.
> 
> Done.
> 
>
http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
> File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.h
> (right):
> 
>
http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
> chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.h:34:
> virtual gfx::NativeAccessibleView GetNativeAccessibleView();
> On 2011/06/10 19:10:09, Chris Guillory wrote:
> > Perhaps the type should be NativeViewAccessible and the function should be
> named
> > GetNativeViewAccessible. Do you think that sounds better? We refer to the
> > accessibility implementation as an accessible in other parts of the code.
> 
> I don't have a strong preference. Changed.
> 
>
http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
> File chrome/browser/ui/views/tab_contents/tab_contents_container_native.h
> (right):
> 
>
http://codereview.chromium.org/6995126/diff/5005/chrome/browser/ui/views/tab_...
> chrome/browser/ui/views/tab_contents/tab_contents_container_native.h:14:
> #include "ui/base/accessibility/accessibility_types.h"
> On 2011/06/10 19:10:09, Chris Guillory wrote:
> > Nit: Extra line here and this include isn't in sort order.
> 
> Done.
> 
> http://codereview.chromium.org/6995126/diff/5005/ui/gfx/native_widget_types.h
> File ui/gfx/native_widget_types.h (right):
> 
>
http://codereview.chromium.org/6995126/diff/5005/ui/gfx/native_widget_types.h...
> ui/gfx/native_widget_types.h:42: struct IAccessible;
> On 2011/06/10 19:10:09, Chris Guillory wrote:
> > Nit: We don't want to include oleacc.h for this definition?
> 
> I think this is better, because hundreds of files include
native_widget_types.h
> but only a tiny fraction of them actually need oleacc.h. I think I only needed
> to add #include <oleacc.h> to 1 or 2 cc files.
SGTM

> 
>
http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
> File views/controls/native/native_view_host.cc (right):
> 
>
http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
> views/controls/native/native_view_host.cc:204: gfx::NativeAccessibleView
> accessible_view =
> On 2011/06/10 19:10:09, Chris Guillory wrote:
> > Do we need to verify native_wrapper_.get()?
> 
> Done.
> 
>
http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
> File views/controls/native/native_view_host_wrapper.h (right):
> 
>
http://codereview.chromium.org/6995126/diff/5005/views/controls/native/native...
> views/controls/native/native_view_host_wrapper.h:61: // Return the native
> accessible view corresponding to this view.
> On 2011/06/10 19:10:09, Chris Guillory wrote:
> > If you decide to rename the type:
> > accessible view -> view accessible
> 
> Done.
> 
> http://codereview.chromium.org/6995126/diff/5005/views/view.h
> File views/view.h (right):
> 
> http://codereview.chromium.org/6995126/diff/5005/views/view.h#newcode1444
> views/view.h:1444: #ifdef OS_WIN
> On 2011/06/10 20:09:01, tfarina wrote:
> > the old code is corrected and the preferred way.
> 
> Done.

[tfarina, 2011-06-11 02:06:07]

http://codereview.chromium.org/6995126/diff/3070/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6995126/diff/3070/views/view.h#newcode871
views/view.h:871: // Returns an instance of the native accessibility interface
for this View.
nit: View -> view

http://codereview.chromium.org/6995126/diff/3070/views/view.h#newcode1443
views/view.h:1443: // The Windows-specific accessibility implementation for this
View.
please, leave this comment where it was?

http://codereview.chromium.org/6995126/diff/3070/views/view.h#newcode1443
views/view.h:1443: // The Windows-specific accessibility implementation for this
View.
nit: View -> view

[dmazzoni, 2011-06-13 19:31:53]

http://codereview.chromium.org/6995126/diff/3070/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6995126/diff/3070/views/view.h#newcode871
views/view.h:871: // Returns an instance of the native accessibility interface
for this View.
On 2011/06/11 02:06:07, tfarina wrote:
> nit: View -> view

Done.

http://codereview.chromium.org/6995126/diff/3070/views/view.h#newcode1443
views/view.h:1443: // The Windows-specific accessibility implementation for this
View.
On 2011/06/11 02:06:07, tfarina wrote:
> nit: View -> view

Done.

[Ben Goodger (Google), 2011-06-13 19:37:44]

Thank you very much for fixing this. LGTM. Just a couple of nits below.

http://codereview.chromium.org/6995126/diff/3076/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc
(right):

http://codereview.chromium.org/6995126/diff/3076/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc:140:
NativeTabContentsContainerWin::GetNativeViewAccessible() {
this line should be indented 4 spaces

http://codereview.chromium.org/6995126/diff/3076/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/tab_contents_container_native.h
(right):

http://codereview.chromium.org/6995126/diff/3076/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/tab_contents_container_native.h:12:
#include "ui/base/accessibility/accessibility_types.h"
??

[Chris Guillory, 2011-06-13 20:49:27]

http://codereview.chromium.org/6995126/diff/3076/chrome/browser/ui/views/tab_...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc
(right):

http://codereview.chromium.org/6995126/diff/3076/chrome/browser/ui/views/tab_...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.cc:140:
NativeTabContentsContainerWin::GetNativeViewAccessible() {
On 2011/06/13 19:37:44, Ben Goodger wrote:
> this line should be indented 4 spaces
My mistake. Looks like you had it right the first time.

[commit-bot: I haz the power, 2011-06-13 22:25:27]

Change committed as 88913