Fix keyboard focus 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 bb4128c8f0fda709aef73e59c5b2819c0d1bd7d6..c474c501157e6686fa6e2b750ca6421977660a05 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -328,41 +328,57 @@ WebContentsImpl::ColorChooserInfo::~ColorChooserInfo() {
 }
 
 // WebContentsImpl::WebContentsTreeNode ----------------------------------------
-WebContentsImpl::WebContentsTreeNode::WebContentsTreeNode()
-    : outer_web_contents_(nullptr),
+WebContentsImpl::WebContentsTreeNode::WebContentsTreeNode(
+    WebContentsImpl* web_contents)
+    : web_contents_(web_contents),
+      outer_web_contents_(nullptr),
       outer_contents_frame_tree_node_id_(
-          FrameTreeNode::kFrameTreeNodeInvalidId) {
-}
+          FrameTreeNode::kFrameTreeNodeInvalidId),
+      focused_web_contents_(nullptr) {}
 
 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(this);
+    child_ptrs_in_parent.erase(iter);
   }
 
   // Remove parent pointers from our children.
   // 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->outer_web_contents_ = nullptr;
 }
 
 void WebContentsImpl::WebContentsTreeNode::ConnectToOuterWebContents(
     WebContentsImpl* outer_web_contents,
     RenderFrameHostImpl* outer_contents_frame) {
+  DCHECK(!focused_web_contents_) << "Should not attach a root node.";
   outer_web_contents_ = outer_web_contents;
   outer_contents_frame_tree_node_id_ =
       outer_contents_frame->frame_tree_node()->frame_tree_node_id();
 
-  if (!outer_web_contents_->node_)
-    outer_web_contents_->node_.reset(new WebContentsTreeNode());
+  if (!outer_web_contents_->node_) {
+    outer_web_contents_->node_.reset(
+        new WebContentsTreeNode(outer_web_contents_));
+    outer_web_contents_->node_->SetFocusedWebContents(outer_web_contents_);

[alexmos, 2016/05/26 00:32:25]

nit: maybe add a comment that we can only get here if outer_web_contents_ is the
the root of the WebContents tree?  (something similar to what you already have
in GetFocusedWebContents)

[avallee, 2016/06/07 15:37:43]

Done.

Does it make sense to assert that we don't have children when we connect to an
outer web contents? I think the assert above would trip anyways since we connect
an set focus in the outer contents which would then fail the assert if trying to
connect up to an even more outer contents.

[alexmos, 2016/06/07 22:03:43]

Yeah, it seems that the DCHECK should be sufficient.

+  }
 
-  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_] =
+      this;
+}
+
+void WebContentsImpl::WebContentsTreeNode::SetFocusedWebContents(
+    WebContentsImpl* web_contents) {
+  DCHECK(!outer_web_contents())
+      << "Only the outermost WebContents tracks focus.";
+  focused_web_contents_ = web_contents;
 }
 
 // WebContentsImpl -------------------------------------------------------------
@@ -1393,7 +1409,7 @@ void WebContentsImpl::AttachToOuterWebContentsFrame(
     CreateRenderWidgetHostViewForRenderManager(GetRenderViewHost());
 
   // Create a link to our outer WebContents.
-  node_.reset(new WebContentsTreeNode());
+  node_.reset(new WebContentsTreeNode(this));
   node_->ConnectToOuterWebContents(
       static_cast<WebContentsImpl*>(outer_web_contents),
       static_cast<RenderFrameHostImpl*>(outer_contents_frame));
@@ -1789,7 +1805,8 @@ RenderWidgetHostImpl* WebContentsImpl::GetFocusedRenderWidgetHost(
   if (receiving_widget != GetMainFrame()->GetRenderWidgetHost())
     return receiving_widget;
 
-  FrameTreeNode* focused_frame = frame_tree_.GetFocusedFrame();
+  FrameTreeNode* focused_frame =
+      GetFocusedWebContents()->frame_tree_.GetFocusedFrame();
   if (!focused_frame)
     return receiving_widget;
 
@@ -4087,6 +4104,29 @@ void WebContentsImpl::RemoveBrowserPluginEmbedder() {
     browser_plugin_embedder_.reset();
 }
 
+WebContentsImpl* WebContentsImpl::GetOuterWebContents() {
+  if (BrowserPluginGuestMode::UseCrossProcessFramesForGuests()) {
+    if (node_)
+      return node_->outer_web_contents();
+  } else {
+    if (GetBrowserPluginGuest())
+      return GetBrowserPluginGuest()->embedder_web_contents();
+  }
+  return nullptr;
+}
+
+WebContentsImpl* WebContentsImpl::GetFocusedWebContents() {
+  // There is no inner web contents.
+  if (!node_) {
+    return this;
+  }
+
+  // If we've found root by iterating through outer WebContents, root must have
+  // a valid node_ since the inner WebContents will have created one when
+  // connecting.
+  return GetOutermostWebContents()->node_->get_focused_web_contents();
+}
+
 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
@@ -4485,6 +4525,30 @@ void WebContentsImpl::EnsureOpenerProxiesExist(RenderFrameHost* source_rfh) {
   }
 }
 
+void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,
+                                      SiteInstance* source) {
+  // Focus
+  // 1. Find old focus and unfocus.
+  // 2. Focus current tree
+  // 3. Set current contents as focus

[alexmos, 2016/05/26 00:32:25]

a couple of nits (just to be more precise with class names):
1. Find old focused frame and unfocus it.
2. Focus new frame in the current FrameTree.
3. Set current WebContents as focused.

[avallee, 2016/06/07 15:37:43]

Done.

+  //
+  // Find root contents. Find focused contents.

[alexmos, 2016/05/26 00:32:25]

nit: this comment is probably unnecessary (root contents is an implementation
detail of GetFocusedWebContents).

[avallee, 2016/06/07 15:37:43]

Done.

+  WebContentsImpl* old_focused_contents = GetFocusedWebContents();
+  if (old_focused_contents != this) {
+    // Focus is moving between frame trees, unfocus the frame in the old tree.
+    old_focused_contents->frame_tree_.SetFocusedFrame(nullptr, source);
+    GetOutermostWebContents()->node_->SetFocusedWebContents(this);
+  }
+
+  // Notify proxy?

[alexmos, 2016/05/26 00:32:25]

what does this refer to?

[avallee, 2016/06/07 15:37:43]

Mental implementation detail. Should have been removed.

+  frame_tree_.SetFocusedFrame(node, source);
+
+  // TODO(avallee): Remove this once page focus is fixed.
+  RenderWidgetHostImpl* rwh = node->current_frame_host()->GetRenderWidgetHost();
+  if (rwh)
+    rwh->Focus();

[alexmos, 2016/05/26 00:32:25]

We don't want this to break default modes in Chrome.  If we have to fix this in
a followup, let's at least restrict this to only be done when
--use-cross-process-frames-for-guests is on, and only if old_focused_contents
doesn't match |this|.  This is the only case when it matters, right?

[alexmos, 2016/05/26 00:32:25]

I poked at this a bit more to refresh my memory on how page focus works
following our chat.

I think there are two main things to take care of:

1. When the outermost WC changes page focus, you need to do the same for all
inner WC.  Since ReplicatePageFocus only does it for proxies and not the main
frame's SiteInstance, you can just use RWH::Focus instead, which does both.  So,
something like this in WC::ReplicatePageFocus:

    for (const auto& inner : node_->inner_web_contents_tree_nodes_) {
      WebContentsImpl *inner_contents = inner.second->web_contents_;
      RenderWidgetHostImpl* rwh =
inner_contents->frame_tree_.root()->current_frame_host()->GetRenderWidgetHost();
      if (is_focused)
        rwh->Focus();
      else
        rwh->Blur();
    }

2. When a new inner WC gets created, it probably needs to inherit page focus
from its outer WC.  So if the outer WC was focused, the new inner WC needs to be
focused also.  We had to do something similar to pass page focus to new OOP
subframe renderers:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

In this case, you probably want to put some sort of check in
AttachToOuterWebContentsFrame, something along the lines of 
  if (outer_web_contents->GetRenderWidgetHostView()->HasFocus())
    GetMainFrame()->GetRenderWidgetHost()->Focus();

[avallee, 2016/06/07 15:37:43]

I've restricted for now.

My trial implementation of WC::ReplicatePageFocus was pretty much the same but
had issues with Focus() calling right back into WC::ReplicatePageFocus.

[alexmos, 2016/06/07 22:03:43]

OK, let's deal with that in a followup CL.  It should be ok that Focus()
actually calls ReplicatePageFocus again, since that'll now happen on inner WCs,
i.e. this is what you can use to recurse down into all inner contents.

+}
+
 bool WebContentsImpl::AddMessageToConsole(int32_t level,
                                           const base::string16& message,
                                           int32_t line_no,
@@ -4862,15 +4926,11 @@ RenderFrameHostManager* WebContentsImpl::GetRenderManager() const {
   return frame_tree_.root()->render_manager();
 }
 
-WebContentsImpl* WebContentsImpl::GetOuterWebContents() {
-  if (BrowserPluginGuestMode::UseCrossProcessFramesForGuests()) {
-    if (node_)
-      return node_->outer_web_contents();
-  } else {
-    if (GetBrowserPluginGuest())
-      return GetBrowserPluginGuest()->embedder_web_contents();
-  }
-  return nullptr;
+WebContentsImpl* WebContentsImpl::GetOutermostWebContents() {

[alexmos, 2016/05/26 00:32:25]

nit: prefer to match up the order of all new functions in web_contents_impl.h
and here.  (e.g., GetFocusedWebContents can go above this to match header
order.)

[avallee, 2016/06/07 15:37:43]

Done.

+  WebContentsImpl* root = this;
+  while (root->GetOuterWebContents())
+    root = root->GetOuterWebContents();
+  return root;
 }
 
 BrowserPluginGuest* WebContentsImpl::GetBrowserPluginGuest() const {