NTP: simplify mojo connection setup

chrome/browser/ui/search/search_ipc_router.h

Index: chrome/browser/ui/search/search_ipc_router.h
diff --git a/chrome/browser/ui/search/search_ipc_router.h b/chrome/browser/ui/search/search_ipc_router.h
index 4339824965a9ee0c2b12c51ba55b4e7d28530187..687b1a663d3b8f44bbc4172d4ca67ad771be1f99 100644
--- a/chrome/browser/ui/search/search_ipc_router.h
+++ b/chrome/browser/ui/search/search_ipc_router.h
@@ -239,7 +239,10 @@ class SearchIPCRouter : public content::WebContentsObserver,
   // 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_;