Migrate accessibility from RenderView to RenderFrame.

content/browser/renderer_host/render_widget_host_view_aura.cc

Index: content/browser/renderer_host/render_widget_host_view_aura.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc b/content/browser/renderer_host/render_widget_host_view_aura.cc
index 116a6f4d96102303b0b2a113386c1cf1add0657a..0725caa0a522841f6dc042cbedf5773e65009a6d 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -636,7 +636,7 @@ gfx::NativeViewAccessible RenderWidgetHostViewAura::GetNativeViewAccessible() {
     return static_cast<gfx::NativeViewAccessible>(NULL);
   HWND hwnd = host->GetAcceleratedWidget();
 
-  CreateBrowserAccessibilityManagerIfNeeded();
+  CreateBrowserAccessibilityManagerIfNeeded(NULL);
   BrowserAccessibilityManager* manager = GetBrowserAccessibilityManager();
   if (manager)
     return manager->GetRoot()->ToBrowserAccessibilityWin();
@@ -1191,9 +1191,13 @@ void RenderWidgetHostViewAura::SetScrollOffsetPinning(
   // Not needed. Mac-only.
 }
 
-void RenderWidgetHostViewAura::CreateBrowserAccessibilityManagerIfNeeded() {
-  if (GetBrowserAccessibilityManager())
+void RenderWidgetHostViewAura::CreateBrowserAccessibilityManagerIfNeeded(
+    BrowserAccessibilityDelegate* delegate) {
+  if (GetBrowserAccessibilityManager()) {
+    if (delegate)
+      GetBrowserAccessibilityManager()->set_delegate(delegate);

[ncarter (slow), 2014/05/14 23:56:23]

Is this delegate relationship correct?

Previously the accessibility delegate was the rwh, which always was reachable
from the rwhv. Same same, no lifetime issues. Now the delegate is a rfh, and
(for sanity's sake) I would hope that this means that it's the RFH of the main
frame. But I actually don't see any such guarantee in the code?

It is possible for two rfh's exist that are sharing a rvh; hence sharing a rwh
and hence sharing a rwhv.

The BrowserAccessibilityManager, which is owned by the rwhv, retains a pointer
to one of these rfh's (as a delegate). So at any given time, the
BrowserAccessibilityManager has a pointer to the most recent rfh that called
view->CreateBrowserAccessibilityManagerIfNeeded(this), which is done in response
to an IPC. But what happens if that rfh goes away (say, it's the RFH of a
same-site iframe, and it is deleted)? Is the RWHV or its accessibility manager
informed somehow that the accessibility delegate no longer exists? I don't see
where that happens, and so I'm a little worried that the pointer might persist
after the rfh object is deleted.

Should the delegate responsibilities go somewhere other than the RFH, or else
does each RFH need its accessibility manager?

Unfortunately I don't have a solution ready to suggest, as I don't fully grok
what powers the BrowserAccessibilityDelegate really needs. So let's figure it
out together. I'd definitely like nasko's thoughts on this too -- or it might
help to pick jam's brain.

[dmazzoni, 2014/05/19 07:25:01]

This is a great question. I don't think I fully understand the implications of
two
RFH's sharing a RVH, but let me explain what requirements we need on the
accessibility side.

Right now Blink's AX tree spans all of the frames reachable from the local page.
We need one BrowserAccessibilityManager (henceforth BAM) per Blink AX tree.
Prior to OOPIF, the only case where we need two BAMs is when we have a <webview>
tag.
Note that <webview> is not accessible now; this refactoring should help make
<webview>
accessibility possible.

My understanding is that eventually there will be a separate instance of Blink
for each
frame, even when the frames are from the same origin, right? So eventually we'll
have a
separate AX tree for every frame, and we'll definitely need a BAM for each
frame.

If there's ever a world when there's some mix of both - like some frames will be
handled by the same Blink page, and other frames will be in a separate process,
then
we could have a situation where only some frames need a BAM. However, one
simplification we could make would be to modify Blink accessibility so that
there's
just one AX tree per frame no matter what. I don't see any downsides to this,
once
OOPIF is possible there's no reason not to do this.

OK, so it seems to me that eventually we'll need one BAM per RFH, but that in
the
interim there may be some RFHs that don't need a BAM, because their parent's
RFH's
BAM already covers that frame. I don't think there's any problem with this - the
Blink
AX code will establish which BAM is a descendant of which node of another BAM,
so having an unused BAM in one RFH shouldn't break anything.

In addition, the *root* BAM needs to be connected to its RWHV, because the root
RWHV will query the BAM when implementing accessibility APIs on its view, and
the BAM needs a bit of information about its containing view in order to
implement
native accessibility APIs for web elements.

Child BAMs don't really care about their RWHV, which I'm assuming will be
something like
a RWHVChildFrame anyway. Child BAMs should know they're children, and they
should
only communicate everything up through the root BAM that's the one immediately
contained in some native View.

BrowserAccessibilityDelegate currently conflates two different needs: it needs
to pass
messages to its corresponding render-process frame, and it also needs to pass
information to its native view.

I'm starting to think that the best solution might be for RFH to own the BAM and
to
implement the delegate. Instead of having the RWHV create and own the BAM, the
RFH
should just query the RWHV as needed by the BAM.

How about this: if the BAM asks the Delegate for, e.g., its native view, only
the RFH for the
main frame would return it (by asking its RWHV). Any other RFH would return
NULL, forcing
the BAM to pass its message up through its parent BAM.

Am I correct in my understanding that one WebContents will only have one "real"
RWHV, and
any other subframe will only have a RWHVChildFrame or something like that?

[ncarter (slow), 2014/05/28 00:50:11]

I think you have it right. Where possible I think it is best (from a design
simplicity and maintenance perspective) to arrange things so there is, as you
say, "one AX tree per frame no matter what". If we do that then same-process and
cross-process frames work in essentially the same way as far as the goo in the
browser process is concerned.
This makes sense. You're heading in a direction, I think, where the BAMs are
going to exist one per frame but necessarily somewhat tree-aware. An open
question in my mind is whether the top level BAM (which is the one interacting
with a "real" RWHV) will ultimately have the same runtime type as the lower
level BAMs, or whether it will need a specialized subclass or something. The act
of coding this up will probably make the design clear.
Makes sense, and I see you've implemented this.
Yes, this understanding (only one real RWHV in the frame tree) is correct.

     return;
+  }
 
   BrowserAccessibilityManager* manager = NULL;
 #if defined(OS_WIN)
@@ -1206,17 +1210,20 @@ void RenderWidgetHostViewAura::CreateBrowserAccessibilityManagerIfNeeded() {
   // it down to this instance (by way of the RenderViewHost and
   // RenderWidgetHost) when it is known. This instance will then set it on its
   // BrowserAccessibilityManager.
+  RenderViewHost* rvh = RenderViewHost::From(host_);
+  RenderFrameHostImpl* rfh =
+      rvh->GetDelegate()->GetFrameTree()->GetMainFrame();
   gfx::NativeViewAccessible accessible_parent =
-      host_->GetParentNativeViewAccessible();
+      rfh->GetParentNativeViewAccessible();
 
   if (legacy_render_widget_host_HWND_) {
     manager = new BrowserAccessibilityManagerWin(
         legacy_render_widget_host_HWND_.get(), accessible_parent,
-        BrowserAccessibilityManagerWin::GetEmptyDocument(), host_);
+        BrowserAccessibilityManagerWin::GetEmptyDocument(), delegate);

[ncarter (slow), 2014/05/14 23:56:23]

Since this delegate is sometimes null, I wonder if there's an advantage to not
passing it in the ctor, and requiring instead for it always to arrive via
set_delegate? Seems to me that this would force the null and non-null callers of
this function down a more similar code path.

[dmazzoni, 2014/05/19 07:25:01]

This would also be simplified if the RFHI owns the BAM.

   }
 #else
   manager = BrowserAccessibilityManager::Create(
-      BrowserAccessibilityManager::GetEmptyDocument(), host_);
+      BrowserAccessibilityManager::GetEmptyDocument(), delegate);
 #endif
   SetBrowserAccessibilityManager(manager);
 }