Add support for UIA accessibility interfaces like IAccessibleEx and IRawElementProviderSimple.

Add support for UIA accessibility interfaces like IAccessibleEx and IRawElementProviderSimple.
This is required for the on screen keyboard to show up in Windows 8 metro. At this point
we only implement the GetPatternProvider/GetPropertyValue methods in the IRawElementProviderSimple.
interface and the rest of the methods in the interfaces are left unimplemented.

The GetPatternProvider/GetPropertyValue only handle the case where the user touches an editable
portion on the window/page. This ensures that the OSK shows up on the omnibox and
the web page. The ITextProvider and IValueProvider implementation is provided by
the UIATextProvider class which lives in base/win/accessibility_misc_utils.cc/.h files.

BUG=118641
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130744

[dmazzoni, 2012-04-04 06:02:00]

Looks totally reasonable to me, just some nits.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.cc:7: #include
<atlsafe.h>
Are you actually using atlsafe? I don't see CComSafeArray, etc.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.cc:2539: // is needed
for the On screen Keyboard to show up in metro mode, when the
Nit: On screen -> on-screen

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.cc:2568: VARIANT* ret) {
Nit: indentation

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:13: #include
<UIAutomationCore.h>
Nit: Group this with the other Windows includes, above - rather than with STL
includes

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:700: // IAccessibleEx
methods not implemented.
Nit: Use a large comment block to be consistent:

//
// IAccessibleEx methods not implemented.
//

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:721: //
IRawElementProviderSimple methods.
Same

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:723: // the on screen
keyboard to show up in Windows 8 metro.
Nit: on screen -> on-screen.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:726: CONTENT_EXPORT
STDMETHOD(GetPropertyValue)(PROPERTYID id, VARIANT* ret);
Does it work if you use STDMETHODIMP GetPropertyValue(...) rather than
STDMETHOD(GetPropertyValue)(...) to be consistent?

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
File ui/base/accessibility/accessible_text_utils.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.cc:100:
text_provider->set_editable(true);
set_editable(editable)

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.cc:102: HRESULT hr =
text_provider->QueryInterface(
Is this necessary? I think you should be able to just return text_provider
directly, like:

*provider = text_provider;

Then the Release() shouldn't be necessary.

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
File ui/base/accessibility/accessible_text_utils.h (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.h:61: class UI_EXPORT
UIATextProvider
After looking at this, I think a better place for it would be base/win. It's
most similar to base/win/enum_variant.h, which implements a Windows-only helper
class used by accessibility code in both content/browser/accessibility and
ui/views/accessibility.

Perhaps at some point we may split these into base/win/accessibility.

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
File ui/views/accessibility/native_view_accessibility_win.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
ui/views/accessibility/native_view_accessibility_win.cc:5: #define _ATL_DEBUG_QI
Delete this before checking in?

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
ui/views/accessibility/native_view_accessibility_win.cc:11: #include
<UIAutomationClient.h>
Nit: group above

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
File ui/views/accessibility/native_view_accessibility_win.h (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
ui/views/accessibility/native_view_accessibility_win.h:294: // IAccessibleEx
methods not implemented.
Nit: same

[ananta, 2012-04-04 19:23:00]

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.cc:7: #include
<atlsafe.h>
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Are you actually using atlsafe? I don't see CComSafeArray, etc.
Removed

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.cc:2539: // is needed
for the On screen Keyboard to show up in metro mode, when the
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Nit: On screen -> on-screen

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.cc:2568: VARIANT* ret) {
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Nit: indentation

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:13: #include
<UIAutomationCore.h>
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Nit: Group this with the other Windows includes, above - rather than with STL
> includes

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:700: // IAccessibleEx
methods not implemented.
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Nit: Use a large comment block to be consistent:
> 
> //
> // IAccessibleEx methods not implemented.
> //

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:721: //
IRawElementProviderSimple methods.
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Same

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:723: // the on screen
keyboard to show up in Windows 8 metro.
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Nit: on screen -> on-screen.

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/content/browser/acce...
content/browser/accessibility/browser_accessibility_win.h:726: CONTENT_EXPORT
STDMETHOD(GetPropertyValue)(PROPERTYID id, VARIANT* ret);
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Does it work if you use STDMETHODIMP GetPropertyValue(...) rather than
> STDMETHOD(GetPropertyValue)(...) to be consistent?

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
File ui/base/accessibility/accessible_text_utils.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.cc:100:
text_provider->set_editable(true);
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> set_editable(editable)

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.cc:102: HRESULT hr =
text_provider->QueryInterface(
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Is this necessary? I think you should be able to just return text_provider
> directly, like:
> 
> *provider = text_provider;
> 
> Then the Release() shouldn't be necessary.

That returns the following error.
error C2594: '=' : ambiguous conversions from 'ATL::CComObject<Base> *' to
'IUnknown *'

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
File ui/base/accessibility/accessible_text_utils.h (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.h:61: class UI_EXPORT
UIATextProvider
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> After looking at this, I think a better place for it would be base/win. It's
> most similar to base/win/enum_variant.h, which implements a Windows-only
helper
> class used by accessibility code in both content/browser/accessibility and
> ui/views/accessibility.
> 
> Perhaps at some point we may split these into base/win/accessibility.

Done. Moved the class to the new files accessibility_misc_utils.cc/.h under
base\win

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
File ui/views/accessibility/native_view_accessibility_win.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
ui/views/accessibility/native_view_accessibility_win.cc:5: #define _ATL_DEBUG_QI
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Delete this before checking in?

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
ui/views/accessibility/native_view_accessibility_win.cc:11: #include
<UIAutomationClient.h>
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Nit: group above

Done.

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
File ui/views/accessibility/native_view_accessibility_win.h (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/views/accessibili...
ui/views/accessibility/native_view_accessibility_win.h:294: // IAccessibleEx
methods not implemented.
On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> Nit: same

Done.

[dmazzoni, 2012-04-04 19:30:14]

lgtm

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
File ui/base/accessibility/accessible_text_utils.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.cc:102: HRESULT hr =
text_provider->QueryInterface(
On 2012/04/04 19:23:00, ananta wrote:
> On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> > Is this necessary? I think you should be able to just return text_provider
> > directly, like:
> > 
> > *provider = text_provider;
> > 
> > Then the Release() shouldn't be necessary.
> 
> That returns the following error.
> error C2594: '=' : ambiguous conversions from 'ATL::CComObject<Base> *' to
> 'IUnknown *'

This is because of the multiple inheritance. You should be able to cast
text_provider to ITextProvider first - that only has one path to IUnknown:

*provider = static_cast<ITextProvider*>(text_provider);

[jar (doing other things), 2012-04-04 21:38:59]

Predominantly rubber stamp LGTM for base landing.

[ananta, 2012-04-04 21:40:17]

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
File ui/base/accessibility/accessible_text_utils.cc (right):

https://chromiumcodereview.appspot.com/9958139/diff/7001/ui/base/accessibilit...
ui/base/accessibility/accessible_text_utils.cc:102: HRESULT hr =
text_provider->QueryInterface(
On 2012/04/04 19:30:15, Dominic Mazzoni wrote:
> On 2012/04/04 19:23:00, ananta wrote:
> > On 2012/04/04 06:02:00, Dominic Mazzoni wrote:
> > > Is this necessary? I think you should be able to just return text_provider
> > > directly, like:
> > > 
> > > *provider = text_provider;
> > > 
> > > Then the Release() shouldn't be necessary.
> > 
> > That returns the following error.
> > error C2594: '=' : ambiguous conversions from 'ATL::CComObject<Base> *' to
> > 'IUnknown *'
> 
> This is because of the multiple inheritance. You should be able to cast
> text_provider to ITextProvider first - that only has one path to IUnknown:
> 
> *provider = static_cast<ITextProvider*>(text_provider);

Done.