Make IPC::Channel polymorphic

Make IPC::Channel polymorphic

This change makes each platform specific ChannelImpl into
a subclass of Channel: ChannelPosix, ChannelWin, ChannelNacl.
delegated functions are now virtual.

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

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

[jam, 2014-06-04 00:33:38]

I understand that now it's obvious at the callsites that a method called through
GetPosixExtras() is posix-only. However it's not clear to me why this is needed.
If in the future we want to eliminate posix-only methods somehow, the list will
be clear by looking at the ifdef section in ipc_channel.h. is there another
reason for this?

[Hajime Morrita, 2014-06-04 00:49:17]

On 2014/06/04 00:33:38, jam wrote:
> I understand that now it's obvious at the callsites that a method called
through
> GetPosixExtras() is posix-only. However it's not clear to me why this is
needed.
> If in the future we want to eliminate posix-only methods somehow, the list
will
> be clear by looking at the ifdef section in ipc_channel.h. is there another
> reason for this?

The idea is that these POSIX-specific API might not be available for Mojo-based
Channels.
We can return NULL to indicate the availability.

[darin (slow to review), 2014-06-04 04:48:57]

On 2014/06/04 00:49:17, morrita wrote:
> On 2014/06/04 00:33:38, jam wrote:
> > I understand that now it's obvious at the callsites that a method called
> through
> > GetPosixExtras() is posix-only. However it's not clear to me why this is
> needed.
> > If in the future we want to eliminate posix-only methods somehow, the list
> will
> > be clear by looking at the ifdef section in ipc_channel.h. is there another
> > reason for this?
> 
> The idea is that these POSIX-specific API might not be available for
Mojo-based
> Channels.
> We can return NULL to indicate the availability.

Perhaps there is a better way to factor these out? When you and I spoke,
we posited that perhaps IPC::Channel could be factored into two interfaces:
one for receiving and sending messages, and one for setting up the channel.

Maybe we could end up with different OS-flavors of Channel setup interfaces?

  IPC::ChannelSetupPosix
  IPC::ChannelSetupMojo
  IPC::ChannelSetupNaCl
  IPC::ChannelSetupWin

Then, maybe we'd have IPC::{Sync}Channel{Proxy} that can be constructed from
a IPC::ChannelSetup?

Or if these methods are needed for more than just setup, we could use another
name, like PlatformChannel?

[Hajime Morrita, 2014-06-04 17:49:34]

On 2014/06/04 04:48:57, darin wrote:
> On 2014/06/04 00:49:17, morrita wrote:
> > On 2014/06/04 00:33:38, jam wrote:
> > > I understand that now it's obvious at the callsites that a method called
> > through
> > > GetPosixExtras() is posix-only. However it's not clear to me why this is
> > needed.
> > > If in the future we want to eliminate posix-only methods somehow, the list
> > will
> > > be clear by looking at the ifdef section in ipc_channel.h. is there
another
> > > reason for this?
> > 
> > The idea is that these POSIX-specific API might not be available for
> Mojo-based
> > Channels.
> > We can return NULL to indicate the availability.
> 
> Perhaps there is a better way to factor these out? When you and I spoke,
> we posited that perhaps IPC::Channel could be factored into two interfaces:
> one for receiving and sending messages, and one for setting up the channel.
> 
> Maybe we could end up with different OS-flavors of Channel setup interfaces?
> 
>   IPC::ChannelSetupPosix
>   IPC::ChannelSetupMojo
>   IPC::ChannelSetupNaCl
>   IPC::ChannelSetupWin
> 
> Then, maybe we'd have IPC::{Sync}Channel{Proxy} that can be constructed from
> a IPC::ChannelSetup?
> 
> Or if these methods are needed for more than just setup, we could use another
> name, like PlatformChannel?

Oh I see. I thought setup part is basically just about Create() but you're
right; PosixExtra() thing is also a part of that.
Let me remove PosixExtra from this change and try factor it out in different way
in following changes.

[Hajime Morrita, 2014-06-04 19:46:05]

PTAL? Got rid of PosixExtras.
I'd leave extracting setup bit for another cl.

[jam, 2014-06-04 21:59:06]

On 2014/06/04 19:46:05, morrita wrote:
> PTAL? Got rid of PosixExtras.
> I'd leave extracting setup bit for another cl.

seems that this includes https://codereview.chromium.org/310853003/. can you
separate it out? that way reviewers don't need to look at already reviewed
changes.

[Hajime Morrita, 2014-06-05 00:20:27]

On 2014/06/04 21:59:06, jam wrote:
> On 2014/06/04 19:46:05, morrita wrote:
> > PTAL? Got rid of PosixExtras.
> > I'd leave extracting setup bit for another cl.
> 
> seems that this includes https://codereview.chromium.org/310853003/. can you
> separate it out? that way reviewers don't need to look at already reviewed
> changes.
Oops. I'm sorry for that. This was wrong one.
I uploaded the correct one. PTAL? Now the diff fits in ipc/ directory.

[jam, 2014-06-05 00:28:14]

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:171: virtual base::ProcessId peer_pid() const = 0;
nit: now that this is a virtual method, per style guide this should use camel
case, i.e. GetPeerPID

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:177: virtual bool Send(Message* message) OVERRIDE;
why isn't this one virtual too? it seems that this is a pretty core method that
every platform must implement

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:247: Channel() { }
nit: not needed for an interface

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel_posix.h#...
ipc/ipc_channel_posix.h:61: // Cannel implementation
nit: spelling

[gmorrita, 2014-06-05 01:04:09]

PTAL?

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:171: virtual base::ProcessId peer_pid() const = 0;
On 2014/06/05 00:28:14, jam wrote:
> nit: now that this is a virtual method, per style guide this should use camel
> case, i.e. GetPeerPID

Done.

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:177: virtual bool Send(Message* message) OVERRIDE;
On 2014/06/05 00:28:14, jam wrote:
> why isn't this one virtual too? it seems that this is a pretty core method
that
> every platform must implement

Done.

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:247: Channel() { }
On 2014/06/05 00:28:14, jam wrote:
> nit: not needed for an interface

Done.

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

https://codereview.chromium.org/310293002/diff/40001/ipc/ipc_channel_posix.h#...
ipc/ipc_channel_posix.h:61: // Cannel implementation
On 2014/06/05 00:28:14, jam wrote:
> nit: spelling

Done.

[jam, 2014-06-05 06:02:41]

lgtm

[Hajime Morrita, 2014-06-05 17:15:16]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-06-05 17:16:38]

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

[Hajime Morrita, 2014-06-05 18:44:32]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-06-05 18:44:56]

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

[Hajime Morrita, 2014-06-05 18:45:10]

The CQ bit was unchecked by morrita@chromium.org

[Hajime Morrita, 2014-06-05 18:52:41]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-06-05 18:55:21]

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

[Hajime Morrita, 2014-06-05 19:44:00]

Can anyone owning win8/ rubberstamp this cl?
It just follows a rename on ipc/

[Hajime Morrita, 2014-06-05 22:01:57]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-06-05 22:03:24]

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

[Hajime Morrita, 2014-06-05 23:06:52]

Carlos, Darin: Could you please rubberstamp for win8/ ? Just follows a rename.

[commit-bot: I haz the power, 2014-06-06 02:58:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-06 03:03:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 03:03:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Hajime Morrita, 2014-06-06 16:16:27]

darin, cpu: Would you mind to give me a rubberstamp for win8/ ?

[cpu_(ooo_6.6-7.5), 2014-06-06 19:45:54]

win8 lgtm

[cpu_(ooo_6.6-7.5), 2014-06-06 19:45:54]

win8 lgtm

[Hajime Morrita, 2014-06-06 20:05:30]

Thank you !!

[Hajime Morrita, 2014-06-06 20:05:35]

The CQ bit was checked by morrita@chromium.org

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

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

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

Change committed as 275505