SyncSocket Transit Descriptor - refactoring

base/sync_socket_posix.cc

Index: base/sync_socket_posix.cc
diff --git a/base/sync_socket_posix.cc b/base/sync_socket_posix.cc
index 93a01d1b1c20699e134136c47d403f26837c9f44..710653a551e8ec102248f0b052a71efabb142838 100644
--- a/base/sync_socket_posix.cc
+++ b/base/sync_socket_posix.cc
@@ -96,6 +96,21 @@ bool SyncSocket::CreatePair(SyncSocket* socket_a, SyncSocket* socket_b) {
   return true;
 }
 
+// static
+SyncSocket::Handle SyncSocket::UnwrapHandle(
+    const SyncSocket::TransitDescriptor& descriptor) {
+  return (SyncSocket::Handle)descriptor.fd;

[tommi (sloooow) - chröme, 2014/09/04 11:34:10]

use C++ style casts

[burnik, 2014/09/06 18:17:09]

Done. And casting removed.

+}
+
+bool SyncSocket::PrepareTransitDescriptor(
+    ProcessHandle peer_process_handle,
+    SyncSocket::TransitDescriptor* descriptor) {
+  // |TransitDescriptor| on POSIX is a |FileDescriptor|.

[henrika (OOO until Aug 14), 2014/09/02 07:46:06]

Not clear what this comment provides. The type FileDescriptor is not used in the
method. Need another reference perhaps?

[burnik, 2014/09/02 10:02:54]

TransitDescriptor is a platform dependent type.
typedef FileDescriptor TransitDescriptor; // for POSIX

Comment is here to state that this should be the point where one should actually
use the members of the |TransitDescriptor|. Externally from the SyncSocket, the
descriptor should just be prepared and/or unwrapped.

Maybe the comment should be:
See |TransitDescriptor| when OS_POSIX.



On 2014/09/02 07:46:06, henrika wrote:

[henrika (OOO until Aug 14), 2014/09/02 10:26:38]

It does look odd to emphasize a member which is not used as function argument.

[burnik, 2014/09/02 15:01:34]

Suggestions?
On 2014/09/02 10:26:38, henrika wrote:

[tommi (sloooow) - chröme, 2014/09/04 11:34:10]

Perhaps it would be more explicit to simply say here that:
// On POSIX, TransitDescriptor is typedef'ed as:
// typedef FileDescriptor TransitDescriptor;
// (etc)

[burnik, 2014/09/06 18:17:09]

I've removed this comment alltogether.
On 2014/09/04 11:34:10, tommi wrote:

+  descriptor->fd = handle();
+  descriptor->auto_close = false;

[henrika (OOO until Aug 14), 2014/09/02 07:46:06]

Is this a new behavior or was it false before as well?

[burnik, 2014/09/02 10:02:54]

In all uses it was done this way. 
See lefts on:
- audio_input_sync_writer.cc
- audio_sync_reader.cc

On 2014/09/02 07:46:06, henrika wrote:

+  return (descriptor->fd != -1);

[tommi (sloooow) - chröme, 2014/09/04 11:34:10]

nit: no need for ()

[burnik, 2014/09/06 18:17:09]

Done.

+}
+
 bool SyncSocket::Close() {
   const bool retval = CloseHandle(handle_);
   handle_ = kInvalidHandle;