<webview>: Correctly shift focus between WebContents.

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 ae0a6ecea2d81617526e2ec85bd4aa8e25c63d73..71f83fbf5f31293b7e9fca0342c583d78d61e7e5 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1847,6 +1847,10 @@ RenderWidgetHostImpl* WebContentsImpl::GetFocusedRenderWidgetHost(
   return RenderWidgetHostImpl::From(view->GetRenderWidgetHost());
 }
 
+RenderWidgetHostImpl* WebContentsImpl::GetRenderWidgetHostWithPageFocus() {
+  return GetFocusedWebContents()->GetMainFrame()->GetRenderWidgetHost();
+}
+
 void WebContentsImpl::EnterFullscreenMode(const GURL& origin) {
   // This method is being called to enter renderer-initiated fullscreen mode.
   // Make sure any existing fullscreen widget is shut down first.
@@ -4626,6 +4630,49 @@ void WebContentsImpl::EnsureOpenerProxiesExist(RenderFrameHost* source_rfh) {
   }
 }
 
+void WebContentsImpl::ChangeFocusIfNecessary() {
+  // Only change focus if we are not currently focused.
+  WebContentsImpl* old_contents = GetFocusedWebContents();
+  if (old_contents == this)
+    return;
+
+  // Ensure that the embedding frame in each outer WebContent's frame tree is
+  // focused, if present.
+  WebContentsImpl* web_contents = this;
+  while (web_contents->GetOuterWebContents()) {
+    FrameTreeNode* tree_node = FrameTreeNode::GloballyFindByID(
+        web_contents->GetOuterDelegateFrameTreeNodeId());
+    RenderFrameProxyHost* outer_proxy = web_contents->frame_tree_.root()
+                                            ->render_manager()
+                                            ->GetProxyToOuterDelegate();
+    // In BrowserPlugin, we don't have a node in the embedder, we are implicitly
+    // embedded in the main frame.
+    // TODO(http://crbug.com/533069): Delete once BrowserPlugin is gone.
+    if (!tree_node) {
+      tree_node = web_contents->GetOuterWebContents()
+                      ->GetMainFrame()
+                      ->frame_tree_node();
+    }
+
+    web_contents = web_contents->GetOuterWebContents();

[alexmos, 2016/10/31 18:57:57]

nit: seems like you could move this up to just before the "if (!tree_node)" and
use it there.

[avallee, 2016/11/07 19:18:44]

Loop deleted. I think this will not be necessary.

+    if (tree_node == web_contents->frame_tree_.GetFocusedFrame())
+      break;
+    if (tree_node)
+      web_contents->frame_tree_.SetFocusedFrame(tree_node,
+                                                outer_proxy->GetSiteInstance());
+
+    if (outer_proxy)
+      outer_proxy->SetFocusedFrame();

[alexmos, 2016/10/31 18:57:57]

Does this need to be done if the tree_node is already the focused node in
web_contents->frame_tree_?

[avallee, 2016/11/07 19:18:44]

I'm not sure, but deleted for now, it seems notifying parents is
inconsequential.

+  }
+
+  // Send a page level blur to the old contents so that it displays inactive UI
+  // and focus this contents to activate it.
+  if (old_contents)
+    old_contents->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(false);
+  GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(true);
+  GetOutermostWebContents()->node_->SetFocusedWebContents(this);
+}
+
 void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,
                                       SiteInstance* source) {
   if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) {
@@ -4633,22 +4680,9 @@ void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,
     return;
   }
 
-  // 1. Find old focused frame and unfocus it.
-  // 2. Focus the new frame in the current FrameTree.
-  // 3. Set current WebContents as focused.
-  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);
-  }
+  ChangeFocusIfNecessary();
 
   frame_tree_.SetFocusedFrame(node, source);
-
-  // TODO(avallee): Remove this once page focus is fixed.
-  RenderWidgetHostImpl* rwh = node->current_frame_host()->GetRenderWidgetHost();
-  if (rwh && old_focused_contents != this)
-    rwh->Focus();
 }
 
 bool WebContentsImpl::AddMessageToConsole(int32_t level,
@@ -4678,6 +4712,20 @@ void WebContentsImpl::OnUserInteraction(
     rdh->OnUserGesture();
 }
 
+void WebContentsImpl::EnsureOwningContentsIsFocused(
+    RenderWidgetHostImpl* render_widget_host) {
+  if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_)

[alexmos, 2016/10/31 18:57:57]

Can you please explain this check?  So if this is a BrowserPlugin guest we
return early, but if it's either a --use-cpffg guest or not a guest at all, we
proceed?  Or maybe you meant !browser_plugin_guest_ in the condition?  Perhaps
add a comment?

[avallee, 2016/11/07 19:18:44]

This fixes an issue with TextInputState. This is the same condition that is at
the top of WebContentsImpl::SetFocusedFrame. +ekaramad@ Added the original
condition, I'm still not sure I wrap my head around it.

[EhsanK, 2016/11/08 16:05:47]

This is actually for making sure PDF runs on BrowserPlugin even when other
guests use OOPIF. The initial method would change the focused WebContents but we
did not want to do it if the WebContents was a guest based on BrowserPlugin.

It was superseded with a line of code like
if (!UseCrossProcessFramesForGuests())
  return;

which would not quite cut it if had the flag to run guests on OOPIF but the
WebContentsImpl was for a MimeHandlerViewGuest, which is the only guest as of
now intentionally forced to used BrowserPlugin.

In short, we early return here is 'this' WebContents is for a guest which is NOT
using OOPIF.

As to why we would early return I think the reason was that focus was supposed
to go through embedder->guest since it was input related. Are we changing that
logic here, i.e., BrowserPlugin WebContents' exposed to OOPIF?

[alexmos, 2016/11/09 02:47:54]

I think the intent of the early return in WebContentsImpl::SetFocusedFrame
originally (see https://codereview.chromium.org/1934703002/) was to fall back to
single-WebContents behavior if --use-cpffg is off.  This included both
BrowserPlugin guests and the cases of ordinary WebContents without any guests.  

I'm not sure if I fully understand the new check in
WebContentsImpl::SetFocusedFrame: it will early-return only for BrowserPlugin
guests, but it seems like it will do the multi-WebContents work below even when
there are no guests, which I don't think we intended.  Ehsan, does this
reasoning make sense, or am I off somewhere?  I don't understand why just
!GuestMode::IsCrossProcessFrameGuest(this) isn't sufficient both in
WebContentsImpl::SetFocusedFrame and here, as IsCrossProcessFrameGuest seems to
already return false for PDF guests even with --use-cpffg, IIUC.  It also
returns false when there's no guest (right?), which is another case where we
want to exit early.

[EhsanK, 2016/11/09 16:58:02]

alexmos@: If I understand your question, we are asking about the check in the
WebContentsImpl::SetFocusedFrame at ToT now (without this change), right? If
that is the case, here is my justification:

We still need to go through the multi-WebContents checks. The example to show
this is a <webview> with <input> inside an app window which itself has an
<input>. If we early return on a non-guest WebContents, then the 'WebContents
focus' will not be tracked properly.

Imagine I clicked the <webview> input and then clicked the <input> in app. This
leads us to a call to WebContentsImpl::SetFocusedFrame where |this| ptr is the
outer WebContents. But the |old_web_contents|, aka GetFocusedWebContents(), is
still the inner WebContents. By early returning we are losing the change to
reset the focused WebContents to embedder.

The result would be that the frame tree of the outer WebContents receives focus,
but the GetFocusedWebContents() will return the inner WebContents and hence,
GetFocusedRenderWidgetHost() will fail. So after clicking on app's <input>,
keyboard input will go to <webview>.

I tested this on ToT and it fails as expected.

[alexmos, 2016/11/09 18:16:57]

Right.
I'm assuming the example is without --use-cpffg?
What would change it to the inner WebContents in the first place?  Seems like
you'd early return in WCI::SetFocusedFrame when clicking on the <input> in the
<webview>, and WCI::SetFocusedFrame is the only thing that can change it (to the
inner WC).
Was your example broken (when done without --use-cpffg) before you fixed that
check in WebContentsImpl::SetFocusedFrame?

[EhsanK, 2016/11/09 19:17:15]

No, actually this is using --use-cpffg. BTW, the new way to enable ucpffg
is to add --enable-features=GuestViewCrossProcessFrames. I also had
--site-per-process but I believe it might be irrelevant.
Since the <webview> is using cross process frames, we will set it as focused
when we first click on the <input> inside the <webview>. The problem is by early
returning, we do not set the outer WebContents as focused when we click the
<input> in the app.
Without --ucpfg the app works fine. I think the reason is that no one ever
changes the focused WebContents in that case.

[alexmos, 2016/11/10 18:35:44]

Thanks for clarifying.  What worried me is that if --use-cpffg is *off*, the old
check in WebContentsImpl::SetFocusedFrame always returned early, but the new
check only returns early for BrowserPlugin guests.  It's not a big deal - things
are still correct, but we end up doing a bit of extra work for the common case
of WebContents without any embedders/guests when we don't need to.

It seems the early return condition could instead be
  if (!GuestViewCrossProcessFrames_is_on_for_everything ||
this_wc_is_a_pdf_guest)

(where this_wc_is_a_pdf_guest is the current condition)
which should work with your example, the pdf case, and still return early when
the GuestViewCrossProcessFrames trial is off.  But I think it's fine to leave it
as-is and just have the code behave the same way whether or not the trial is on.

[EhsanK, 2016/11/10 19:04:56]

You are right and comparing with the old code we are doing some extra work here.
I actually knew this when I added the change but the reason was that after
discussing with creis@ we decided to only expose one method for determining
--ucpffg is on, and that method would take a WC,

FWIW in the case the flag is off for all WCs, we always have (old_web_contents
== this). Therefore, all we will do is to make two comparisons and also get the
value of RWH from node, which I believe will not be too costly.

[alexmos, 2016/11/10 19:16:55]

Coming back to this CL, I think this check is fine here given the discussion. 
Also, avallee@: can you check if the PDF case that Ehsan mentioned works
properly, whether or not --use-cpffg is enabled?  Specifically, typing into a
form on a PDF, then typing in the embedder?

[avallee, 2016/11/16 05:38:09]

I tested http://foersom.com/net/HowTo/data/OoPdfFormExample.pdf in the browser
sample in each of: no flags, --site-per-process and --use-cpffg (i did not sync
past lfg's flag cl yet).

Input seems to work correctly, but in all cases the pdf does not blur on window
blur.

[alexmos, 2016/11/16 23:29:03]

Acknowledged.

+    return;
+
+  RenderWidgetHostImpl* focused_widget =
+      GetFocusedRenderWidgetHost(render_widget_host);
+
+  if (focused_widget != render_widget_host &&
+      focused_widget->delegate() != render_widget_host->delegate()) {
+    ChangeFocusIfNecessary();
+  }
+}
+
 void WebContentsImpl::OnIgnoredUIEvent() {
   // Notify observers.
   for (auto& observer : observers_)