Add ChildMessageFilter, a base class for renderer/worker cross-thread MessageFilter

Add ChildMessageFilter, a base class for renderer/worker cross-thread MessageFilter

- Save some dup'ed code in similar message filters
- Centralize WorkerRunner thread-handling code into one class (which could be helpful when we migrate worker thread implementation)

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236004

[kinuko, 2013-11-07 07:20:38]

I may need to add one more message filter which needs cross-thread dispatching
and I'd like to save some typing. This patch introduces a base class for all
similar filters (similar attempt to what alecf tried before, but in a more
modest way).  Please review, thanks.

[jam, 2013-11-07 19:33:04]

cool, looks like we need this in a number of places, see also
https://codereview.chromium.org/24636002/

adding scherkus

For a generic class, I prefer one that leaves the cracking the message to get
the thread id to individual sub classes, as not all subclasses will use this.

[kinuko, 2013-11-08 05:33:03]

On 2013/11/07 19:33:04, jam wrote:
> cool, looks like we need this in a number of places, see also
> https://codereview.chromium.org/24636002/

Ah cool, one day all webkit thread migrates to task runner we can use it.

> adding scherkus
> 
> For a generic class, I prefer one that leaves the cracking the message to get
> the thread id to individual sub classes, as not all subclasses will use this.

Made it a pure virtual method.

I can also make this use private internal class so that we have no worries for
OnMessageReceived wrongly overriden by child classes. Should I follow the
pattern?

[kinuko, 2013-11-08 06:29:36]

On 2013/11/08 05:33:03, kinuko wrote:
> On 2013/11/07 19:33:04, jam wrote:
> > cool, looks like we need this in a number of places, see also
> > https://codereview.chromium.org/24636002/
> 
> Ah cool, one day all webkit thread migrates to task runner we can use it.
> 
> > adding scherkus
> > 
> > For a generic class, I prefer one that leaves the cracking the message to
get
> > the thread id to individual sub classes, as not all subclasses will use
this.
> 
> Made it a pure virtual method.
> 
> I can also make this use private internal class so that we have no worries for
> OnMessageReceived wrongly overriden by child classes. Should I follow the
> pattern?

Took the pattern in patch set 3. PTAL?

When ChildThread lands we may be able to merge this into it, but I suppose we
don't probably want to mix the usage (task runner + webkit worker threads).

[jam, 2013-11-08 19:51:58]

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
File content/child/cross_thread_message_filter.cc (right):

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.cc:30: if (!ipc_thread_id) {
we should keep the ipc_thread_id stuff in child classes. this class should be
generic so that it could be used with more than just blink's worker threads.  so
like BrowserMessageFilter, OverrideTaskRunnerForMessage would return a
base::TaskRunner

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
File content/child/cross_thread_message_filter.h (right):

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.h:13: // to blink's worker threads via
WorkerTaskRunner.
nit: no need to mention blink here, this class can be used to service any thread
other than the main

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.h:14: class CrossThreadMessageFilter
nit: I think ChildMessageFilter is sufficient, that would match
BrowserMessageFilter

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.h:35: virtual void
OnStaleMessageReceived(const IPC::Message& msg) {}
can you also add a Send() method, i.e. see
https://codereview.chromium.org/24636002/diff/16001/content/child/child_messa...

[michaeln, 2013-11-09 01:52:10]

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
File content/child/cross_thread_message_filter.cc (right):

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.cc:30: if (!ipc_thread_id) {
On 2013/11/08 19:51:59, jam wrote:
> we should keep the ipc_thread_id stuff in child classes. this class should be
> generic so that it could be used with more than just blink's worker threads. 
so
> like BrowserMessageFilter, OverrideTaskRunnerForMessage would return a
> base::TaskRunner

+1 using the base::TaskRunner abstraction for this, we just need an impl that
uses WorkerTaskRunner::Instance()->PostTask() under the covers.

[kinuko, 2013-11-15 14:40:35]

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
File content/child/cross_thread_message_filter.cc (right):

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.cc:30: if (!ipc_thread_id) {
Done.  (If I should separate a CL for adding TaskRunner for webkit worker
threads I can do so too)

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
File content/child/cross_thread_message_filter.h (right):

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.h:13: // to blink's worker threads via
WorkerTaskRunner.
On 2013/11/08 19:51:59, jam wrote:
> nit: no need to mention blink here, this class can be used to service any
thread
> other than the main

Done.

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.h:14: class CrossThreadMessageFilter
On 2013/11/08 19:51:59, jam wrote:
> nit: I think ChildMessageFilter is sufficient, that would match
> BrowserMessageFilter

Ok, done. (scherkus@, hope it's ok that I'm stealing your CL now)

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.h:35: virtual void
OnStaleMessageReceived(const IPC::Message& msg) {}
On 2013/11/08 19:51:59, jam wrote:
> can you also add a Send() method, i.e. see
>
https://codereview.chromium.org/24636002/diff/16001/content/child/child_messa...

Done.

[scherkus (not reviewing), 2013-11-15 19:12:18]

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
File content/child/cross_thread_message_filter.h (right):

https://codereview.chromium.org/63843002/diff/860001/content/child/cross_thre...
content/child/cross_thread_message_filter.h:14: class CrossThreadMessageFilter
On 2013/11/15 14:40:36, kinuko wrote:
> On 2013/11/08 19:51:59, jam wrote:
> > nit: I think ChildMessageFilter is sufficient, that would match
> > BrowserMessageFilter
> 
> Ok, done. (scherkus@, hope it's ok that I'm stealing your CL now)

go for it! I don't have the bandwidth at the moment to tackle it :)

[jam, 2013-11-18 05:50:01]

lgtm

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
content/child/child_message_filter.h:27: // If implementors want to run
OnMessageReceived on a different task
nit: implementers

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
content/child/child_message_filter.h:32: const IPC::Message& msg);
nit: seems that this should be pure virtual as that's the whole reason for using
this class?

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
content/child/child_message_filter.h:39: // for the target thread.  Note that
there's still a listtle chance that
nit: little chance

[kinuko, 2013-11-18 06:35:46]

Thanks!

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
File content/child/child_message_filter.h (right):

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
content/child/child_message_filter.h:27: // If implementors want to run
OnMessageReceived on a different task
On 2013/11/18 05:50:02, jam wrote:
> nit: implementers

Done.

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
content/child/child_message_filter.h:32: const IPC::Message& msg);
On 2013/11/18 05:50:02, jam wrote:
> nit: seems that this should be pure virtual as that's the whole reason for
using
> this class?

Yup... done.

https://codereview.chromium.org/63843002/diff/1010001/content/child/child_mes...
content/child/child_message_filter.h:39: // for the target thread.  Note that
there's still a listtle chance that
On 2013/11/18 05:50:02, jam wrote:
> nit: little chance

Done.

[michaeln, 2013-11-18 17:51:41]

lgtm2

https://codereview.chromium.org/63843002/diff/1010001/content/child/worker_th...
File content/child/worker_thread_task_runner.cc (right):

https://codereview.chromium.org/63843002/diff/1010001/content/child/worker_th...
content/child/worker_thread_task_runner.cc:27: base::TimeDelta /* delay */) {
should we assert if delay != 0 for now?

[kinuko, 2013-11-19 04:51:34]

https://codereview.chromium.org/63843002/diff/1010001/content/child/worker_th...
File content/child/worker_thread_task_runner.cc (right):

https://codereview.chromium.org/63843002/diff/1010001/content/child/worker_th...
content/child/worker_thread_task_runner.cc:27: base::TimeDelta /* delay */) {
On 2013/11/18 17:51:42, michaeln wrote:
> should we assert if delay != 0 for now?

Done.

[commit-bot: I haz the power, 2013-11-19 04:55:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/63843002/1460001

[commit-bot: I haz the power, 2013-11-19 07:16:37]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-19 08:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/63843002/1460001

[commit-bot: I haz the power, 2013-11-19 10:24:12]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-19 10:45:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/63843002/1460001

[commit-bot: I haz the power, 2013-11-19 12:04:21]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-19 14:16:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/63843002/1920001

[commit-bot: I haz the power, 2013-11-19 16:34:39]

Change committed as 236004