SyncSocket Transit Descriptor - refactoring

base/sync_socket_posix.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/sync_socket.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
 #include <stdio.h>
@@ skipping 78 matching lines @@
   }
 #endif
 
   // Copy the handles out for successful return.
   socket_a->handle_ = handles[0];
   socket_b->handle_ = handles[1];
 
   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;
   return retval;
 }
 
 size_t SyncSocket::Send(const void* buffer, size_t length) {
   ThreadRestrictions::AssertIOAllowed();
   return SendHelper(handle_, buffer, length);
 }
@@ skipping 114 matching lines @@
   return len;
 }
 
 // static
 bool CancelableSyncSocket::CreatePair(CancelableSyncSocket* socket_a,
                                       CancelableSyncSocket* socket_b) {
   return SyncSocket::CreatePair(socket_a, socket_b);
 }
 
 }  // namespace base