Rewrite renderer accessibility to not use WebAccessibilityCache.

Rewrites renderer accessibility to not use WebAccessibilityCache.

This fixes the primary underlying cause of a number of bugs where the
browser's accessibility tree got out of sync with the renderer's accessibility
tree, which makes the page seem not functional anymore.

Splits renderer accessibility code out of render_view.cc into its own file.
All code now uses the axID directly from WebCore::AccessibilityObject rather
than WebAccessibilityCache, which can now be deleted. One implication of this
is that the top-level node can now change its ID, so we introduce a new
role ROLE_ROOT_WEB_AREA to distinguish it from an iframe's ROLE_WEB_AREA node.

To solve the problem of getting out of sync, the renderer now
maintains a tree of axIDs that reflects the state of the tree as of
the most recent message sent to the browser. Whenever a new
accessibility notification is posted, it refers to that tree to
determine precisely what information needs to be sent to the browser,
eliminating problems where the renderer was posting notifications
about nodes that the browser didn't know about.

BUG=93095, 93232, 92716, 90787, 90768
TEST=updated lots of tests

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

[dmazzoni, 2011-09-26 15:27:41]

dtseng: primary reviewer
jam: content/
tony: webkit/glue/

[tony, 2011-09-26 17:18:55]

webkit/glue LGTM

[jam, 2011-09-26 17:52:32]

rubberstamp lgtm (moving stuff out of RenderView is great)

[David Tseng, 2011-09-27 21:26:13]

http://codereview.chromium.org/7966013/diff/15001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

http://codereview.chromium.org/7966013/diff/15001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:116: {
WebAccessibility::ROLE_ROOT_WEB_AREA, @"AXWebArea" },
I think this was in another patch.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
File content/renderer/renderer_accessibility.cc (right):

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:74: return false;
NOTREACHED?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:116: return;
Should this be a DCHECK?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:155: return;
DCHECK?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:164: sent_load_complete_ = true;
Is this necessary given the below if clause?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:180: for (uint32 i = 0; i <
pending_notifications_.size(); i++) {
++i

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:202: return;
DCHECK?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:217: continue;
Do we want to place some logging here? Is this ever expected?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:248: for (unsigned int i = 0; i <
parent.childCount(); i++) {
++i

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:302:
browser_id_map_.erase(browser_root_->id);
optional: you could do this within ClearBrowserTree if you also deleted the
parameter node.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:311: for (size_t i = 0; i <
renderer_node.children.size(); i++) {
++i

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:321: for (size_t i = 0; i <
browser_node->children.size(); i++) {
++i

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:335: return;
DCHECK?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:339: return;
DCHECK? (out of sync renderer <-> browser).

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:356: if (render_view()->webview()) {
GetMainDocument().isNull()?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:361:
render_view()->webview()->accessibilityObject(),
GetMainDocument().accessibilityObject()?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
File content/renderer/renderer_accessibility.h (right):

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.h:76: // Clear the given node and all of
its descendants from the browser tree.
It looks like this method clears the children but not the parameter node?

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.h:106: // Set if we are waiting for a
accessibility notification ack.
a -> an

[dmazzoni, 2011-09-28 05:28:49]

http://codereview.chromium.org/7966013/diff/15001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

http://codereview.chromium.org/7966013/diff/15001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:116: {
WebAccessibility::ROLE_ROOT_WEB_AREA, @"AXWebArea" },
On 2011/09/27 21:26:13, David Tseng wrote:
> I think this was in another patch.

Thanks. The other one was the error; I had merged in this change while writing
it. It belongs in this change.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
File content/renderer/renderer_accessibility.cc (right):

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:74: return false;
On 2011/09/27 21:26:13, David Tseng wrote:
> NOTREACHED?

Done.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:116: return;
On 2011/09/27 21:26:13, David Tseng wrote:
> Should this be a DCHECK?

I don't think so; I hit this when the page was first loading or being torn down
and we received a stale notification.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:164: sent_load_complete_ = true;
On 2011/09/27 21:26:13, David Tseng wrote:
> Is this necessary given the below if clause?

Done.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:180: for (uint32 i = 0; i <
pending_notifications_.size(); i++) {
On 2011/09/27 21:26:13, David Tseng wrote:
> ++i

Fixed throughout.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:217: continue;
On 2011/09/27 21:26:13, David Tseng wrote:
> Do we want to place some logging here? Is this ever expected?

This does happen, but it's a WebKit bug when it does. One example I found is
where a children changed notification is sent on an object being removed from
the tree, rather than on its parent.

I added a log in debug mode.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:302:
browser_id_map_.erase(browser_root_->id);
On 2011/09/27 21:26:13, David Tseng wrote:
> optional: you could do this within ClearBrowserTree if you also deleted the
> parameter node.

I don't always want to delete the parameter node; in the above case I just want
to clear it.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:339: return;
On 2011/09/27 21:26:13, David Tseng wrote:
> DCHECK? (out of sync renderer <-> browser).

I added logging instead; I think this can happen legitimately during a race
condition.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:356: if (render_view()->webview()) {
On 2011/09/27 21:26:13, David Tseng wrote:
> GetMainDocument().isNull()?

Done.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.cc:361:
render_view()->webview()->accessibilityObject(),
On 2011/09/27 21:26:13, David Tseng wrote:
> GetMainDocument().accessibilityObject()?

Done.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
File content/renderer/renderer_accessibility.h (right):

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.h:76: // Clear the given node and all of
its descendants from the browser tree.
On 2011/09/27 21:26:13, David Tseng wrote:
> It looks like this method clears the children but not the parameter node?

You're right. I renamed it ClearBrowserTreeNode and clarified the documentation.

http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
content/renderer/renderer_accessibility.h:106: // Set if we are waiting for a
accessibility notification ack.
On 2011/09/27 21:26:13, David Tseng wrote:
> a -> an

Done.

[dtseng, 2011-09-28 17:21:16]

LGTM

Sent from my phone

On Sep 27, 2011, at 10:28 PM, "dmazzoni@chromium.org"
<dmazzoni@chromium.org> wrote:

>
>
http://codereview.chromium.org/7966013/diff/15001/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility_cocoa.mm
> (right):
>
>
http://codereview.chromium.org/7966013/diff/15001/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_cocoa.mm:116: {
> WebAccessibility::ROLE_ROOT_WEB_AREA, @"AXWebArea" },
> On 2011/09/27 21:26:13, David Tseng wrote:
>> I think this was in another patch.
>
> Thanks. The other one was the error; I had merged in this change while
> writing it. It belongs in this change.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> File content/renderer/renderer_accessibility.cc (right):
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:74: return false;
> On 2011/09/27 21:26:13, David Tseng wrote:
>> NOTREACHED?
>
> Done.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:116: return;
> On 2011/09/27 21:26:13, David Tseng wrote:
>> Should this be a DCHECK?
>
> I don't think so; I hit this when the page was first loading or being
> torn down and we received a stale notification.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:164: sent_load_complete_ =
> true;
> On 2011/09/27 21:26:13, David Tseng wrote:
>> Is this necessary given the below if clause?
>
> Done.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:180: for (uint32 i = 0; i <
> pending_notifications_.size(); i++) {
> On 2011/09/27 21:26:13, David Tseng wrote:
>> ++i
>
> Fixed throughout.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:217: continue;
> On 2011/09/27 21:26:13, David Tseng wrote:
>> Do we want to place some logging here? Is this ever expected?
>
> This does happen, but it's a WebKit bug when it does. One example I
> found is where a children changed notification is sent on an object
> being removed from the tree, rather than on its parent.
>
> I added a log in debug mode.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:302:
> browser_id_map_.erase(browser_root_->id);
> On 2011/09/27 21:26:13, David Tseng wrote:
>> optional: you could do this within ClearBrowserTree if you also
> deleted the
>> parameter node.
>
> I don't always want to delete the parameter node; in the above case I
> just want to clear it.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:339: return;
> On 2011/09/27 21:26:13, David Tseng wrote:
>> DCHECK? (out of sync renderer <-> browser).
>
> I added logging instead; I think this can happen legitimately during a
> race condition.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:356: if
> (render_view()->webview()) {
> On 2011/09/27 21:26:13, David Tseng wrote:
>> GetMainDocument().isNull()?
>
> Done.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.cc:361:
> render_view()->webview()->accessibilityObject(),
> On 2011/09/27 21:26:13, David Tseng wrote:
>> GetMainDocument().accessibilityObject()?
>
> Done.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> File content/renderer/renderer_accessibility.h (right):
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.h:76: // Clear the given node
> and all of its descendants from the browser tree.
> On 2011/09/27 21:26:13, David Tseng wrote:
>> It looks like this method clears the children but not the parameter
> node?
>
> You're right. I renamed it ClearBrowserTreeNode and clarified the
> documentation.
>
>
http://codereview.chromium.org/7966013/diff/15001/content/renderer/renderer_a...
> content/renderer/renderer_accessibility.h:106: // Set if we are waiting
> for a accessibility notification ack.
> On 2011/09/27 21:26:13, David Tseng wrote:
>> a -> an
>
> Done.
>
> http://codereview.chromium.org/7966013/

[commit-bot: I haz the power, 2011-09-29 06:28:51]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/7966013/30002

[commit-bot: I haz the power, 2011-09-29 07:54:36]

Change committed as 103249