Enable renderer accessibility by default....

Enable renderer accessibility by default.

BUG=25564
TEST=none


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

[dmazzoni, 2010-06-01 13:51:54]

I like the refactoring, it puts the logic in just one place.

Returning a Document with state BUSY would be a great addition to this CL.

Also, what about having views/accessibility/view_accessibility.cc also detect
the presence of a screenreader and automatically enable renderer accessibility?
Or you could save that for a future CL.

http://codereview.chromium.org/2358003/diff/13001/14002
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/2358003/diff/13001/14002#newcode1102
chrome/browser/renderer_host/render_widget_host.cc:1102:
switches::kDisableRendererAccessibility))
Put brackets around this

http://codereview.chromium.org/2358003/diff/13001/14003
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/2358003/diff/13001/14003#newcode673
chrome/browser/renderer_host/render_widget_host.h:673: // Whether the renderer
is made accessible.
Maybe add a comment here that this potentially slows down Chrome, so should only
be used if a screenreader is detected.

http://codereview.chromium.org/2358003/diff/13001/14004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/13001/14004#newcode1541
chrome/browser/renderer_host/render_widget_host_view_win.cc:1541:
render_widget_host_->EnableRendererAccessibility();
Need to also return something here, like a document with state "busy".

[tfarina, 2010-06-01 14:02:43]

Some style comments.

http://codereview.chromium.org/2358003/diff/13001/14003
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/2358003/diff/13001/14003#newcode1
chrome/browser/renderer_host/render_widget_host.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/2358003/diff/13001/14004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/13001/14004#newcode1546
chrome/browser/renderer_host/render_widget_host_view_win.cc:1546:
IID_IAccessible, wparam,
the indentation looks wrong here.

http://codereview.chromium.org/2358003/diff/13001/14005
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/2358003/diff/13001/14005#newcode7
chrome/browser/renderer_host/render_widget_host_view_win.h:7: 
remove this blank line.

http://codereview.chromium.org/2358003/diff/13001/14007
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2358003/diff/13001/14007#newcode16
chrome/browser/tab_contents/tab_contents.h:16: #include "build/build_config.h"
please, left it on the old place, since this is the common case for this
particular include.

http://codereview.chromium.org/2358003/diff/13001/14009
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/2358003/diff/13001/14009#newcode112
chrome/common/chrome_switches.h:112: extern const char
kDisableRendererAccessibility[];
not sure if this is the best place, maybe sort alphabetical?

http://codereview.chromium.org/2358003/diff/13001/14011
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2358003/diff/13001/14011#newcode1
chrome_frame/chrome_launcher.cc:1: // Copyright (c) 2009 The Chromium Authors.
All rights reserved.
nit: 2010

[Chris Guillory, 2010-06-07 20:33:08]

Thanks for reviews. I'm back from my vacation last week now.

http://codereview.chromium.org/2358003/diff/13001/14002
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/2358003/diff/13001/14002#newcode1102
chrome/browser/renderer_host/render_widget_host.cc:1102:
switches::kDisableRendererAccessibility))
On 2010/06/01 13:51:54, Dominic Mazzoni wrote:
> Put brackets around this

Done.

http://codereview.chromium.org/2358003/diff/13001/14003
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/2358003/diff/13001/14003#newcode673
chrome/browser/renderer_host/render_widget_host.h:673: // Whether the renderer
is made accessible.
On 2010/06/01 13:51:54, Dominic Mazzoni wrote:
> Maybe add a comment here that this potentially slows down Chrome, so should
only
> be used if a screenreader is detected.

Done.

http://codereview.chromium.org/2358003/diff/13001/14004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/13001/14004#newcode1541
chrome/browser/renderer_host/render_widget_host_view_win.cc:1541:
render_widget_host_->EnableRendererAccessibility();
On 2010/06/01 13:51:54, Dominic Mazzoni wrote:
> Need to also return something here, like a document with state "busy".
> 

Yes. I'll make that a part of this change.

http://codereview.chromium.org/2358003/diff/13001/14004#newcode1546
chrome/browser/renderer_host/render_widget_host_view_win.cc:1546:
IID_IAccessible, wparam,
On 2010/06/01 14:02:44, tfarina wrote:
> the indentation looks wrong here.

Done.

http://codereview.chromium.org/2358003/diff/13001/14005
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/2358003/diff/13001/14005#newcode7
chrome/browser/renderer_host/render_widget_host_view_win.h:7: 
On 2010/06/01 14:02:44, tfarina wrote:
> remove this blank line.

Done.

http://codereview.chromium.org/2358003/diff/13001/14007
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2358003/diff/13001/14007#newcode16
chrome/browser/tab_contents/tab_contents.h:16: #include "build/build_config.h"
On 2010/06/01 14:02:44, tfarina wrote:
> please, left it on the old place, since this is the common case for this
> particular include.

If I do that gcl lint reports errors:
"Found C++ system header after other header. Should be: tab_contents.h, c
system, c++ system, other.  [build/include_order] [4]"

http://codereview.chromium.org/2358003/diff/13001/14009
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/2358003/diff/13001/14009#newcode112
chrome/common/chrome_switches.h:112: extern const char
kDisableRendererAccessibility[];
On 2010/06/01 14:02:44, tfarina wrote:
> not sure if this is the best place, maybe sort alphabetical?

Done. Thanks. I moved the definition in chrome_switches.cc as well.

[tfarina, 2010-06-09 18:10:29]

LGTM, but it needs the approval of Dominic too. My style nits below.

http://codereview.chromium.org/2358003/diff/44001/45004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/44001/45004#newcode43
chrome/browser/renderer_host/render_widget_host_view_win.cc:43: // Included for
views::kReflectedMessage - TODO(beng): move this to win_util.h!
seems like this comment belong to widget_win.h not to view.h

http://codereview.chromium.org/2358003/diff/44001/45004#newcode1571
chrome/browser/renderer_host/render_widget_host_view_win.cc:1571:
IID_IAccessible, wparam,
please, fix the indentation here. IID_IAccessible could be on the same line of
Lresult...

http://codereview.chromium.org/2358003/diff/44001/45005
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/2358003/diff/44001/45005#newcode254
chrome/browser/renderer_host/render_widget_host_view_win.h:254: // MSAA
IAccessible returned while page contents is loading
Please, end with period.

[Chris Guillory, 2010-06-09 18:22:41]

http://codereview.chromium.org/2358003/diff/44001/45004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/44001/45004#newcode43
chrome/browser/renderer_host/render_widget_host_view_win.cc:43: // Included for
views::kReflectedMessage - TODO(beng): move this to win_util.h!
On 2010/06/09 18:10:30, tfarina wrote:
> seems like this comment belong to widget_win.h not to view.h

Oops. Done.

http://codereview.chromium.org/2358003/diff/44001/45004#newcode1571
chrome/browser/renderer_host/render_widget_host_view_win.cc:1571:
IID_IAccessible, wparam,
On 2010/06/09 18:10:30, tfarina wrote:
> please, fix the indentation here. IID_IAccessible could be on the same line of
> Lresult...

I've made it look like this:
        return LresultFromObject(IID_IAccessible, wparam,
            static_cast<IAccessible*>(root->NewReference()));

http://codereview.chromium.org/2358003/diff/44001/45005
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/2358003/diff/44001/45005#newcode254
chrome/browser/renderer_host/render_widget_host_view_win.h:254: // MSAA
IAccessible returned while page contents is loading
On 2010/06/09 18:10:30, tfarina wrote:
> Please, end with period.

Done.

[dmazzoni, 2010-06-09 22:09:00]

LGTM!

Two tiny style issues, then feel free to submit.

http://codereview.chromium.org/2358003/diff/56001/57004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/56001/57004#newcode292
chrome/browser/renderer_host/render_widget_host_view_win.cc:292:
screenreader_running) {
indent one more character

http://codereview.chromium.org/2358003/diff/56001/57008
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2358003/diff/56001/57008#newcode190
chrome/common/chrome_switches.cc:190: const char kDisableRendererAccessibility[]
  = "disable-renderer-accessibility";
indent the equals sign one less

[dmazzoni, 2010-06-09 22:11:54]

http://codereview.chromium.org/2358003/diff/56001/57004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/56001/57004#newcode1550
chrome/browser/renderer_host/render_widget_host_view_win.cc:1550: if (lparam ==
OBJID_CLIENT) {
One other idea to improve readability: how about changing this to if (lparam !=
OBJID_CLIENT), so the majority of the function body doesn't need to be indented?

http://codereview.chromium.org/2358003/diff/56001/57004#newcode1561
chrome/browser/renderer_host/render_widget_host_view_win.cc:1561: // Annotate
with STATE_SYSTEM_BUSY to indicate that the page is loading.
Maybe note that the HWND itself is being annotated, not the loading_accessible_
object. I think it works, but the comment may want to note that.

[Chris Guillory, 2010-06-09 22:12:07]

http://codereview.chromium.org/2358003/diff/56001/57004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/56001/57004#newcode292
chrome/browser/renderer_host/render_widget_host_view_win.cc:292:
screenreader_running) {
On 2010/06/09 22:09:00, Dominic Mazzoni wrote:
> indent one more character

Done.

http://codereview.chromium.org/2358003/diff/56001/57008
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2358003/diff/56001/57008#newcode190
chrome/common/chrome_switches.cc:190: const char kDisableRendererAccessibility[]
  = "disable-renderer-accessibility";
On 2010/06/09 22:09:00, Dominic Mazzoni wrote:
> indent the equals sign one less

Done.

[Chris Guillory, 2010-06-09 22:27:30]

http://codereview.chromium.org/2358003/diff/56001/57004
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/2358003/diff/56001/57004#newcode1550
chrome/browser/renderer_host/render_widget_host_view_win.cc:1550: if (lparam ==
OBJID_CLIENT) {
On 2010/06/09 22:11:56, Dominic Mazzoni wrote:
> One other idea to improve readability: how about changing this to if (lparam
!=
> OBJID_CLIENT), so the majority of the function body doesn't need to be
indented?

Done.

http://codereview.chromium.org/2358003/diff/56001/57004#newcode1561
chrome/browser/renderer_host/render_widget_host_view_win.cc:1561: // Annotate
with STATE_SYSTEM_BUSY to indicate that the page is loading.
On 2010/06/09 22:11:56, Dominic Mazzoni wrote:
> Maybe note that the HWND itself is being annotated, not the
loading_accessible_
> object. I think it works, but the comment may want to note that.

Done.

[dmazzoni, 2010-06-09 22:35:53]

LGTM.