SyncSocket Transit Descriptor - refactoring

Preparing |SyncSocket|'s handle for the peer process is different for POSIX and Windows 
which leads to code duplication, platform #ifdef checks on multiple levels and 
general confusion on how to work with the SyncSocket. 

Typical use case for |SyncSocket|:
1. Browser creates and connects the socket pair - one for browser and one for renderer.
2. Browser prepares the foreign socket (Windows duplicates, POSIX creates file descriptor).
3. Browser relays the foreign socket handle to the renderer via IPC.
4. Renderer receives the IPC and creates the socket using the handle provided.
5. Sockets are ready for send/receive on both ends.

Steps 1-4 get simplified since there is no need to check the platform in order to prepare the socket for transit.

What this CL brings:
1. Adds |SyncSocket::TransitDescriptor| type which wraps the socket handle and is cross-platform.
2. Adds |SyncSocket::PrepareTransitDescriptor| method which is implemented depending on the platform.
3. Adds |SyncSocket::UnwrapHandle| method which unwraps |SyncSocket::Handle| from |SyncSocket::TransitDescriptor|.
4. Removes #ifdefs for platform-checks in code using |SyncSocket| and simplifies preparing the SyncSocket.

Note:
- There is still some less evident duplication in the ppapi and pepper-broker code which should be addressed.
- SyncSocket unit test should also be reviewed.
- There is a similar pattern when using SharedMemory.

BUG=409656
Committed: https://crrev.com/3d67005036e98d91a329e5f70dbc4f1ddb27a1ab
Cr-Commit-Position: refs/heads/master@{#293680}

[burnik, 2014-09-01 08:35:15]

burnik@chromium.org changed reviewers:
+ henrika@chromium.org, tommi@chromium.org, xians@chromium.org

[burnik, 2014-09-01 14:22:14]

Updated description. Please have a look.

[henrika (OOO until Aug 14), 2014-09-01 14:30:43]

Looks like really nice and useful code. I don't know the underlying details well
enough to provide detailed comments. Adding some nits for now.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:59: TransitDescriptor * descriptor);
nit, TransitDescriptor*

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:33: NOTIMPLEMENTED();
Add TODO and link to issue in crbug.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:39: NOTIMPLEMENTED();
Same here.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:106: SyncSocket::TransitDescriptor * descriptor) {
remove space

[no longer working on chromium, 2014-09-01 14:56:19]

Looking good.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:34: typedef Handle TransitDescriptor;
The existing sync socket code is dealing with "handle", how about using
TransitHandle instead of TransitDescriptor?
But I will defer the decision to Tommi, please check with him.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:57: // the foreign handle to the peer process.
s/foreign handle/transit handle/g

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:38: SyncSocket::TransitDescriptor * descriptor) {
indentation. run git-clang-format

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:102: return (SyncSocket::Handle) descriptor.fd;
c++ style, use static_cast

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:106: SyncSocket::TransitDescriptor * descriptor) {
indentation

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:107: // |TransitDescriptor| on POSIX is a
|FileDescriptor|
period by the end

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return (SyncSocket::Handle) descriptor;
ditto

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:217: SyncSocket::TransitDescriptor * descriptor) {
ditto

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_sync_reader.h:43:
base::SyncSocket::TransitDescriptor * descriptor);
remove the space between base::SyncSocket::TransitDescriptor and *
This comment applies to the rest of the CL.

[burnik, 2014-09-01 14:59:34]

Nits done and crbug created: http://crbug.com/409656

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:59: TransitDescriptor * descriptor);
On 2014/09/01 14:30:43, henrika wrote:
> nit, TransitDescriptor*

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:33: NOTIMPLEMENTED();
On 2014/09/01 14:30:43, henrika wrote:
> Add TODO and link to issue in crbug.

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:39: NOTIMPLEMENTED();
On 2014/09/01 14:30:43, henrika wrote:
> Same here.

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:106: SyncSocket::TransitDescriptor * descriptor) {
On 2014/09/01 14:30:43, henrika wrote:
> remove space

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:217: SyncSocket::TransitDescriptor * descriptor) {
Also TransitDescriptor*
Done.

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:112:
base::SyncSocket::TransitDescriptor * descriptor) {
 Nit done.

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.h (right):

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.h:46:
base::SyncSocket::TransitDescriptor * descriptor);
 Nit done.

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_sync_reader.cc:90: process_handle,
descriptor);
Pulled to one line.

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

https://codereview.chromium.org/525313002/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_sync_reader.h:43:
base::SyncSocket::TransitDescriptor * descriptor);
Nit done.

[burnik, 2014-09-01 15:08:03]

Thanks for the clang-format tip. :-)

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:34: typedef Handle TransitDescriptor;
Both work good for me. Agreed. Tommi should have a say.
On 2014/09/01 14:56:18, xians1 wrote:
> The existing sync socket code is dealing with "handle", how about using
> TransitHandle instead of TransitDescriptor?
> But I will defer the decision to Tommi, please check with him.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:57: // the foreign handle to the peer process.
If we change to |TransitHandle| then agreed.
On 2014/09/01 14:56:18, xians1 wrote:
> s/foreign handle/transit handle/g

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:38: SyncSocket::TransitDescriptor * descriptor) {
On 2014/09/01 14:56:18, xians1 wrote:
> indentation. run git-clang-format

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:107: // |TransitDescriptor| on POSIX is a
|FileDescriptor|
On 2014/09/01 14:56:18, xians1 wrote:
> period by the end

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return (SyncSocket::Handle) descriptor;
On 2014/09/01 14:56:18, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:217: SyncSocket::TransitDescriptor * descriptor) {
On 2014/09/01 14:56:18, xians1 wrote:
> ditto

Done.

[burnik, 2014-09-01 15:18:21]

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:219: // |TransitDescriptor| on Windows is the |Handle|
itself
I don't always finish a sentence, but when I do

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

LGTM

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:33: // TODO(burnik): Still unclear how NaCl uses
SyncSocket.
Nit, not sure if you should add put it on xians instead. Just don't want it to
be hanging when you leave.

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:108: // |TransitDescriptor| on POSIX is a
|FileDescriptor|.
Not clear what this comment provides. The type FileDescriptor is not used in the
method. Need another reference perhaps?

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:110: descriptor->auto_close = false;
Is this a new behavior or was it false before as well?

https://codereview.chromium.org/525313002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (left):

https://codereview.chromium.org/525313002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_renderer_host.cc:181: #if
defined(OS_WIN)
Nice to get rid of this ;-)

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:53: uint32 /* length */, uint32 /* segment
count */)
I think these should go on one line each.

https://codereview.chromium.org/525313002/diff/40001/content/renderer/media/a...
File content/renderer/media/audio_message_filter_unittest.cc (right):

https://codereview.chromium.org/525313002/diff/40001/content/renderer/media/a...
content/renderer/media/audio_message_filter_unittest.cc:88: //
AudioMsg_NotifyStreamCreated
What does this comment mean really?

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

Thanks!

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:33: // TODO(burnik): Still unclear how NaCl uses
SyncSocket.
If Shijing agrees, I will make it so.
On 2014/09/02 07:46:05, henrika wrote:
> Nit, not sure if you should add put it on xians instead. Just don't want it to
> be hanging when you leave.

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:108: // |TransitDescriptor| on POSIX is a
|FileDescriptor|.
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:
> Not clear what this comment provides. The type FileDescriptor is not used in
the
> method. Need another reference perhaps?

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:110: descriptor->auto_close = false;
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:
> Is this a new behavior or was it false before as well?

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:219: // |TransitDescriptor| on Windows is the |Handle|
itself
On 2014/09/01 15:18:20, burnik wrote:
> I don't always finish a sentence, but when I do

Done.

https://codereview.chromium.org/525313002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (left):

https://codereview.chromium.org/525313002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_renderer_host.cc:181: #if
defined(OS_WIN)
:-)
On 2014/09/02 07:46:06, henrika wrote:
> Nice to get rid of this ;-)

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:53: uint32 /* length */, uint32 /* segment
count */)
clang-format applied to all -- oops.
Done.

On 2014/09/02 07:46:06, henrika wrote:
> I think these should go on one line each.

https://codereview.chromium.org/525313002/diff/40001/content/renderer/media/a...
File content/renderer/media/audio_message_filter_unittest.cc (right):

https://codereview.chromium.org/525313002/diff/40001/content/renderer/media/a...
content/renderer/media/audio_message_filter_unittest.cc:88: //
AudioMsg_NotifyStreamCreated
Not my doing. I guess it means that a message named
'AudioMsg_NotifyStreamCreated' is hooked here. :-D
On 2014/09/02 07:46:06, henrika wrote:
> What does this comment mean really?

[burnik, 2014-09-02 10:05:20]

The CQ bit was checked by burnik@chromium.org

[burnik, 2014-09-02 10:05:22]

The CQ bit was unchecked by burnik@chromium.org

[no longer working on chromium, 2014-09-02 10:25:04]

I think you missed the audio_output_device_unittest.cc

Also, you should add stamps from owners of base/ and
content/common/media/audio_messages.h to be able to land this CL.

some more nits, lgtm iff you address them.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:57: // the foreign handle to the peer process.
On 2014/09/01 15:08:02, burnik wrote:
> If we change to |TransitHandle| then agreed.
> On 2014/09/01 14:56:18, xians1 wrote:
> > s/foreign handle/transit handle/g
> 

There is no "foreign" concept in the code or comments of sync_socket.h, and in
real use case, clients are passing the TransmitDescript to the peer process.
I think you you need to change foreign to transit regardless the naming of
TransitHandle.

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:219: // |TransitDescriptor| on Windows is the |Handle|
itself
On 2014/09/02 10:02:54, burnik wrote:
> On 2014/09/01 15:18:20, burnik wrote:
> > I don't always finish a sentence, but when I do
> 
> Done.

It doesn't seem done yet.

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:44: base::SyncSocket::TransitDescriptor /*
socket handle */,
Either change the name to TransitHandle or update the comment to socket
descriptor

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:52: base::SyncSocket::TransitDescriptor /*
socket handle */,
ditto

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:53: uint32 /* length */, uint32 /* segment
count */)
On 2014/09/02 10:02:54, burnik wrote:
> clang-format applied to all -- oops.
> Done.
> 
> On 2014/09/02 07:46:06, henrika wrote:
> > I think these should go on one line each.
> 


?? could you double check, it should be one line each.

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

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:108: // |TransitDescriptor| on POSIX is a
|FileDescriptor|.
It does look odd to emphasize a member which is not used as function argument.

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

Added audio_output_device_unittest.cc

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket.h#newco...
base/sync_socket.h:57: // the foreign handle to the peer process.
On 2014/09/02 10:25:04, xians1 wrote:
> On 2014/09/01 15:08:02, burnik wrote:
> > If we change to |TransitHandle| then agreed.
> > On 2014/09/01 14:56:18, xians1 wrote:
> > > s/foreign handle/transit handle/g
> > 
> 
> There is no "foreign" concept in the code or comments of sync_socket.h, and in
> real use case, clients are passing the TransmitDescript to the peer process.
> I think you you need to change foreign to transit regardless the naming of
> TransitHandle.

Done.

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/20001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:102: return (SyncSocket::Handle) descriptor.fd;
On 2014/09/01 14:56:18, xians1 wrote:
> c++ style, use static_cast

Done.

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:108: // |TransitDescriptor| on POSIX is a
|FileDescriptor|.
Suggestions?
On 2014/09/02 10:26:38, henrika wrote:
> It does look odd to emphasize a member which is not used as function argument.

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:44: base::SyncSocket::TransitDescriptor /*
socket handle */,
On 2014/09/02 10:25:04, xians1 wrote:
> Either change the name to TransitHandle or update the comment to socket
> descriptor

Done.

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:52: base::SyncSocket::TransitDescriptor /*
socket handle */,
On 2014/09/02 10:25:04, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:53: uint32 /* length */, uint32 /* segment
count */)
I've ran 
git-cl-format -f master -- content/common/media/audio_messages.h
It doesn't put them on separate lines.

On 2014/09/02 10:25:04, xians1 wrote:
> On 2014/09/02 10:02:54, burnik wrote:
> > clang-format applied to all -- oops.
> > Done.
> > 
> > On 2014/09/02 07:46:06, henrika wrote:
> > > I think these should go on one line each.
> > 
> 
> 
> ?? could you double check, it should be one line each.

[burnik, 2014-09-02 15:08:46]

burnik@chromium.org changed reviewers:
+ darin@chromium.org, dcheng@chromium.org

[no longer working on chromium, 2014-09-02 15:09:01]

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:53: uint32 /* length */, uint32 /* segment
count */)
On 2014/09/02 15:01:34, burnik wrote:
> I've ran 
> git-cl-format -f master -- content/common/media/audio_messages.h
> It doesn't put them on separate lines.
> 
> On 2014/09/02 10:25:04, xians1 wrote:
> > On 2014/09/02 10:02:54, burnik wrote:
> > > clang-format applied to all -- oops.
> > > Done.
> > > 
> > > On 2014/09/02 07:46:06, henrika wrote:
> > > > I think these should go on one line each.
> > > 
> > 
> > 
> > ?? could you double check, it should be one line each.
> 

The script might not be always correct, you should check the existing
audio_messages.h.

[burnik, 2014-09-04 09:04:57]

I would appreciate any thoughts or suggestions from darin@ and tommi@.

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/525313002/diff/40001/content/common/media/aud...
content/common/media/audio_messages.h:53: uint32 /* length */, uint32 /* segment
count */)
On 2014/09/02 15:09:01, xians1 wrote:
> On 2014/09/02 15:01:34, burnik wrote:
> > I've ran 
> > git-cl-format -f master -- content/common/media/audio_messages.h
> > It doesn't put them on separate lines.
> > 
> > On 2014/09/02 10:25:04, xians1 wrote:
> > > On 2014/09/02 10:02:54, burnik wrote:
> > > > clang-format applied to all -- oops.
> > > > Done.
> > > > 
> > > > On 2014/09/02 07:46:06, henrika wrote:
> > > > > I think these should go on one line each.
> > > > 
> > > 
> > > 
> > > ?? could you double check, it should be one line each.
> > 
> 
> The script might not be always correct, you should check the existing
> audio_messages.h.

Done.

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

I think this is good cleanup.  I found one issue below (some nits too).  I see
that the issue was there before, but please fix it :)

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:35: NOTIMPLEMENTED();
what platforms will this run on?

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:102: return (SyncSocket::Handle)descriptor.fd;
use C++ style casts

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:108: // |TransitDescriptor| on POSIX is a
|FileDescriptor|.
On 2014/09/02 10:26:38, henrika wrote:
> It does look odd to emphasize a member which is not used as function argument.

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

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:111: return (descriptor->fd != -1);
nit: no need for ()

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return (SyncSocket::Handle)descriptor;
use C++ casts

(is a cast actually necessary here?)

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:219: // |TransitDescriptor| on Windows is the |Handle|
itself
On 2014/09/02 10:25:04, xians1 wrote:
> On 2014/09/02 10:02:54, burnik wrote:
> > On 2014/09/01 15:18:20, burnik wrote:
> > > I don't always finish a sentence, but when I do
> > 
> > Done.
> 
> It doesn't seem done yet.

Yeah, that sentence is only half

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:222: return (*descriptor != 0);
nit: no ()

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:111: return (descriptor->fd != -1);
Should you be using kInvalidHandle here?

nit: () not necessary

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return static_cast<SyncSocket::Handle>(descriptor);
nit: "SyncSocket::" necessary?

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:220: ::DuplicateHandle(GetCurrentProcess(), handle(),
peer_process_handle,
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.

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:222: return (*descriptor != 0);
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]

Checks for windows added. Nits addressed.

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:111: return (descriptor->fd != -1);
On 2014/09/04 11:34:10, tommi wrote:
> Should you be using kInvalidHandle here?
> 
> nit: () not necessary

Done.

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return static_cast<SyncSocket::Handle>(descriptor);
Guess not. Although rest of the code looks kind of this way.
On 2014/09/04 11:34:10, tommi wrote:
> nit: "SyncSocket::" necessary?

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:220: ::DuplicateHandle(GetCurrentProcess(), handle(),
peer_process_handle,
On 2014/09/04 11:34:10, tommi wrote:
> 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.

Done.

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:222: return (*descriptor != 0);
Yes, good point since this is on the stack.
On 2014/09/04 11:34:10, tommi wrote:
> 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 ()

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

only very minor things

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return static_cast<SyncSocket::Handle>(descriptor);
On 2014/09/04 12:24:44, burnik wrote:
> Guess not. Although rest of the code looks kind of this way.
> On 2014/09/04 11:34:10, tommi wrote:
> > nit: "SyncSocket::" necessary?
> 

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.

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:111: return descriptor->fd !=
SyncSocket::kInvalidHandle;
nit: SyncSocket:: not necessary

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:226: if (*descriptor == 0) {
I would remove this block.  If DuplicateHandle has succeeded and *descriptor is
NULL, then the world is not what we expect it to be :)

Checking for NULL (preferred over 0 for HANDLE), might also not be correct
anyway since INVALID_HANDLE_VALUE on windows, is -1 (that's also what
kInvalidHandle is set to).

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

Agreed.

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/60001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return static_cast<SyncSocket::Handle>(descriptor);
On 2014/09/04 17:02:25, tommi wrote:
> On 2014/09/04 12:24:44, burnik wrote:
> > Guess not. Although rest of the code looks kind of this way.
> > On 2014/09/04 11:34:10, tommi wrote:
> > > nit: "SyncSocket::" necessary?
> > 
> 
> 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.

Done.

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:111: return descriptor->fd !=
SyncSocket::kInvalidHandle;
On 2014/09/04 17:02:25, tommi wrote:
> nit: SyncSocket:: not necessary

Done.

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/80001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:226: if (*descriptor == 0) {
On 2014/09/04 17:02:25, tommi wrote:
> I would remove this block.  If DuplicateHandle has succeeded and *descriptor
is
> NULL, then the world is not what we expect it to be :)
> 
> Checking for NULL (preferred over 0 for HANDLE), might also not be correct
> anyway since INVALID_HANDLE_VALUE on windows, is -1 (that's also what
> kInvalidHandle is set to).

Done.

[tommi (sloooow) - chröme, 2014-09-04 19:47:00]

lgtm

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_posix....
base/sync_socket_posix.cc:107: DCHECK(descriptor);
nit: we generally don't add DCHECKs like this when you dereference the pointer
anyway right thereafter.

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_win.cc...
base/sync_socket_win.cc:218: DCHECK(descriptor);
this dcheck on the other hand is useful

[burnik, 2014-09-05 08:17:42]

Thx. :)

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_win.cc...
base/sync_socket_win.cc:218: DCHECK(descriptor);
On 2014/09/04 19:47:00, tommi wrote:
> this dcheck on the other hand is useful

Acknowledged.

[burnik, 2014-09-05 08:43:55]

burnik@chromium.org changed reviewers:
+ nasko@chromium.org
- dcheng@chromium.org

[nasko, 2014-09-05 17:31:52]

IPC LGTM and a couple of drive-by style nits.

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
content/renderer/media/audio_input_message_filter.cc:128:
base::SyncSocket::TransitDescriptor socket_descriptor, uint32 length,
style: all parameters must be on their own line

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
content/renderer/media/audio_message_filter.cc:170:
base::SyncSocket::TransitDescriptor socket_descriptor, uint32 length) {
style: all parameters must be on their own line

[darin (slow to review), 2014-09-05 17:52:08]

LGTM

[burnik, 2014-09-06 18:17:10]

Thanks all!

FYI:
Looks like some of the bots have unrelated build/test issues.

Anyone care to land this for me (tommi@, xians@)? :-)

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc
File base/sync_socket_nacl.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_nacl.cc...
base/sync_socket_nacl.cc:35: NOTIMPLEMENTED();
NOTIMPLEMENTED is used in the rest of this NaCl SyncSocket so it should be safe
to use it here. It should run in any NaCl env. Point is that a test should fail
on this code once someone starts using it. Looks to me like a lot of SyncSocket
methods on NaCl are yet to be implemented here.

On 2014/09/04 11:34:09, tommi wrote:
> what platforms will this run on?

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:102: return (SyncSocket::Handle)descriptor.fd;
On 2014/09/04 11:34:10, tommi wrote:
> use C++ style casts

Done. And casting removed.

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:108: // |TransitDescriptor| on POSIX is a
|FileDescriptor|.
I've removed this comment alltogether.
On 2014/09/04 11:34:10, tommi wrote:
> On 2014/09/02 10:26:38, henrika wrote:
> > It does look odd to emphasize a member which is not used as function
argument.
> 
> Perhaps it would be more explicit to simply say here that:
> // On POSIX, TransitDescriptor is typedef'ed as:
> // typedef FileDescriptor TransitDescriptor;
> // (etc)

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_posix.c...
base/sync_socket_posix.cc:111: return (descriptor->fd != -1);
On 2014/09/04 11:34:10, tommi wrote:
> nit: no need for ()

Done.

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:213: return (SyncSocket::Handle)descriptor;
Casting removed.
On 2014/09/04 11:34:10, tommi wrote:
> use C++ casts
> 
> (is a cast actually necessary here?)

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:219: // |TransitDescriptor| on Windows is the |Handle|
itself
Comment removed.
On 2014/09/04 11:34:10, tommi wrote:
> On 2014/09/02 10:25:04, xians1 wrote:
> > On 2014/09/02 10:02:54, burnik wrote:
> > > On 2014/09/01 15:18:20, burnik wrote:
> > > > I don't always finish a sentence, but when I do
> > > 
> > > Done.
> > 
> > It doesn't seem done yet.
> 
> Yeah, that sentence is only half

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:219: // |TransitDescriptor| on Windows is the |Handle|
itself
On 2014/09/02 10:25:04, xians1 wrote:
> On 2014/09/02 10:02:54, burnik wrote:
> > On 2014/09/01 15:18:20, burnik wrote:
> > > I don't always finish a sentence, but when I do
> > 
> > Done.
> 
> It doesn't seem done yet.

Acknowledged.

https://codereview.chromium.org/525313002/diff/40001/base/sync_socket_win.cc#...
base/sync_socket_win.cc:222: return (*descriptor != 0);
Refactored in latest.
On 2014/09/04 11:34:10, tommi wrote:
> nit: no ()

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/525313002/diff/120001/base/sync_socket_posix....
base/sync_socket_posix.cc:107: DCHECK(descriptor);
On 2014/09/04 19:47:00, tommi wrote:
> nit: we generally don't add DCHECKs like this when you dereference the pointer
> anyway right thereafter.

Done.

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
content/renderer/media/audio_input_message_filter.cc:128:
base::SyncSocket::TransitDescriptor socket_descriptor, uint32 length,
On 2014/09/05 17:31:52, nasko wrote:
> style: all parameters must be on their own line

Done.

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/525313002/diff/120001/content/renderer/media/...
content/renderer/media/audio_message_filter.cc:170:
base::SyncSocket::TransitDescriptor socket_descriptor, uint32 length) {
On 2014/09/05 17:31:52, nasko wrote:
> style: all parameters must be on their own line

Done.

[tommi (sloooow) - chröme, 2014-09-08 04:05:14]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-09-08 04:06:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/burnik@chromium.org/525313002/140001

[burnik, 2014-09-08 06:51:37]

The CQ bit was unchecked by burnik@chromium.org

[burnik, 2014-09-08 06:51:37]

The CQ bit was checked by burnik@chromium.org

[commit-bot: I haz the power, 2014-09-08 07:01:39]

Committed patchset #8 (id:140001) as 35fa8e415c1b9c71f5b806998ef310be048b5183

[commit-bot: I haz the power, 2014-09-10 03:44:52]

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