Binding media stream audio track to speech recognition [renderer]

base/native_sync_socket.h

+// Copyright 2014 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.
+
+#ifndef BASE_NATIVE_SYNC_SOCKET_H_
+#define BASE_NATIVE_SYNC_SOCKET_H_
+
+#include "base/process/process_handle.h"
+#include "base/sync_socket.h"
+
+#if defined(OS_POSIX)
+#include "base/file_descriptor_posix.h"
+#endif
+
+namespace base {
+
+// Helper class for using |SyncSocket| cross-platform
+class NativeSyncSocket {

[tommi (sloooow) - chröme, 2014/08/29 11:25:30]

This class seems to rely on SyncSocket but doesn't provide any non-static
functionality (i.e. it appears to be more of a namespace).  Why wouldn't we
simply add these methods to SyncSocket instead of introducing new files for
these very basic helpers?

[no longer working on chromium, 2014/08/29 12:23:06]

You and I should have a discussion with Tommi on if we should have this new
class in base/ or not.

[burnik, 2014/08/29 13:26:16]

Agreed. Socket should be more cross-platform friendly.
On 2014/08/29 12:23:06, xians1 wrote:

[burnik, 2014/08/29 13:26:16]

I agree. SyncSocket should have a descriptor which is cross-platform to avoid
these checks (as it's done in multiple places where code relies on using the
|SyncSocket|). Also should provide a method to prepare it for transit.
On 2014/08/29 11:25:30, tommi wrote:

+ public:
+#if defined(OS_WIN)
+  typedef base::SyncSocket::Handle Descriptor;
+
+  static bool PrepareForeignSocketDescriptor(

[tommi (sloooow) - chröme, 2014/08/29 11:25:30]

implementation should be in the cc file

[tommi (sloooow) - chröme, 2014/08/29 11:25:30]

I know that this method name comes from elsewhere, but if we're moving it into
base/ I think we should come up with a more descriptive name that makes it clear
that handover from one process to another is taking place.

[burnik, 2014/08/29 13:26:16]

Acknowledged.

[burnik, 2014/08/29 13:26:16]

Agreed. I think name should be something like |PreparePeerSocketDescriptor|. 

On 2014/08/29 11:25:30, tommi wrote:

+      base::ProcessHandle process_handle,
+      base::SyncSocket * foreign_socket,
+      base::SyncSocket::Handle * foreign_handle) {
+    ::DuplicateHandle(GetCurrentProcess(), foreign_socket->handle(),
+                      process_handle, foreign_handle,
+                      0, FALSE, DUPLICATE_SAME_ACCESS);
+    return (*foreign_handle != 0);
+  }
+
+  static base::SyncSocket::Handle Unwrap(const Descriptor& descriptor) {
+    return descriptor;
+  }
+
+#else
+  typedef base::FileDescriptor Descriptor;
+
+  static bool PrepareForeignSocketDescriptor(
+      base::ProcessHandle process_handle,
+      base::SyncSocket * foreign_socket,
+      base::FileDescriptor * foreign_handle) {
+    foreign_handle->fd = foreign_socket->handle();
+    foreign_handle->auto_close = false;
+    return (foreign_handle->fd != -1);
+  }
+
+  static int Unwrap(const Descriptor& descriptor) {

[tommi (sloooow) - chröme, 2014/08/29 11:25:30]

use base::SyncSocket::Handle here as well as the return value.

The implementation should be in the .cc file and once you've fixed the return
type, the #if defined() check can be inside the function and we can avoid having
multiple declarations of the function.
I also think that this could cause compiler warnings (which could break the
build) at some higher levels since although Handle might be typedefed as an int,
it's still a specific type.  Calling code should ideally not have to do the #if
checks.

[burnik, 2014/08/29 13:26:16]

Good point. When impl moves to .cc it will make more sense.
On 2014/08/29 11:25:30, tommi wrote:

+    return descriptor.fd;
+  }
+#endif
+};
+
+}  // namespace base
+#endif  // BASE_NATIVE_SYNC_SOCKET_H_