NTP: simplify mojo connection setup

chrome/browser/ui/search/search_ipc_router.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 "chrome/browser/ui/search/search_ipc_router.h"
 
 #include <utility>
 
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/common/render_messages.h"
 #include "components/search/search.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/associated_interface_provider.h"
 #include "content/public/common/child_process_host.h"
 
 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.

[Marc Treib, 2017/02/08 10:00:36]

Does this describe a possible optimization that would have to be undone in an
OOPIF world? In that case, I wouldn't even call it a TODO.

Also, note that the IsRenderedInInstantProcess state of a WebContents can change
during its lifetime; not sure if that matters here.

[tibell, 2017/02/09 03:35:28]

I've updated the comment to what I think we should do in the future (including
in an OOPIF world). Basically we want a
search::IsRenderedInInstantProcess(RenderFrameHost*) method to check the current
frame. The process type of frame should change over its lifetime even under
OOPIF (right?).

   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   return search::IsRenderedInInstantProcess(web_contents, profile);
 }
 
 class SearchBoxClientFactoryImpl
-    : public SearchIPCRouter::SearchBoxClientFactory {
+    : public SearchIPCRouter::SearchBoxClientFactory,
+      public chrome::mojom::NTPConnector {
  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 search_box) 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 goes into

[Marc Treib, 2017/02/08 10:00:36]

nit: s/goes/go/

[tibell, 2017/02/09 03:35:28]

Done.

+  // 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 incomming interface requests to the implementation, which

[dcheng, 2017/02/08 09:01:39]

Nit: incomming -> incoming

[tibell, 2017/02/09 03:35:28]

Done.

+  // lives in SearchIPCRouter.
+  mojo::AssociatedBinding<chrome::mojom::Instant>* binding_;
+
+  // Binding used to listen to connection requests.
+  content::WebContentsFrameBindingSet<chrome::mojom::NTPConnector> 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 search_box) {
+  if (!IsRenderedInInstantProcess(web_contents_)) {
+    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(search_box));
 }
 
 }  // namespace
 
 SearchIPCRouter::SearchIPCRouter(content::WebContents* web_contents,
                                  Delegate* delegate,
                                  std::unique_ptr<Policy> policy)
     : WebContentsObserver(web_contents),
       delegate_(delegate),
       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_;
   search_box()->SetPageSequenceNumber(commit_counter_);
 }
 
 void SearchIPCRouter::DetermineIfPageSupportsInstant() {
   search_box()->DetermineIfPageSupportsInstant();
 }
 
@@ skipping 224 matching lines @@
 
 void SearchIPCRouter::set_delegate_for_testing(Delegate* delegate) {
   DCHECK(delegate);
   delegate_ = delegate;
 }
 
 void SearchIPCRouter::set_policy_for_testing(std::unique_ptr<Policy> policy) {
   DCHECK(policy);
   policy_ = std::move(policy);
 }