<webview>: Correctly shift focus between WebContents.

content/browser/renderer_host/render_widget_host_impl.cc

Index: content/browser/renderer_host/render_widget_host_impl.cc
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index 0ce169540b3cae2c0e67d865f585d0d48d8763a3..24fdb73baac47aafaf8162e021d33d85483c8ec6 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -692,6 +692,26 @@ void RenderWidgetHostImpl::GotFocus() {
 }
 
 void RenderWidgetHostImpl::Focus() {
+  RenderWidgetHostImpl* focused_widget =
+      delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr;

[alexmos, 2016/10/28 06:36:39]

I think in the case where GetFocusedRenderWidgetHost returns null, we're
supposed to drop the event to avoid giving it to the wrong frame (see comments
inside that function).  Can you check how this works if the guest crashes while
having page focus, and then you get into here?

[avallee, 2016/10/28 19:19:56]

The main place where Focus() is called is from the RenderWidgetHostView which
only knows about the top level RenderWidget. If an internal RenderWidget dies, I
think it's fine to refocus the outermost RenderWidget when we refocus the
window.

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

It might not always be safe: a malicious embedder could load some  sensitive
site in a <webview>, then crash it just as you start typing sensitive data
inside the <webview>.  The embedder might then start getting the rest of the
data if we focus it, until user realizes that the keystrokes are going to the
wrong place?  See
https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
for a discussion when this came up for OOPIFs.

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

Would crashing the renderer lead a focus change? Currently, the embedder can
just call element.focus() and steal from the webview anyways.

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

I'm not sure -- that's why I was curious if you could try it. :)  But given your
latter point, I think the current code is ok.

+
+  if (focused_widget)
+    focused_widget->FocusDirect();

[alexmos, 2016/10/28 06:36:39]

I'm trying to understand the implication of this change for regular OOPIF cases.
 Say we have a page with frame A embedding frame B, and B is an OOPIF.  If we
focus frame A and then alt-tab back and forth, this will behave as before: send
InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy in
process B (thanks to ReplicatePageFocus).  So both renderers get the page focus
messages.

If we now focus frame B and then alt-tab back and forth, this seems to behave
differently: GetFocusedRenderWidgetHost will return B's RWH, hence we will send
the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) is
false for that RWH, we don't replicate the page focus message to A.  

That will also cause the is_focused_ bit to become inconsistent for both main
frame and subframe's RWHs, though I'm not sure what that entails.

That doesn't seem consistent/desirable.  Is the different behavior intentional? 
Was this meant to only apply to the RWH for embedders/guests, or to subframes as
well?

[avallee, 2016/10/28 19:19:56]

I believe that GetFocusedWebContents()->GetMainFrame() is the right thing to do
here.

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

Yes, that looks better.  However, are we sure that this can't ever be called on
a non-main-frame widget?  At first glance in codesearch, most callsites get here
via smth like GetRenderViewHost()->GetWidget()->Focus(), which implies main
frame widget, but I didn't check all the paths.  I'm especially wondering if
this is ever called for fullscreen RWHs (especially Flash fullscreen) and/or
popup menu widgets (i.e., <select> dropdowns).  Do you mind double-checking
those cases?  See https://crbug.com/593522 for some examples of Flash
fullscreen.  I'm curious if we need to keep the old behavior of just sending
InputMsg_SetFocus to the RWH itself for those cases.

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

Still curious about this.  Might be worth trying putting a DCHECK here and in
Blur(), to see if we ever have a focused_widget != this, yet they point to the
same delegates.  I'm curious if this breaks any tests.

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

This DCHECK would fail if a webview is focused. Once the webview is supposed to
get keyboard input, if we blur or focus the embedder's window we would come
here, this == embedder, focused == webview.

As for flash and select, I messed around in a webview and they seem to work.
Losing window focus closes the full screen flash and select widget. Coming back
leave the flash or select element as the focused element on the page.

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

Sorry, the DCHECK I had in mind was assuming a single WebContents.  I.e.,
something like DCHECK(focused_widget == this || focused_widget->delegate() !=
delegate_).  This would check that if we can't get here for a non-main-frame
widget in a single WebContents (because otherwise, we'd change behavior and send
the IPC to a different widget).

+  else
+    FocusDirect();
+}
+
+void RenderWidgetHostImpl::Blur() {
+  RenderWidgetHostImpl* focused_widget =
+      delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr;
+
+  if (focused_widget)
+    focused_widget->BlurDirect();
+  else
+    BlurDirect();
+}
+
+void RenderWidgetHostImpl::FocusDirect() {

[alexmos, 2016/10/28 06:36:39]

I find these names a bit hard to interpret.  Since this is just setting page
focus, wdyt about calling it SetPageFocus()/ClearPageFocus()?  (You could even
go one step further and merge the two nearly identical functions and take a bool
instead)

[avallee, 2016/10/28 19:19:56]

Done.

   is_focused_ = true;
 
   Send(new InputMsg_SetFocus(routing_id_, true));
@@ -702,7 +722,7 @@ void RenderWidgetHostImpl::Focus() {
     delegate_->ReplicatePageFocus(true);
 }
 
-void RenderWidgetHostImpl::Blur() {
+void RenderWidgetHostImpl::BlurDirect() {
   is_focused_ = false;
 
   // If there is a pending mouse lock request, we don't want to reject it at
@@ -1886,11 +1906,17 @@ InputEventAckState RenderWidgetHostImpl::FilterInputEvent(
   if (!process_->HasConnection())
     return INPUT_EVENT_ACK_STATE_UNKNOWN;
 
-  if (delegate_ && (event.type == WebInputEvent::MouseDown ||
-                    event.type == WebInputEvent::GestureScrollBegin ||
-                    event.type == WebInputEvent::TouchStart ||
-                    event.type == WebInputEvent::RawKeyDown)) {
-    delegate_->OnUserInteraction(this, event.type);
+  if (delegate_) {
+    if (event.type == WebInputEvent::MouseDown ||
+        event.type == WebInputEvent::TouchStart) {
+      delegate_->EnsureOwningContentsIsFocused(this);
+    }
+    if (event.type == WebInputEvent::MouseDown ||
+        event.type == WebInputEvent::GestureScrollBegin ||
+        event.type == WebInputEvent::TouchStart ||
+        event.type == WebInputEvent::RawKeyDown) {
+      delegate_->OnUserInteraction(this, event.type);
+    }
   }
 
   return view_ ? view_->FilterInputEvent(event)