Fix keyboard focus for OOPIF-<webview>.

content/browser/frame_host/frame_tree.cc

Index: content/browser/frame_host/frame_tree.cc
diff --git a/content/browser/frame_host/frame_tree.cc b/content/browser/frame_host/frame_tree.cc
index 9977976aaba40eb87ec1c970367ba9b3f85a4053..979253d31629b7a8cc6f9aba9cf07149d5db7b51 100644
--- a/content/browser/frame_host/frame_tree.cc
+++ b/content/browser/frame_host/frame_tree.cc
@@ -251,6 +251,17 @@ void FrameTree::SetFocusedFrame(FrameTreeNode* node, SiteInstance* source) {
   if (node == GetFocusedFrame())
     return;
 
+  if (node == nullptr) {
+    if (GetFocusedFrame())
+      GetFocusedFrame()->current_frame_host()->UnsetFocusedFrame();

[alexmos, 2016/05/12 20:28:39]

Do proxies need to be notified also?  E.g., suppose the guest has a main frame A
and a cross-process subframe B.  Suppose A is focused, and then the embedder
receives focus, so we get here to clear focus from A.  It seems the renderer for
B also needs to clear focus.

[avallee, 2016/05/16 20:26:43]

Agreed that they would need to be notified.

However, my current understanding is that all frames inside a webview are local.
I'm not sure if there are cases other than webview where the WebContents has a
child WebContents that loses focus like this.

[alexmos, 2016/05/19 00:08:08]

I brought this up on site isolation chat, and indeed, today there's no support
for OOPIFs inside a <webview>.  However, that's going to be fixed in the future,
and it's better not to rely on this assumption when we can.  Ideally, it'd be
nice to make this future-proof, but I guess notifying proxies would require
adding all the ClearFocusedFrame plumbing for proxies?  Can we at least add a
detailed TODO to fix this once <webview> supports OOPIFs, along with a bug
reference?

+    focused_frame_tree_node_id_ = FrameTreeNode::kFrameTreeNodeInvalidId;
+
+    // TODO(avallee): https://crbug.com/610795 This line is not sufficient to

[dmazzoni, 2016/05/12 02:58:56]

I'm okay with landing this change as-is and fixing the accessibility issue in a
follow-up.
As I described in a separate email, I think you probably need to call
UpdateAXTreeData
on the root frame of the embedder WebContents.

[avallee, 2016/05/16 20:26:41]

Thanks for the follow up dmazzoni.

I have an idea on getting the root updated correctly. I'll follow up on that
thread with my additional questions.

+    // make the test pass. There seems to be no focus change events generated.
+    root()->current_frame_host()->UpdateAXTreeData();
+    return;
+  }
+
   std::set<SiteInstance*> frame_tree_site_instances =
       CollectSiteInstances(this);
 
@@ -272,13 +283,14 @@ void FrameTree::SetFocusedFrame(FrameTreeNode* node, SiteInstance* source) {
       DCHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
       RenderFrameProxyHost* proxy =
           node->render_manager()->GetRenderFrameProxyHost(instance);
-      proxy->SetFocusedFrame();
+      if (proxy)

[alexmos, 2016/05/12 20:28:39]

Why is adding this check necessary?

[avallee, 2016/05/16 20:26:41]

I don't think this is necessary anymore, but was initially getting an instance
that had no proxy and thus ended up being null. I'll check, but I think my code
no longer triggers this.

+        proxy->SetFocusedFrame();
     }
   }
 
   // If |node| was focused from a cross-process frame (i.e., via
   // window.focus()), tell its RenderFrame that it should focus.
-  if (current_instance != source)
+  if (current_instance && current_instance != source)

[alexmos, 2016/05/12 20:28:39]

Similar question here.  I can't see how this can be null.

[avallee, 2016/05/16 20:26:41]

Removed.

     node->current_frame_host()->SetFocusedFrame();
 
   focused_frame_tree_node_id_ = node->frame_tree_node_id();