Adds propagation and handling of render-side focus events, for the benefit of...

Adds propagation and handling of render-side focus events, for the benefit of assistive technologies (accessibility). Also cleans up the handling of WM_GETOBJECT in RenderWidgetHostViewWin and WidgetWin, as well as in BrowserAccessibilityManager.


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

[jcampan, 2009-05-15 17:33:24]

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

http://codereview.chromium.org/115374/diff/1/16#newcode61
Line 61: MapEntry(rvh->process()->pid(), instance));
Nit: I think you can use std::make_pair(rvh->process()->pid(), instance) instead
of typedefing MapEntry, but that's up to you.

http://codereview.chromium.org/115374/diff/1/16#newcode131
Line 131: // multiple BrowserAccessibility connected to the given |process_id|,
but they
Are you sure? I think there could be 2 RenderView in the same process each one
in its own tab.
You probably need to id with the pair process_id, routing_id.
Actually that would probably be a good test-case for a browser test.

http://codereview.chromium.org/115374/diff/1/16#newcode147
Line 147: RenderProcessHostMap::iterator it =
render_process_host_map_.find(rph->pid());
I think you need to call lower_bound instead of find, and then you can get rid
of the next test.

http://codereview.chromium.org/115374/diff/1/23
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/115374/diff/1/23#newcode2733
Line 2733: if (!web_accessibility_manager_.get()->
Why the get()?

http://codereview.chromium.org/115374/diff/1/23#newcode2749
Line 2749: if (!web_accessibility_manager_.get()->ClearAccObjMap(acc_obj_id,
Why the get()?

http://codereview.chromium.org/115374/diff/1/23#newcode3020
Line 3020: int acc_obj_id =
web_accessibility_manager_.get()->FocusAccObj(acc_obj);
Why the get()?

http://codereview.chromium.org/115374/diff/1/9
File views/widget/widget_win.cc (left):

http://codereview.chromium.org/115374/diff/1/9#oldcode561
Line 561: ::NotifyWinEvent(EVENT_OBJECT_CREATE, GetNativeView(), OBJID_CLIENT,
Why don't we need to notify anymore?

http://codereview.chromium.org/115374/diff/1/8
File webkit/glue/webaccessibilitymanager.h (right):

http://codereview.chromium.org/115374/diff/1/8#newcode31
Line 31: // Create a new instance of WebAccessibilityManager.
Nit: Creates

http://codereview.chromium.org/115374/diff/1/6
File webkit/glue/webaccessibilitymanager_impl.cc (right):

http://codereview.chromium.org/115374/diff/1/6#newcode176
Line 176: acc_obj_to_int_map_.find(out_acc_obj.get()->accessibilityObject());
You don't need the get()

http://codereview.chromium.org/115374/diff/1/6#newcode185
Line 185: acc_obj_to_int_map_[out_acc_obj.get()->accessibilityObject()] =
You don't need the get()

http://codereview.chromium.org/115374/diff/1/6#newcode218
Line 218: acc_obj_to_int_map_[root_->acc_obj_root_.get()->accessibilityObject()]
=
You don't need the get()

http://codereview.chromium.org/115374/diff/1/6#newcode276
Line 276: return acc_obj_id_++;
I am confused: this is not returning the id of the object created but id + 1.

[Jonas Klink (Google), 2009-05-15 22:43:29]

On 2009/05/15 17:33:24, jcampan wrote:
> http://codereview.chromium.org/115374/diff/1/16
> File chrome/browser/browser_accessibility_manager.cc (right):
> 
> http://codereview.chromium.org/115374/diff/1/16#newcode61
> Line 61: MapEntry(rvh->process()->pid(), instance));
> Nit: I think you can use std::make_pair(rvh->process()->pid(), instance)
instead
> of typedefing MapEntry, but that's up to you.

I think using the MapEntry gives a little bit better readability. If you don't
feel strongly about it, I'll keep it. 

> 
> http://codereview.chromium.org/115374/diff/1/16#newcode131
> Line 131: // multiple BrowserAccessibility connected to the given
|process_id|,
> but they
> Are you sure? I think there could be 2 RenderView in the same process each one
> in its own tab.
> You probably need to id with the pair process_id, routing_id.
> Actually that would probably be a good test-case for a browser test.

Ok, added the extra verification, and also changed the parent_hwnd function to a
private GetBrowserAccessibility, which does the extra verification, and enables
me to get rid of the code duplication that existed in parent_hwnd and
ChangeAccessibilityFocus. Now both those lookups are done with checking both
process and routing ids.

Good suggestion for the browser test, I am doing some major improvements on the
accessibility tests starting next week, and I just wanted to keep the complexity
of this CL lower, since it's also still all behind the
--enable-renderer-accessibility switch.

> 
> http://codereview.chromium.org/115374/diff/1/16#newcode147
> Line 147: RenderProcessHostMap::iterator it =
> render_process_host_map_.find(rph->pid());
> I think you need to call lower_bound instead of find, and then you can get rid
> of the next test.

Changed.

> 
> http://codereview.chromium.org/115374/diff/1/23
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/115374/diff/1/23#newcode2733
> Line 2733: if (!web_accessibility_manager_.get()->
> Why the get()?

Removed.

> 
> http://codereview.chromium.org/115374/diff/1/23#newcode2749
> Line 2749: if (!web_accessibility_manager_.get()->ClearAccObjMap(acc_obj_id,
> Why the get()?

Removed.

> 
> http://codereview.chromium.org/115374/diff/1/23#newcode3020
> Line 3020: int acc_obj_id =
> web_accessibility_manager_.get()->FocusAccObj(acc_obj);
> Why the get()?

Removed.

> 
> http://codereview.chromium.org/115374/diff/1/9
> File views/widget/widget_win.cc (left):
> 
> http://codereview.chromium.org/115374/diff/1/9#oldcode561
> Line 561: ::NotifyWinEvent(EVENT_OBJECT_CREATE, GetNativeView(), OBJID_CLIENT,
> Why don't we need to notify anymore?

This was actually not used even in the original code. I realized that the
creation of a new instance happened on the explicit request of the assistive
technology, so the notification was unnecessary. I did no harm, but also did no
good. Removed it to keep codesize down.

> 
> http://codereview.chromium.org/115374/diff/1/8
> File webkit/glue/webaccessibilitymanager.h (right):
> 
> http://codereview.chromium.org/115374/diff/1/8#newcode31
> Line 31: // Create a new instance of WebAccessibilityManager.
> Nit: Creates

Changed.

> 
> http://codereview.chromium.org/115374/diff/1/6
> File webkit/glue/webaccessibilitymanager_impl.cc (right):
> 
> http://codereview.chromium.org/115374/diff/1/6#newcode176
> Line 176: acc_obj_to_int_map_.find(out_acc_obj.get()->accessibilityObject());
> You don't need the get()

Removed.

> 
> http://codereview.chromium.org/115374/diff/1/6#newcode185
> Line 185: acc_obj_to_int_map_[out_acc_obj.get()->accessibilityObject()] =
> You don't need the get()

Removed.

> 
> http://codereview.chromium.org/115374/diff/1/6#newcode218
> Line 218:
acc_obj_to_int_map_[root_->acc_obj_root_.get()->accessibilityObject()]
> =
> You don't need the get()

Removed.

> 
> http://codereview.chromium.org/115374/diff/1/6#newcode276
> Line 276: return acc_obj_id_++;
> I am confused: this is not returning the id of the object created but id + 1.


Actually, the return happens before the post-increment. The returned number will
be that of the created accessibility object.

[Jonas Klink (Google), 2009-05-15 22:48:55]

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

http://codereview.chromium.org/115374/diff/1/16#newcode61
Line 61: MapEntry(rvh->process()->pid(), instance));
On 2009/05/15 17:33:24, jcampan wrote:
> Nit: I think you can use std::make_pair(rvh->process()->pid(), instance)
instead
> of typedefing MapEntry, but that's up to you.

I think using the MapEntry gives a little bit better readability. If you don't
feel strongly about it, I'll keep it.

http://codereview.chromium.org/115374/diff/1/16#newcode131
Line 131: // multiple BrowserAccessibility connected to the given |process_id|,
but they
On 2009/05/15 17:33:24, jcampan wrote:
> Are you sure? I think there could be 2 RenderView in the same process each one
> in its own tab.
> You probably need to id with the pair process_id, routing_id.
> Actually that would probably be a good test-case for a browser test.

Ok, added the extra verification, and also changed the parent_hwnd function to a
private GetBrowserAccessibility, which does the extra verification, and enables
me to get rid of the code duplication that existed in parent_hwnd and
ChangeAccessibilityFocus. Now both those lookups are done with checking both
process and routing ids.

Good suggestion for the browser test, I am doing some major improvements on the
accessibility tests starting next week, and I just wanted to keep the complexity
of this CL lower, since it's also still all behind the
--enable-renderer-accessibility switch.

http://codereview.chromium.org/115374/diff/1/16#newcode147
Line 147: RenderProcessHostMap::iterator it =
render_process_host_map_.find(rph->pid());
On 2009/05/15 17:33:24, jcampan wrote:
> I think you need to call lower_bound instead of find, and then you can get rid
> of the next test.

Changed.

http://codereview.chromium.org/115374/diff/1/23
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/115374/diff/1/23#newcode2733
Line 2733: if (!web_accessibility_manager_.get()->
On 2009/05/15 17:33:24, jcampan wrote:
> Why the get()?

Removed.

http://codereview.chromium.org/115374/diff/1/23#newcode2749
Line 2749: if (!web_accessibility_manager_.get()->ClearAccObjMap(acc_obj_id,
On 2009/05/15 17:33:24, jcampan wrote:
> Why the get()?

Removed.

http://codereview.chromium.org/115374/diff/1/23#newcode3020
Line 3020: int acc_obj_id =
web_accessibility_manager_.get()->FocusAccObj(acc_obj);
On 2009/05/15 17:33:24, jcampan wrote:
> Why the get()?

Removed.

http://codereview.chromium.org/115374/diff/1/9
File views/widget/widget_win.cc (left):

http://codereview.chromium.org/115374/diff/1/9#oldcode561
Line 561: ::NotifyWinEvent(EVENT_OBJECT_CREATE, GetNativeView(), OBJID_CLIENT,
On 2009/05/15 17:33:24, jcampan wrote:
> Why don't we need to notify anymore?

This was actually not used even in the original code. I realized that the
creation of a new instance happened on the explicit request of the assistive
technology, so the notification was unnecessary. I did no harm, but also did no
good. Removed it to keep codesize down.

http://codereview.chromium.org/115374/diff/1/8
File webkit/glue/webaccessibilitymanager.h (right):

http://codereview.chromium.org/115374/diff/1/8#newcode31
Line 31: // Create a new instance of WebAccessibilityManager.
On 2009/05/15 17:33:24, jcampan wrote:
> Nit: Creates

Changed.

http://codereview.chromium.org/115374/diff/1/6
File webkit/glue/webaccessibilitymanager_impl.cc (right):

http://codereview.chromium.org/115374/diff/1/6#newcode176
Line 176: acc_obj_to_int_map_.find(out_acc_obj.get()->accessibilityObject());
On 2009/05/15 17:33:24, jcampan wrote:
> You don't need the get()

Removed.

http://codereview.chromium.org/115374/diff/1/6#newcode185
Line 185: acc_obj_to_int_map_[out_acc_obj.get()->accessibilityObject()] =
On 2009/05/15 17:33:24, jcampan wrote:
> You don't need the get()

Removed.

http://codereview.chromium.org/115374/diff/1/6#newcode218
Line 218: acc_obj_to_int_map_[root_->acc_obj_root_.get()->accessibilityObject()]
=
On 2009/05/15 17:33:24, jcampan wrote:
> You don't need the get()

Removed.

http://codereview.chromium.org/115374/diff/1/6#newcode276
Line 276: return acc_obj_id_++;
On 2009/05/15 17:33:24, jcampan wrote:
> I am confused: this is not returning the id of the object created but id + 1.

Actually, the return happens before the post-increment. The returned number will
be that of the created accessibility object.

[darin (slow to review), 2009-05-18 22:43:21]

I reviewed only the webkit/* files for now.  The changes there seem fine to me.

http://codereview.chromium.org/115374/diff/1035/40
File webkit/glue/webaccessibilitymanager_impl.cc (right):

http://codereview.chromium.org/115374/diff/1035/40#newcode268
Line 268: } else {
nit: no need for else after return.  would be nice to reduce indentation of
remainder of function.

[jcampan, 2009-05-19 21:50:03]

LGTM

http://codereview.chromium.org/115374/diff/1/6
File webkit/glue/webaccessibilitymanager_impl.cc (right):

http://codereview.chromium.org/115374/diff/1/6#newcode276
Line 276: return acc_obj_id_++;
On 2009/05/15 22:48:55, Jonas Klink (Google) wrote:
> On 2009/05/15 17:33:24, jcampan wrote:
> > I am confused: this is not returning the id of the object created but id +
1.
> Actually, the return happens before the post-increment. The returned number
will
> be that of the created accessibility object.
Doh! You are right, I am stupid!

http://codereview.chromium.org/115374/diff/1035/40#newcode60
Line 60: // The object to be retrived is referred to by the
|in_params.child_id|, as
typo: retrived -> retrieved