Adds MSAA/IAccessible exposure of web content.

Adds MSAA/IAccessible exposure of web content.

[Jonas Klink (Google), 2008-09-23 23:42:06]

Adds the glue and browser-layer functionality for exposing MSAA/IAccessible
information from WebKit, through IPC and to the browser side (and assistive
technologies, such as screen readers). 

This change does NOT contain WebKit MSAA event generation, which will be part of
a separate change. 

With this change, users should be able to use MSAA SDK tools (such as Inspect
Object) to browse the MSAA tree of a page.

[macdome_gmail.com, 2008-09-24 01:18:24]

I looked through it briefly.  Looks sane, but I'm anything but an AX expert. 
(eseidel)  Maybe Jay has deeper thoughts to offer you.

[jcampan, 2008-09-24 20:14:21]

http://codereview.chromium.org/4057/diff/1/13
File chrome/browser/browser_accessibility.h (right):

http://codereview.chromium.org/4057/diff/1/13#newcode5
Line 5: #ifndef CHROME_BROWSER_BROWSER_ACCESSIBILITY_H__
New style guide says only one underscore at the end.

http://codereview.chromium.org/4057/diff/1/13#newcode103
Line 103: int iaccessible_id() { return iaccessible_id_; }
Make it const.

http://codereview.chromium.org/4057/diff/1/13#newcode109
Line 109: int instance_id() { return instance_id_; }
Make it const.

http://codereview.chromium.org/4057/diff/1/13#newcode124
Line 124: // iaccessible_id and parent_id.
Params in comments should between |.

http://codereview.chromium.org/4057/diff/1/13#newcode131
Line 131: bool RequestAccessibilityInfo(int iaccessible_func_id, VARIANT var_id,
Style: in .h files, if all parameters don't fit on 1 line, you must put them 1
by line.

http://codereview.chromium.org/4057/diff/1/6
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/4057/diff/1/6#newcode11
Line 11: // The time in ms after which we give up and returns an error when
processing an
returns -> return

http://codereview.chromium.org/4057/diff/1/6#newcode86
Line 86: // Necassary for the send to keep the UI responsive.
Typo Necassary -> Necessary

http://codereview.chromium.org/4057/diff/1/6#newcode91
Line 91: if (!success)
Just do:
return success;

http://codereview.chromium.org/4057/diff/1/6#newcode120
Line 120: instance_map_.insert(
Just do instance_map_[instance_id_] = new UniqueMembers(...

http://codereview.chromium.org/4057/diff/1/12
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/4057/diff/1/12#newcode35
Line 35: class BrowserAccessibilityManager {
If this class is a singleton, you should use the singleton.h  feature.

http://codereview.chromium.org/4057/diff/1/9
File chrome/browser/iaccessible_function_ids.h (right):

http://codereview.chromium.org/4057/diff/1/9#newcode9
Line 9: #ifndef CHROME_BROWSER_IACCESSIBLE_FUNCTION_IDS_H__
New style guide says only 1 underscore at the end.

http://codereview.chromium.org/4057/diff/1/5
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/4057/diff/1/5#newcode820
Line 820: Send(new ViewMsg_ClearAccessibilityInfo(routing_id_, -1, true));
Do we really need such a message?
The render view knows when a navigation occurs.

http://codereview.chromium.org/4057/diff/1/8
File chrome/browser/render_widget_host_hwnd.cc (right):

http://codereview.chromium.org/4057/diff/1/8#newcode783
Line 783: // Paranoia check. Return with failure.
If this should not happen, add a NOTREACHED().

http://codereview.chromium.org/4057/diff/1/14
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/4057/diff/1/14#newcode378
Line 378: // Identifier to uniqely distinguish which instance of IAccessible is
being
Typo: uniqely -> uniquely

http://codereview.chromium.org/4057/diff/1/14#newcode398
Line 398: // Identifier to uniqely distinguish which instance of IAccessible is
being
Same typo as above.

http://codereview.chromium.org/4057/diff/1/2
File webkit/glue/glue_accessibility.cc (right):

http://codereview.chromium.org/4057/diff/1/2#newcode24
Line 24: class GlueAccessibility::GlueAccessibilityRoot {
You could probably make GlueAccessibilityRoot a struct since   its purpose is
just to hold a value.

http://codereview.chromium.org/4057/diff/1/2#newcode43
Line 43: if (root_)
You don't need the if, delete does nothing if provided with NULL. Actually,
couldn't you use a scoped_ptr?

http://codereview.chromium.org/4057/diff/1/2#newcode63
Line 63: BSTR* output_bstr = static_cast<BSTR*>(::malloc(sizeof(BSTR*)));
You could use scoped_ptr_malloc for output_bstr (see scoped_ptr.h). Right now
you'll leak it when returning false.

http://codereview.chromium.org/4057/diff/1/2#newcode80
Line 80: // Requested IAccessible not found.
Can that actually happen? We should not store a NULL iaccessible in the map,
right?

http://codereview.chromium.org/4057/diff/1/2#newcode120
Line 120: break;
Shouln't we have a 
string_output = true;
in that case?

http://codereview.chromium.org/4057/diff/1/2#newcode191
Line 191: int_to_iaccessible_map_.insert(
I think you can write this more simply:
int_to_iaccessible_map_[iaccessible_id] = reinterpret_cast<IAccessible
*>(output_variant.pdispVal)

http://codereview.chromium.org/4057/diff/1/2#newcode208
Line 208: ::free(output_bstr);
We have not done the clean-up every time we returned early.

http://codereview.chromium.org/4057/diff/1/2#newcode240
Line 240: root_->accessibility_root_.get()));
I think you can write this more simply:
int_to_iaccessible_map_[iaccessible_id] = root_->accessibility_root_.get();

http://codereview.chromium.org/4057/diff/1/2#newcode243
Line 243: iaccessible_id_++));
Same remark.

http://codereview.chromium.org/4057/diff/1/2#newcode254
Line 254: root_->accessibility_root_ = 0;
You can return true here.

http://codereview.chromium.org/4057/diff/1/2#newcode268
Line 268: reinterpret_cast<IAccessible *>(it->second));
Why do you need a cast here? Isn't it->second an IAccessible*?

http://codereview.chromium.org/4057/diff/1/2#newcode271
Line 271: iaccessible_to_int_map_.erase(it2);
We should probably make the if a DCHECK. Is there any case in which the 2 maps
might get out of sync?

http://codereview.chromium.org/4057/diff/1/3
File webkit/glue/glue_accessibility.h (right):

http://codereview.chromium.org/4057/diff/1/3#newcode5
Line 5: #ifndef WEBKIT_GLUE_GLUE_ACCESSIBILITY_H__
Nit: the new style guide asks for only one underscore _ at the end.

http://codereview.chromium.org/4057/diff/1/3#newcode43
Line 43: // Erases the entry identified by the iaccessible_id from the hash map.
If
Nit: in comments, we usually put parameters between |.
Ex: ... identified by the |iaccessible_id| from ...
Same for clear_all.

http://codereview.chromium.org/4057/diff/1/3#newcode58
Line 58: // pointer to its id (int). Needed for reverse lookup, to ensure
unnessecary
Nit: typo unnecessary

[jcampan, 2008-09-26 17:53:03]

http://codereview.chromium.org/4057/diff/401/605
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/4057/diff/401/605#newcode51
Line 51: int iaccessible_id, int instance_id, int iaccessible_func_id, VARIANT
var_id,
This looks like more than 80 lines.

http://codereview.chromium.org/4057/diff/401/605#newcode81
Line 81: bool success = members->render_widget_host_->process()->channel()->
Have you tested retrieving an IAccessible than killing the renderer process
(with the task manager) than calling again a method of the IAccessible? I
believe this will cause a crasher as if the process terminates, the
RenderProcessHost is deleted, so you are keeping a pointer to a deleted object.
It will crash next time we try to send a message.
You should probably listen for the NOTIFY_RENDERER_PROCESS_TERMINATED
notification in BrowserAccessibility and if it is fired set a flag in it that
BrowserAccessibility to mark it as disconnected (or any better term), in which
case, any IAccessible call would just fail.

http://codereview.chromium.org/4057/diff/401/611
File chrome/browser/browser_accessibility_manager.h (right):

http://codereview.chromium.org/4057/diff/401/611#newcode69
Line 69: : instance_id_(0) {
You could probably put all this on 1 line.

http://codereview.chromium.org/4057/diff/401/601
File webkit/glue/glue_accessibility.cc (right):

http://codereview.chromium.org/4057/diff/401/601#newcode33
Line 33: : root_(new GlueAccessibilityRoot) {
4 spaces indent before the :

http://codereview.chromium.org/4057/diff/401/601#newcode168
Line 168: 
I believe you still need to do the SysFreeString. The iaccessible has allocated
the string.

http://codereview.chromium.org/4057/diff/401/601#newcode195
Line 195: reinterpret_cast<IAccessible *>(output_variant.pdispVal);
Small nit: the code would probably be easier to read if you were to create a
local IAccessible and then using it in the maps.

[pink (ping after 24hrs), 2008-09-30 13:10:10]

http://codereview.chromium.org/4057/diff/806/232
File webkit/glue/glue_accessibility.h (right):

http://codereview.chromium.org/4057/diff/806/232#newcode5
Line 5: #ifndef WEBKIT_GLUE_GLUE_ACCESSIBILITY_H_
drive-by comment....

how about naming this glue_accessibility_win, since it's very windows specific?
Or is the intent to refactor/ifdef this header when we figure our our mac/linux
story?

[jcampan, 2008-09-30 22:39:57]

OK, LGTM.

Note that I think it would have been easier to deal with the RPH notifications
if each BrowserAccessibility had been listening for the notification from its
specific RPH. That way BrowserAccessibilityManager would not need a map
RPH<->BrowserAccessibility.