Update browser cache of accessibility tree on renderer sub-tree changes....

Update browser cache of accessibility tree on renderer sub-tree changes.

WebKit sister patch: http://trac.webkit.org/changeset/66305

BUG=13291
TEST=interactive_ui_tests --gtest_filter=AccessibilityWinBrowserTest.TestDynamicAccessibilityTree
TEST=unit_tests --gtest_filter=BrowserAccessibilityTest.TestChildrenChange
TEST=unit_tests --gtest_filter=BrowserAccessibilityTest.TestChildrenChangeNoLeaks


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

[Chris Guillory, 2010-08-24 01:08:44]

The webkit side of this patch is attached to this webkit bug.
https://bugs.webkit.org/show_bug.cgi?id=44472

[dmazzoni, 2010-08-30 21:41:10]

LGTM, just a few small issues.

http://codereview.chromium.org/3117036/diff/15001/16001
File chrome/browser/browser_accessibility_manager_win.cc (right):

http://codereview.chromium.org/3117036/diff/15001/16001#newcode137
chrome/browser/browser_accessibility_manager_win.cc:137: for
(std::vector<webkit_glue::WebAccessibility>::const_iterator acc_obj =
acc_obj is actually an iterator, not an obj.

I would maybe iterate with an index, and then let acc_obj be a WebAccessibility
reference.

http://codereview.chromium.org/3117036/diff/15001/16001#newcode147
chrome/browser/browser_accessibility_manager_win.cc:147: BrowserAccessibility*
old_acc = GetFromChildID(child_id);
"acc" is used to mean a WebAccessibility or a BrowserAccessibility in this
function; please make it easier to tell them apart.

http://codereview.chromium.org/3117036/diff/15001/16013
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3117036/diff/15001/16013#newcode4306
chrome/renderer/render_view.cc:4306: if (!accessibility_changes_.empty())
Use braces

http://codereview.chromium.org/3117036/diff/15001/16013#newcode5293
chrome/renderer/render_view.cc:5293: NOTIMPLEMENTED();
Isn't this function cross-platform? Would it hurt to have it run on the Mac, so
we have less code to change to implement the Mac version of this update code?

[Chris Guillory, 2010-08-30 21:53:56]

Thanks. I'm adding tests to unit_tests and interactive_ui_tests.

http://codereview.chromium.org/3117036/diff/15001/16001
File chrome/browser/browser_accessibility_manager_win.cc (right):

http://codereview.chromium.org/3117036/diff/15001/16001#newcode137
chrome/browser/browser_accessibility_manager_win.cc:137: for
(std::vector<webkit_glue::WebAccessibility>::const_iterator acc_obj =
On 2010/08/30 21:41:11, Dominic Mazzoni wrote:
> acc_obj is actually an iterator, not an obj.
> 
> I would maybe iterate with an index, and then let acc_obj be a
WebAccessibility
> reference.
> 

Done.

http://codereview.chromium.org/3117036/diff/15001/16001#newcode147
chrome/browser/browser_accessibility_manager_win.cc:147: BrowserAccessibility*
old_acc = GetFromChildID(child_id);
On 2010/08/30 21:41:11, Dominic Mazzoni wrote:
> "acc" is used to mean a WebAccessibility or a BrowserAccessibility in this
> function; please make it easier to tell them apart.
BrowserAccessiblility -> browser_acc

http://codereview.chromium.org/3117036/diff/15001/16013
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3117036/diff/15001/16013#newcode4306
chrome/renderer/render_view.cc:4306: if (!accessibility_changes_.empty())
On 2010/08/30 21:41:11, Dominic Mazzoni wrote:
> Use braces

Done.

http://codereview.chromium.org/3117036/diff/15001/16013#newcode5293
chrome/renderer/render_view.cc:5293: NOTIMPLEMENTED();
On 2010/08/30 21:41:11, Dominic Mazzoni wrote:
> Isn't this function cross-platform? Would it hurt to have it run on the Mac,
so
> we have less code to change to implement the Mac version of this update code?

I've removed the windows check. The accessibility_changes_ vector would grow
without ever being cleared on macs with accessibility enabled as long as this
remains unimplemented on mac.

[Chris Guillory, 2010-08-31 03:02:10]

This is ready for another look - not sure if an email was sent.

[dmazzoni, 2010-08-31 03:07:58]

LGTM.

The tests look great, just one comment.

http://codereview.chromium.org/3117036/diff/29001/30006
File chrome/browser/browser_accessibility_win_unittest.cc (right):

http://codereview.chromium.org/3117036/diff/29001/30006#newcode125
chrome/browser/browser_accessibility_win_unittest.cc:125: _pAtlModule = &module;
Put this and CoInitialize in a SetUp method instead, so it doesn't need to be
repeated for every test. Or try SetUpTestCase, a static method which is called
just once for all of the tests in this class.

[Chris Guillory, 2010-08-31 03:15:10]

Crossing fingers for try job.

http://codereview.chromium.org/3117036/diff/29001/30006
File chrome/browser/browser_accessibility_win_unittest.cc (right):

http://codereview.chromium.org/3117036/diff/29001/30006#newcode125
chrome/browser/browser_accessibility_win_unittest.cc:125: _pAtlModule = &module;
On 2010/08/31 03:07:58, Dominic Mazzoni wrote:
> Put this and CoInitialize in a SetUp method instead, so it doesn't need to be
> repeated for every test. Or try SetUpTestCase, a static method which is called
> just once for all of the tests in this class.
> 

Done.