<webview>: Correctly shift focus between WebContents.

content/browser/renderer_host/render_widget_host_delegate.h

Index: content/browser/renderer_host/render_widget_host_delegate.h
diff --git a/content/browser/renderer_host/render_widget_host_delegate.h b/content/browser/renderer_host/render_widget_host_delegate.h
index 46129fdd012affd19c87fbf2b028bc03bc6fb141..093a2185e143ee780173676baf768c81da28c3c9 100644
--- a/content/browser/renderer_host/render_widget_host_delegate.h
+++ b/content/browser/renderer_host/render_widget_host_delegate.h
@@ -145,6 +145,18 @@ class CONTENT_EXPORT RenderWidgetHostDelegate {
   virtual RenderWidgetHostImpl* GetFocusedRenderWidgetHost(
       RenderWidgetHostImpl* receiving_widget);
 
+  // Get the RenderWidgetHost that should receive page level focus events. This
+  // will be the widget that is rendering the main frame of the currently
+  // focused WebContents.

[Charlie Reis, 2016/11/16 20:28:23]

I'm having trouble understanding this vs GetFocusedRenderWidgetHost, since I'm
not familiar with page level focus events.  Why is knowing the focused widget
the wrong thing?  Can you elaborate in the comment?

[avallee, 2016/11/16 21:18:10]

See the <webview> focus explainer for extra details.
https://docs.google.com/document/d/1ysPHsls5CFRhIfW19p8uYRXtVpSwKt6ZY39IX_6AnjY/

The point is that in a guest view case for example <webview> either the webview
is active or the embedder is. Each is considered a different page, they don't
really have insight into each other's content. ekaramad has a tabbed <webview>
browser and when switching from tab to tab, the old webview should receive
events to tell it that the page is no longer focused, and the new page will
receive focus.


This is used by RenderWidgetHostImpl::Focus/Blur to forward the event to the
RenderWidgetHostImpl that is currently "active" or have page focus.

The main frame widget for a single WebContents in a tree of WebContents will be
focused.

Knowing the focused widget is the wrong thing since this might be the widget for
some oopif but in the same WebContents, the single WebContents case is already
handled and works.

[Charlie Reis, 2016/11/16 21:51:19]

Ok.  I think more of this needs to come across in the comment in the code, since
these methods don't sound like they're specific to the inner/outer WebContents
case.  Maybe the idea from web_contents_impl.h of having a larger explanation
for these focus methods would help here as well?

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

+1 to making all relevant comments emphasize that the main use case is for
inner/outer WebContents.

[Charlie Reis, 2016/11/17 00:49:47]

Looks like this was overlooked.

+  virtual RenderWidgetHostImpl* GetRenderWidgetHostWithPageFocus();
+
+  // In cases with multiple RenderWidgetHosts involved in rendering a page, only
+  // one widget should be focused and active. This ensures that
+  // |render_widget_host| is focused and that its owning WebContents is also
+  // focused.
+  virtual void EnsureOwningContentsIsFocused(

[Charlie Reis, 2016/11/16 20:28:23]

We should try to be more direct in the name, since it's not clear what action
"Ensure" is taking.  Would FocusOwningWebContents be accurate?

[avallee, 2016/11/16 21:18:10]

Good idea. Done.

+      RenderWidgetHostImpl* render_widget_host) {}
+
   // Notification that the renderer has become unresponsive. The
   // delegate can use this notification to show a warning to the user.
   virtual void RendererUnresponsive(RenderWidgetHostImpl* render_widget_host,