Introduce content::ChildMessageFilter.

Introduce content::ChildMessageFilter.

ChildMessageFilter is the child thread equivalent of BrowserMessageFilter. ChildMessageFilter is a convenience base class for implementing message filters that run on the child thread and can optionally dispatch messages to specific task runners.

[scherkus (not reviewing), 2013-09-25 18:23:16]

here's that refactoring -- let me know what you think

[jam, 2013-09-25 20:58:57]

thanks for doing this

https://codereview.chromium.org/24636002/diff/1/content/child/child_message_f...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/24636002/diff/1/content/child/child_message_f...
content/child/child_message_filter.h:23: // DON'T OVERRIDE THIS! Implement
HandleMessage() instead.
in retrospect, I should have made BrowserMessageFilter have an inner class that
is an IPC::ChannelProxy::MessageFilter, and hid that implementation detail from
child classes. That would remove this issue of OnMessageReceived confusion

[scherkus (not reviewing), 2013-09-25 23:07:14]

PTAL and let me know what you think

it definitely fixes the OnMessageReceived() issue but the ref counting junk
makes me a bit sad

https://codereview.chromium.org/24636002/diff/1/content/child/child_message_f...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/24636002/diff/1/content/child/child_message_f...
content/child/child_message_filter.h:23: // DON'T OVERRIDE THIS! Implement
HandleMessage() instead.
On 2013/09/25 20:58:58, jam wrote:
> in retrospect, I should have made BrowserMessageFilter have an inner class
that
> is an IPC::ChannelProxy::MessageFilter, and hid that implementation detail
from
> child classes. That would remove this issue of OnMessageReceived confusion

Done.

I find it ends up being a biiiittt confusing for subclasses since they're no
longer forced to implement any methods, but maybe that's OK.

[jam, 2013-09-26 18:09:36]

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
content/child/child_message_filter.h:16: class ChildMessageFilter : public
IPC::ChannelProxy::MessageFilter,
While it's nice to reuse the method definitions from this interface, I think
there are two issues with this:
1) a child class can incorrectly override the single parameter OnMessageReceived
version, which is confusing and would also interfere with the base class'
operation
2) someone could attach this class to a ChannelProxy, instead of the internal
class. i.e. in this cl :)

this is why for BMF I duplicated the methods that are needed from it here, with
the exception of the single parameter OnMessageReceived.

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
content/child/child_message_filter.h:37: class Internal : public
IPC::ChannelProxy::MessageFilter {
nit: you could just hide this whole class in the cc file, no reason to expose it
here

[scherkus (not reviewing), 2013-09-26 19:33:48]

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
content/child/child_message_filter.h:16: class ChildMessageFilter : public
IPC::ChannelProxy::MessageFilter,
On 2013/09/26 18:09:37, jam wrote:
> While it's nice to reuse the method definitions from this interface, I think
> there are two issues with this:
> 1) a child class can incorrectly override the single parameter
OnMessageReceived
> version, which is confusing and would also interfere with the base class'
> operation
> 2) someone could attach this class to a ChannelProxy, instead of the internal
> class. i.e. in this cl :)
> 
> this is why for BMF I duplicated the methods that are needed from it here,
with
> the exception of the single parameter OnMessageReceived.

Yeah I realized this after I sent this out ... it's very non-obvious that you
need to implement the MessageFilter::OnMessageReceived() method and because it
has an impl C++ won't enforce subclasses to implement it and you can end up with
a message filter that doesn't filter any messages :)

I'll take a look at the BMF CL

[scherkus (not reviewing), 2013-11-07 21:24:22]

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
content/child/child_message_filter.h:16: class ChildMessageFilter : public
IPC::ChannelProxy::MessageFilter,
On 2013/09/26 19:33:48, scherkus wrote:
> On 2013/09/26 18:09:37, jam wrote:
> > While it's nice to reuse the method definitions from this interface, I think
> > there are two issues with this:
> > 1) a child class can incorrectly override the single parameter
> OnMessageReceived
> > version, which is confusing and would also interfere with the base class'
> > operation
> > 2) someone could attach this class to a ChannelProxy, instead of the
internal
> > class. i.e. in this cl :)
> > 
> > this is why for BMF I duplicated the methods that are needed from it here,
> with
> > the exception of the single parameter OnMessageReceived.
> 
> Yeah I realized this after I sent this out ... it's very non-obvious that you
> need to implement the MessageFilter::OnMessageReceived() method and because it
> has an impl C++ won't enforce subclasses to implement it and you can end up
with
> a message filter that doesn't filter any messages :)
> 
> I'll take a look at the BMF CL

Updated to no longer implement IPC::ChannelProxy::MessageFilter.

https://codereview.chromium.org/24636002/diff/7001/content/child/child_messag...
content/child/child_message_filter.h:37: class Internal : public
IPC::ChannelProxy::MessageFilter {
On 2013/09/26 18:09:37, jam wrote:
> nit: you could just hide this whole class in the cc file, no reason to expose
it
> here

Done.

https://codereview.chromium.org/24636002/diff/16001/content/child/child_messa...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/24636002/diff/16001/content/child/child_messa...
content/child/child_message_filter.h:33: virtual bool OnMessageReceived(const
IPC::Message& message) = 0;
reminder to self: needs docs

https://codereview.chromium.org/24636002/diff/16001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/24636002/diff/16001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:1331: // updating all existing clients.
jam: this is the real question I have in mind

if you look at browser-side code, I believe you're forced to use
BrowserMessageFilter and the RenderProcessHost API enforces this

in render-side code, the RenderThread API offers
{Add,Remove}Filter(IPC::ChannelProxy::MessageFilter*) ... I haven't finished
auditing all current callers but we might be able to tighten up that API to
enforce using ChildMessageFilter.

WDYT?

[scherkus (not reviewing), 2014-01-06 21:56:09]

Closing as kinuko@ landed equivalent in https://codereview.chromium.org/63843002