Add support for attaching filters to an IPC TestSink.

Add support for attaching filters to an IPC TestSink.

The filters can see every message that is sent to the sink, which is useful
when if the test is waiting for a specific message to arrive.

BUG=none
TEST=none

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

[Brian Ryner, 2011-01-28 01:53:07]

I'm planning to use this as part of the test for an upcoming change to
safe_browsing::ClientSideDetectionService
(http://codereview.chromium.org/6398001/).

[noelutz, 2011-01-28 02:13:06]

LGTM

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.cc
File ipc/ipc_test_sink.cc (right):

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.cc#newcode66
ipc/ipc_test_sink.cc:66: observer_list_.RemoveObserver(obs);
Maybe check in the destructor that all observers have been removed?

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.h
File ipc/ipc_test_sink.h (right):

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.h#newcode50
ipc/ipc_test_sink.h:50: //     public:
nit: indentation is wrong I think.

[Brian Ryner, 2011-01-28 02:19:59]

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.cc
File ipc/ipc_test_sink.cc (right):

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.cc#newcode66
ipc/ipc_test_sink.cc:66: observer_list_.RemoveObserver(obs);
On 2011/01/28 02:13:06, noelutz wrote:
> Maybe check in the destructor that all observers have been removed?

I'm not sure how common this is for observer interfaces in Chrome -- Brett?

[brettw, 2011-01-28 16:26:15]

Why do you need the test sink if you're waiting for a message? It seems just as
easy to write a class that's a IPC::Message::Receiver and implement
OnMessageReceived as to write one of these new observers, and it involves fewer
moving parts.

[Brian Ryner, 2011-01-28 18:38:27]

On 2011/01/28 16:26:15, brettw wrote:
> Why do you need the test sink if you're waiting for a message? It seems just
as
> easy to write a class that's a IPC::Message::Receiver and implement
> OnMessageReceived as to write one of these new observers, and it involves
fewer
> moving parts.

Hm, do you mean IPC::Channel::Listener? I don't see an IPC::Message::Receiver
class.

In any case, the reason I wanted to do it this way is that I'm planning to use
RenderViewHostTestHarness to run my test (since it relies on TabContents objects
being created, and the notifications that TabContents sends).  This setup uses a
MockRenderProcessHost, which is already sending all of the messages for the
renderer to an IPC::TestSink.

Is there an easier way to do this that I'm missing?

[brettw, 2011-01-28 19:02:20]

On Fri, Jan 28, 2011 at 10:38 AM,  <bryner@chromium.org> wrote:
> On 2011/01/28 16:26:15, brettw wrote:
>>
>> Why do you need the test sink if you're waiting for a message? It seems
>> just
>
> as
>>
>> easy to write a class that's a IPC::Message::Receiver and implement
>> OnMessageReceived as to write one of these new observers, and it involves
>
> fewer
>>
>> moving parts.
>
> Hm, do you mean IPC::Channel::Listener? I don't see an
> IPC::Message::Receiver
> class.

Yes, sorry.

> In any case, the reason I wanted to do it this way is that I'm planning to
> use
> RenderViewHostTestHarness to run my test (since it relies on TabContents
> objects
> being created, and the notifications that TabContents sends).  This setup
> uses a
> MockRenderProcessHost, which is already sending all of the messages for the
> renderer to an IPC::TestSink.
>
> Is there an easier way to do this that I'm missing?

That makes sense, I didn't realize you were hooking into some existing
test code.

I would probably re-use the Listener class rather than writing your
own observer class, and maybe calling the function something like
AddFilter or something? If OnMessageReceived returns true it's
supposed to drop the message, and if it returns false, the sink should
probably store it. This will make it work like our other IPC
machinery.

Brett

[Brian Ryner, 2011-01-28 20:43:07]

Updated to use a filtering model and IPC::Channel::Listeners per Brett's
suggestion.  I didn't add a check on destruction that the observers have all
been removed, since I don't see that in the RenderViewObserver code I was
modeling this on.

Please take another look.

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.h
File ipc/ipc_test_sink.h (right):

http://codereview.chromium.org/6387007/diff/2001/ipc/ipc_test_sink.h#newcode50
ipc/ipc_test_sink.h:50: //     public:
On 2011/01/28 02:13:06, noelutz wrote:
> nit: indentation is wrong I think.

Done.

[brettw, 2011-01-28 21:12:32]

LGTM

http://codereview.chromium.org/6387007/diff/12001/ipc/ipc_test_sink.cc
File ipc/ipc_test_sink.cc (right):

http://codereview.chromium.org/6387007/diff/12001/ipc/ipc_test_sink.cc#newcode26
ipc/ipc_test_sink.cc:26: while ((observer = it.GetNext()) != NULL)
This outer while() needs {} since it contains > 1 line (Chrome team style rule).

[Brian Ryner, 2011-01-28 22:04:23]

Thanks, fixed and submitting.

http://codereview.chromium.org/6387007/diff/12001/ipc/ipc_test_sink.cc
File ipc/ipc_test_sink.cc (right):

http://codereview.chromium.org/6387007/diff/12001/ipc/ipc_test_sink.cc#newcode26
ipc/ipc_test_sink.cc:26: while ((observer = it.GetNext()) != NULL)
On 2011/01/28 21:12:32, brettw wrote:
> This outer while() needs {} since it contains > 1 line (Chrome team style
rule).

Done.