Add non-blocking peek for the syncsocket...

Add non-blocking peek for the syncsocket
- Windows only

BUG=none
TEST=unit test included



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33876

[cpu_(ooo_6.6-7.5), 2009-12-03 18:55:12]

ideas for posix are welcome

[Paweł Hajdan Jr., 2009-12-03 18:57:45]

Drive-by.

http://codereview.chromium.org/464020/diff/1/3
File base/sync_socket_posix.cc (right):

http://codereview.chromium.org/464020/diff/1/3#newcode101
base/sync_socket_posix.cc:101: // TODO: port. Some kind of select?
nit: It's TODO(port).

http://codereview.chromium.org/464020/diff/1/3#newcode102
base/sync_socket_posix.cc:102: size_t SyncSocket::Peek() {
How about a NOTIMPLEMENTED here? Looks like a little debug mystery in case
someone tries to use it.

[John Grabowski, 2009-12-03 19:10:22]

drive by

http://codereview.chromium.org/464020/diff/1/3
File base/sync_socket_posix.cc (right):

http://codereview.chromium.org/464020/diff/1/3#newcode102
base/sync_socket_posix.cc:102: size_t SyncSocket::Peek() {
Perhaps have Peek() return a boolean?  select() doesn't tell you data length,
only that data is available.

[cpu_(ooo_6.6-7.5), 2009-12-03 19:45:14]

pawel, john I've updated the code please look again.

http://codereview.chromium.org/464020/diff/1/3
File base/sync_socket_posix.cc (right):

http://codereview.chromium.org/464020/diff/1/3#newcode101
base/sync_socket_posix.cc:101: // TODO: port. Some kind of select?
On 2009/12/03 18:57:45, Paweł Hajdan Jr. wrote:
> nit: It's TODO(port).

Done.

http://codereview.chromium.org/464020/diff/1/3#newcode102
base/sync_socket_posix.cc:102: size_t SyncSocket::Peek() {
On 2009/12/03 19:10:22, John Grabowski wrote:
> Perhaps have Peek() return a boolean?  select() doesn't tell you data length,
> only that data is available.

I think testing for > 0 will work for cross platform code. That is not to say
that I am vehemently opposed but I'll like to wait until the posix version lands
to make this change.



Done.

[Paweł Hajdan Jr., 2009-12-03 19:47:34]

LGTM (for the drive-by comments).

[John Grabowski, 2009-12-03 20:19:28]

http://codereview.chromium.org/464020/diff/1/3
File base/sync_socket_posix.cc (right):

http://codereview.chromium.org/464020/diff/1/3#newcode102
base/sync_socket_posix.cc:102: size_t SyncSocket::Peek() {
On 2009/12/03 19:45:14, cpu wrote:
> On 2009/12/03 19:10:22, John Grabowski wrote:
> > Perhaps have Peek() return a boolean?  select() doesn't tell you data
length,
> > only that data is available.
> 
> I think testing for > 0 will work for cross platform code. That is not to say
> that I am vehemently opposed but I'll like to wait until the posix version
lands
> to make this change.
> 
> 
> 
> Done.

You wouldn't want a caller to allocate a buffer based on the return value if on
some platforms you don't have the actual size.  Yes, ">0" solves the problem but
a bool makes it explicit in the API.  Better to make the API correct the first
time to be sure assumptions aren't made.

In any case, LGTM.

[sehr (please use chromium), 2009-12-03 22:17:08]

On 2009/12/03 20:19:28, John Grabowski wrote:
> http://codereview.chromium.org/464020/diff/1/3
> File base/sync_socket_posix.cc (right):
> 
> http://codereview.chromium.org/464020/diff/1/3#newcode102
> base/sync_socket_posix.cc:102: size_t SyncSocket::Peek() {
> On 2009/12/03 19:45:14, cpu wrote:
> > On 2009/12/03 19:10:22, John Grabowski wrote:
> > > Perhaps have Peek() return a boolean?  select() doesn't tell you data
> length,
> > > only that data is available.
> > 
> > I think testing for > 0 will work for cross platform code. That is not to
say
> > that I am vehemently opposed but I'll like to wait until the posix version
> lands
> > to make this change.
> > 
> > 
> > 
> > Done.
> 
> You wouldn't want a caller to allocate a buffer based on the return value if
on
> some platforms you don't have the actual size.  Yes, ">0" solves the problem
but
> a bool makes it explicit in the API.  Better to make the API correct the first
> time to be sure assumptions aren't made.
> 
> In any case, LGTM.

LGTM.
Consulting with my Unix buddies I think it's going to be hard to determine the
number of bytes read but not written because they could be written by multiple
calls.  It's probably better to have a boolean, as suggested.  Otherwise, I'll
be glad to work on the ioctl magic to get it to work for Linux and Mac.

[cpu_(ooo_6.6-7.5), 2009-12-04 21:45:22]

I accept the help for posix :)

I added a big warning on the header about using the returned value. I am ok if
after the posix impl we change it to be a boolean.