Service CreateNewWindow on the UI thread with a new mojo interface

content/test/test_render_frame.cc

 // Copyright 2015 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 "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) {
   return new TestRenderFrame(params);
 }
 
 TestRenderFrame::TestRenderFrame(const RenderFrameImpl::CreateParams& params)
     : RenderFrameImpl(params) {
+  mock_frame_host_ipc_ = base::MakeUnique<MockFrameHostIPC>();
 }
 
 TestRenderFrame::~TestRenderFrame() {
 }
 
 void TestRenderFrame::Navigate(const CommonNavigationParams& common_params,
                                const StartNavigationParams& start_params,
                                const RequestNavigationParams& request_params) {
   // PlzNavigate
   if (IsBrowserSideNavigationEnabled()) {
@@ skipping 49 matching lines @@
       info.url_request.CheckForBrowserSideNavigation() &&
       GetWebFrame()->Parent() && info.form.IsNull()) {
     // RenderViewTest::LoadHTML already disables PlzNavigate for the main frame
     // requests. However if the loaded html has a subframe, the WebURLRequest
     // will be created inside Blink and it won't have this flag set.
     info.url_request.SetCheckForBrowserSideNavigation(false);
   }
   return RenderFrameImpl::DecidePolicyForNavigation(info);
 }
 
+mojom::FrameHostIPC* TestRenderFrame::GetFrameHostIPC() {
+  return mock_frame_host_ipc_.get();
+}
+
 }  // namespace content