IPC: Introduce MessageAttachment and PlatformFileAttachment.

IPC: Introduce MessageAttachment and PlatformFileAttachment.

This is another preparation for crbug.com/448190.
This change encapsulates a platform file as a MessageAttachment
abstract class and its subclass called PlatformFileAttachment.
This abstraction allows MessageAttachmentSet to contain not only
platform files, but also a mojo MessagePipes, which will be
encapsulated by another MessageAttachment subclass.

BUG=448190
R=agl@chromium.org, viettrungluu@chromium.org

Committed: https://crrev.com/98b7aaad5eddcdadbba66380d2ac7a1b5868af3c
Cr-Commit-Position: refs/heads/master@{#313157}

[agl, 2015-01-15 03:00:54]

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h
File ipc/ipc_message_attachment.h (right):

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h...
ipc/ipc_message_attachment.h:13: // An auxiliary data sent with |Message|. This
can be a platform file descriptor
// Auxiliary data …

("data" is self-plural.)

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h...
ipc/ipc_message_attachment.h:20: TYPE_MOJO_MESSAGE_PIPE  // The instance is a
mojo-based class.
comma at the end

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h...
ipc/ipc_message_attachment.h:23: MessageAttachment();
Why have this empty constructor? (Chromium even allows the "= deleted" syntax
now.)

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
File ipc/ipc_message_attachment_set.h (right):

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:33: // Return the number of attachmentes
attachments

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:36: unsigned file_size() const;
file_size seems like a poor name for this.

num_fds()? num_descriptors()?

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:41: scoped_refptr<MessageAttachment>
TakeAttachmentAt(unsigned index);
It's called TakeAttachmentAt, but it returns a reference—not ownership. Should
it have the word "Take" in the name?

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:120: // A vector of owning descriptors If this
message is sent, then file
. after "descriptors"

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:122: // are closed. If this message has been
received, then all received
no comma before "then" (existing issue).

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_platform_file_attach...
File ipc/ipc_platform_file_attachment.h (right):

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_platform_file_attach...
ipc/ipc_platform_file_attachment.h:14: // A platform file that is sent over
|Channel| with as a part of |Message|.
typo: "with as part of"

[Hajime Morrita, 2015-01-23 00:29:05]

I'm back! Sorry for the slow response. PTAL?

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h
File ipc/ipc_message_attachment.h (right):

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h...
ipc/ipc_message_attachment.h:13: // An auxiliary data sent with |Message|. This
can be a platform file descriptor
On 2015/01/15 03:00:53, agl wrote:
> // Auxiliary data …
> 
> ("data" is self-plural.)

Done.

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h...
ipc/ipc_message_attachment.h:20: TYPE_MOJO_MESSAGE_PIPE  // The instance is a
mojo-based class.
On 2015/01/15 03:00:53, agl wrote:
> comma at the end

Done.

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment.h...
ipc/ipc_message_attachment.h:23: MessageAttachment();
On 2015/01/15 03:00:53, agl wrote:
> Why have this empty constructor? (Chromium even allows the "= deleted" syntax
> now.)
I was confused with some other chromium-specific rules. Removed.
Thanks for the catch!

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
File ipc/ipc_message_attachment_set.h (right):

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:33: // Return the number of attachmentes
On 2015/01/15 03:00:53, agl wrote:
> attachments

Done.

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:36: unsigned file_size() const;
On 2015/01/15 03:00:53, agl wrote:
> file_size seems like a poor name for this.
> 
> num_fds()? num_descriptors()?
Makes sense. Renamed to num_descriptors().

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:41: scoped_refptr<MessageAttachment>
TakeAttachmentAt(unsigned index);
On 2015/01/15 03:00:53, agl wrote:
> It's called TakeAttachmentAt, but it returns a reference—not ownership. Should
> it have the word "Take" in the name?
Uh, the name was leftover of my failed experiment. Ranamed to Get...().

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:120: // A vector of owning descriptors If this
message is sent, then file
On 2015/01/15 03:00:53, agl wrote:
> . after "descriptors"

Done.

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_message_attachment_s...
ipc/ipc_message_attachment_set.h:122: // are closed. If this message has been
received, then all received
On 2015/01/15 03:00:53, agl wrote:
> no comma before "then" (existing issue).

Done.

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_platform_file_attach...
File ipc/ipc_platform_file_attachment.h (right):

https://codereview.chromium.org/856443003/diff/1/ipc/ipc_platform_file_attach...
ipc/ipc_platform_file_attachment.h:14: // A platform file that is sent over
|Channel| with as a part of |Message|.
On 2015/01/15 03:00:53, agl wrote:
> typo: "with as part of"

Done.

[Hajime Morrita, 2015-01-26 19:55:23]

Adam, could you take another look at this as well?
Thanks!

[agl, 2015-01-26 20:00:24]

lgtm

[Hajime Morrita, 2015-01-26 20:34:00]

Thanks!

[Hajime Morrita, 2015-01-26 20:34:05]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2015-01-26 20:35:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856443003/20001

[commit-bot: I haz the power, 2015-01-26 20:56:05]

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

[commit-bot: I haz the power, 2015-01-26 20:56:06]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Hajime Morrita, 2015-01-26 21:14:33]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2015-01-26 21:15:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856443003/40001

[commit-bot: I haz the power, 2015-01-26 22:43:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-26 22:45:25]

Patchset 3 (id:??) landed as
https://crrev.com/98b7aaad5eddcdadbba66380d2ac7a1b5868af3c
Cr-Commit-Position: refs/heads/master@{#313157}