Decouple IPC::MessageFilter from IPC::Channel

Decouple IPC::MessageFilter from IPC::Channel

This change gets rid of Channel dependency from MessageFilter
so that it depends only on IPC::Sender.

TEST=none
BUG=377980
R=jam@chromium.org, darin@chromium.org

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

[Hajime Morrita, 2014-06-10 17:59:26]

Most of the changes are superficial. 

Changes that matter are:

- MessageFilter
- BrowserMessageFilter
- RenderProcessHostImpl

[darin (slow to review), 2014-06-10 19:39:15]

https://codereview.chromium.org/324143002/diff/20001/ipc/message_filter.h
File ipc/message_filter.h (right):

https://codereview.chromium.org/324143002/diff/20001/ipc/message_filter.h#new...
ipc/message_filter.h:37: virtual void OnSenderConnected();
I think it is a bit awkward to use the term Sender in these methods. It is not
the sender that is connected exactly. It is the channel that became connected.
Maybe it is still OK to use the term "Channel" here? I get that OnFilterAdded
only provides a Sender. That makes sense. It doesn't have to be a channel for
these methods to make sense using the term channel, right?

[gmorrita, 2014-06-10 19:50:23]

On 2014/06/10 19:39:15, darin wrote:
> https://codereview.chromium.org/324143002/diff/20001/ipc/message_filter.h
> File ipc/message_filter.h (right):
> 
>
https://codereview.chromium.org/324143002/diff/20001/ipc/message_filter.h#new...
> ipc/message_filter.h:37: virtual void OnSenderConnected();
> I think it is a bit awkward to use the term Sender in these methods. It is not
> the sender that is connected exactly. It is the channel that became connected.
> Maybe it is still OK to use the term "Channel" here? I get that OnFilterAdded
> only provides a Sender. That makes sense. It doesn't have to be a channel for
> these methods to make sense using the term channel, right?
Right. It can be a "channel" of some generic term. I'll keep the name as is.

[jam, 2014-06-10 21:11:48]

I'm not sure I understand the purpose of the BrowserMessageFilterPeer class? It
appears that you're using it to remove knowledge of the peer pid from
BrowserMessageFilter. But really, all the subclasses of BMF who use the pid now
just get it from a class (BMFPeer) that hangs off it. So why not just keep
IPC::MessageFilter::OnChannelConnected with the pid argument? I understand this
may not mesh well with Mojo's IPC channel since it could be redirected to other
processes. But when this actually happens, we will need to modify all the code
that's currently using the pid from BMF anyways.

[Hajime Morrita, 2014-06-10 21:39:58]

On 2014/06/10 21:11:48, jam wrote:

> I'm not sure I understand the purpose of the BrowserMessageFilterPeer class?
It
> appears that you're using it to remove knowledge of the peer pid from
> BrowserMessageFilter. But really, all the subclasses of BMF who use the pid
now
> just get it from a class (BMFPeer) that hangs off it. So why not just keep
> IPC::MessageFilter::OnChannelConnected with the pid argument? I understand
this
> may not mesh well with Mojo's IPC channel since it could be redirected to
other
> processes. But when this actually happens, we will need to modify all the code
> that's currently using the pid from BMF anyways.

My intention here is that
- I don't want to let IPC::MessageFilter know about peer_pid, and 
- I hope BrowserMessageFilter API aligns ones of IPC::MessageFilter.

Probably This CL can get rid of PeerHandle() and peer_pid() from
BrowserMessageFilter and add peer() to it,
so that we can decouple the peer concept from BMF completely (I think this is
reasonable responsibility split)
and clarify who needs this extra knowledge about the peer process.

[Hajime Morrita, 2014-06-10 22:29:34]

Anyway, updated the patch with the rename Darin mentioned.

[jam, 2014-06-12 15:34:52]

On 2014/06/10 21:39:58, morrita wrote:
> On 2014/06/10 21:11:48, jam wrote:
> 
> > I'm not sure I understand the purpose of the BrowserMessageFilterPeer class?
> It
> > appears that you're using it to remove knowledge of the peer pid from
> > BrowserMessageFilter. But really, all the subclasses of BMF who use the pid
> now
> > just get it from a class (BMFPeer) that hangs off it. So why not just keep
> > IPC::MessageFilter::OnChannelConnected with the pid argument? I understand
> this
> > may not mesh well with Mojo's IPC channel since it could be redirected to
> other
> > processes. But when this actually happens, we will need to modify all the
code
> > that's currently using the pid from BMF anyways.
> 
> My intention here is that
> - I don't want to let IPC::MessageFilter know about peer_pid, and 

I don't understand the reasoning for this, since the places that need it now
just get it from another place. So it seemed like introducing more complexity
without addressing the problem that pids are still used.

> - I hope BrowserMessageFilter API aligns ones of IPC::MessageFilter.
> 
> Probably This CL can get rid of PeerHandle() and peer_pid() from
> BrowserMessageFilter and add peer() to it,
> so that we can decouple the peer concept from BMF completely (I think this is
> reasonable responsibility split)
> and clarify who needs this extra knowledge about the peer process.

[gmorrita, 2014-06-12 15:59:03]

On 2014/06/12 15:34:52, jam wrote:
> 
> I don't understand the reasoning for this, since the places that need it now
> just get it from another place. So it seemed like introducing more complexity
> without addressing the problem that pids are still used.
> 
This is true.
I'll update the cl to pass the pid parameter to BrowserMessageFilter as before.

[Hajime Morrita, 2014-06-12 18:48:42]

jam: PTAL?
Now peer_pid is back and this change becomes 
just about replacing Channel with Sender.

[darin (slow to review), 2014-06-12 22:32:31]

LGTM

[gmorrita, 2014-06-12 22:50:18]

Thanks you!

[Hajime Morrita, 2014-06-12 22:51:23]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-06-12 22:54:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/324143002/100001

[jam, 2014-06-13 05:20:42]

lgtm

[commit-bot: I haz the power, 2014-06-13 06:07:20]

Change committed as 276939