Fix keyboard focus for OOPIF-<webview>.

content/browser/web_contents/web_contents_impl.h

 // 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.
 
 #ifndef CONTENT_BROWSER_WEB_CONTENTS_WEB_CONTENTS_IMPL_H_
 #define CONTENT_BROWSER_WEB_CONTENTS_WEB_CONTENTS_IMPL_H_
 
 #include <stdint.h>
 
 #include <functional>
@@ skipping 445 matching lines @@
   GeolocationServiceContext* GetGeolocationServiceContext() override;
   WakeLockServiceContext* GetWakeLockServiceContext() override;
   void EnterFullscreenMode(const GURL& origin) override;
   void ExitFullscreenMode(bool will_cause_resize) override;
   bool ShouldRouteMessageEvent(
       RenderFrameHost* target_rfh,
       SiteInstance* source_site_instance) const override;
   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;
   bool OnMessageReceived(RenderViewHost* render_view_host,
                          const IPC::Message& message) override;
   // RenderFrameHostDelegate has the same method, so list it there because this
   // interface is going away.
   // WebContents* GetAsWebContents() override;
   void RenderViewCreated(RenderViewHost* render_view_host) override;
   void RenderViewReady(RenderViewHost* render_view_host) override;
@@ skipping 327 matching lines @@
   //
   // Two WebContents with separate FrameTrees can be connected by
   // outer/inner relationship using this class. Note that their FrameTrees
   // still remain disjoint.
   // The parent is referred to as "outer WebContents" and the descendents are
   // referred to as "inner WebContents".
   // For each inner WebContents, the outer WebContents will have a
   // 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.
   WebContentsImpl(BrowserContext* browser_context);
 
   // Add and remove observers for page navigation notifications. The order in
   // which notifications are sent to observers is undefined. Clients must be
   // sure to remove the observer before they go away.
   void AddObserver(WebContentsObserver* observer);
   void RemoveObserver(WebContentsObserver* observer);
@@ skipping 119 matching lines @@
                                const base::string16& main_text,
                                const base::string16& sub_text);
   void OnHideValidationMessage();
   void OnMoveValidationMessage(const gfx::Rect& anchor_in_root_view);
 
   // Called by derived classes to indicate that we're no longer waiting for a
   // response. This won't actually update the throbber, but it will get picked
   // 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().
 
   // Handles post-navigation tasks in DidNavigate AFTER the entry has been
   // committed to the navigation controller. Note that the navigation entry is
   // not provided since it may be invalid/changed after being committed. The
   // current navigation entry is in the NavigationController at this point.
 
   // If our controller was restored, update the max page ID associated with the
@@ skipping 413 matching lines @@
   // Adds/removes a callback called on creation of each new WebContents.
   static void AddCreatedCallbackForTesting(const CreatedCallback& callback);
   static void RemoveCreatedCallbackForTesting(const CreatedCallback& callback);
 
   DISALLOW_COPY_AND_ASSIGN(FriendZone);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_WEB_CONTENTS_WEB_CONTENTS_IMPL_H_