Reimplement web content accessibility by caching in browser process

Reimplement accessibility of web content by caching the entire 
accessibility tree in the browser process.

Adds new RPCs for a browser tab to request accessibility info from
a renderer; the renderer responds with a complete tree of
accessibility metadata for the entire DOM, which is then cached
in the RenderWidgetHostView. This part is cross-platform and will
help with accessibility on both Windows and Mac OS X.

For Windows, MSAA support for web content has been rewritten to
use this new cache. Tested in JAWS and NVDA screen readers.
Using Chrome with a screen reader is now fast and stable,
unlike the previous implementation. However, note that most
advanced functionality is still not supported, and much work remains
to make Chrome work well with a screen reader. This is a necessary
step to improve stability first.

BUG=25564
BUG=13291
TEST=See http://codereview.chromium.org/1806001


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

[Chris Guillory, 2010-04-28 02:36:03]

Here are some comments after looking this over a bit. I'd like to play with it a
bit more.

Can we make the browser project depend on the iaccessible2 project? If a clean
the iaccessible2 project then try to build browser it currently doesn't
automatically rebuild iaccessible2.

http://codereview.chromium.org/1637018/diff/44001/45001
File app/win/window_impl.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45001#newcode66
app/win/window_impl.cc:66: //name->assign(L"MozillaUIWindowClass");
Perhaps we can use add a temporary flag for the window class name change.

http://codereview.chromium.org/1637018/diff/44001/45013
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45013#newcode39
chrome/browser/browser_accessibility.cc:39: if (role_ == ROLE_SYSTEM_DOCUMENT)
Can you add some comments explaining this role and state logic. This doesn't
seem like the right place for it.

http://codereview.chromium.org/1637018/diff/44001/45013#newcode162
chrome/browser/browser_accessibility.cc:162: BrowserAccessibility* target;
Nit: Use CComPtr for BrowserAccessibility pointers

http://codereview.chromium.org/1637018/diff/44001/45015
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/44001/45015#newcode217
chrome/browser/browser_accessibility.h:217: std::vector<BrowserAccessibility*>
children_;
Nit: Use CComPtr for simple reference management.

http://codereview.chromium.org/1637018/diff/44001/45005
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45005#newcode16
chrome/browser/browser_accessibility_manager.cc:16: long
BrowserAccessibilityManager::next_child_id_ = 1000000;
Can you add some comments regarding why this value is so large and where its
uniqueness is important

http://codereview.chromium.org/1637018/diff/44001/45005#newcode110
chrome/browser/browser_accessibility_manager.cc:110: if (next_child_id_ ==
2000000000)
Comments here please. And possibly an assert?

http://codereview.chromium.org/1637018/diff/44001/45005#newcode124
chrome/browser/browser_accessibility_manager.cc:124: return
reinterpret_cast<BrowserAccessibility*>(instance);
Cast should be static but probably just left off.

http://codereview.chromium.org/1637018/diff/44001/45014
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/1637018/diff/44001/45014#newcode51
chrome/browser/browser_accessibility_manager.h:51: IAccessible*
window_iaccessible_;
We should use CComPtr to make reference tracking simpler for window_iaccessible,
root_, and focus_.

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

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1491
chrome/browser/renderer_host/render_widget_host_view_win.cc:1491:
RenderProcessHost* render_process_host =
Comments explaining whats going on here please.

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1506
chrome/browser/renderer_host/render_widget_host_view_win.cc:1506: // if
(!renderer_accessible_)
Un-comment these lines?

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1512
chrome/browser/renderer_host/render_widget_host_view_win.cc:1512: return
E_NOTIMPL;
The return value of WM_GETOBJECT is an accessible object identifier. Looks like
we should just pass this message to DefWindowProc here. Or perhaps in the case
of OBJID_WINDOW we should return the window_iaccessible_ from
BrowserAccessibilityManager.

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1515
chrome/browser/renderer_host/render_widget_host_view_win.cc:1515: IAccessible*
result = browser_accessibility_manager_->GetRootIAccessible();
Nit: Use CComPtr.

http://codereview.chromium.org/1637018/diff/44001/45019
File chrome/common/common_param_traits.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45019#newcode332
chrome/common/common_param_traits.cc:332: int role;
Do we need to check the bounds here? If so perhaps we can move this logic into a
ParamTraits<webkit_glue::WebAccessibility::Role>::Read specialization. Same for
state.

http://codereview.chromium.org/1637018/diff/44001/45018
File chrome/common/common_param_traits_unittest.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45018#newcode220
chrome/common/common_param_traits_unittest.cc:220: input.role =
webkit_glue::WebAccessibility::ROLE_CHECKBUTTON;
Nit: Role and state should be shifted.

http://codereview.chromium.org/1637018/diff/44001/45018#newcode254
chrome/common/common_param_traits_unittest.cc:254: outer.role =
webkit_glue::WebAccessibility::ROLE_GROUPING;
Nit: Role and should be shifted.

http://codereview.chromium.org/1637018/diff/44001/45018#newcode269
chrome/common/common_param_traits_unittest.cc:269: inner2.role =
webkit_glue::WebAccessibility::ROLE_RADIOBUTTON;
Nit: Role and state should be shifted.

http://codereview.chromium.org/1637018/diff/44001/45020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45020#newcode3924
chrome/renderer/render_view.cc:3924: void
RenderView::RecursiveCreateAccessibilityTree(
Can this functionality move into the WebAccessibility constructor?

http://codereview.chromium.org/1637018/diff/44001/45020#newcode3948
chrome/renderer/render_view.cc:3948: printf("  Sent accessibility tree.\n");
Nit: Remove printf.

http://codereview.chromium.org/1637018/diff/44001/45003
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45003#newcode107
webkit/glue/webaccessibility.cc:107: state |= static_cast<int32>(1 <<
WebAccessibility::STATE_CHECKED);
I don't believe static_cast<int32> is needed.

http://codereview.chromium.org/1637018/diff/44001/45004
File webkit/glue/webaccessibility.h (left):

http://codereview.chromium.org/1637018/diff/44001/45004#oldcode178
webkit/glue/webaccessibility.h:178: };
DISALLOW_COPY_AND_ASSIGN(WebAccessibility);

http://codereview.chromium.org/1637018/diff/44001/45004
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/1637018/diff/44001/45004#newcode87
webkit/glue/webaccessibility.h:87: void ConvertFromWebKit(const
WebKit::WebAccessibilityObject& src);
Can we make this function into the constructor.

http://codereview.chromium.org/1637018/diff/44001/45004#newcode89
webkit/glue/webaccessibility.h:89: int32 id;
Perhaps these members should be private.

http://codereview.chromium.org/1637018/diff/44001/45002
File webkit/glue/webkit_glue.gypi (left):

http://codereview.chromium.org/1637018/diff/44001/45002#oldcode331
webkit/glue/webkit_glue.gypi:331: # These files are Windows-only now but may be
ported to other
By removing these lines the webaccessibility.h/cc files are no longer excluded
on non-Windows platforms correct?

[dmazzoni, 2010-04-28 17:58:25]

http://codereview.chromium.org/1637018/diff/44001/45001
File app/win/window_impl.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45001#newcode66
app/win/window_impl.cc:66: //name->assign(L"MozillaUIWindowClass");
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Perhaps we can use add a temporary flag for the window class name change.

I've removed this from the CL for now. I think the better solution is just to
patch NVDA. SAtoGO already sent us a
patched version that knows about Chrome's window classes.

http://codereview.chromium.org/1637018/diff/44001/45013
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45013#newcode39
chrome/browser/browser_accessibility.cc:39: if (role_ == ROLE_SYSTEM_DOCUMENT)
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Can you add some comments explaining this role and state logic. This doesn't
> seem like the right place for it.

I've deleted this code for now. It helped a lot with NVDA accessibility, but I
agree it doesn't belong here. Let's add it in the next CL.

http://codereview.chromium.org/1637018/diff/44001/45013#newcode162
chrome/browser/browser_accessibility.cc:162: BrowserAccessibility* target;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Nit: Use CComPtr for BrowserAccessibility pointers

In general yes, but how would it help here?

http://codereview.chromium.org/1637018/diff/44001/45015
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/44001/45015#newcode217
chrome/browser/browser_accessibility.h:217: std::vector<BrowserAccessibility*>
children_;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Nit: Use CComPtr for simple reference management.

I tried and I thought it just made it a lot uglier. We don't actually want to
dereference our children automatically when this object is deleted, but actually
earlier when DerefTree is called.

http://codereview.chromium.org/1637018/diff/44001/45005
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45005#newcode16
chrome/browser/browser_accessibility_manager.cc:16: long
BrowserAccessibilityManager::next_child_id_ = 1000000;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Can you add some comments regarding why this value is so large and where its
> uniqueness is important

Done.

http://codereview.chromium.org/1637018/diff/44001/45005#newcode110
chrome/browser/browser_accessibility_manager.cc:110: if (next_child_id_ ==
2000000000)
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Comments here please. And possibly an assert?

Done.

http://codereview.chromium.org/1637018/diff/44001/45005#newcode124
chrome/browser/browser_accessibility_manager.cc:124: return
reinterpret_cast<BrowserAccessibility*>(instance);
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Cast should be static but probably just left off.

Done.

http://codereview.chromium.org/1637018/diff/44001/45014
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/1637018/diff/44001/45014#newcode51
chrome/browser/browser_accessibility_manager.h:51: IAccessible*
window_iaccessible_;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> We should use CComPtr to make reference tracking simpler for
window_iaccessible,
> root_, and focus_.

Done for window_iaccessible_, agreed.

The BrowserAccessibilityManager owns the whole tree and dereferences the entire
tree once on destruction. I added a
comment to the header file explaining why I think it should be done this way,
please see if that makes sense:

// The root of the tree of IAccessible objects and a pointer to the
  // element that currently has focus, if any. These are not CComPtrs
  // because in our destructor, we want to instead call root_->DerefTree,
  // which recursively releases references to all of its descendants,
  // rather than just using Release. If we didn't do this, then if a client
  // kept a reference to the root node, the entire tree would have to remain
  // in memory.
  BrowserAccessibility* root_;
  BrowserAccessibility* focus_;

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

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1491
chrome/browser/renderer_host/render_widget_host_view_win.cc:1491:
RenderProcessHost* render_process_host =
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Comments explaining whats going on here please.

Done.

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1506
chrome/browser/renderer_host/render_widget_host_view_win.cc:1506: // if
(!renderer_accessible_)
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Un-comment these lines?

Done.

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1512
chrome/browser/renderer_host/render_widget_host_view_win.cc:1512: return
E_NOTIMPL;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> The return value of WM_GETOBJECT is an accessible object identifier. Looks
like
> we should just pass this message to DefWindowProc here. Or perhaps in the case
> of OBJID_WINDOW we should return the window_iaccessible_ from
> BrowserAccessibilityManager.

It looks like ATL will take care of this for us if we set handled to false.
Done.

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1515
chrome/browser/renderer_host/render_widget_host_view_win.cc:1515: IAccessible*
result = browser_accessibility_manager_->GetRootIAccessible();
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Nit: Use CComPtr.

Not needed, right?

http://codereview.chromium.org/1637018/diff/44001/45019
File chrome/common/common_param_traits.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45019#newcode332
chrome/common/common_param_traits.cc:332: int role;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Do we need to check the bounds here? If so perhaps we can move this logic into
a
> ParamTraits<webkit_glue::WebAccessibility::Role>::Read specialization. Same
for
> state.

Hmmm, I don't think the wrong role would do any harm.

http://codereview.chromium.org/1637018/diff/44001/45018
File chrome/common/common_param_traits_unittest.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45018#newcode220
chrome/common/common_param_traits_unittest.cc:220: input.role =
webkit_glue::WebAccessibility::ROLE_CHECKBUTTON;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Nit: Role and state should be shifted.

Oops, good catch! Actually not role, but state should be.

Done.

http://codereview.chromium.org/1637018/diff/44001/45018#newcode254
chrome/common/common_param_traits_unittest.cc:254: outer.role =
webkit_glue::WebAccessibility::ROLE_GROUPING;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Nit: Role and should be shifted.

Done.

http://codereview.chromium.org/1637018/diff/44001/45018#newcode269
chrome/common/common_param_traits_unittest.cc:269: inner2.role =
webkit_glue::WebAccessibility::ROLE_RADIOBUTTON;
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Nit: Role and state should be shifted.

Done.

http://codereview.chromium.org/1637018/diff/44001/45020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45020#newcode3924
chrome/renderer/render_view.cc:3924: void
RenderView::RecursiveCreateAccessibilityTree(
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Can this functionality move into the WebAccessibility constructor?

Done.

http://codereview.chromium.org/1637018/diff/44001/45020#newcode3948
chrome/renderer/render_view.cc:3948: printf("  Sent accessibility tree.\n");
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Nit: Remove printf.

Done.

http://codereview.chromium.org/1637018/diff/44001/45004
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/1637018/diff/44001/45004#newcode87
webkit/glue/webaccessibility.h:87: void ConvertFromWebKit(const
WebKit::WebAccessibilityObject& src);
On 2010/04/28 02:36:11, Chris Guillory wrote:
> Can we make this function into the constructor.

Done.

http://codereview.chromium.org/1637018/diff/44001/45002
File webkit/glue/webkit_glue.gypi (left):

http://codereview.chromium.org/1637018/diff/44001/45002#oldcode331
webkit/glue/webkit_glue.gypi:331: # These files are Windows-only now but may be
ported to other
On 2010/04/28 02:36:11, Chris Guillory wrote:
> By removing these lines the webaccessibility.h/cc files are no longer excluded
> on non-Windows platforms correct?

Yes. We want to be able to use them on Mac OS X. They won't hurt on Linux.

[Chris Guillory, 2010-04-28 18:46:50]

Some follow up comments.

http://codereview.chromium.org/1637018/diff/44001/45013
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45013#newcode162
chrome/browser/browser_accessibility.cc:162: BrowserAccessibility* target;
For reference counting simplicity. I think we should avoid writing code that
includes AddRef and Release calls. That becomes error prone very quickly.

http://codereview.chromium.org/1637018/diff/44001/45015
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/44001/45015#newcode217
chrome/browser/browser_accessibility.h:217: std::vector<BrowserAccessibility*>
children_;
We want to delete the COM object we it's reference count goes to 0. We don't
want to delete it if it's reference count is not 0. Anyone who is using the
object should have a reference to it.

http://codereview.chromium.org/1637018/diff/44001/45014
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/1637018/diff/44001/45014#newcode51
chrome/browser/browser_accessibility_manager.h:51: IAccessible*
window_iaccessible_;
If we are worried about clients keeping references to our objects we can give
them "weak" references. I'm like a broken record here. CComPtr makes the
reference counting simpler especially when if we'll attempt to change a part of
the tree. I'll hold further CComPtr comments, let's discuss this.

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

http://codereview.chromium.org/1637018/diff/44001/45012#newcode1515
chrome/browser/renderer_host/render_widget_host_view_win.cc:1515: IAccessible*
result = browser_accessibility_manager_->GetRootIAccessible();
I think we should avoid calling AddRef and Release explicitly.

http://codereview.chromium.org/1637018/diff/44001/45018
File chrome/common/common_param_traits_unittest.cc (right):

http://codereview.chromium.org/1637018/diff/44001/45018#newcode220
chrome/common/common_param_traits_unittest.cc:220: input.role =
webkit_glue::WebAccessibility::ROLE_CHECKBUTTON;
Ah yes, just State.

[jam, 2010-04-28 22:45:11]

Hi, because comments were sent before Dominic published this, it's not clear
whether this change is ready for review or not?

[dmazzoni, 2010-04-28 23:31:23]

Sorry for the confusion. Yes, this is ready for review, thanks!

- Dominic

On Wed, Apr 28, 2010 at 3:45 PM, <jam@chromium.org> wrote:

> Hi, because comments were sent before Dominic published this, it's not
> clear
> whether this change is ready for review or not?
>
>
> http://codereview.chromium.org/1637018/show
>

[dmazzoni, 2010-04-29 15:32:58]

http://codereview.chromium.org/1637018/diff/44001/45004
File webkit/glue/webaccessibility.h (left):

http://codereview.chromium.org/1637018/diff/44001/45004#oldcode178
webkit/glue/webaccessibility.h:178: };
On 2010/04/28 02:36:11, Chris Guillory wrote:
> DISALLOW_COPY_AND_ASSIGN(WebAccessibility);

Actually it turns out that objects that we serialize using IPCs do need to be
copied and assigned. So it's important that all data members of this class are
copyable objects and not pointers or references.

[jam, 2010-04-29 16:48:04]

very cool!

mostly nits below in the comments.  some questions:

-what's the plan for when the DOM changes?  will a full copy of the tree be sent
each time?  that would seem inefficient with a constantly changing page for
example.
-any data on how long it takes to generate the full tree and the size?  i.e.
what if there's a very large and complicated page, what would be the size of the
serialized data?  if it's very large, I don't know if we want to send it through
IPC, perhaps we'd want to use shared memory.  reason being that I believe pipes
on OS X become slow if messages on the order of 100K are sent through.  Another
reason is that there's a lot of copying of the data when sending through IPC.
-I see that the tree is requested asynchronously.  what happens if an
accessibility request from the OS comes in before we have tree?
-are you a committer?  I ask because I don't see any try bot results, and you'll
probably want to do that before checking this in

http://codereview.chromium.org/1637018/diff/63001/23028
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23028#newcode57
chrome/browser/browser_accessibility.cc:57: if (this == ancestor)
nit: need brace brackets here per style guide

http://codereview.chromium.org/1637018/diff/63001/23028#newcode74
chrome/browser/browser_accessibility.cc:74: index_in_parent_ <
static_cast<int>(parent_->children_.size() - 1))
nit: ditto

http://codereview.chromium.org/1637018/diff/63001/23028#newcode185
chrome/browser/browser_accessibility.cc:185: (*disp_child) = target;
nit: why the extra brackets?

http://codereview.chromium.org/1637018/diff/63001/23030
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/63001/23030#newcode16
chrome/browser/browser_accessibility.h:16: #include
"chrome/browser/browser_accessibility_manager.h"
nit: the chrome includes should be listed alphabetically

http://codereview.chromium.org/1637018/diff/63001/23030#newcode43
chrome/browser/browser_accessibility.h:43: : manager_(NULL),
nit: constructors usually go in the cc file (I know it was already like that,
but would be nice to move it)

http://codereview.chromium.org/1637018/diff/63001/23030#newcode175
chrome/browser/browser_accessibility.h:175: IAccessibleRelation **relation)
{return E_NOTIMPL; }
nit: I haven't sen this style before.  we usually one line statements in the
same line as the function name if they all fit on the same line.

http://codereview.chromium.org/1637018/diff/63001/23020
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23020#newcode52
chrome/browser/browser_accessibility_manager.cc:52: return obj;
nit: why not just return iter->second?

http://codereview.chromium.org/1637018/diff/63001/23020#newcode77
chrome/browser/browser_accessibility_manager.cc:77: if (iter !=
renderer_id_to_child_id_map_.end()) {
nit: we try to avoid functions which are all indented, especially because of the
80 char restriction.  so in this case it's nice to just do

if (iter == renderer_id_to_child_id_map_.end())
  return;

then the rest of the code has a little more room to work with

http://codereview.chromium.org/1637018/diff/63001/23020#newcode123
chrome/browser/browser_accessibility_manager.cc:123: for (int i = 0; i <
static_cast<int>(src.children.size()); i++) {
nit: convention is to use ++i, even with ints where admittedly it doesn't make a
difference

http://codereview.chromium.org/1637018/diff/63001/23027
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23027#newcode1492
chrome/browser/renderer_host/render_widget_host_view_win.cc:1492: // sure that's
why.
nit: for this comment and below, we try to avoid comments that are explaining
code in english, i.e. something that a developer can figure out right away by
looking at the code.

http://codereview.chromium.org/1637018/diff/63001/23027#newcode1520
chrome/browser/renderer_host/render_widget_host_view_win.cc:1520: LPARAM
object_id = (LPARAM)(DWORD)lparam;
huh, why the (LPARAM)(DWORD)?

http://codereview.chromium.org/1637018/diff/63001/23023
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/1637018/diff/63001/23023#newcode39
chrome/browser/renderer_host/render_widget_host_view_win.h:39: //   
L"MozillaContentWindowClass";
nit: no need to check this in right?

http://codereview.chromium.org/1637018/diff/63001/23021
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23021#newcode2275
chrome/browser/tab_contents/tab_contents.cc:2275: printf("DidNavigate %s\n",
GetURL().spec().c_str());
nit: please no printfs, only DLOGs and LOGs

http://codereview.chromium.org/1637018/diff/63001/23022
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/1637018/diff/63001/23022#newcode1267
chrome/browser/tab_contents/tab_contents.h:1267: // TODO(jcampan):
http://b/issue?id=1432077 This is a temporary work-around
we shouldn't be having bugs that point internally since external contributors
won't see them.  can this be ported to our bug tracker?  and should the TODO
still be assigned to jcampan?

http://codereview.chromium.org/1637018/diff/63001/23036
File chrome/common/common_param_traits.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23036#newcode8
chrome/common/common_param_traits.cc:8: #include
"chrome/common/webkit_param_traits.h"
nit: it seems that these two files are independent, why are you including this
file?

http://codereview.chromium.org/1637018/diff/63001/23036#newcode336
chrome/common/common_param_traits.cc:336: p->role =
static_cast<webkit_glue::WebAccessibility::Role>(role);
nit: need brace brackets here (any if/else statement that takes more than 4
lines)

http://codereview.chromium.org/1637018/diff/63001/23036#newcode350
chrome/common/common_param_traits.cc:350: l->append(L"<WebAccessibility>");
nit: we only put <struct name> when we're too lazy to actually display it.  In
this case, no need to add that.  also by convention we put brackets in the
beginning and end (see render_messages.h for examples, unfortunately this file
has examples that aren't consistent with the rest of the code)

http://codereview.chromium.org/1637018/diff/63001/23033
File chrome/common/common_param_traits.h (right):

http://codereview.chromium.org/1637018/diff/63001/23033#newcode439
chrome/common/common_param_traits.h:439: struct
ParamTraits<webkit_glue::WebAccessibility> {
why is the traits going in common_param_traits.h?  that file should be for
traits that are sent to/from multiple type of child processes (i.e. renderer,
plugin).  if it's just to renderer, it should be in render_messages.h

http://codereview.chromium.org/1637018/diff/63001/23037
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23037#newcode1331
chrome/renderer/render_view.cc:1331: accessibility_.reset(NULL);
nit: we usually just do .reset()

http://codereview.chromium.org/1637018/diff/63001/23015
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23015#newcode149
webkit/glue/webaccessibility.cc:149: : id(-1),
nit: this should be indented 4 spaces

http://codereview.chromium.org/1637018/diff/63001/23015#newcode155
webkit/glue/webaccessibility.cc:155: WebKit::WebAccessibilityCache* cache) {
no tabs (and other places below)

http://codereview.chromium.org/1637018/diff/63001/23017
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/1637018/diff/63001/23017#newcode98
webkit/glue/webaccessibility.h:98: int32 id;
member variables of a class should all end in underscore

[tfarina (gmail-do not use), 2010-04-29 20:33:55]

Hi Dominic,

Just minor comments, in complement of Chris and John's comments.

Thanks!

http://codereview.chromium.org/1637018/diff/63001/23028
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23028#newcode1
chrome/browser/browser_accessibility.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1637018/diff/63001/23028#newcode18
chrome/browser/browser_accessibility.cc:18: manager_ = manager;
nit: could we initialize everything in the initializer list?

http://codereview.chromium.org/1637018/diff/63001/23028#newcode45
chrome/browser/browser_accessibility.cc:45: children_.push_back(child);
nit: shouldn't we check here for duplicates before adding a child?

http://codereview.chromium.org/1637018/diff/63001/23028#newcode49
chrome/browser/browser_accessibility.cc:49: for (size_t i = 0; i <
children_.size(); i++) {
nit: ++i, also please for single line statement no need of {}

http://codereview.chromium.org/1637018/diff/63001/23028#newcode61
chrome/browser/browser_accessibility.cc:61: else
nit: no need of this last else. We will return anyway, when we arrive here.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode68
chrome/browser/browser_accessibility.cc:68: else
nit: here too. And the same below. :)

http://codereview.chromium.org/1637018/diff/63001/23028#newcode445
chrome/browser/browser_accessibility.cc:445: STDMETHODIMP
BrowserAccessibility::role(LONG *role) {
nit: LONG*

Also could you fix the other occurrences below?

http://codereview.chromium.org/1637018/diff/63001/23028#newcode523
chrome/browser/browser_accessibility.cc:523: if (guidService == IID_IAccessible
|| guidService == IID_IAccessible2) {
nit: no need of {}

http://codereview.chromium.org/1637018/diff/63001/23030
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/63001/23030#newcode1
chrome/browser/browser_accessibility.h:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1637018/diff/63001/23030#newcode156
chrome/browser/browser_accessibility.h:156: STDMETHODIMP role(LONG *role);
nit: LONG*

Also, could you fix the other occurrences below?

http://codereview.chromium.org/1637018/diff/63001/23020
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23020#newcode58
chrome/browser/browser_accessibility_manager.cc:58: IAccessible*
BrowserAccessibilityManager::GetParentWindowIAccessible() {
nit: this could be moved to header file and inlined as:

parent_window_IAccessible() { return window_iaccessible_; }

Same below. But this is up to you, I'm fine either way.

http://codereview.chromium.org/1637018/diff/63001/23020#newcode70
chrome/browser/browser_accessibility_manager.cc:70: else
nit: no need of else.

http://codereview.chromium.org/1637018/diff/63001/23029
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/1637018/diff/63001/23029#newcode1
chrome/browser/browser_accessibility_manager.h:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1637018/diff/63001/23029#newcode27
chrome/browser/browser_accessibility_manager.h:27: HWND parent_hwnd, const
webkit_glue::WebAccessibility& src);
nit: it seems that you could move |parent_hwnd| to line 26. And align |src| with
it.

http://codereview.chromium.org/1637018/diff/63001/23025
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23025#newcode17
chrome/browser/renderer_host/render_view_host.cc:17: #include
"base/utf_string_conversions.h"
nit: do we really need this?

http://codereview.chromium.org/1637018/diff/63001/23027
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23027#newcode1512
chrome/browser/renderer_host/render_widget_host_view_win.cc:1512: //
TODO(jcampan): http://b/issue?id=1432077 Disabling accessibility in the
nit: Move private bug to crbug.com?

http://codereview.chromium.org/1637018/diff/63001/23035
File chrome/common/common_param_traits_unittest.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23035#newcode1
chrome/common/common_param_traits_unittest.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1637018/diff/63001/23032
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/1637018/diff/63001/23032#newcode2538
chrome/common/render_messages.h:2538: ReadParam(m, iter, &paths);
nit: doesn't this fit into the same line?

http://codereview.chromium.org/1637018/diff/63001/23037
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23037#newcode3925
chrome/renderer/render_view.cc:3925: if (accessibility_.get()) {
nit: please, remove {} ?

http://codereview.chromium.org/1637018/diff/63001/23015
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23015#newcode1
webkit/glue/webaccessibility.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1637018/diff/63001/23015#newcode104
webkit/glue/webaccessibility.cc:104: int32 ConvertState(const
WebAccessibilityObject& o) {
nit: why not uint32?

http://codereview.chromium.org/1637018/diff/63001/23015#newcode160
webkit/glue/webaccessibility.cc:160: WebKit::WebAccessibilityCache* cache) {
nit: line > 80;

http://codereview.chromium.org/1637018/diff/63001/23015#newcode177
webkit/glue/webaccessibility.cc:177: for (int i = 0; i < child_count; i++) {
nit: ++i. I like using int in for loops, but we generally use iterators in
chromium.

http://codereview.chromium.org/1637018/diff/63001/23017
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/1637018/diff/63001/23017#newcode1
webkit/glue/webaccessibility.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1637018/diff/63001/23017#newcode67
webkit/glue/webaccessibility.h:67: //   int mask = (1 << STATE_CHECKED) | (1 <<
STATE_FOCUSED);
I think this usage could be simplified if we move this into the enum.

What about this:

STATE_CHECKED = 1 << 0,
...

unit32 mask = STATE_CHECKED | STATE_FOCUSED?

Note: maybe uint16 fits into our needs too.

[dmazzoni, 2010-04-29 21:39:16]

On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> very cool!
> 
> mostly nits below in the comments.  some questions:

Thanks! I'll address the nits when I upload the next version.

> -what's the plan for when the DOM changes?  will a full copy of the tree be
sent
> each time?  that would seem inefficient with a constantly changing page for
> example.

Good question, and I don't think I know the answer yet, but Chris and I are
working on it. So far, copying the whole tree each time is surprisingly fast for
most real-world webpages, even large ones, but clearly we should try to come up
with a way to send as little as possible. 

> -any data on how long it takes to generate the full tree and the size?  i.e.
> what if there's a very large and complicated page, what would be the size of
the
> serialized data?  if it's very large, I don't know if we want to send it
through
> IPC, perhaps we'd want to use shared memory.  reason being that I believe
pipes
> on OS X become slow if messages on the order of 100K are sent through. 
Another
> reason is that there's a lot of copying of the data when sending through IPC.

I'll try benchmarking this for a bunch of sites.

I've never tried to use shared memory for something like this, do we already
have some cross-platform infrastructure I could use?

> -I see that the tree is requested asynchronously.  what happens if an
> accessibility request from the OS comes in before we have tree?

No problem.

> -are you a committer?  I ask because I don't see any try bot results, and
you'll
> probably want to do that before checking this in

Yes, I am. Don't worry, I'll try before committing. I just didn't bother now
because I want to address the first round of feedback first and the trybots
wouldn't tell me anything interesting at this point.

> http://codereview.chromium.org/1637018/diff/63001/23017#newcode98
> webkit/glue/webaccessibility.h:98: int32 id;
> member variables of a class should all end in underscore

What if this was a struct? The only purpose of this class is as a container to
serialize. I thought the correct style for copyable structs of simple data types
was to have public member variables with no underscore and no getters/setters.
I'm happy either way, though...

[tfarina (gmail-do not use), 2010-04-29 21:42:59]

On 2010/04/29 21:39:16, Dominic Mazzoni wrote:
> What if this was a struct?
Per style guide:
"Data members in structs should be named like regular variables without the
trailing underscores that data members in classes have."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names

[jam, 2010-04-29 21:49:09]

On Thu, Apr 29, 2010 at 2:39 PM, <dmazzoni@chromium.org> wrote:

> On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
>
>> very cool!
>>
>
>  mostly nits below in the comments.  some questions:
>>
>
> Thanks! I'll address the nits when I upload the next version.
>
>
>  -what's the plan for when the DOM changes?  will a full copy of the tree
>> be
>>
> sent
>
>> each time?  that would seem inefficient with a constantly changing page
>> for
>> example.
>>
>
> Good question, and I don't think I know the answer yet, but Chris and I are
> working on it. So far, copying the whole tree each time is surprisingly
> fast for
> most real-world webpages, even large ones, but clearly we should try to
> come up
> with a way to send as little as possible.
>
>
>  -any data on how long it takes to generate the full tree and the size?
>>  i.e.
>> what if there's a very large and complicated page, what would be the size
>> of
>>
> the
>
>> serialized data?  if it's very large, I don't know if we want to send it
>>
> through
>
>> IPC, perhaps we'd want to use shared memory.  reason being that I believe
>>
> pipes
>
>> on OS X become slow if messages on the order of 100K are sent through.
>>
> Another
>
>> reason is that there's a lot of copying of the data when sending through
>> IPC.
>>
>
> I'll try benchmarking this for a bunch of sites.
>
> I've never tried to use shared memory for something like this, do we
> already
> have some cross-platform infrastructure I could use?


see base\shared_memory.h

>
>
>  -I see that the tree is requested asynchronously.  what happens if an
>> accessibility request from the OS comes in before we have tree?
>>
>
> No problem.


Can you elaborate a bit?  i.e. does the user miss anything?


>
>  -are you a committer?  I ask because I don't see any try bot results, and
>>
> you'll
>
>> probably want to do that before checking this in
>>
>
> Yes, I am. Don't worry, I'll try before committing. I just didn't bother
> now
> because I want to address the first round of feedback first and the trybots
> wouldn't tell me anything interesting at this point.
>
>
>  http://codereview.chromium.org/1637018/diff/63001/23017#newcode98
>> webkit/glue/webaccessibility.h:98: int32 id;
>> member variables of a class should all end in underscore
>>
>
> What if this was a struct?


structs don't need underscores


> The only purpose of this class is as a container to
> serialize. I thought the correct style for copyable structs of simple data
> types
> was to have public member variables with no underscore and no
> getters/setters.
>

oh really?  I didn't hear this before, if that's the case, that's fine.  (is
it in the style guide?).

> I'm happy either way, though...
>
>
>
> http://codereview.chromium.org/1637018/show
>

[dmazzoni, 2010-04-30 18:21:19]

On 2010/04/29 21:49:09, John Abd-El-Malek wrote:
> >  -I see that the tree is requested asynchronously.  what happens if an
> >> accessibility request from the OS comes in before we have tree?
> >
> > No problem.
> 
> Can you elaborate a bit?  i.e. does the user miss anything?

It will just appear to the user as if the tab contents are empty. Once the
accessible content is loaded, it will be accessible. It's possible that the user
might not realize when the content is loaded, but we could fix this by
generating a focus change event. I don't want to worry about this just yet, we
have a lot of bigger problems still to solve.

Almost finished with most of the other requested changes, will upload in a
minute...

- Dominic

[dmazzoni, 2010-04-30 18:28:20]

Most issues addressed below.

I added a unittest to make sure that we're not leaking objects.

http://codereview.chromium.org/1637018/diff/63001/23028
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23028#newcode18
chrome/browser/browser_accessibility.cc:18: manager_ = manager;
On 2010/04/29 20:33:55, tfarina wrote:
> nit: could we initialize everything in the initializer list?

No, a COM object has to have a no-argument constructor.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode45
chrome/browser/browser_accessibility.cc:45: children_.push_back(child);
On 2010/04/29 20:33:55, tfarina wrote:
> nit: shouldn't we check here for duplicates before adding a child?

Shouldn't be needed. This will never occur as written now, but if for some
reason you did want to include a child twice nothing will break.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode49
chrome/browser/browser_accessibility.cc:49: for (size_t i = 0; i <
children_.size(); i++) {
On 2010/04/29 20:33:55, tfarina wrote:
> nit: ++i, also please for single line statement no need of {}

Done.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode57
chrome/browser/browser_accessibility.cc:57: if (this == ancestor)
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: need brace brackets here per style guide

Done.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode61
chrome/browser/browser_accessibility.cc:61: else
On 2010/04/29 20:33:55, tfarina wrote:
> nit: no need of this last else. We will return anyway, when we arrive here.

Done.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode68
chrome/browser/browser_accessibility.cc:68: else
On 2010/04/29 20:33:55, tfarina wrote:
> nit: here too. And the same below. :)

Done.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode74
chrome/browser/browser_accessibility.cc:74: index_in_parent_ <
static_cast<int>(parent_->children_.size() - 1))
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: ditto

Done.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode185
chrome/browser/browser_accessibility.cc:185: (*disp_child) = target;
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: why the extra brackets?

I thought it enhanced readability when paired with the following line (i.e. it's
clearer at a glance that AddRef is being called on the same object that just got
assigned to *disp_child). Will remove if you feel strongly.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode445
chrome/browser/browser_accessibility.cc:445: STDMETHODIMP
BrowserAccessibility::role(LONG *role) {
On 2010/04/29 20:33:55, tfarina wrote:
> nit: LONG*
> 
> Also could you fix the other occurrences below?

Done.

http://codereview.chromium.org/1637018/diff/63001/23028#newcode523
chrome/browser/browser_accessibility.cc:523: if (guidService == IID_IAccessible
|| guidService == IID_IAccessible2) {
On 2010/04/29 20:33:55, tfarina wrote:
> nit: no need of {}

Done.

http://codereview.chromium.org/1637018/diff/63001/23030
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/63001/23030#newcode16
chrome/browser/browser_accessibility.h:16: #include
"chrome/browser/browser_accessibility_manager.h"
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: the chrome includes should be listed alphabetically

Done.

http://codereview.chromium.org/1637018/diff/63001/23030#newcode43
chrome/browser/browser_accessibility.h:43: : manager_(NULL),
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: constructors usually go in the cc file (I know it was already like that,
> but would be nice to move it)

Done.

http://codereview.chromium.org/1637018/diff/63001/23030#newcode156
chrome/browser/browser_accessibility.h:156: STDMETHODIMP role(LONG *role);
On 2010/04/29 20:33:55, tfarina wrote:
> nit: LONG*
> 
> Also, could you fix the other occurrences below?

Done.

http://codereview.chromium.org/1637018/diff/63001/23030#newcode175
chrome/browser/browser_accessibility.h:175: IAccessibleRelation **relation)
{return E_NOTIMPL; }
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: I haven't sen this style before.  we usually one line statements in the
> same line as the function name if they all fit on the same line.

I'm not sure I understand. It wouldn't actually fit in this case.

http://codereview.chromium.org/1637018/diff/63001/23020
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23020#newcode52
chrome/browser/browser_accessibility_manager.cc:52: return obj;
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: why not just return iter->second?

Done.

http://codereview.chromium.org/1637018/diff/63001/23020#newcode58
chrome/browser/browser_accessibility_manager.cc:58: IAccessible*
BrowserAccessibilityManager::GetParentWindowIAccessible() {
On 2010/04/29 20:33:55, tfarina wrote:
> nit: this could be moved to header file and inlined as:
> 
> parent_window_IAccessible() { return window_iaccessible_; }
> 
> Same below. But this is up to you, I'm fine either way.

I think I'll leave them here because another possible implementation would be
defer creating the parent IAccessible until needed; I don't feel like it's a
simple accessor.

http://codereview.chromium.org/1637018/diff/63001/23020#newcode70
chrome/browser/browser_accessibility_manager.cc:70: else
On 2010/04/29 20:33:55, tfarina wrote:
> nit: no need of else.

Done.

http://codereview.chromium.org/1637018/diff/63001/23020#newcode77
chrome/browser/browser_accessibility_manager.cc:77: if (iter !=
renderer_id_to_child_id_map_.end()) {
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: we try to avoid functions which are all indented, especially because of
the
> 80 char restriction.  so in this case it's nice to just do
> 
> if (iter == renderer_id_to_child_id_map_.end())
>   return;
> 
> then the rest of the code has a little more room to work with

Thanks, it's cleaner this way.

http://codereview.chromium.org/1637018/diff/63001/23020#newcode123
chrome/browser/browser_accessibility_manager.cc:123: for (int i = 0; i <
static_cast<int>(src.children.size()); i++) {
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: convention is to use ++i, even with ints where admittedly it doesn't make
a
> difference

Done.

http://codereview.chromium.org/1637018/diff/63001/23025
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23025#newcode17
chrome/browser/renderer_host/render_view_host.cc:17: #include
"base/utf_string_conversions.h"
On 2010/04/29 20:33:55, tfarina wrote:
> nit: do we really need this?

Done.

http://codereview.chromium.org/1637018/diff/63001/23027
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23027#newcode1492
chrome/browser/renderer_host/render_widget_host_view_win.cc:1492: // sure that's
why.
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: for this comment and below, we try to avoid comments that are explaining
> code in english, i.e. something that a developer can figure out right away by
> looking at the code.

Done.

http://codereview.chromium.org/1637018/diff/63001/23027#newcode1512
chrome/browser/renderer_host/render_widget_host_view_win.cc:1512: //
TODO(jcampan): http://b/issue?id=1432077 Disabling accessibility in the
On 2010/04/29 20:33:55, tfarina wrote:
> nit: Move private bug to crbug.com?

Done.

http://codereview.chromium.org/1637018/diff/63001/23027#newcode1520
chrome/browser/renderer_host/render_widget_host_view_win.cc:1520: LPARAM
object_id = (LPARAM)(DWORD)lparam;
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> huh, why the (LPARAM)(DWORD)?

That's what I get for copying examples from MSDN. :)

http://codereview.chromium.org/1637018/diff/63001/23023
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/1637018/diff/63001/23023#newcode39
chrome/browser/renderer_host/render_widget_host_view_win.h:39: //   
L"MozillaContentWindowClass";
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: no need to check this in right?

Done.

http://codereview.chromium.org/1637018/diff/63001/23021
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23021#newcode2275
chrome/browser/tab_contents/tab_contents.cc:2275: printf("DidNavigate %s\n",
GetURL().spec().c_str());
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: please no printfs, only DLOGs and LOGs

Thanks! (Sorry, left over from early debugging.)

http://codereview.chromium.org/1637018/diff/63001/23022
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/1637018/diff/63001/23022#newcode1267
chrome/browser/tab_contents/tab_contents.h:1267: // TODO(jcampan):
http://b/issue?id=1432077 This is a temporary work-around
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> we shouldn't be having bugs that point internally since external contributors
> won't see them.  can this be ported to our bug tracker?  and should the TODO
> still be assigned to jcampan?

Switched to the equivalent bug in crbug.com and changed TODO to me.

http://codereview.chromium.org/1637018/diff/63001/23036
File chrome/common/common_param_traits.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23036#newcode336
chrome/common/common_param_traits.cc:336: p->role =
static_cast<webkit_glue::WebAccessibility::Role>(role);
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: need brace brackets here (any if/else statement that takes more than 4
> lines)

Done.

http://codereview.chromium.org/1637018/diff/63001/23036#newcode350
chrome/common/common_param_traits.cc:350: l->append(L"<WebAccessibility>");
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: we only put <struct name> when we're too lazy to actually display it.  In
> this case, no need to add that.  also by convention we put brackets in the
> beginning and end (see render_messages.h for examples, unfortunately this file
> has examples that aren't consistent with the rest of the code)

Done.

http://codereview.chromium.org/1637018/diff/63001/23033
File chrome/common/common_param_traits.h (right):

http://codereview.chromium.org/1637018/diff/63001/23033#newcode439
chrome/common/common_param_traits.h:439: struct
ParamTraits<webkit_glue::WebAccessibility> {
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> why is the traits going in common_param_traits.h?  that file should be for
> traits that are sent to/from multiple type of child processes (i.e. renderer,
> plugin).  if it's just to renderer, it should be in render_messages.h

OK, I moved it and added a new render_messages_unittest.cc

http://codereview.chromium.org/1637018/diff/63001/23037
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23037#newcode1331
chrome/renderer/render_view.cc:1331: accessibility_.reset(NULL);
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: we usually just do .reset()

Done.

http://codereview.chromium.org/1637018/diff/63001/23015
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23015#newcode104
webkit/glue/webaccessibility.cc:104: int32 ConvertState(const
WebAccessibilityObject& o) {
On 2010/04/29 20:33:55, tfarina wrote:
> nit: why not uint32?

Done.

http://codereview.chromium.org/1637018/diff/63001/23015#newcode149
webkit/glue/webaccessibility.cc:149: : id(-1),
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> nit: this should be indented 4 spaces

Done.

http://codereview.chromium.org/1637018/diff/63001/23015#newcode155
webkit/glue/webaccessibility.cc:155: WebKit::WebAccessibilityCache* cache) {
On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> no tabs (and other places below)

Done.

http://codereview.chromium.org/1637018/diff/63001/23015#newcode160
webkit/glue/webaccessibility.cc:160: WebKit::WebAccessibilityCache* cache) {
On 2010/04/29 20:33:55, tfarina wrote:
> nit: line > 80;

Done.

http://codereview.chromium.org/1637018/diff/63001/23017
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/1637018/diff/63001/23017#newcode67
webkit/glue/webaccessibility.h:67: //   int mask = (1 << STATE_CHECKED) | (1 <<
STATE_FOCUSED);
On 2010/04/29 20:33:55, tfarina wrote:
> I think this usage could be simplified if we move this into the enum.
> 
> What about this:
> 
> STATE_CHECKED = 1 << 0,
> ...
> 
> unit32 mask = STATE_CHECKED | STATE_FOCUSED?
> 
> Note: maybe uint16 fits into our needs too.

I think the main reason not to do this is that we will probably be adding new
states more frequently than we will be modifying code that uses this bitmask.
WebKit has been slowly adding more and more states, and after this CL is done we
need to start catching up.

[jam, 2010-04-30 19:15:10]

On Fri, Apr 30, 2010 at 11:21 AM, <dmazzoni@chromium.org> wrote:

> On 2010/04/29 21:49:09, John Abd-El-Malek wrote:
>
>> >  -I see that the tree is requested asynchronously.  what happens if an
>> >> accessibility request from the OS comes in before we have tree?
>> >
>> > No problem.
>>
>
>  Can you elaborate a bit?  i.e. does the user miss anything?
>>
>
> It will just appear to the user as if the tab contents are empty. Once the
> accessible content is loaded, it will be accessible. It's possible that the
> user
> might not realize when the content is loaded, but we could fix this by
> generating a focus change event. I don't want to worry about this just yet,
> we
> have a lot of bigger problems still to solve.
>

I see, thanks for the explanation.  It seems that this will have the effect
of making the feature look flakey to users.  I agree it shouldn't block this
change, but we'll need to track it (bug tracker is the best place) so we're
aware of all the issues we need to fix before releasing this to stable.


>
> Almost finished with most of the other requested changes, will upload in a
> minute...
>
> - Dominic
>
>
>
> http://codereview.chromium.org/1637018/show
>

[dmazzoni, 2010-04-30 19:22:44]

On Fri, Apr 30, 2010 at 12:14 PM, John Abd-El-Malek <jam@chromium.org>wrote:

> I see, thanks for the explanation.  It seems that this will have the effect
> of making the feature look flakey to users.  I agree it shouldn't block this
> change, but we'll need to track it (bug tracker is the best place) so we're
> aware of all the issues we need to fix before releasing this to stable
>

I forgot, there's another possible solution: we can return a top level
object with role=DOCUMENT and state=BUSY, that seems to be what other
browsers do. Screenreaders will wait until the state changes back to NOT
BUSY before digging into it.

I'll track this.

- Dominic

[jam, 2010-04-30 21:45:49]

On Fri, Apr 30, 2010 at 12:22 PM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Fri, Apr 30, 2010 at 12:14 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> I see, thanks for the explanation.  It seems that this will have the
>> effect of making the feature look flakey to users.  I agree it shouldn't
>> block this change, but we'll need to track it (bug tracker is the best
>> place) so we're aware of all the issues we need to fix before releasing this
>> to stable
>>
>
> I forgot, there's another possible solution: we can return a top level
> object with role=DOCUMENT and state=BUSY, that seems to be what other
> browsers do. Screenreaders will wait until the state changes back to NOT
> BUSY before digging into it.
>
> I'll track this.
>

Thanks, sounds good.


>
> - Dominic
>
>

[jam, 2010-04-30 21:50:50]

lgtm with at least the second comment taken care of

http://codereview.chromium.org/1637018/diff/63001/23028
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/63001/23028#newcode185
chrome/browser/browser_accessibility.cc:185: (*disp_child) = target;
On 2010/04/30 18:28:21, Dominic Mazzoni wrote:
> On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> > nit: why the extra brackets?
> 
> I thought it enhanced readability when paired with the following line (i.e.
it's
> clearer at a glance that AddRef is being called on the same object that just
got
> assigned to *disp_child). Will remove if you feel strongly.
> 

I'm personally wary of code that tries to be aligned with lines above/below.  I
thought we don't try to do that usually (i.e. by aligning settings of member
variables, aligning member variable names etc).  But this is a personal style
since I don't see anything about it in the style guide, so I'll leave it to you.

http://codereview.chromium.org/1637018/diff/63001/23030
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/63001/23030#newcode175
chrome/browser/browser_accessibility.h:175: IAccessibleRelation **relation)
{return E_NOTIMPL; }
On 2010/04/30 18:28:21, Dominic Mazzoni wrote:
> On 2010/04/29 16:48:04, John Abd-El-Malek wrote:
> > nit: I haven't sen this style before.  we usually one line statements in the
> > same line as the function name if they all fit on the same line.
> 
> I'm not sure I understand. It wouldn't actually fit in this case.
> 

what I meant is we only put the function body in the same line as the function
name and parameters if they all fit on one line.  so if you have multiple lines
for the parameters, then the body should be on a separate line like this:

STDMETHODIMP get_relation(LONG relation_index,
                          IAccessibleRelation **relation) {
  return E_NOTIMPL;
}

[dmazzoni, 2010-05-03 23:45:26]

On 2010/04/30 21:50:50, John Abd-El-Malek wrote:
> lgtm with at least the second comment taken care of

Thanks. Just sent to try servers, will hopefully submit tomorrow morning if
there are no more comments to address.

I added a couple more checks to browser_accessibility_unittest.cc, but otherwise
just the suggested fixes.

> what I meant is we only put the function body in the same line as the function
> name and parameters if they all fit on one line.  so if you have multiple
lines
> for the parameters, then the body should be on a separate line like this:
> 
> STDMETHODIMP get_relation(LONG relation_index,
>                           IAccessibleRelation **relation) {
>   return E_NOTIMPL;
> }

Oh, OK. Done. I made them all like that for consistency.

[Jonas Klink (Google), 2010-05-04 17:51:16]

http://codereview.chromium.org/1637018/diff/138001/108013
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108013#newcode118
chrome/browser/browser_accessibility.cc:118: 
Just out of curiosity: why did we remove the implementation of the HitTest?
IIRC, WebKit has a pretty good implementation on it on the renderer side, and
it's not a function I would call deprecated quite yet.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode175
chrome/browser/browser_accessibility.cc:175: case NAVDIR_FIRSTCHILD:
Can we add back the check for navigating to first/last child only being done
from self? It's an obscure rule from MSAA, I know, but I'd like to stick to the
spec where we can.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode195
chrome/browser/browser_accessibility.cc:195: end->vt = VT_EMPTY;
return S_FALSE if an object is not found in the specified direction.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode212
chrome/browser/browser_accessibility.cc:212: return E_INVALIDARG;
Should we also set *disp_child = NULL?

http://codereview.chromium.org/1637018/diff/138001/108013#newcode246
chrome/browser/browser_accessibility.cc:246: 
Add check for empty string, return S_FALSE if true.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode266
chrome/browser/browser_accessibility.cc:266: 
Add check for empty string, return S_FALSE if true.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode308
chrome/browser/browser_accessibility.cc:308: 
Add check for empty string, return S_FALSE if true.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode328
chrome/browser/browser_accessibility.cc:328: 
Add check for empty string, return S_FALSE if true.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode347
chrome/browser/browser_accessibility.cc:347: 
Add check for empty string, return S_FALSE if true.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode439
chrome/browser/browser_accessibility.cc:439: 
Why do we need the checks below when we're returning E_NOTIMPL anyway?

http://codereview.chromium.org/1637018/diff/138001/108013#newcode458
chrome/browser/browser_accessibility.cc:458: 
Are we planning to add this in a separate CL? According to Serotek, it's
critical for supporting the virtual buffer... Or have we found otherwise? Just
curious...

http://codereview.chromium.org/1637018/diff/138001/108008
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/1637018/diff/138001/108008#newcode318
chrome/browser/renderer_host/render_widget_host_view_win.h:318: // Whether the
renderer is made accessible.
Also point this comment to http://crbug.com/25564

[Chris Guillory, 2010-05-04 19:58:36]

LGTM.

Some nits.

http://codereview.chromium.org/1637018/diff/138001/108013
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108013#newcode31
chrome/browser/browser_accessibility.cc:31: manager_ = manager;
Nit: Use constructor initializer list?

http://codereview.chromium.org/1637018/diff/138001/108015
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/138001/108015#newcode278
chrome/browser/browser_accessibility.h:278: // so that calls to any of this
objects' methods immediately return
Nit: object's not objects'

http://codereview.chromium.org/1637018/diff/138001/108005
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108005#newcode114
chrome/browser/browser_accessibility_manager.cc:114: BrowserAccessibility*
instance = factory_->Create();
For simplicity can we have BrowserAccessibilityFactory::Create AddRef and remove
the NewReference call at the end of this function.

http://codereview.chromium.org/1637018/diff/138001/108012
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108012#newcode1505
chrome/browser/renderer_host/render_widget_host_view_win.cc:1505: // we have is
now stale.
Nit: Reword last sentence. "what we have is now stale"?

http://codereview.chromium.org/1637018/diff/138001/108022
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/1637018/diff/138001/108022#newcode2616
chrome/common/render_messages.h:2616: if (role >=
webkit_glue::WebAccessibility::ROLE_NONE &&
Nit: Can we remove the bounds check for role?

http://codereview.chromium.org/1637018/diff/138001/108017
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108017#newcode3948
chrome/renderer/render_view.cc:3948: WebAccessibilityObject src_tree =
webview()->accessibilityObject();
Nit Nit: src/dest naming seems strange. I don't have a good suggestion though.

http://codereview.chromium.org/1637018/diff/138001/108004
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/1637018/diff/138001/108004#newcode82
webkit/glue/webaccessibility.h:82: NUM_STATES
NUM_STATES isn't needed anymore.

[dmazzoni, 2010-05-04 21:04:20]

Thanks for the comments! I made the changes and accessibility_win_browsertest
now passes.

http://codereview.chromium.org/1637018/diff/138001/108013
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108013#newcode31
chrome/browser/browser_accessibility.cc:31: manager_ = manager;
On 2010/05/04 19:58:37, Chris Guillory wrote:
> Nit: Use constructor initializer list?

Can't, because a COM object has to use an empty constructor.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode118
chrome/browser/browser_accessibility.cc:118: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Just out of curiosity: why did we remove the implementation of the HitTest?
> IIRC, WebKit has a pretty good implementation on it on the renderer side, and
> it's not a function I would call deprecated quite yet.

Unfortunately there's no way we could query WebKit for this without having all
of the same problems we had before, with either blocking the UI or crashing in a
nested message pump.

I think we'll have to decide whether it makes sense to implement an
approximation of this on the browser side, or make this method an exception to
our rule and query the renderer.

For now, I think it's better to leave it out rather than check in something
potentially wrong or unstable.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode175
chrome/browser/browser_accessibility.cc:175: case NAVDIR_FIRSTCHILD:
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Can we add back the check for navigating to first/last child only being done
> from self? It's an obscure rule from MSAA, I know, but I'd like to stick to
the
> spec where we can.

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode195
chrome/browser/browser_accessibility.cc:195: end->vt = VT_EMPTY;
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> return S_FALSE if an object is not found in the specified direction.

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode212
chrome/browser/browser_accessibility.cc:212: return E_INVALIDARG;
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Should we also set *disp_child = NULL?

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode246
chrome/browser/browser_accessibility.cc:246: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Add check for empty string, return S_FALSE if true.

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode266
chrome/browser/browser_accessibility.cc:266: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Add check for empty string, return S_FALSE if true.

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode308
chrome/browser/browser_accessibility.cc:308: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Add check for empty string, return S_FALSE if true.

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode328
chrome/browser/browser_accessibility.cc:328: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Add check for empty string, return S_FALSE if true.

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode347
chrome/browser/browser_accessibility.cc:347: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Add check for empty string, return S_FALSE if true.

Done.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode439
chrome/browser/browser_accessibility.cc:439: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Why do we need the checks below when we're returning E_NOTIMPL anyway?

Agreed, in this case we probably won't ever implement it.

http://codereview.chromium.org/1637018/diff/138001/108013#newcode458
chrome/browser/browser_accessibility.cc:458: 
On 2010/05/04 17:51:16, Jonas Klink (Google) wrote:
> Are we planning to add this in a separate CL? According to Serotek, it's
> critical for supporting the virtual buffer... Or have we found otherwise? Just
> curious...

You can browse NVDA's virtual buffer without this function. I think this is
already enough for this CL, but let's add it soon.

http://codereview.chromium.org/1637018/diff/138001/108015
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/1637018/diff/138001/108015#newcode278
chrome/browser/browser_accessibility.h:278: // so that calls to any of this
objects' methods immediately return
On 2010/05/04 19:58:37, Chris Guillory wrote:
> Nit: object's not objects'

Done.

http://codereview.chromium.org/1637018/diff/138001/108005
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108005#newcode114
chrome/browser/browser_accessibility_manager.cc:114: BrowserAccessibility*
instance = factory_->Create();
On 2010/05/04 19:58:37, Chris Guillory wrote:
> For simplicity can we have BrowserAccessibilityFactory::Create AddRef and
remove
> the NewReference call at the end of this function. 

Done.

http://codereview.chromium.org/1637018/diff/138001/108012
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/1637018/diff/138001/108012#newcode1505
chrome/browser/renderer_host/render_widget_host_view_win.cc:1505: // we have is
now stale.
On 2010/05/04 19:58:37, Chris Guillory wrote:
> Nit: Reword last sentence. "what we have is now stale"?

Done.

http://codereview.chromium.org/1637018/diff/138001/108022
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/1637018/diff/138001/108022#newcode2616
chrome/common/render_messages.h:2616: if (role >=
webkit_glue::WebAccessibility::ROLE_NONE &&
On 2010/05/04 19:58:37, Chris Guillory wrote:
> Nit: Can we remove the bounds check for role?

I think the check does add some safety, it guarantees the role will always be
something from the enum when unserializing.

http://codereview.chromium.org/1637018/diff/138001/108004
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/1637018/diff/138001/108004#newcode82
webkit/glue/webaccessibility.h:82: NUM_STATES
On 2010/05/04 19:58:37, Chris Guillory wrote:
> NUM_STATES isn't needed anymore.

Done.