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

content/browser/frame_host/render_frame_host_manager_unittest.cc

 // Copyright 2013 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.
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/histogram_tester.h"
 #include "base/time/time.h"
 #include "content/browser/compositor/test/no_transport_image_transport_factory.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_manager.h"
 #include "content/browser/frame_host/render_frame_proxy_host.h"
 #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"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
@@ skipping 2395 matching lines @@
   EXPECT_EQ(tree3, opener_frame_trees[2]);
   EXPECT_EQ(tree4, opener_frame_trees[3]);
 
   EXPECT_EQ(2U, nodes_with_back_links.size());
   EXPECT_TRUE(nodes_with_back_links.find(root1->child_at(1)) !=
               nodes_with_back_links.end());
   EXPECT_TRUE(nodes_with_back_links.find(root4->child_at(0)) !=
               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