Adds routed interface support between RenderFrameHost and RenderFrame

Adds associated interface support between RenderFrameHost and RenderFrame.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to serve as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f62002a08f3025d4d9fe488c0986369ae94e1f10
Cr-Commit-Position: refs/heads/master@{#418717}

[Ken Rockot(use gerrit already), 2016-09-02 19:55:07]

Description was changed from

==========
Adds routed interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces via a primordial
RouteProvider interface on RenderProcessHost and RenderThread.
RenderFrameHost and RenderFrame can make use of this to expose
per-route interface bindings.

Adds WebContentsInterfaceRegistry as a helper class for managing
these bindings on the browser side. WebContentsInterfaceRegistry
allows one to bind arbitrarily many interface implementations
to every RenderFrameHost belonging to a WebContents, and
subsequently allows the interface implementations to determine
which frame the request came from.

This support is used in the follow-up CL:
[TODO: link]

BUG=612500
==========

to

==========
Adds routed interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces via a primordial
RouteProvider interface on RenderProcessHost and RenderThread.
RenderFrameHost and RenderFrame can make use of this to expose
per-route interface bindings.

Adds WebContentsInterfaceRegistry as a helper class for managing
these bindings on the browser side. WebContentsInterfaceRegistry
allows one to bind arbitrarily many interface implementations
to every RenderFrameHost belonging to a WebContents, and
subsequently allows the interface implementations to determine
which frame the request came from.

This support is used in the follow-up CL:
[TODO: link]

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-09-02 19:55:12]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 19:55:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-09-02 20:01:49]

Description was changed from

==========
Adds routed interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces via a primordial
RouteProvider interface on RenderProcessHost and RenderThread.
RenderFrameHost and RenderFrame can make use of this to expose
per-route interface bindings.

Adds WebContentsInterfaceRegistry as a helper class for managing
these bindings on the browser side. WebContentsInterfaceRegistry
allows one to bind arbitrarily many interface implementations
to every RenderFrameHost belonging to a WebContents, and
subsequently allows the interface implementations to determine
which frame the request came from.

This support is used in the follow-up CL:
[TODO: link]

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adds routed interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces via a primordial
RouteProvider interface on RenderProcessHost and RenderThread.
RenderFrameHost and RenderFrame can make use of this to expose
per-route interface bindings.

Adds WebContentsInterfaceRegistry as a helper class for managing
these bindings on the browser side. WebContentsInterfaceRegistry
allows one to bind arbitrarily many interface implementations
to every RenderFrameHost belonging to a WebContents, and
subsequently allows the interface implementations to determine
which frame the request came from.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2301123004
  2. https://codereview.chromium.org/2309513002
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-09-02 20:12:29]

rockot@chromium.org changed reviewers:
+ jam@chromium.org

[Ken Rockot(use gerrit already), 2016-09-02 20:12:30]

Apologies for the size. I could split it into two patches, one for host->frame
and one for frame->host, but they'll still be pretty large anyway and both sides
look similar.

The followup CL (see description) uses WebContentsInterfaceRegistry and
RemoteFrameInterface<T>.

[commit-bot: I haz the power, 2016-09-02 20:53:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 20:53:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tibell, 2016-09-05 00:32:13]

tibell@chromium.org changed reviewers:
+ tibell@chromium.org

[tibell, 2016-09-05 00:32:13]

https://codereview.chromium.org/2310563002/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2310563002/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:167: virtual void AddRoutedInterface(
It's a bit unfortunate that we couldn't instead have

    virtual shell::InterfaceRegistry* GetRoutedInterfaceRegistry() = 0;

and thus keep the interface more uniform (i.e. not have RenderFrame act like an
InterfaceRegistry itself. I guess that would require making InterfaceRegistry
entirely virtual.

Just a thought.

[Ken Rockot(use gerrit already), 2016-09-05 00:46:05]

Note that InterfaceRegistry's interface doesn't support associated
interfaces anyway, and long-term I don't think we necessarily want a
generic associated interface registry of any kind anywhere.

On Sep 4, 2016 5:32 PM, <tibell@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/2310563002/diff/1/content/public/renderer/ren...
> File content/public/renderer/render_frame.h (right):
>
>
https://codereview.chromium.org/2310563002/diff/1/content/public/renderer/ren...
> content/public/renderer/render_frame.h:167: virtual void
> AddRoutedInterface(
> It's a bit unfortunate that we couldn't instead have
>
> virtual shell::InterfaceRegistry* GetRoutedInterfaceRegistry() = 0;
>
> and thus keep the interface more uniform (i.e. not have RenderFrame act
> like an InterfaceRegistry itself. I guess that would require making
> InterfaceRegistry entirely virtual.
>
> Just a thought.
>
> https://codereview.chromium.org/2310563002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-07 00:37:01]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 00:37:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-09-07 00:50:42]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 00:50:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 01:28:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 01:29:00]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sam McNally, 2016-09-07 01:39:33]

On 2016/09/05 00:46:05, Ken Rockot wrote:
> Note that InterfaceRegistry's interface doesn't support associated
> interfaces anyway, and long-term I don't think we necessarily want a
> generic associated interface registry of any kind anywhere.

It seems odd to add an inconsistent way to create connections between the
browser and renderer because we want it to go away one day. This feels like
unnecessary API surface in RenderFrame(Host) compared to something like
GetAssociatedRemoteInterfaces() and GetAssociatedInterfaceRegistry().

> 
> On Sep 4, 2016 5:32 PM, <mailto:tibell@chromium.org> wrote:
> >
> >
> >
>
https://codereview.chromium.org/2310563002/diff/1/content/public/renderer/ren...
> > File content/public/renderer/render_frame.h (right):
> >
> >
>
https://codereview.chromium.org/2310563002/diff/1/content/public/renderer/ren...
> > content/public/renderer/render_frame.h:167: virtual void
> > AddRoutedInterface(
> > It's a bit unfortunate that we couldn't instead have
> >
> > virtual shell::InterfaceRegistry* GetRoutedInterfaceRegistry() = 0;
> >
> > and thus keep the interface more uniform (i.e. not have RenderFrame act
> > like an InterfaceRegistry itself. I guess that would require making
> > InterfaceRegistry entirely virtual.
> >
> > Just a thought.
> >
> > https://codereview.chromium.org/2310563002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-07 01:50:11]

On Tue, Sep 6, 2016 at 6:39 PM, <sammc@chromium.org> wrote:

> On 2016/09/05 00:46:05, Ken Rockot wrote:
> > Note that InterfaceRegistry's interface doesn't support associated
> > interfaces anyway, and long-term I don't think we necessarily want a
> > generic associated interface registry of any kind anywhere.
>
> It seems odd to add an inconsistent way to create connections between the
> browser and renderer because we want it to go away one day. This feels like
> unnecessary API surface in RenderFrame(Host) compared to something like
> GetAssociatedRemoteInterfaces() and GetAssociatedInterfaceRegistry().
>

I agree that it seems odd, but it's not clear to me what alternatives we
have here.

If you're suggesting GetAssociatedInterfaceRegistry or something similar, I
refer back to what I've already said: a concept like an "associated
interface registry" is not something we should ever want to have long-term.


>
>
> >
> > On Sep 4, 2016 5:32 PM, <mailto:tibell@chromium.org> wrote:
> > >
> > >
> > >
> >
> https://codereview.chromium.org/2310563002/diff/1/content/
> public/renderer/render_frame.h
> > > File content/public/renderer/render_frame.h (right):
> > >
> > >
> >
> https://codereview.chromium.org/2310563002/diff/1/content/
> public/renderer/render_frame.h#newcode167
> > > content/public/renderer/render_frame.h:167: virtual void
> > > AddRoutedInterface(
> > > It's a bit unfortunate that we couldn't instead have
> > >
> > > virtual shell::InterfaceRegistry* GetRoutedInterfaceRegistry() = 0;
> > >
> > > and thus keep the interface more uniform (i.e. not have RenderFrame act
> > > like an InterfaceRegistry itself. I guess that would require making
> > > InterfaceRegistry entirely virtual.
> > >
> > > Just a thought.
> > >
> > > https://codereview.chromium.org/2310563002/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2310563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tibell, 2016-09-07 07:30:06]

There are quite a few new types/concepts here so I tried to rationalize them for
myself:

RenderFrameHost/RenderProcessHost: These host can now act as an interface
registry/provider of sorts (with their new AddRoutedInterface,
GetRemoteRoutedInterface, etc methods). This is like a normal registry/provider,
but with the routing ID bound.
RenderFrame/RenderThread: Analogously to the above, these two now also act as
interface registries/providers.
WebContentsInterfaceRegistry: A new interface registry that uses the registry
functionality now exposed on RenderFrameHost/RenderProcessHost.
RemoteFrameInterface: Lazily creates and caches AssociatedInterfacePtrs per
frame.
WebContentsFrameInterfaceBinding: Helps tie the interface impl lifetime to a
WebContents.

Is that right?

[Sam McNally, 2016-09-07 07:39:05]

On 2016/09/07 01:50:11, Ken Rockot wrote:
> On Tue, Sep 6, 2016 at 6:39 PM, <mailto:sammc@chromium.org> wrote:
> 
> > On 2016/09/05 00:46:05, Ken Rockot wrote:
> > > Note that InterfaceRegistry's interface doesn't support associated
> > > interfaces anyway, and long-term I don't think we necessarily want a
> > > generic associated interface registry of any kind anywhere.
> >
> > It seems odd to add an inconsistent way to create connections between the
> > browser and renderer because we want it to go away one day. This feels like
> > unnecessary API surface in RenderFrame(Host) compared to something like
> > GetAssociatedRemoteInterfaces() and GetAssociatedInterfaceRegistry().
> >
> 
> I agree that it seems odd, but it's not clear to me what alternatives we
> have here.
> 
> If you're suggesting GetAssociatedInterfaceRegistry or something similar, I
> refer back to what I've already said: a concept like an "associated
> interface registry" is not something we should ever want to have long-term.

Are you OK with the extra methods on RenderFrame(Host) remaining long-term?
They're effectively inline AssociatedInterface{Provider,Registry} interfaces.
> 
> 
> >
> >
> > >
> > > On Sep 4, 2016 5:32 PM, <mailto:tibell@chromium.org> wrote:
> > > >
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2310563002/diff/1/content/
> > public/renderer/render_frame.h
> > > > File content/public/renderer/render_frame.h (right):
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2310563002/diff/1/content/
> > public/renderer/render_frame.h#newcode167
> > > > content/public/renderer/render_frame.h:167: virtual void
> > > > AddRoutedInterface(
> > > > It's a bit unfortunate that we couldn't instead have
> > > >
> > > > virtual shell::InterfaceRegistry* GetRoutedInterfaceRegistry() = 0;
> > > >
> > > > and thus keep the interface more uniform (i.e. not have RenderFrame act
> > > > like an InterfaceRegistry itself. I guess that would require making
> > > > InterfaceRegistry entirely virtual.
> > > >
> > > > Just a thought.
> > > >
> > > > https://codereview.chromium.org/2310563002/
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/2310563002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-07 13:42:13]

On 2016/09/07 at 07:30:06, tibell wrote:
> There are quite a few new types/concepts here so I tried to rationalize them
for myself:
> 
> RenderFrameHost/RenderProcessHost: These host can now act as an interface
registry/provider of sorts (with their new AddRoutedInterface,
GetRemoteRoutedInterface, etc methods). This is like a normal registry/provider,
but with the routing ID bound.
> RenderFrame/RenderThread: Analogously to the above, these two now also act as
interface registries/providers.
> WebContentsInterfaceRegistry: A new interface registry that uses the registry
functionality now exposed on RenderFrameHost/RenderProcessHost.
> RemoteFrameInterface: Lazily creates and caches AssociatedInterfacePtrs per
frame.
> WebContentsFrameInterfaceBinding: Helps tie the interface impl lifetime to a
WebContents.
> 
> Is that right?

Yes, all correct.

[Ken Rockot(use gerrit already), 2016-09-07 13:44:31]

On 2016/09/07 at 07:39:05, sammc wrote:
> On 2016/09/07 01:50:11, Ken Rockot wrote:
> > On Tue, Sep 6, 2016 at 6:39 PM, <mailto:sammc@chromium.org> wrote:
> > 
> > > On 2016/09/05 00:46:05, Ken Rockot wrote:
> > > > Note that InterfaceRegistry's interface doesn't support associated
> > > > interfaces anyway, and long-term I don't think we necessarily want a
> > > > generic associated interface registry of any kind anywhere.
> > >
> > > It seems odd to add an inconsistent way to create connections between the
> > > browser and renderer because we want it to go away one day. This feels
like
> > > unnecessary API surface in RenderFrame(Host) compared to something like
> > > GetAssociatedRemoteInterfaces() and GetAssociatedInterfaceRegistry().
> > >
> > 
> > I agree that it seems odd, but it's not clear to me what alternatives we
> > have here.
> > 
> > If you're suggesting GetAssociatedInterfaceRegistry or something similar, I
> > refer back to what I've already said: a concept like an "associated
> > interface registry" is not something we should ever want to have long-term.
> 
> Are you OK with the extra methods on RenderFrame(Host) remaining long-term?
They're effectively inline AssociatedInterface{Provider,Registry} interfaces.

No, not at all OK with them remaining long-term. I'm sure we will port other
interfaces on different kinds of routes (i.e. non-frame routes), so maybe a
shared abstraction would make sense. I thought I'd start here though to see what
it looks like.

> > 
> > 
> > >
> > >
> > > >
> > > > On Sep 4, 2016 5:32 PM, <mailto:tibell@chromium.org> wrote:
> > > > >
> > > > >
> > > > >
> > > >
> > > https://codereview.chromium.org/2310563002/diff/1/content/
> > > public/renderer/render_frame.h
> > > > > File content/public/renderer/render_frame.h (right):
> > > > >
> > > > >
> > > >
> > > https://codereview.chromium.org/2310563002/diff/1/content/
> > > public/renderer/render_frame.h#newcode167
> > > > > content/public/renderer/render_frame.h:167: virtual void
> > > > > AddRoutedInterface(
> > > > > It's a bit unfortunate that we couldn't instead have
> > > > >
> > > > > virtual shell::InterfaceRegistry* GetRoutedInterfaceRegistry() = 0;
> > > > >
> > > > > and thus keep the interface more uniform (i.e. not have RenderFrame
act
> > > > > like an InterfaceRegistry itself. I guess that would require making
> > > > > InterfaceRegistry entirely virtual.
> > > > >
> > > > > Just a thought.
> > > > >
> > > > > https://codereview.chromium.org/2310563002/
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2310563002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-07 22:46:09]

Description was changed from

==========
Adds routed interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces via a primordial
RouteProvider interface on RenderProcessHost and RenderThread.
RenderFrameHost and RenderFrame can make use of this to expose
per-route interface bindings.

Adds WebContentsInterfaceRegistry as a helper class for managing
these bindings on the browser side. WebContentsInterfaceRegistry
allows one to bind arbitrarily many interface implementations
to every RenderFrameHost belonging to a WebContents, and
subsequently allows the interface implementations to determine
which frame the request came from.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2301123004
  2. https://codereview.chromium.org/2309513002
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adds routed interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces via a primordial
RouteProvider interface on RenderProcessHost and RenderThread.
RenderFrameHost and RenderFrame can make use of this to expose
per-route interface bindings.

Adds WebContentsInterfaceRegistry as a helper class for managing
these bindings on the browser side. WebContentsInterfaceRegistry
allows one to bind arbitrarily many interface implementations
to every RenderFrameHost belonging to a WebContents, and
subsequently allows the interface implementations to determine
which frame the request came from.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-09-08 19:34:24]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 19:35:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 21:48:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 21:48:22]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-09-08 22:50:46]

Please take another look if you get a chance before our chat today.

I've introduced AssociatedInterfaceRegistry and AssociatedInterfaceProvider in
the public content API, and the changes to public RPH and ChildThread interfaces
are reduced to a single new call for GetRemoteRouteProvider().

RFH and RF both get GetAssociatedInterfaceRegistry() and
GetRemoteAssociatedInterfaces().

[tibell, 2016-09-09 04:05:51]

lgtm

[Sam McNally, 2016-09-09 05:33:15]

LGTM

https://codereview.chromium.org/2310563002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2310563002/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:847: while
(!queued_messages_.empty())
queued_messages_.clear();

https://codereview.chromium.org/2310563002/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:976: // Push any
pending messages to the channel.now. Note that if the child
s/channel.now/channel now/

https://codereview.chromium.org/2310563002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_interface_registry_impl.cc
(right):

https://codereview.chromium.org/2310563002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_interface_registry_impl.cc:22:
RemoveInterfacesFromFrame(frame);
Is this necessary after WebContentsDestroyed() is called?

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/render_frame_host.h:18: #include
"mojo/public/cpp/bindings/associated_interface_ptr.h"
I don't think these extra includes are needed anymore.

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/render_process_host.h:272: virtual mojom::RouteProvider*
GetRemoteRouteProvider() = 0;
Does this need to be public?

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
File content/public/browser/web_contents_frame_interface_binding.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/web_contents_frame_interface_binding.h:32: void
AddToWebContents(WebContents* web_contents) {
This could be clearer that an instance can be added to at most one WebContents.
Maybe SetWebContents()/ClearWebContents().

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/web_contents_frame_interface_binding.h:44: }
Please add a current RenderFrameHost accessor. It should be able to pull it from
|weak_registry_|.

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
File content/public/browser/web_contents_interface_registry.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/web_contents_interface_registry.h:45: // Incoming
messages on Interface are routed to |impl|, and during the extent
impl?

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/web_contents_interface_registry.h:50: void
AddFrameInterface(Interface* iface) {
What's wrong with interface?

https://codereview.chromium.org/2310563002/diff/120001/content/public/child/c...
File content/public/child/child_thread.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/child/c...
content/public/child/child_thread.h:85: virtual mojom::RouteProvider*
GetRemoteRouteProvider() = 0;
Does this need to be public?

https://codereview.chromium.org/2310563002/diff/120001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:16: #include
"content/public/renderer/render_thread.h"
These extra includes aren't needed anymore.

https://codereview.chromium.org/2310563002/diff/120001/ipc/message_router.h
File ipc/message_router.h (right):

https://codereview.chromium.org/2310563002/diff/120001/ipc/message_router.h#n...
ipc/message_router.h:62: // Returns the Listener associated with |routing_id|/.
Extra /

[Ken Rockot(use gerrit already), 2016-09-09 14:58:03]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 14:58:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-09-09 15:02:34]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 15:02:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-09-09 15:02:57]

Patchset #8 (id:140001) has been deleted

[Ken Rockot(use gerrit already), 2016-09-09 15:06:47]

Description was changed from

==========
Adds routed interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces via a primordial
RouteProvider interface on RenderProcessHost and RenderThread.
RenderFrameHost and RenderFrame can make use of this to expose
per-route interface bindings.

Adds WebContentsInterfaceRegistry as a helper class for managing
these bindings on the browser side. WebContentsInterfaceRegistry
allows one to bind arbitrarily many interface implementations
to every RenderFrameHost belonging to a WebContents, and
subsequently allows the interface implementations to determine
which frame the request came from.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adds associated interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to servce as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-09-09 15:10:19]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-09 16:01:50]

https://codereview.chromium.org/2310563002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2310563002/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:847: while
(!queued_messages_.empty())
On 2016/09/09 at 05:33:14, Sam McNally wrote:
> queued_messages_.clear();

std::queue has no clear(). I've change these to destructive assignment though.

https://codereview.chromium.org/2310563002/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:976: // Push any
pending messages to the channel.now. Note that if the child
On 2016/09/09 at 05:33:14, Sam McNally wrote:
> s/channel.now/channel now/

Done

https://codereview.chromium.org/2310563002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_interface_registry_impl.cc
(right):

https://codereview.chromium.org/2310563002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_interface_registry_impl.cc:22:
RemoveInterfacesFromFrame(frame);
On 2016/09/09 at 05:33:14, Sam McNally wrote:
> Is this necessary after WebContentsDestroyed() is called?

Removed. This was added before WebContentsImpl owned the registry.

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/render_frame_host.h:18: #include
"mojo/public/cpp/bindings/associated_interface_ptr.h"
On 2016/09/09 at 05:33:14, Sam McNally wrote:
> I don't think these extra includes are needed anymore.

Removed

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/render_process_host.h:272: virtual mojom::RouteProvider*
GetRemoteRouteProvider() = 0;
On 2016/09/09 at 05:33:14, Sam McNally wrote:
> Does this need to be public?

Nope. Good call. Fixed.

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
File content/public/browser/web_contents_frame_interface_binding.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/web_contents_frame_interface_binding.h:32: void
AddToWebContents(WebContents* web_contents) {
On 2016/09/09 at 05:33:15, Sam McNally wrote:
> This could be clearer that an instance can be added to at most one
WebContents. Maybe SetWebContents()/ClearWebContents().

Done

https://codereview.chromium.org/2310563002/diff/120001/content/public/browser...
content/public/browser/web_contents_frame_interface_binding.h:44: }
On 2016/09/09 at 05:33:15, Sam McNally wrote:
> Please add a current RenderFrameHost accessor. It should be able to pull it
from |weak_registry_|.

Done

https://codereview.chromium.org/2310563002/diff/120001/content/public/child/c...
File content/public/child/child_thread.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/child/c...
content/public/child/child_thread.h:85: virtual mojom::RouteProvider*
GetRemoteRouteProvider() = 0;
On 2016/09/09 at 05:33:15, Sam McNally wrote:
> Does this need to be public?

Nope, removed!

https://codereview.chromium.org/2310563002/diff/120001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2310563002/diff/120001/content/public/rendere...
content/public/renderer/render_frame.h:16: #include
"content/public/renderer/render_thread.h"
On 2016/09/09 at 05:33:15, Sam McNally wrote:
> These extra includes aren't needed anymore.

Removed

https://codereview.chromium.org/2310563002/diff/120001/ipc/message_router.h
File ipc/message_router.h (right):

https://codereview.chromium.org/2310563002/diff/120001/ipc/message_router.h#n...
ipc/message_router.h:62: // Returns the Listener associated with |routing_id|/.
On 2016/09/09 at 05:33:15, Sam McNally wrote:
> Extra /

Removed

[Ken Rockot(use gerrit already), 2016-09-09 16:42:28]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 16:42:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-09-09 16:58:42]

rockot@chromium.org changed reviewers:
+ yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-09-09 16:58:42]

Yuzhu, would you mind please looking at the changes to ipc_mojo_bootstrap.cc?
They fix a race which this CL hit, where this can happen:

A ----------- B  (an associated interface end-to-end)

A sends a message to B, then closes itself.
B closes itself before receiving the message, sending a peer closed control
message to A.
A destroys its Endpoint because both ends are known to be closed.
B's controller receives the message A originally sent, and has nowhere to
dispatch.
B's controller creates a new Endpoint and closes it immediately, sending a peer
closed control message to A.
A's controller receives a peer closed control message and creates a new Endpoint
for A.

Now A is left with a dangling Endpoint with a closed peer, and no attached
client to close it. We'll hit a DCHECK on channel shutdown because there's an
unclosed Endpoint.

It seems to me that we simply don't need to create a new Endpoint if a
(non-control) message arrives for an endpoint that doesn't exist, so I've
removed this logic.

[Ken Rockot(use gerrit already), 2016-09-09 17:17:45]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 17:18:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 18:32:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:32:35]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-09-12 18:06:51]

On 2016/09/09 16:58:42, Ken Rockot wrote:
> Yuzhu, would you mind please looking at the changes to ipc_mojo_bootstrap.cc?
> They fix a race which this CL hit, where this can happen:
> 
> A ----------- B  (an associated interface end-to-end)
> 
> A sends a message to B, then closes itself.
> B closes itself before receiving the message, sending a peer closed control
> message to A.
> A destroys its Endpoint because both ends are known to be closed.
> B's controller receives the message A originally sent, and has nowhere to
> dispatch.
> B's controller creates a new Endpoint and closes it immediately, sending a
peer
> closed control message to A.
> A's controller receives a peer closed control message and creates a new
Endpoint
> for A.

At this point, because A doesn't have an existing endpoint for the peer closed
control message, I think it is safe to *not* creating a new endpoint, but
sending a peer closed control message back to B. (That is equivalent to creating
a new endpoint, setting it to close/peer_close immediately and removing it.)

> 
> Now A is left with a dangling Endpoint with a closed peer, and no attached
> client to close it. We'll hit a DCHECK on channel shutdown because there's an
> unclosed Endpoint.
> 
> It seems to me that we simply don't need to create a new Endpoint if a
> (non-control) message arrives for an endpoint that doesn't exist, so I've
> removed this logic.
In this case, do we send back a peer closed message? If no, we will delay
connection error notification for the other side; if yes, we will send peer
closed as many times as the other side sends us non-control messages.

WDYT? Thanks!

[Ken Rockot(use gerrit already), 2016-09-12 21:33:02]

On 2016/09/12 at 18:06:51, yzshen wrote:
> On 2016/09/09 16:58:42, Ken Rockot wrote:
> > Yuzhu, would you mind please looking at the changes to
ipc_mojo_bootstrap.cc?
> > They fix a race which this CL hit, where this can happen:
> > 
> > A ----------- B  (an associated interface end-to-end)
> > 
> > A sends a message to B, then closes itself.
> > B closes itself before receiving the message, sending a peer closed control
> > message to A.
> > A destroys its Endpoint because both ends are known to be closed.
> > B's controller receives the message A originally sent, and has nowhere to
> > dispatch.
> > B's controller creates a new Endpoint and closes it immediately, sending a
peer
> > closed control message to A.
> > A's controller receives a peer closed control message and creates a new
Endpoint
> > for A.
> 
> At this point, because A doesn't have an existing endpoint for the peer closed
control message, I think it is safe to *not* creating a new endpoint, but
sending a peer closed control message back to B. (That is equivalent to creating
a new endpoint, setting it to close/peer_close immediately and removing it.)

But if that policy is adopted, can't both sides end up ping-ponging peer-closed
messages indefinitely for the same pair of endpoints? i.e. neither A nor B
exists anymore, but non-existent A receives a peer-closed for B. A's controller
sends a peer-closed back to B's controller. Non-existent B receives a
peer-closed for A. B's controller sends a peer-closed back to A's controller,
and so on.

> 
> > 
> > Now A is left with a dangling Endpoint with a closed peer, and no attached
> > client to close it. We'll hit a DCHECK on channel shutdown because there's
an
> > unclosed Endpoint.
> > 
> > It seems to me that we simply don't need to create a new Endpoint if a
> > (non-control) message arrives for an endpoint that doesn't exist, so I've
> > removed this logic.
> In this case, do we send back a peer closed message? If no, we will delay
connection error notification for the other side; if yes, we will send peer
closed as many times as the other side sends us non-control messages.
> 

No, in this case we don't send back a peer closed message. In fact we only send
a peer closed message when the local endpoint is closed, and with this change we
now only close a local endpoint in response to explicit closure or receiving a
closed-before-sent control message.

> WDYT? Thanks!

[yzshen1, 2016-09-12 22:18:31]

On 2016/09/12 21:33:02, Ken Rockot wrote:
> On 2016/09/12 at 18:06:51, yzshen wrote:
> > On 2016/09/09 16:58:42, Ken Rockot wrote:
> > > Yuzhu, would you mind please looking at the changes to
> ipc_mojo_bootstrap.cc?
> > > They fix a race which this CL hit, where this can happen:
> > > 
> > > A ----------- B  (an associated interface end-to-end)
> > > 
> > > A sends a message to B, then closes itself.
> > > B closes itself before receiving the message, sending a peer closed
control
> > > message to A.
> > > A destroys its Endpoint because both ends are known to be closed.
> > > B's controller receives the message A originally sent, and has nowhere to
> > > dispatch.
> > > B's controller creates a new Endpoint and closes it immediately, sending a
> peer
> > > closed control message to A.
> > > A's controller receives a peer closed control message and creates a new
> Endpoint
> > > for A.
> > 
> > At this point, because A doesn't have an existing endpoint for the peer
closed
> control message, I think it is safe to *not* creating a new endpoint, but
> sending a peer closed control message back to B. (That is equivalent to
creating
> a new endpoint, setting it to close/peer_close immediately and removing it.)
> 
> But if that policy is adopted, can't both sides end up ping-ponging
peer-closed
> messages indefinitely for the same pair of endpoints? i.e. neither A nor B
> exists anymore, but non-existent A receives a peer-closed for B. A's
controller
> sends a peer-closed back to B's controller. Non-existent B receives a
> peer-closed for A. B's controller sends a peer-closed back to A's controller,
> and so on.

Ah, you are right. :)
> 
> > 
> > > 
> > > Now A is left with a dangling Endpoint with a closed peer, and no attached
> > > client to close it. We'll hit a DCHECK on channel shutdown because there's
> an
> > > unclosed Endpoint.
> > > 
> > > It seems to me that we simply don't need to create a new Endpoint if a
> > > (non-control) message arrives for an endpoint that doesn't exist, so I've
> > > removed this logic.
> > In this case, do we send back a peer closed message? If no, we will delay
> connection error notification for the other side; if yes, we will send peer
> closed as many times as the other side sends us non-control messages.
> > 
> 
> No, in this case we don't send back a peer closed message. In fact we only
send
> a peer closed message when the local endpoint is closed, and with this change
we
> now only close a local endpoint in response to explicit closure or receiving a
> closed-before-sent control message.

Makes sense.

LGTM for the changes to ipc_mojo_bootstrap

> 
> > WDYT? Thanks!

[jam, 2016-09-13 00:08:47]

Description was changed from

==========
Adds associated interface support between RenderFrameHost and RenderFrame

Adds support for per-frame associated interfaces.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to servce as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adds associated interface support between RenderFrameHost and RenderFrame.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to serve as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ken Rockot(use gerrit already), 2016-09-13 22:09:36]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 22:10:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-09-13 22:21:49]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2016-09-13 22:22:09]

Patchset #11 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-09-13 22:22:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 00:30:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 00:30:47]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-14 17:05:06]

lgtm

https://codereview.chromium.org/2310563002/diff/200001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2310563002/diff/200001/content/public/rendere...
content/public/renderer/render_frame.h:18: #include "ipc/ipc_sync_channel.h"
nit: not needed

https://codereview.chromium.org/2310563002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2310563002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2417: GetRoutingID(),
nit: usually RF just uses routing_id_ instead of the public method

[jam, 2016-09-14 17:05:59]

https://codereview.chromium.org/2310563002/diff/200001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2310563002/diff/200001/content/public/rendere...
content/public/renderer/render_frame.h:18: #include "ipc/ipc_sync_channel.h"
On 2016/09/14 17:05:06, jam wrote:
> nit: not needed

oops, ignore this was from an earlier patchset and it's gone now. i forgot to
delete the comment

[Ken Rockot(use gerrit already), 2016-09-14 17:40:35]

rockot@chromium.org changed reviewers:
+ tsepez@chromium.org

[Ken Rockot(use gerrit already), 2016-09-14 17:40:36]

+tsepez@ for mojom review

[Tom Sepez, 2016-09-14 17:47:40]

Mojom LGTM

[nasko, 2016-09-14 18:42:22]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2016-09-14 18:42:24]

Drive-by comment.

https://codereview.chromium.org/2310563002/diff/240001/content/public/rendere...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2310563002/diff/240001/content/public/rendere...
content/public/renderer/render_frame.h:167: virtual AssociatedInterfaceProvider*
GetRemoteAssociatedInterfaces() = 0;
Nothing in this CL is using these methods outside of content/. Please don't add
them in this CL, but rather the CL that ends up using them.

[Ken Rockot(use gerrit already), 2016-09-14 19:08:23]

On 2016/09/14 at 18:42:24, nasko wrote:
> Drive-by comment.
> 
>
https://codereview.chromium.org/2310563002/diff/240001/content/public/rendere...
> File content/public/renderer/render_frame.h (right):
> 
>
https://codereview.chromium.org/2310563002/diff/240001/content/public/rendere...
> content/public/renderer/render_frame.h:167: virtual
AssociatedInterfaceProvider* GetRemoteAssociatedInterfaces() = 0;
> Nothing in this CL is using these methods outside of content/. Please don't
add them in this CL, but rather the CL that ends up using them.

I'm trying to keep the follow-up CL as minimal as possible so it can serve as a
simple example of using the interfaces. Is this really a blocking concern?

[Ken Rockot(use gerrit already), 2016-09-14 19:34:03]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:35:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2016-09-14 20:16:04]

On 2016/09/14 19:08:23, Ken Rockot wrote:
> On 2016/09/14 at 18:42:24, nasko wrote:
> > Drive-by comment.
> > 
> >
>
https://codereview.chromium.org/2310563002/diff/240001/content/public/rendere...
> > File content/public/renderer/render_frame.h (right):
> > 
> >
>
https://codereview.chromium.org/2310563002/diff/240001/content/public/rendere...
> > content/public/renderer/render_frame.h:167: virtual
> AssociatedInterfaceProvider* GetRemoteAssociatedInterfaces() = 0;
> > Nothing in this CL is using these methods outside of content/. Please don't
> add them in this CL, but rather the CL that ends up using them.
> 
> I'm trying to keep the follow-up CL as minimal as possible so it can serve as
a
> simple example of using the interfaces. Is this really a blocking concern?

Not a blocking concern, just a rule we want to uphold in content/. However,
given the goal of very simple follow up CL, I'm fine with it.

[commit-bot: I haz the power, 2016-09-14 20:36:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 20:36:19]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Ken Rockot(use gerrit already), 2016-09-14 22:42:51]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-14 22:42:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
tibell@chromium.org, yzshen@chromium.org, jam@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2310563002/#ps260001
(title: "nit")

[commit-bot: I haz the power, 2016-09-14 22:43:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 00:10:09]

Description was changed from

==========
Adds associated interface support between RenderFrameHost and RenderFrame.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to serve as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adds associated interface support between RenderFrameHost and RenderFrame.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to serve as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-15 00:10:10]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-09-15 00:14:31]

Description was changed from

==========
Adds associated interface support between RenderFrameHost and RenderFrame.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to serve as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adds associated interface support between RenderFrameHost and RenderFrame.

Introduces a new content::AssociatedInterfaceRegistry and
content::AssociatedInterfaceProvider to serve as Channel-associated
interface counterparts to their shell namesakes.

RenderFrameHost and RenderFrame each have both of these things on them,
and they're routed to the other side using a new RouteProvider interface
buried in RenderProcessHostImpl / ChildThreadImpl on either end.

Also adds a WebContentsInterfaceRegistry to every WebContents,
and a helper WebContentsFrameBindingSet<T> to allow consumers to
very easily manage the lifetime of associated interface bindings
for each frame in a WebContents.

See the follow-up patch for example usage.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. https://codereview.chromium.org/2309513002
  2. https://codereview.chromium.org/2316963005
  3. This CL
  4. https://codereview.chromium.org/2310583002

BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f62002a08f3025d4d9fe488c0986369ae94e1f10
Cr-Commit-Position: refs/heads/master@{#418717}
==========

[commit-bot: I haz the power, 2016-09-15 00:14:33]

Patchset 12 (id:??) landed as
https://crrev.com/f62002a08f3025d4d9fe488c0986369ae94e1f10
Cr-Commit-Position: refs/heads/master@{#418717}