NTP: simplify mojo connection setup

chrome/browser/ui/search/search_ipc_router.h

 // 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.
 
 #ifndef CHROME_BROWSER_UI_SEARCH_SEARCH_IPC_ROUTER_H_
 #define CHROME_BROWSER_UI_SEARCH_SEARCH_IPC_ROUTER_H_
 
 #include <memory>
 #include <vector>
 
@@ skipping 221 matching lines @@
   Delegate* delegate_;
   std::unique_ptr<Policy> policy_;
 
   // Holds the number of main frame commits executed in this tab. Used by the
   // SearchIPCRouter to ensure that delayed IPC replies are ignored.
   int commit_counter_;
 
   // Set to true, when the tab corresponding to |this| instance is active.
   bool is_active_tab_;
 
-  content::WebContentsFrameBindingSet<chrome::mojom::Instant> bindings_;
+  // Binding for the connected main frame. We only allow one frame to connect at
+  // the moment, but this could be extended to a map of connected frames, if
+  // desired.

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

I don't really understand what this means.
The NTP has two frames (main frame + iframe), both of which need the
embeddedSearch APIs. Does this CL break that?

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

tl;dr: this CL doesn't change any behavior, just how we set up the connections
that I established in my initial CL.

Ellaboration:

Before my initial conversion:
 * Renderer state was stored in SearchBox. The state was updated through
messages sent from SearchIPCRouter.
 * searchbox_extension.cc would look up the SearchBox through the RenderView and
get the state it needed. Both frames would look up the same SearchBox.

My initial CL:
 * Renderer state is still stored in SearchBox.
 * Changed SearchBox from a RenderViewObserver to a RenderFrameObserver and only
installed that observer on the main frame. This change was necessitated by Mojo
not working with RenderViews (because they're supposed to go away).
 * searchbox_extension.cc would instead look up the SearchBox by finding the
main frame (using a changed SearchBoxExtensionWrapper::GetRenderFrame) and get
the state it needs. This will have to be changed if we ever have the two frames
run in different processes under OOPIF.

This CL:
 * Nothing changes w.r.t. how the renderer gets to the state. The state is still
on the main frame (in SearchBox). It's the connection from that main frame
that's stored in |binding_|.

What we could do in the future:

Option A:
 * If duplicating the state on the renderer side is not a problem we could a)
have one SearchBox per frame and b) have each frame connect to SearchIPCRouter.
This would require changing |binding_| to an AssociatedBindingSet and replace
search_box_ with e.g. a vector of InterfacePtrs to update when the browser state
changes.

Option B:
 * If we ever get a new abstraction for tabs on the renderer side (i.e. a new
RenderView): have a Mojo connection between WebContentsObserver and this tab
representation and then keep the renderer state on a per-tab basis again.

Option C:
 * Don't store state in the renderer and instead just ask the browser for it
when needed. I don't know if this is possible e.g. it won't work if the
embeddedSearch JS APIs require synchronous access to the data.

[Marc Treib, 2017/02/09 09:30:47]

Thanks for the detailed explanation!

On 2017/02/09 03:35:28, tibell wrote:
This sounds like it would fit well with OOPIF.
If that new RenderView happens, then this sound like it'd be simpler than a).
They do have a bunch of synchronous getters, so I think this option is out.

+  mojo::AssociatedBinding<chrome::mojom::Instant> binding_;
 
   std::unique_ptr<SearchBoxClientFactory> search_box_client_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SearchIPCRouter);
 };
 
 #endif  // CHROME_BROWSER_UI_SEARCH_SEARCH_IPC_ROUTER_H_