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 e5e2ac711de61360f88b4ecf6897382afe3ca4b1..bd74bfa7df2f461f331af99c03cf258eeca4053f 100644
--- a/chrome/browser/ui/search/search_ipc_router.cc
+++ b/chrome/browser/ui/search/search_ipc_router.cc
@@ -7,6 +7,8 @@
 #include <utility>
 
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/instant_service.h"
+#include "chrome/browser/search/instant_service_factory.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/common/render_messages.h"
 #include "components/search/search.h"
@@ -19,67 +21,63 @@
 
 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.
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  return search::IsRenderedInInstantProcess(web_contents, profile);
+bool IsInstantProcess(content::RenderFrameHost* render_frame) {

[Marc Treib, 2017/02/13 09:35:56]

nit: IsInInstantProcess maybe?

[tibell, 2017/02/19 23:18:45]

Done.

+  content::RenderProcessHost* process_host = render_frame->GetProcess();
+  const InstantService* instant_service = InstantServiceFactory::GetForProfile(
+      Profile::FromBrowserContext(process_host->GetBrowserContext()));
+  if (!instant_service)
+    return false;
+
+  return instant_service->IsInstantProcess(process_host->GetID());
 }
 
 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_(web_contents),
-        last_connected_rfh_(
-            std::make_pair(content::ChildProcessHost::kInvalidUniqueID,
-                           MSG_ROUTING_NONE)) {}
-  chrome::mojom::SearchBox* GetSearchBox() override;
+  // |web_contents| and |binding| must outlive this object.
+  SearchBoxClientFactoryImpl(
+      content::WebContents* web_contents,
+      mojo::AssociatedBinding<chrome::mojom::Instant>* binding)
+      : binding_(binding), bindings_(web_contents, this) {

[Marc Treib, 2017/02/13 09:35:56]

There's both "binding_" and "bindings_" which I find quite confusing. Can we
find more distinctive names?

[tibell, 2017/02/19 23:18:45]

Done.

+    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 (!IsInstantProcess(bindings_.GetCurrentTargetFrame())) {
+    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 +90,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_;
@@ -183,8 +182,6 @@ void SearchIPCRouter::OnTabDeactivated() {
 
 void SearchIPCRouter::InstantSupportDetermined(int page_seq_no,
                                                bool instant_support) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -192,8 +189,6 @@ void SearchIPCRouter::InstantSupportDetermined(int page_seq_no,
 }
 
 void SearchIPCRouter::FocusOmnibox(int page_seq_no, OmniboxFocusState state) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -205,8 +200,6 @@ void SearchIPCRouter::FocusOmnibox(int page_seq_no, OmniboxFocusState state) {
 }
 
 void SearchIPCRouter::DeleteMostVisitedItem(int page_seq_no, const GURL& url) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -219,8 +212,6 @@ void SearchIPCRouter::DeleteMostVisitedItem(int page_seq_no, const GURL& url) {
 
 void SearchIPCRouter::UndoMostVisitedDeletion(int page_seq_no,
                                               const GURL& url) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -232,8 +223,6 @@ void SearchIPCRouter::UndoMostVisitedDeletion(int page_seq_no,
 }
 
 void SearchIPCRouter::UndoAllMostVisitedDeletions(int page_seq_no) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -247,8 +236,6 @@ void SearchIPCRouter::UndoAllMostVisitedDeletions(int page_seq_no) {
 void SearchIPCRouter::LogEvent(int page_seq_no,
                                NTPLoggingEventType event,
                                base::TimeDelta time) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -263,8 +250,6 @@ void SearchIPCRouter::LogMostVisitedImpression(
     int page_seq_no,
     int position,
     ntp_tiles::NTPTileSource tile_source) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -280,8 +265,6 @@ void SearchIPCRouter::LogMostVisitedNavigation(
     int page_seq_no,
     int position,
     ntp_tiles::NTPTileSource tile_source) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -295,8 +278,6 @@ void SearchIPCRouter::LogMostVisitedNavigation(
 
 void SearchIPCRouter::PasteAndOpenDropdown(int page_seq_no,
                                            const base::string16& text) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -309,8 +290,6 @@ void SearchIPCRouter::PasteAndOpenDropdown(int page_seq_no,
 
 void SearchIPCRouter::ChromeIdentityCheck(int page_seq_no,
                                           const base::string16& identity) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;
 
@@ -322,8 +301,6 @@ void SearchIPCRouter::ChromeIdentityCheck(int page_seq_no,
 }
 
 void SearchIPCRouter::HistorySyncCheck(int page_seq_no) {
-  if (!IsRenderedInInstantProcess(web_contents()))
-    return;
   if (page_seq_no != commit_counter_)
     return;