SyncSocket Transit Descriptor - refactoring

base/sync_socket_win.cc

Index: base/sync_socket_win.cc
diff --git a/base/sync_socket_win.cc b/base/sync_socket_win.cc
index 26e76ec2738e6629fbf4b0c41cc61ac0b7e7d6ec..f1f2445376f60f18c0634cd114da7cb579c24bfe 100644
--- a/base/sync_socket_win.cc
+++ b/base/sync_socket_win.cc
@@ -207,6 +207,21 @@ bool SyncSocket::CreatePair(SyncSocket* socket_a, SyncSocket* socket_b) {
   return CreatePairImpl(&socket_a->handle_, &socket_b->handle_, false);
 }
 
+// static
+SyncSocket::Handle SyncSocket::UnwrapHandle(
+    const SyncSocket::TransitDescriptor& descriptor) {
+  return static_cast<SyncSocket::Handle>(descriptor);

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

nit: "SyncSocket::" necessary?

[burnik, 2014/09/04 12:24:44]

Guess not. Although rest of the code looks kind of this way.
On 2014/09/04 11:34:10, tommi wrote:

[tommi (sloooow) - chröme, 2014/09/04 17:02:25]

The rest of the code only uses it outside the scope of the class from what I can
tell.  Here you're inside the scope, so you don't need it.

[burnik, 2014/09/04 18:04:07]

Done.

+}
+
+bool SyncSocket::PrepareTransitDescriptor(
+    ProcessHandle peer_process_handle,
+    SyncSocket::TransitDescriptor* descriptor) {
+  // |TransitDescriptor| on Windows is the |Handle| itself.
+  ::DuplicateHandle(GetCurrentProcess(), handle(), peer_process_handle,

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

I'd like to check the return value of this call and return false if it fails. 
There's a bug below where you assume that the value of |descriptor| is 0, but it
might not be.

Also, if this call fails, I'd like to have a [D]PLOG(ERROR) for that case.

[burnik, 2014/09/04 12:24:44]

Done.

+                    descriptor, 0, FALSE, DUPLICATE_SAME_ACCESS);
+  return (*descriptor != 0);

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

If the caller does this:

SyncSocket::TransitDescriptor descriptor;
if (foo.PrepareTransitDescriptor(handle, &descriptor))
  Success(descriptor);

you'll most likely be out of luck here and return true even if DuplicateHandle
failed.

nit: no need for ()

[burnik, 2014/09/04 12:24:44]

Yes, good point since this is on the stack.
On 2014/09/04 11:34:10, tommi wrote:

+}
+
 bool SyncSocket::Close() {
   if (handle_ == kInvalidHandle)
     return true;