NTP: simplify mojo connection setup

chrome/browser/ui/search/search_ipc_router.cc

Index: chrome/browser/ui/search/search_ipc_router.cc
diff --git a/chrome/browser/ui/search/search_ipc_router.cc b/chrome/browser/ui/search/search_ipc_router.cc
index f3b2e238dee25672b6225e899667d6d04b8377e4..a4ed15de42735f98324427566d13efdec291f8f5 100644
--- a/chrome/browser/ui/search/search_ipc_router.cc
+++ b/chrome/browser/ui/search/search_ipc_router.cc
@@ -20,66 +20,67 @@
 namespace {
 
 bool IsRenderedInInstantProcess(content::WebContents* web_contents) {
-  // TODO(tibell): Instead of rejecting messages check at connection time if we
-  // want to talk to the render frame or not. Later, in an out-of-process iframe
-  // world, the IsRenderedInInstantProcess check will have to be done, as it's
-  // based on the RenderView.
+  // TODO(tibell): Instead of rejecting messages, check at connection time if we
+  // want to talk to the render frame or not. To do that we need a version of
+  // search::IsRenderedInInstantProcess that works on RenderFrameHost*. That
+  // should also work in an out-of-process iframe world. Also tests of message
+  // rejection need to be updated/deleted.
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   return search::IsRenderedInInstantProcess(web_contents, profile);
 }
 
 class SearchBoxClientFactoryImpl
-    : public SearchIPCRouter::SearchBoxClientFactory {
+    : public SearchIPCRouter::SearchBoxClientFactory,
+      public chrome::mojom::EmbeddedSearchConnector {
  public:
-  // The |web_contents| must outlive this object.
-  explicit SearchBoxClientFactoryImpl(content::WebContents* web_contents)
+  // |web_contents| and |binding| must outlive this object.
+  SearchBoxClientFactoryImpl(
+      content::WebContents* web_contents,
+      mojo::AssociatedBinding<chrome::mojom::Instant>* binding)
       : web_contents_(web_contents),
-        last_connected_rfh_(
-            std::make_pair(content::ChildProcessHost::kInvalidUniqueID,
-                           MSG_ROUTING_NONE)) {}
-  chrome::mojom::SearchBox* GetSearchBox() override;
+        binding_(binding),
+        bindings_(web_contents, this) {
+    DCHECK(web_contents);
+    DCHECK(binding);
+    // Before we are connected to a frame we throw away all messages.
+    mojo::GetIsolatedProxy(&search_box_);
+  }
+
+  chrome::mojom::SearchBox* GetSearchBox() override {
+    return search_box_.get();
+  }
 
  private:
-  void OnConnectionError();
+  void Connect(chrome::mojom::InstantAssociatedRequest request,
+               chrome::mojom::SearchBoxAssociatedPtrInfo client) override;
 
   content::WebContents* web_contents_;
+
+  // An interface used to push updates to the frame that connected to us. Before
+  // we've been connected to a frame, messages sent on this interface go into
+  // the void.
   chrome::mojom::SearchBoxAssociatedPtr search_box_;
 
-  // The proccess ID and routing ID of the last connected main frame. May be
-  // (ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE) if we haven't
-  // connected yet.
-  std::pair<int, int> last_connected_rfh_;
+  // Used to bind incoming interface requests to the implementation, which lives
+  // in SearchIPCRouter.
+  mojo::AssociatedBinding<chrome::mojom::Instant>* binding_;
+
+  // Binding used to listen to connection requests.
+  content::WebContentsFrameBindingSet<chrome::mojom::EmbeddedSearchConnector>
+      bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(SearchBoxClientFactoryImpl);
 };
 
-chrome::mojom::SearchBox* SearchBoxClientFactoryImpl::GetSearchBox() {
-  content::RenderFrameHost* frame = web_contents_->GetMainFrame();
-  auto id = std::make_pair(frame->GetProcess()->GetID(), frame->GetRoutingID());
-  // Avoid reconnecting repeatedly to the same RenderFrameHost for performance
-  // reasons.
-  if (id != last_connected_rfh_) {
-    if (IsRenderedInInstantProcess(web_contents_)) {
-      frame->GetRemoteAssociatedInterfaces()->GetInterface(&search_box_);
-      search_box_.set_connection_error_handler(
-          base::Bind(&SearchBoxClientFactoryImpl::OnConnectionError,
-                     base::Unretained(this)));
-    } else {
-      // Renderer is not an instant process. We'll create a connection that
-      // drops all messages.
-      mojo::GetIsolatedProxy(&search_box_);
-    }
-    last_connected_rfh_ = id;
+void SearchBoxClientFactoryImpl::Connect(
+    chrome::mojom::InstantAssociatedRequest request,
+    chrome::mojom::SearchBoxAssociatedPtrInfo client) {
+  if (!IsRenderedInInstantProcess(web_contents_)) {

[dcheng, 2017/02/10 10:24:14]

One thing I was wondering... once a renderer process is an instant process, it's
always an instant process, right? Thus, if we guarantee that we swap renderers
when loading an instant page, we don't need to manually close the pipes on
navigation. Is this true today?

Another thought is if it's OK that this interface method can be reached by a
non-instant process... I think that should be OK, given that we filter like
this.

[tibell, 2017/02/12 23:15:26]

A renderer should stay the same process type during its lifetime, so no need to
manually close its connection. This is true today AFAIK.

It's fine if non-instant processes try to connect here, give this filtering.

As I explained in the very long comment to one of Marc's other questions,
ideally someone would change InstantService to expose a
IsRenderedInInstantProcess(RenderFrame*). Having that and putting in some
efforts in rewriting the tests this code could be nicer (i.e. we'd *only* check
process type on connection but not on each message received.)

+    return;
   }
-  return search_box_.get();
-}
-
-void SearchBoxClientFactoryImpl::OnConnectionError() {
-  search_box_.reset();
-  last_connected_rfh_ = std::make_pair(
-      content::ChildProcessHost::kInvalidUniqueID,
-      MSG_ROUTING_NONE);
+  binding_->Bind(std::move(request));
+  search_box_.Bind(std::move(client));
 }
 
 }  // namespace
@@ -92,14 +93,15 @@ SearchIPCRouter::SearchIPCRouter(content::WebContents* web_contents,
       policy_(std::move(policy)),
       commit_counter_(0),
       is_active_tab_(false),
-      bindings_(web_contents, this),
-      search_box_client_factory_(new SearchBoxClientFactoryImpl(web_contents)) {
+      binding_(this),
+      search_box_client_factory_(
+          new SearchBoxClientFactoryImpl(web_contents, &binding_)) {
   DCHECK(web_contents);
   DCHECK(delegate);
   DCHECK(policy_.get());
 }
 
-SearchIPCRouter::~SearchIPCRouter() {}
+SearchIPCRouter::~SearchIPCRouter() = default;
 
 void SearchIPCRouter::OnNavigationEntryCommitted() {
   ++commit_counter_;