Enable WebViewAccessiblityTest for OOPIF webview.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 2daf76a0a060f226895085cd7feedadfe38e5cd6..d0d5340746a1383e4058f4a429b9a8009503e3db 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -380,9 +380,10 @@ WebContentsImpl::WebContentsTreeNode::WebContentsTreeNode()
 WebContentsImpl::WebContentsTreeNode::~WebContentsTreeNode() {
   // Remove child pointer from our parent.
   if (outer_web_contents_) {
-    ChildrenSet& child_ptrs_in_parent =
+    ChildrenMap& child_ptrs_in_parent =
         outer_web_contents_->node_->inner_web_contents_tree_nodes_;
-    ChildrenSet::iterator iter = child_ptrs_in_parent.find(this);
+    ChildrenMap::iterator iter =
+        child_ptrs_in_parent.find(outer_contents_frame_tree_node_id_);
     DCHECK(iter != child_ptrs_in_parent.end());
     child_ptrs_in_parent.erase(iter);
   }
@@ -391,11 +392,12 @@ WebContentsImpl::WebContentsTreeNode::~WebContentsTreeNode() {
   // TODO(lazyboy): We should destroy the children WebContentses too. If the
   // children do not manage their own lifetime, then we would leak their
   // WebContentses.
-  for (WebContentsTreeNode* child : inner_web_contents_tree_nodes_)
-    child->outer_web_contents_ = nullptr;
+  for (auto child : inner_web_contents_tree_nodes_)
+    child.second->node_->outer_web_contents_ = nullptr;
 }
 
 void WebContentsImpl::WebContentsTreeNode::ConnectToOuterWebContents(
+    WebContentsImpl* web_contents,
     WebContentsImpl* outer_web_contents,
     RenderFrameHostImpl* outer_contents_frame) {
   DCHECK(!focused_web_contents_) << "Should not attach a root node.";
@@ -410,7 +412,9 @@ void WebContentsImpl::WebContentsTreeNode::ConnectToOuterWebContents(
     outer_web_contents_->node_->SetFocusedWebContents(outer_web_contents_);
   }
 
-  outer_web_contents_->node_->inner_web_contents_tree_nodes_.insert(this);
+  outer_web_contents_->node_
+      ->inner_web_contents_tree_nodes_[outer_contents_frame_tree_node_id_] =
+      web_contents;
 }
 
 void WebContentsImpl::WebContentsTreeNode::SetFocusedWebContents(
@@ -420,6 +424,14 @@ void WebContentsImpl::WebContentsTreeNode::SetFocusedWebContents(
   focused_web_contents_ = web_contents;
 }
 
+WebContentsImpl* WebContentsImpl::WebContentsTreeNode::find_contents_at_node(

[alexmos, 2017/02/23 20:07:04]

Name this FindContentsAtNode, since it's not a simple getter/setter.  Or better
yet, FindInnerWebContentsAtNode to clarify exactly what it's getting.

[avallee, 2017/02/28 20:13:00]

Done.

+    int frame_tree_node_id) {
+  auto iter = inner_web_contents_tree_nodes_.find(frame_tree_node_id);
+  if (iter == inner_web_contents_tree_nodes_.end())
+    return nullptr;
+  return iter->second;
+}
+
 // WebContentsImpl -------------------------------------------------------------
 
 WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
@@ -1440,7 +1452,7 @@ void WebContentsImpl::AttachToOuterWebContentsFrame(
   // Create a link to our outer WebContents.
   node_.reset(new WebContentsTreeNode());
   node_->ConnectToOuterWebContents(
-      static_cast<WebContentsImpl*>(outer_web_contents),
+      this, static_cast<WebContentsImpl*>(outer_web_contents),

[alexmos, 2017/02/23 20:07:04]

Maybe it would help if WebContentsTreeNode just knew about the WebContents it's
associated with?  Just something to think about for future cleanup.

[avallee, 2017/02/28 20:13:01]

My original iteration of the downward link did just this, but then nodes are 1
pointer larger each. Walking down focused frame tree nodes, we first look at the
WebContents anyways and then its node_.

       static_cast<RenderFrameHostImpl*>(outer_contents_frame));
 
   DCHECK(outer_contents_frame);
@@ -1459,6 +1471,12 @@ void WebContentsImpl::AttachToOuterWebContentsFrame(
       render_manager->GetRenderWidgetHostView())
       ->RegisterFrameSinkId();
 
+  // Set up the the guest's AX tree to point back at the embedder's AX tree.
+  auto* parent_frame = outer_contents_frame->GetParent();
+  GetMainFrame()->set_browser_plugin_embedder_ax_tree_id(
+      parent_frame->GetAXTreeID());
+  GetMainFrame()->UpdateAXTreeData();
+
   // At this point, we should destroy the TextInputManager which will notify all
   // the RWHV in this WebContents. The RWHV in this WebContents should use the
   // TextInputManager owned by the outer WebContents.
@@ -4329,6 +4347,20 @@ WebContentsImpl* WebContentsImpl::GetOutermostWebContents() {
   return root;
 }
 
+void WebContentsImpl::FocusOuterAttachmentFrameChain() {
+  WebContentsImpl* outer_contents = GetOuterWebContents();
+  if (!outer_contents)
+    return;
+
+  FrameTreeNode* outer_node =
+      FrameTreeNode::GloballyFindByID(GetOuterDelegateFrameTreeNodeId());
+  outer_contents->frame_tree_.SetFocusedFrame(outer_node, nullptr);

[alexmos, 2017/02/23 20:07:04]

Can you please explain why this is now necessary?  I don't see how it's related
to the other changes.

Another issue is that this can be called from WCI::SetFocusedFrame, and I'm
concerned that we'd be losing the source SiteInstance that's passed there if we
just use nullptr here.

[avallee, 2017/02/28 20:13:01]

Maybe the site instance still needs to be passed along, I'm not quite sure. Note
that this is FrameTree::SetFocusedFrame, not WebContentsImpl.

For accessibility, each parent needs to correctly show the container frame tree
node of the inner contents as focused so that the
BrowserAccessibility/BrowserAccessibilityManager can walk down and determine
which one is focused.

[alexmos, 2017/03/03 20:07:57]

Acknowledged.  

Thinking about the source SiteInstance some more, it should matter only within
the same FrameTree, as it's used to prevent notifying a process that already
knows about the focus change.  If we're moving to an outer WebContents, that
implies a process change, so passing |source| shouldn't be necessary (as it
shouldn't match any of the SiteInstances in the outer WC's FrameTree).

+  if (GetRenderManager()->GetProxyToOuterDelegate())
+    GetRenderManager()->GetProxyToOuterDelegate()->SetFocusedFrame();
+
+  outer_contents->FocusOuterAttachmentFrameChain();
+}
+
 void WebContentsImpl::RenderViewCreated(RenderViewHost* render_view_host) {
   // Don't send notifications if we are just creating a swapped-out RVH for
   // the opener chain.  These won't be used for view-source or WebUI, so it's
@@ -4733,8 +4765,7 @@ void WebContentsImpl::SetAsFocusedWebContentsIfNecessary() {
   // Make sure the outer web contents knows our frame is focused. Otherwise, the
   // outer renderer could have the element before or after the frame element
   // focused which would return early without actually advancing focus.
-  if (GetRenderManager()->GetProxyToOuterDelegate())
-    GetRenderManager()->GetProxyToOuterDelegate()->SetFocusedFrame();
+  FocusOuterAttachmentFrameChain();
 
   GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(true);
   GetOutermostWebContents()->node_->SetFocusedWebContents(this);
@@ -4749,9 +4780,40 @@ void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,
     return;
   }
 
-  SetAsFocusedWebContentsIfNecessary();
-
   frame_tree_.SetFocusedFrame(node, source);
+
+  WebContentsImpl* inner_contents = nullptr;
+  if (node_)
+    inner_contents = node_->find_contents_at_node(node->frame_tree_node_id());
+
+  WebContentsImpl* contents_to_focus = inner_contents ? inner_contents : this;

[alexmos, 2017/02/23 20:07:04]

I'm a bit confused about this change also.  See my earlier question about the
"SetFocusedFrame(outer_node, nullptr)" above, which I'm guessing is how you
would get here for the case with a valid inner_contents?  Why does this become
necessary?  

Suppose I click on a <webview>.  As part of WCI::SetFocusedFrame in the inner
contents, we call SetAsFocusedWebContentsIfNecessary on the inner contents. 
That now calls the new FocusOuterAttachmentFrameChain, which calls
SetFocusedFrame(outer_node, nullptr), which ends up here and calls
SetAsFocusedWebContentsIfNecessary on inner_contents again, which seems
unnecessary?

[avallee, 2017/02/28 20:13:00]

If you clicked the webview before routing the input there would be a call to
WebContentsImpl::FocusOwningWebContents which would focus the inner contents and
then the frame in the outer contents, but that wouldn't call back down here. If
the outer WebContents decided to focus the frame with an inner contents, then we
need to tell the inner contents that it is now active which is why we move
downwards like this.

[alexmos, 2017/03/03 20:07:57]

Ack.  I was confused that FocusOuterAttachmentFrameChain would call
WebContentsImpl::SetFocusedFrame on the outer contents, but it actually calls
the FrameTree version, so it doesn't call back down here.

+  contents_to_focus->SetAsFocusedWebContentsIfNecessary();
+}
+
+RenderFrameHost* WebContentsImpl::GetFocusedFrameIncludingInnerWebContents() {
+  if (!node_)
+    return GetFocusedFrame();
+
+  auto* contents = this;
+  auto* focused_node = contents->frame_tree_.GetFocusedFrame();

[alexmos, 2017/02/23 20:07:04]

nit: at least for me, an explicit type here instead of auto would be more
readable, so it's obvious that this is a FrameTreeNode* (and different from
RFHI* that the other GetFocusedFrame() returns above).

[avallee, 2017/02/28 20:13:00]

Done.

+
+  // If there is no focused frame in the outer WebContents, we need to return
+  // null.
+  if (!focused_node)
+    return nullptr;
+
+  // We cannot return the frame where an inner WebContents is attached, if the
+  // inner WebContents does not have a focused frame, return its main frame.
+  while (true) {
+    contents = contents->node_->find_contents_at_node(
+        focused_node->frame_tree_node_id());

[alexmos, 2017/02/23 20:07:04]

Why not just pass in the FTN* and retrieve the FTN ID internally in that helper?

[avallee, 2017/02/28 20:13:01]

Done.

+    if (!contents)
+      return focused_node->current_frame_host();
+
+    focused_node = contents->frame_tree_.GetFocusedFrame();
+    if (!focused_node)
+      return contents->GetMainFrame();

[alexmos, 2017/02/23 20:07:04]

What makes this necessary?  How can we get here in practice?

[avallee, 2017/02/28 20:13:00]

On attach or after reaching the end of content, there is no focused frame.

Should I log a warning here if you think this is unexpected?

[alexmos, 2017/03/03 20:07:57]

Not quite sure what you meant by "after reaching the end of content".  Does AX
call GetFocusedFrameIncludingInnerWebContents on attach when there's no focused
frame in the inner contents yet, but the inner contents is already focused in
the outer contents?  It'd be nice to add rationale for returning the main frame
in the comment above the while().

This is probably fine, and I'm not that surprised about returning the main frame
(knowing how widely Blink's focusedOrMainFrame() is used), but just want to
understand why it's needed here.  It seems a bit weird that if there's no
focused frame, we return null for outer WC and the main frame for inner WC -
would AX break if you returned null here instead?

[avallee, 2017/06/21 02:25:35]

I think this will get called on attach, but as a result of
WebContentsImpl::SetFocusedFrame so there should be one focused already.
However, when you tab (shift-tab) with the last (first) focusable element in the
document, the FocusController will clear the focused frame.
As I understand it, it seems that it wants some frame to be focused overall but
in a subtree it's fine that there's no focused frame. (See
RenderFrameHostImpl::AXContentTreeDataToAXTreeData)

+  }
 }
 
 void WebContentsImpl::OnFocusedElementChangedInFrame(