Fix a few lingering bugs in BrowserAccessibilityManager and BrowserAccessibilityCocoa.

Fix a few lingering bugs in BrowserAccessibilityManager and BrowserAccessibilityCocoa.

-  Actually reuse BrowserAccessibility's when we receive an AXLoadComplete notification (switch from using CreateAccessibilityTree to UpdateNode).
-  add a check in BrowserAccessibilityManager::SetFocus to ensure we never try and ref count the same element.  This means we released an element (and thereby destroy it) before we add ref it again.  As it stands, the code does not encounter this case, since there's an implicit assumption that the focus_ element has ref count >= 2.
-  add logic to BrowserAccessibilityCocoa so that it can inherit directly from NSObject; this brings us in line with the way Webkit's AccessibilityObjectWrapper on Mac works.

This will hopefully set us up to solve the problem of VoiceOver not acknowledging page transitions.  The remaining issue is that RWHV's get destroyed on page transitions, thereby destroying our BrowserAccessibilityManager.  This is likely why VoiceOver won't honor our AXLoadComplete notification.

BUG=none
TEST=manual.


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

[David Tseng, 2011-07-27 01:11:06]

Ready for an initial look and criticism.  This is in effort to fix
crbug.com/55658, but does not get all the way there; see patch for further
details.

[dmazzoni, 2011-08-02 06:19:34]

http://codereview.chromium.org/7461104/diff/7001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7461104/diff/7001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1068:
empty_document.id = 1000;
Could you move this to a common place, so it isn't duplicated in Mac and Windows
code? How about a static method in BrowserAccessibilityManager called
GetEmptyDocument or PopulateEmptyDocument?

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_cocoa.h (left):

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_cocoa.h:19: // Inheriting
from NSView rather than NSObject as clients cannot add
Does this matter? Are we losing anything by inheriting from NSObject?

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_cocoa.mm:249: extern "C"
void NSAccessibilityUnregisterUniqueIdForUIElement(id element);
Is this not in any include files?

Does it maybe belong in base/mac/cocoa_protocols or something like that? Maybe
ask Nico or Stuart or one of the other Mac devs. If all else fails, put it at
the top of the file and add a comment explaining why it's forward-declarered -
like if it was only defined in 10.6.

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:102: // Make sure
we don't overwrite a newer entry (see UpdateNode for a possible
Good catch - would it be possible to write a unit test for this?

[David Tseng, 2011-08-02 21:30:57]

Addressing dmazzoni's comments.

Nico, would you mind taking a look at browser_accessibility_cocoa.mm in this
patch?  Thanks.

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_cocoa.h (left):

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_cocoa.h:19: // Inheriting
from NSView rather than NSObject as clients cannot add
No; with the unique id private API's we no longer need to inherit from NSView.
On 2011/08/02 06:19:35, Dominic Mazzoni wrote:
> Does this matter? Are we losing anything by inheriting from NSObject?

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_cocoa.mm:249: extern "C"
void NSAccessibilityUnregisterUniqueIdForUIElement(id element);
I couldn't find it in any header files. I'm guessing it's defined in a framework
dylib we link against?  I'll add Nico for comment.
On 2011/08/02 06:19:35, Dominic Mazzoni wrote:
> Is this not in any include files?
> 
> Does it maybe belong in base/mac/cocoa_protocols or something like that? Maybe
> ask Nico or Stuart or one of the other Mac devs. If all else fails, put it at
> the top of the file and add a comment explaining why it's forward-declarered -
> like if it was only defined in 10.6.

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7461104/diff/7001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:102: // Make sure
we don't overwrite a newer entry (see UpdateNode for a possible
On 2011/08/02 06:19:35, Dominic Mazzoni wrote:
> Good catch - would it be possible to write a unit test for this?

Done.

[Nico, 2011-08-02 22:07:14]

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

http://codereview.chromium.org/7461104/diff/12001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:20: extern "C" void
NSAccessibilityUnregisterUniqueIdForUIElement(id element);
Is this SPI / a weak symbol? Can it resolve to NULL with future SDKs?

[David Tseng, 2011-08-03 00:13:22]

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

http://codereview.chromium.org/7461104/diff/12001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:20: extern "C" void
NSAccessibilityUnregisterUniqueIdForUIElement(id element);
This is SPI from AppKit. There's no guarantee of its existence in future SDK's.
It's however also used in WebKit.

On 2011/08/02 22:07:14, Nico wrote:
> Is this SPI / a weak symbol? Can it resolve to NULL with future SDKs?

[Nico, 2011-08-04 14:29:01]

Sorry, I forgot to publish my comment!

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

http://codereview.chromium.org/7461104/diff/12001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:20: extern "C" void
NSAccessibilityUnregisterUniqueIdForUIElement(id element);
Ok. Add a comment that says that this is SPI that's been tested to work on
10.5-10.7, file a radar to make this an API, dump the radar into
openradar.appspot.com, add the link to the comment, and give a short description
of what this function does.

[David Tseng, 2011-08-04 16:22:01]

Done.

On 2011/08/04 14:29:01, Nico wrote:
> Sorry, I forgot to publish my comment!
> 
>
http://codereview.chromium.org/7461104/diff/12001/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility_cocoa.mm (right):
> 
>
http://codereview.chromium.org/7461104/diff/12001/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_cocoa.mm:20: extern "C"
void
> NSAccessibilityUnregisterUniqueIdForUIElement(id element);
> Ok. Add a comment that says that this is SPI that's been tested to work on
> 10.5-10.7, file a radar to make this an API, dump the radar into
> http://openradar.appspot.com, add the link to the comment, and give a short
description
> of what this function does.

[Nico, 2011-08-04 16:26:08]

Thanks! mac files lgtm

[dmazzoni, 2011-08-04 16:29:07]

LGTM.

http://codereview.chromium.org/7461104/diff/22001/chrome/browser/accessibilit...
File chrome/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

http://codereview.chromium.org/7461104/diff/22001/chrome/browser/accessibilit...
chrome/browser/accessibility/browser_accessibility_manager_unittest.cc:564: //
Verify the root is as we expect by default.
Indent comment

[David Tseng, 2011-08-04 16:41:58]

http://codereview.chromium.org/7461104/diff/22001/chrome/browser/accessibilit...
File chrome/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

http://codereview.chromium.org/7461104/diff/22001/chrome/browser/accessibilit...
chrome/browser/accessibility/browser_accessibility_manager_unittest.cc:564: //
Verify the root is as we expect by default.
On 2011/08/04 16:29:07, Dominic Mazzoni wrote:
> Indent comment

Done.

[commit-bot: I haz the power, 2011-08-04 22:17:14]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac, revision is 95509, job name
was 7461104-23 (previous was lost) (previous was lost) (previous was lost)
(retry).

[commit-bot: I haz the power, 2011-08-05 04:07:52]

Change committed as 95572

[Chris Guillory, 2011-08-16 02:10:31]

Post review comments. Can you please CC me on logic changes to
BrowserAccessibilityManager as there are known issues in webkit and edge cases
that we need to account for in this code.

http://codereview.chromium.org/7461104/diff/23/content/browser/accessibility/...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7461104/diff/23/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_manager.cc:121: if
(renderer_id_to_child_id_map_[renderer_id] == child_id)
According to our assumptions there should not be a newer entry. We should
DCHECK(renderer_id_to_child_id_map_[renderer_id] == child_id)

http://codereview.chromium.org/7461104/diff/23/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_manager.cc:208: root_ =
UpdateNode(acc_obj, true);
As discussed I think we do not want to use UpdateNode here. The accessibility
tree from load complete can be from a different page.

http://codereview.chromium.org/7461104/diff/23/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_manager.cc:269: if (focus_
!= node) {
Why are we removing this check? If focus_ equals node we shouldn't have to
perform this logic correct?

http://codereview.chromium.org/7461104/diff/23/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_manager.cc:335: if (!focus_
|| !focus_->instance_active())
We should keep the DCHECK here unless we plan to have situations occur in which
we have no focused node. I think that violates our assumptions.

[Chris Guillory, 2011-08-16 17:25:12]

http://codereview.chromium.org/7461104/diff/23/content/browser/accessibility/...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7461104/diff/23/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_manager.cc:269: if (focus_
!= node) {
On 2011/08/16 02:10:31, Chris Guillory wrote:
> Why are we removing this check? If focus_ equals node we shouldn't have to
> perform this logic correct?

Please ignore my previous comment. For some reason I parsed this as if you were
removing this check.