Fix keyboard focus for OOPIF-<webview>.

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 310 matching lines @@
     : render_process_id(render_process_id),
       render_frame_id(render_frame_id),
       chooser(chooser),
       identifier(identifier) {
 }
 
 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 -------------------------------------------------------------
 
 WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
     : delegate_(NULL),
       controller_(this, browser_context),
       render_view_host_delegate_view_(NULL),
       created_with_opener_(false),
       frame_tree_(new NavigatorImpl(&controller_, this),
@@ skipping 1010 matching lines @@
   // created. This is needed because the usual initialization happens during
   // the first navigation, but when attaching a new window we don't navigate
   // before attaching. If the browser side is already initialized, the calls
   // below will just early return.
   render_manager->InitRenderView(GetRenderViewHost(), nullptr);
   GetMainFrame()->Init();
   if (!render_manager->GetRenderWidgetHostView())
     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));
 
   DCHECK(outer_contents_frame);
 
   // Create a proxy in top-level RenderFrameHostManager, pointing to the
   // SiteInstance of the outer WebContents. The proxy will be used to send
   // postMessage to the inner WebContents.
   render_manager->CreateOuterDelegateProxy(
@@ skipping 375 matching lines @@
 RenderWidgetHostImpl* WebContentsImpl::GetFocusedRenderWidgetHost(
     RenderWidgetHostImpl* receiving_widget) {
   if (!SiteIsolationPolicy::AreCrossProcessFramesPossible())
     return receiving_widget;
 
   // Events for widgets other than the main frame (e.g., popup menus) should be
   // forwarded directly to the widget they arrived on.
   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;
 
   // The view may be null if a subframe's renderer process has crashed while
   // the subframe has focus.  Drop the event in that case.  Do not give
   // it to the main frame, so that the user doesn't unexpectedly type into the
   // wrong frame if a focused subframe renderer crashes while they type.
   RenderWidgetHostView* view = focused_frame->current_frame_host()->GetView();
   if (!view)
     return nullptr;
@@ skipping 2277 matching lines @@
   if (delegate_)
     return delegate_->GetRootWindowResizerRect();
   return gfx::Rect();
 }
 
 void WebContentsImpl::RemoveBrowserPluginEmbedder() {
   if (browser_plugin_embedder_)
     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
   // ok to return early.
   if (!static_cast<RenderViewHostImpl*>(render_view_host)->is_active())
     return;
 
   if (delegate_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 
@@ skipping 378 matching lines @@
           GetSiteInstance());
     } else {
       RenderFrameHostImpl* source_rfhi =
           static_cast<RenderFrameHostImpl*>(source_rfh);
       source_rfhi->frame_tree_node()->render_manager()->CreateOpenerProxies(
           GetSiteInstance(), nullptr);
     }
   }
 }
 
+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,
                                           const base::string16& source_id) {
   if (!delegate_)
     return false;
   return delegate_->AddMessageToConsole(this, level, message, line_no,
                                         source_id);
 }
 
@@ skipping 357 matching lines @@
   if (node_ && node_->outer_web_contents())
     return node_->outer_contents_frame_tree_node_id();
 
   return FrameTreeNode::kFrameTreeNodeInvalidId;
 }
 
 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 {
   return browser_plugin_guest_.get();
 }
 
 void WebContentsImpl::SetBrowserPluginGuest(BrowserPluginGuest* guest) {
   CHECK(!browser_plugin_guest_);
   CHECK(guest);
   browser_plugin_guest_.reset(guest);
@@ skipping 131 matching lines @@
   for (RenderViewHost* render_view_host : render_view_host_set)
     render_view_host->OnWebkitPreferencesChanged();
 }
 
 void WebContentsImpl::SetJavaScriptDialogManagerForTesting(
     JavaScriptDialogManager* dialog_manager) {
   dialog_manager_ = dialog_manager;
 }
 
 }  // namespace content