Fix keyboard focus for OOPIF-<webview>.

content/browser/web_contents/web_contents_impl.h

Index: content/browser/web_contents/web_contents_impl.h
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index 645b6c7408ed7b6b940cf0b0a7577a276a0b4937..024b678b53a52c468d134dff1244b302b22dbfc4 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -463,6 +463,7 @@ class CONTENT_EXPORT WebContentsImpl
   void EnsureOpenerProxiesExist(RenderFrameHost* source_rfh) override;
   std::unique_ptr<WebUIImpl> CreateWebUIForRenderFrameHost(
       const GURL& url) override;
+  void SetFocusedFrame(FrameTreeNode* node, SiteInstance* source) override;
 
   // RenderViewHostDelegate ----------------------------------------------------
   RenderViewHostDelegateView* GetDelegateView() override;
@@ -810,26 +811,36 @@ private:
   // corresponding FrameTreeNode.
   struct WebContentsTreeNode {
    public:
-    WebContentsTreeNode();
+    WebContentsTreeNode(WebContentsImpl* inner);

[alexmos, 2016/05/12 20:28:42]

nit: explicit?

[avallee, 2016/05/16 20:26:44]

Done.

     ~WebContentsTreeNode();
 
-    typedef std::set<WebContentsTreeNode*> ChildrenSet;
+    typedef std::unordered_map<int, WebContentsTreeNode*> ChildrenMap;
 
     void ConnectToOuterWebContents(WebContentsImpl* outer_web_contents,
                                    RenderFrameHostImpl* outer_contents_frame);
 
-    WebContentsImpl* outer_web_contents() { return outer_web_contents_; }
-    int outer_contents_frame_tree_node_id() {
+    WebContentsImpl* outer_web_contents() const { return outer_web_contents_; }
+    int outer_contents_frame_tree_node_id() const {
       return outer_contents_frame_tree_node_id_;
     }
 
+    WebContentsImpl* FindContentsForFrame(int frame_id) {

[alexmos, 2016/05/12 20:28:42]

This seems to be a FrameTreeNode id, so I'd rename it to frame_tree_node_id to
make this clear.  Also, maybe even just take a FrameTreeNode* and make the use
of FTN ID an internal detail?

[alexmos, 2016/05/12 20:28:43]

It might be better to mention "Inner" in the name explicitly, since this only
searches for an inner WebContents, right?  So, FindInnerWebContents or
GetInnerWebContents?

[avallee, 2016/05/16 20:26:44]

Did something in that line.

+      return const_cast<WebContentsImpl*>(
+          static_cast<const WebContentsTreeNode*>(this)->FindContentsForFrame(

[alexmos, 2016/05/12 20:28:43]

This doesn't look like it should be inlined (I think usually only trivial
getters/setters are inlined).

[avallee, 2016/05/16 20:26:44]

This is trivial, it compiles down to calling the const version. Casting has no
runtime cost.

+              frame_id));
+    }
+    const WebContentsImpl* FindContentsForFrame(int frame_id) const;

[alexmos, 2016/05/12 20:28:42]

Are two declarations for this really necessary?

[avallee, 2016/05/16 20:26:44]

Find frame makes sense as a const function, but should not return a non-const
pointer to the parent which could get me back with a non-const pointer to this.
Without an explicit const_cast.

This is in the spirit of Effective C++, item 3: Use const whenever possible.
(This pattern is on page 23 of the third edition)

[alexmos, 2016/05/19 00:08:09]

I wasn't sure myself, so I ran this by nick@ and I'm passing on his response.
There is a convention regarding const in the content API: 
"don't add the const identifier to interfaces. For interfaces implemented by the
embedder, we can't make assumptions about what the embedder needs to implement
it. For interfaces implemented by content, the implementation details doesn't
have to be exposed."
(from https://www.chromium.org/developers/content-module/content-api)

So, WebContents isn't supposed to be const-correct, so a function that returns a
const WebContentsImpl isn't very meaningful.  Unfortunately, lots of WebContents
methods are const, which disagrees with the theory and ideally would be cleaned
up -- but nick@ apparently doesn't hate it enough to go rip out const. :)

[avallee, 2016/05/24 20:07:07]

Ack.

So even though we implement WebContents, and apparently an embedder wouldn't be
expected to implement this, the interface is still not supposed to have const
methods? That's fine then. Const correctness is a nice thing for reasoning about
things unfortunately.

+
    private:
-    // The outer WebContents.
+    // The web contents that owns this node

[alexmos, 2016/05/12 20:28:43]

nit: s/web contents/WebContents/
nit: period at the end. (also below)

[avallee, 2016/05/16 20:26:44]

Done.

+    WebContentsImpl* const inner_web_contents_;

[alexmos, 2016/05/12 20:28:43]

I found the naming of this confusing.  It sounds similar to
inner_web_contents_tree_nodes_, which is used for something very different.  In
addition, it means the wrong thing for the topmost WebContents, which isn't an
inner WebContents for anything.  Given that this is just the WebContents for
this node, maybe just name it web_contents_?

[avallee, 2016/05/16 20:26:44]

Agreed, done.

+

[alexmos, 2016/05/12 20:28:42]

nit: either no blank line here, or blank lines separating all of these private
fields for consistency.

[avallee, 2016/05/16 20:26:44]

Done.

+    // The outer WebContents
     WebContentsImpl* outer_web_contents_;
     // The ID of the FrameTreeNode in outer WebContents that is hosting us.
     int outer_contents_frame_tree_node_id_;
     // List of inner WebContents that we host.
-    ChildrenSet inner_web_contents_tree_nodes_;
+    ChildrenMap inner_web_contents_tree_nodes_;
   };
 
   // See WebContents::Create for a description of these parameters.
@@ -969,6 +980,12 @@ private:
   // up at the next animation step if the throbber is going.
   void SetNotWaitingForResponse() { waiting_for_response_ = false; }
 
+  // Called by SetFocusedFrame. Split from SetFocusedFrame to prevent mutual
+  // recursion from inner to outer web contents.
+  void SetFocusedFrameInternal(FrameTreeNode* node,
+                               SiteInstance* source,
+                               bool can_visit_outer_contents);
+
   // Navigation helpers --------------------------------------------------------
   //
   // These functions are helpers for Navigate() and DidNavigate().