OOPIF: Send page-level focus messages to all processes rendering a page.

content/browser/frame_host/render_frame_host_manager_unittest.cc

Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index a6d04cde898ee2a6118990c8c7427b528d7cb1ec..996b587e2940e4f2879fb727bf98bb5b21b5cfa1 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -18,6 +18,7 @@
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/frame_messages.h"
+#include "content/common/input_messages.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/notification_details.h"
@@ -2433,4 +2434,101 @@ TEST_F(RenderFrameHostManagerTest, TraverseComplexOpenerChain) {
               nodes_with_back_links.end());
 }
 
+// Check that when a window is focused, the SetFocus message is sent to all
+// RenderWidgets involved in rendering the page, including those for
+// cross-process subframes.
+TEST_F(RenderFrameHostManagerTest, FocusPropagatesToAllRenderWidgets) {
+  // This test only makes sense when cross-site subframes use separate
+  // processes.
+  if (!SiteIsolationPolicy::AreCrossProcessFramesPossible())

[Charlie Reis, 2015/10/09 21:21:51]

I think you want AreAllSitesIsolatedForTesting.  For example, the test probably
fails if we pass --isolate-extensions.

[alexmos, 2015/10/16 16:50:45]

Indeed.  Done.

+    return;
+
+  const GURL kUrlA("http://www.google.com/");
+  const GURL kUrlB("http://chromium.org/");
+
+  // Create a page with two child frames.
+  contents()->NavigateAndCommit(kUrlA);
+  contents()->GetMainFrame()->OnCreateChildFrame(
+      contents()->GetMainFrame()->GetProcess()->GetNextRoutingID(),
+      blink::WebTreeScopeType::Document, "frame1",
+      blink::WebSandboxFlags::None);
+  contents()->GetMainFrame()->OnCreateChildFrame(
+      contents()->GetMainFrame()->GetProcess()->GetNextRoutingID(),
+      blink::WebTreeScopeType::Document, "frame2",
+      blink::WebSandboxFlags::None);
+
+  RenderFrameHostManager* child1 =
+      contents()->GetFrameTree()->root()->child_at(0)->render_manager();
+  RenderFrameHostManager* child2 =
+      contents()->GetFrameTree()->root()->child_at(1)->render_manager();
+
+  // Navigate both frames cross-site.
+  NavigationEntryImpl entry(NULL /* instance */, -1 /* page_id */, kUrlB,

[Charlie Reis, 2015/10/09 21:21:50]

nit: nullptr

[alexmos, 2015/10/16 16:50:44]

Done.

+                            Referrer(kUrlA, blink::WebReferrerPolicyDefault),
+                            base::string16() /* title */,
+                            ui::PAGE_TRANSITION_LINK,
+                            false /* is_renderer_init */);
+  TestRenderFrameHost* host1 =
+      static_cast<TestRenderFrameHost*>(NavigateToEntry(child1, entry));
+  TestRenderFrameHost* host2 =
+      static_cast<TestRenderFrameHost*>(NavigateToEntry(child2, entry));
+  child1->DidNavigateFrame(host1, true);
+  child2->DidNavigateFrame(host2, true);
+
+  // Make sure three RenderWidgetHosts are created.
+  EXPECT_NE(main_test_rfh()->GetRenderWidgetHost(),
+            host1->GetRenderWidgetHost());
+  EXPECT_NE(main_test_rfh()->GetRenderWidgetHost(),
+            host2->GetRenderWidgetHost());
+  EXPECT_NE(host1->GetRenderWidgetHost(), host2->GetRenderWidgetHost());
+
+  // Make sure the two child frames are placed in a distinct process from their
+  // parent.
+  EXPECT_NE(host1->GetProcess(), main_test_rfh()->GetProcess());
+  EXPECT_EQ(host1->GetProcess(), host2->GetProcess());
+
+  // Helper to check that the SetFocus message was sent to main frame's
+  // process and to the two RenderWidgets in the child process.
+  auto verify_focus_messages = [this, host1, host2](bool enable_focus) {

[Charlie Reis, 2015/10/09 21:21:51]

Wow, first time I've come across lambdas in Chrome.  :)

I'm not terribly familiar with C++ lambdas, but I think this is ok style-wise. 
Google style says to keep them short unless they're named, and this is named. 
Chrome style says not to store them, but I think that means as a member.

Ok, works for me.  (Out of curiosity, do you know if we have precedents
elsewhere in the code?  And is the stack understandable if one of these EXPECTs
fails?)

[alexmos, 2015/10/16 16:50:44]

I saw it used in lazyboy's CL
(https://codereview.chromium.org/1307013004/diff/320001/content/browser/site_p...)
and checked out the style guide to see that they're allowed then.  Looking
around codesearch, I saw a few more uses in various tests (a couple of examples:
https://code.google.com/p/chromium/codesearch#search/&q==%5C%20%5C%5B.*%5C%5D...
).  There were a few uses in content/, including in
RenderFrameHostManager::RenderFrameProxyHostMap::Add/Remove, so I figured why
not. :)

Failed EXPECTs show up normally, and crashes just have a
"...FocusPropagatesToAllRenderWidgets_Test::TestBody()::$_0::operator()()" for
the lambda frame.

[Charlie Reis, 2015/10/16 18:56:09]

Acknowledged.

+    const IPC::Message* message =
+        main_test_rfh()->GetProcess()->sink().GetUniqueMessageMatching(
+            InputMsg_SetFocus::ID);
+    EXPECT_TRUE(message);
+    EXPECT_EQ(main_test_rfh()->GetRenderWidgetHost()->GetRoutingID(),
+              message->routing_id());
+    InputMsg_SetFocus::Param params;
+    EXPECT_TRUE(InputMsg_SetFocus::Read(message, &params));
+    EXPECT_EQ(enable_focus, base::get<0>(params));

[Charlie Reis, 2015/10/09 21:21:51]

nit: enable_focus -> focus_expected?

[alexmos, 2015/10/16 16:50:44]

Changed to expected_focus in the updated test.

+
+    EXPECT_EQ(2U, host1->GetProcess()->sink().message_count());
+    std::set<int> routing_ids;
+    for (size_t i = 0; i < host1->GetProcess()->sink().message_count(); ++i) {
+      const IPC::Message* message = host1->GetProcess()->sink().GetMessageAt(i);
+      EXPECT_EQ(InputMsg_SetFocus::ID, message->type());
+      routing_ids.insert(message->routing_id());
+      InputMsg_SetFocus::Param params;
+      EXPECT_TRUE(InputMsg_SetFocus::Read(message, &params));
+      EXPECT_EQ(enable_focus, base::get<0>(params));

[Charlie Reis, 2015/10/09 21:21:51]

I'm puzzled.  Why would we tell the subframes that they have focus if the main
frame was focused?  Or does this message mean something else?

[alexmos, 2015/10/16 16:50:44]

Yes, this message means that the whole page was focused, not the main frame, so
each Page object in the renderer processes needs to find out about that.

[Charlie Reis, 2015/10/16 18:56:09]

Acknowledged.

+    }
+    EXPECT_NE(routing_ids.find(host1->GetRenderWidgetHost()->GetRoutingID()),
+              routing_ids.end());
+    EXPECT_NE(routing_ids.find(host2->GetRenderWidgetHost()->GetRoutingID()),
+              routing_ids.end());
+  };
+
+  // Focus the main page, and verify that the focus message was sent to all
+  // widgets.
+  main_test_rfh()->GetProcess()->sink().ClearMessages();
+  host1->GetProcess()->sink().ClearMessages();
+  main_test_rfh()->GetRenderWidgetHost()->Focus();
+  verify_focus_messages(true);
+
+  // Simulate focus loss on main page, and verify that the focus message was
+  // sent to all widgets.
+  main_test_rfh()->GetProcess()->sink().ClearMessages();
+  host1->GetProcess()->sink().ClearMessages();
+  main_test_rfh()->GetRenderWidgetHost()->Blur();
+  verify_focus_messages(false);
+}

[Charlie Reis, 2015/10/09 21:21:51]

Should we be testing the case where one of the subframes gets focus?  I guess
that wouldn't be able to easily use the lambda, but it seems like we'd want to
verify that the correct frame ended up with focus.

[alexmos, 2015/10/16 16:50:44]

I added a test for some of that in https://codereview.chromium.org/1370013003/,
but as we discussed, this test is for page-level focus, not frame focus.

[Charlie Reis, 2015/10/16 18:56:09]

Acknowledged.

+
 }  // namespace content