Added a ResourceMessageFilter for handling resource messages on another thread.

Added a ResourceMessageFilter for handling resource messages on another thread.
    
ResourceHosts make it difficult to handle messages on background threads. This adds a ResourceMessageFilter class which can be subclassed to easily handle resource messages on another thread. It should be added to a ResourceHost using |AddFilter|.

BUG=None
TEST=Added a unittest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168729

[raymes, 2012-11-12 22:27:55]

This is currently untested but I thought I'd send it out for quick feedback and
there were a few people who wanted this functionality. 

Brett, Yuzhu please take a look and maybe provide high level comments first. 

I'd like to add unit tests but I believe they have to be in browser_tests in
order to test stuff running on browser threads. I don't think ppapi_browsertests
is the right place because it only has tests which test the pepper API. Any
suggestions on this?

[yzshen1, 2012-11-13 21:51:48]

First round of comments.
I will look into the details when you have tested it. :)

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/DEPS
File ppapi/host/DEPS (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/DEPS#newcode2
ppapi/host/DEPS:2: "+content/public/browser",
I am not sure whether we should add this here.

Maybe the BackgroundThreadMessageFilter should live in some browser folder.
And the ResourceHost deal with some more general filter interface.

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/background_threa...
File ppapi/host/background_thread_message_filter.h (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/background_threa...
ppapi/host/background_thread_message_filter.h:113: // Non-owning pointer to the
resource host owning this filter.
Please comment that it should only be accessed on the IO thread.

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/ppapi_host.cc#ne...
ppapi/host/ppapi_host.cc:127: SendReply(reply_context, context->reply_msg);
Maybe the message doesn't expect a reply.

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/resource_host_me...
File ppapi/host/resource_host_message_filter.h (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/resource_host_me...
ppapi/host/resource_host_message_filter.h:25: class PPAPI_HOST_EXPORT
ResourceHostMessageFilter {
[nit; just for discussion] Resource(Host)?MessageHandler sounds better to me.
Looking at 'Filter' I thought it would filter some messages out of ResourceHost.

[raymes, 2012-11-16 18:56:35]

I've updated and added a test (and also tested in my other CL). PTAL.

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/DEPS
File ppapi/host/DEPS (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/DEPS#newcode2
ppapi/host/DEPS:2: "+content/public/browser",
I was able to do this without having the dependency on content (in fact we can't
do this because it leads to a circular reference in gyp).

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/background_threa...
File ppapi/host/background_thread_message_filter.h (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/background_threa...
ppapi/host/background_thread_message_filter.h:113: // Non-owning pointer to the
resource host owning this filter.
On 2012/11/13 21:51:48, yzshen1 wrote:
> Please comment that it should only be accessed on the IO thread.

Done.

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/ppapi_host.cc#ne...
ppapi/host/ppapi_host.cc:127: SendReply(reply_context, context->reply_msg);
On 2012/11/13 21:51:48, yzshen1 wrote:
> Maybe the message doesn't expect a reply.

Done.

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/resource_host_me...
File ppapi/host/resource_host_message_filter.h (right):

http://codereview.chromium.org/11410029/diff/4001/ppapi/host/resource_host_me...
ppapi/host/resource_host_message_filter.h:25: class PPAPI_HOST_EXPORT
ResourceHostMessageFilter {
I agree the last names were bad. I like your suggestion. I changed it to
ResourceMessageFilter and ResourceMessageHandler.

[brettw, 2012-11-16 19:27:54]

I looked mostly at a high level and this looks like a good approach. I noted
some style nits. :) I'll let Yuzhu finish reviewing the details.

https://codereview.chromium.org/11410029/diff/22001/ppapi/host/resource_messa...
File ppapi/host/resource_message_filter.h (right):

https://codereview.chromium.org/11410029/diff/22001/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.h:67: // AddFilter(make_scoped_ptr(new
MyMessageFilter));
I think it's worth calling out explicitly in this comment that the resource
hosts lifetimes are managed by the PpapiHost and they could be destroyed before
or during processing of your messages on different threads.

https://codereview.chromium.org/11410029/diff/22001/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.h:100: private:
Nit: one more space.

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
File ppapi/host/resource_message_filter.cc (right):

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.cc:42: const IPC::Message& msg,
Style nit: put after the ( and align the rest to there if it fits (I think most
of the functions in this file fit).

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
File ppapi/host/resource_message_filter.h (right):

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.h:109: // accessed from the IO thread.
Can you also clarify that this may be NULL and when that happens?

[raymes, 2012-11-16 19:42:49]

https://codereview.chromium.org/11410029/diff/22001/ppapi/host/resource_messa...
File ppapi/host/resource_message_filter.h (right):

https://codereview.chromium.org/11410029/diff/22001/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.h:67: // AddFilter(make_scoped_ptr(new
MyMessageFilter));
On 2012/11/16 19:27:54, brettw wrote:
> I think it's worth calling out explicitly in this comment that the resource
> hosts lifetimes are managed by the PpapiHost and they could be destroyed
before
> or during processing of your messages on different threads.

Done.

https://codereview.chromium.org/11410029/diff/22001/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.h:100: private:
On 2012/11/16 19:27:54, brettw wrote:
> Nit: one more space.

Done.

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
File ppapi/host/resource_message_filter.cc (right):

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.cc:42: const IPC::Message& msg,
On 2012/11/16 19:27:54, brettw wrote:
> Style nit: put after the ( and align the rest to there if it fits (I think
most
> of the functions in this file fit).

Done.

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
File ppapi/host/resource_message_filter.h (right):

https://codereview.chromium.org/11410029/diff/17003/ppapi/host/resource_messa...
ppapi/host/resource_message_filter.h:109: // accessed from the IO thread.
On 2012/11/16 19:27:54, brettw wrote:
> Can you also clarify that this may be NULL and when that happens?

Done.

[yzshen1, 2012-11-19 18:59:25]

Only a few nits. Thanks!

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_host.h
File ppapi/host/resource_host.h (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_host.h...
ppapi/host/resource_host.h:24: class ResourceMessageFilter;
Order these forward declarations, please.

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_host.h...
ppapi/host/resource_host.h:52: // handled by the resource hosts own message
handler. This allows
hosts -> host's

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_filter.cc (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter.cc:33: resource_host_ = resource_host;
nit: better to add DCHECK() to make sure it is run on the correct thread. (this
one and also other methods.)

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_filter.h (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter.h:107: // accessed from the IO thread. This
will be NULL upon creation of the filter
The part of it only accessed on the IO thread may be confusing for the renderer
case. Please consider revising it. (And maybe naming of |io_message_loop_proxy|
and other comments as well.)

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_filter_unittest.cc (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter_unittest.cc:34: // Dummy resource host which
simply stores a copy messages it handles.
a copy -> a copy of ? :)

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter_unittest.cc:52: const IPC::Message&
last_handled_msg() { return last_handled_msg_; }
nit: const, please. (and also do that for some other getters)

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_handler.cc (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_handler.cc:39: DCHECK(context->reply_msg.type() ==
0);
I notice that you removed a DLOG_IF here, any reason for that?

[raymes, 2012-11-19 22:35:04]

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_host.h
File ppapi/host/resource_host.h (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_host.h...
ppapi/host/resource_host.h:24: class ResourceMessageFilter;
On 2012/11/19 18:59:25, yzshen1 wrote:
> Order these forward declarations, please.

Done.

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_host.h...
ppapi/host/resource_host.h:52: // handled by the resource hosts own message
handler. This allows
On 2012/11/19 18:59:25, yzshen1 wrote:
> hosts -> host's

Done.

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_filter.cc (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter.cc:33: resource_host_ = resource_host;
This is difficult to check because we don't actually know the correct thread
here. I could compare the message loops which should work in practice but is not
100% correct since someone could run their own message loop. What do you think?

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_filter.h (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter.h:107: // accessed from the IO thread. This
will be NULL upon creation of the filter
On 2012/11/19 18:59:25, yzshen1 wrote:
> The part of it only accessed on the IO thread may be confusing for the
renderer
> case. Please consider revising it. (And maybe naming of
|io_message_loop_proxy|
> and other comments as well.)

Done.

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_filter_unittest.cc (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter_unittest.cc:34: // Dummy resource host which
simply stores a copy messages it handles.
On 2012/11/19 18:59:25, yzshen1 wrote:
> a copy -> a copy of ? :)

Done.

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_filter_unittest.cc:52: const IPC::Message&
last_handled_msg() { return last_handled_msg_; }
On 2012/11/19 18:59:25, yzshen1 wrote:
> nit: const, please. (and also do that for some other getters)

Done.

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
File ppapi/host/resource_message_handler.cc (right):

http://codereview.chromium.org/11410029/diff/15020/ppapi/host/resource_messag...
ppapi/host/resource_message_handler.cc:39: DCHECK(context->reply_msg.type() ==
0);
I think I may have started this CL prior to adding that DLOG and then missed it
in the merge. Good catch.

[yzshen1, 2012-11-19 23:07:32]

LGTM

Thanks

http://codereview.chromium.org/11410029/diff/11007/ppapi/host/resource_messag...
File ppapi/host/resource_message_filter.h (right):

http://codereview.chromium.org/11410029/diff/11007/ppapi/host/resource_messag...
ppapi/host/resource_message_filter.h:73: // ResourceMessagefilters are usually
constructed in the constructor of the
filter -> Filter

http://codereview.chromium.org/11410029/diff/11007/ppapi/host/resource_messag...
ppapi/host/resource_message_filter.h:76: // Test constructor. Allows you to
specify the IO message loop (which will be
nit: please also change 'IO' in this comment.

[commit-bot: I haz the power, 2012-11-20 01:16:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11410029/25007

[commit-bot: I haz the power, 2012-11-20 02:01:10]

Retried try job too often for step(s) sync_integration_tests

[commit-bot: I haz the power, 2012-11-20 03:28:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11410029/25007

[commit-bot: I haz the power, 2012-11-20 04:31:38]

Change committed as 168729