Service CreateNewWindow on the UI thread with a new mojo interface

content/test/test_render_frame.cc

Index: content/test/test_render_frame.cc
diff --git a/content/test/test_render_frame.cc b/content/test/test_render_frame.cc
index 6c0bb0133b81b4af5f5e4e615a24e6bb8f48f943..a74ab69d2c080c4aee7ec699be58cdea6987fecf 100644
--- a/content/test/test_render_frame.cc
+++ b/content/test/test_render_frame.cc
@@ -4,14 +4,47 @@
 
 #include "content/test/test_render_frame.h"
 
+#include "base/memory/ptr_util.h"
+#include "content/common/frame.mojom.h"
 #include "content/common/navigation_params.h"
 #include "content/common/resource_request_body_impl.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/resource_response.h"
+#include "content/public/test/mock_render_thread.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 
 namespace content {
 
+class MockFrameHostIPC : public mojom::FrameHostIPC {
+ public:
+  MockFrameHostIPC() = default;
+  ~MockFrameHostIPC() override = default;
+
+  void CreateNewWindow(mojom::CreateNewWindowParamsPtr params,
+                       const CreateNewWindowCallback& callback) override {
+    // NOTE: This implementation of mojom::FrameHostIPC is used client-
+    // side only. Because sync mojom methods have a different interface for
+    // bindings- and client-side, we only implement the client-side interface
+    // on this object.
+    NOTREACHED();
+  }
+
+  bool CreateNewWindow(mojom::CreateNewWindowParamsPtr params,
+                       mojom::CreateNewWindowReplyPtr* reply) override {
+    *reply = mojom::CreateNewWindowReply::New();
+
+    // It is not very nice to reach directly into the MockRenderThread here. The
+    // logic to create mock frames/windows should potentially live here.
+    MockRenderThread* mock_render_thread =
+        static_cast<MockRenderThread*>(RenderThread::Get());
+    mock_render_thread->OnCreateWindow(*params, reply->get());

[ncarter (slow), 2017/04/20 21:30:36]

Could we consider having MockRenderThread provide the FrameHost/FrameHostIPC
interfaces naturally, so that the
GetRemoteAssociatedInterfaces()->GetInterface() call in RenderFrameImpl just
works? My thinking is that it's best if the "mock browser process" behaviors are
implemented in terms of the IPC conversation, rather than overrides of real
code.

Having said that, I'm okay with things as you've implemented them --
particularly because I'm not sure if the above suggestion is actually practical.

rockot@ may also have ideas here.

[Charlie Harrison, 2017/04/21 15:30:08]

I kept this here, but vended it via the AssociatedInterfaceProviderImpl (which
required some tweaking). I saw that that was the way password autofilled did it.
LMK what you think

[ncarter (slow), 2017/04/21 20:42:13]

This looks like a better pattern. Thanks!

+    return true;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockFrameHostIPC);
+};
+
 // static
 RenderFrameImpl* TestRenderFrame::CreateTestRenderFrame(
     const RenderFrameImpl::CreateParams& params) {
@@ -20,6 +53,7 @@ RenderFrameImpl* TestRenderFrame::CreateTestRenderFrame(
 
 TestRenderFrame::TestRenderFrame(const RenderFrameImpl::CreateParams& params)
     : RenderFrameImpl(params) {
+  mock_frame_host_ipc_ = base::MakeUnique<MockFrameHostIPC>();
 }
 
 TestRenderFrame::~TestRenderFrame() {
@@ -89,4 +123,8 @@ blink::WebNavigationPolicy TestRenderFrame::DecidePolicyForNavigation(
   return RenderFrameImpl::DecidePolicyForNavigation(info);
 }
 
+mojom::FrameHostIPC* TestRenderFrame::GetFrameHostIPC() {
+  return mock_frame_host_ipc_.get();
+}
+
 }  // namespace content