[GCM] Initial work to set up directory structure and introduce socket integration

[GCM] Initial work to set up directory structure and introduce socket integration

Create new google_api/gcm directory, and add some of the initial base logic
for integrating protobufs with chrome sockets.

A SocketStream is an implementation of the ZeroCopyStream interfaces that allows
for asynchronously retrieving a message (with timeout support) which can then
be parsed by a CodedInputStream, or alternatively allows a CodedOutputStream
to write into a Chrome socket.

Once a SocketStream closes itself (which will happen on error or timeout), the
stream cannot be reused, and a new one should be created (typically with a
socket that has been reconnected). In general, the state of the socket stream
must be checked before interacting with it.

Also fixes issue in SocketTestUtil where mock Sockets aren't taking ownership
of IOBuffers that they're using (exposed by tests).

TBR=darin@chromium.org
BUG=284553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229533

[Nicolas Zea, 2013-09-03 20:14:57]

Ryan, this is the socket integration I mentioned during the eng review. Mind
taking a look to make sure I'm using the sockets in a appropriate method (or
passing this on to someone else familiar with the usage)?

[Ryan Sleevi, 2013-09-03 23:04:07]

Initial comments, based in part on what we chatted about.

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/gcm_e...
File google_apis/gcm/base/gcm_export.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/gcm_e...
google_apis/gcm/base/gcm_export.h:16: #define GCM_EXPORT_PRIVATE
__declspec(dllimport)
You shouldn't need a _PRIVATE export for new components

See this thread at
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/XMDgIB2nn-0/j6WUq...

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
google_apis/gcm/base/socket_stream.h:35: : public
google::protobuf::io::ZeroCopyInputStream {
So, as I mentioned over Chat, I think we need to be careful here.

In order to really leverage Chrome's network stack, you'll want to be doing the
IO reads on the actual IO thread. Even Chromoting is designed around having an
IO thread that doesn't block.

However, the requirement for the ZCIS is that it *does* block, particularly when
Next is called. With StreamSocket, you always have the chance that it will
enqueue a read operation and return ERR_IO_PENDING (due to the normalizing of
the Reactor/Proactor patterns of Windows vs Posix I/O that net/ does).

In an ideal world, we could fix Protobuf so that it could do something more
intelligent here (such as advising message lengths / expected consumption side).
Barring that, if we could change GCM to have explicit message lengths before the
protobufs, we could make intelligent decisions about how much data to read from
the socket (This is what Chromoting does, FWIW).

Absent both of those things, we'll need to split the ZCIS to be something that
can live on the 'blocking' thread (GCM thread?), and communicates with a
TaskRunner that it can issue Read/Write requests to. The pre-condition for the
TaskRunner is !RunsTasksOnCurrentThread().

You basically have three classes at play:
A ZCIS implementation [that lives on the GCM thread]
A (inner class) shared ZCIS<->network object that holds the synchronization
primitives used to coordinate [either WeakPtr or RefCountedThreadSafe]
A (inner class) network object that handles the reading and writing from the
network and signalling back to the shared state object

When ZCIS::Next() is called, the ZCIS would first check if it has any data
internally buffered (eg: from a prior call to BackUp?). If it does, it can
return that immediately.
If not, ZCIS needs to PostTask to the network TaskRunner to schedule a Read. It
then waits on a CV for your read timeout duration.
When the Read completes, it signals the CV, waking up the GCM thread.

The tricky thing here is going to be maintaining ownership and thread safety.
Requirements:
1) Any call to StreamSocket Read/Write happens on the IO thread
2) Ergo, any callbacks to Read/Write need to remain valid until the StreamSocket
is destroyed
3) The StreamSocket can only be destroyed on the IO thread
4) The CV needs to be accessible to both the network task runner and the GCM
task runner
5) [figure out which task runner lives longer. Likely network > GCM]
6) When the GCM ZCIS is destroyed, it should clean up the StreamSocket

You should be able to do this by having an (inner, private) state object that
lives on the network task runner, that holds onto things like the CV, the
socket, etc. The CV & a buffer is shared between the two, but all other
members/methods 'live' on the network task runner. Wait on the CV for the
timeout. If the CV times out, post to the network task runner a DeleteSoon of
that object, then return. Otherwise, if the CV is signalled, snag the buffer and
use that for the ZCIS.

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
File google_apis/gcm/base/socket_stream_test_data_provider.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
google_apis/gcm/base/socket_stream_test_data_provider.h:17: class
SocketStreamTestDataProvider : public net::SocketDataProvider {
I'm not clear the motivation why you needed to do a new SocketDataProvider. Was
this purely for the async write bits? I'd like to see if we can fit this into
the existing SDPs.

That is, we've tried to fit all of the types of SocketDataProviders into
socket_test_util - static, dynamic, ordered, delayed, etc - so I'm trying to
understand what use case this is trying to address.

As best I can tell, it's closest to the Dynamic SDP, but I may be misreading,
and this may just be an instance of a Static+Delayed SDP use case.

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_util.h
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_uti...
net/socket/socket_test_util.h:194: // complete the asynchronous read operation.
Comment is out of date

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_uti...
net/socket/socket_test_util.h:204: virtual void OnWriteComplete(const
MockWriteResult& result) = 0;
For review sake, and because socket_test_util itself is a huge file, can we
split this into a separate CL?

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_uti...
net/socket/socket_test_util.h:204: virtual void OnWriteComplete(const
MockWriteResult& result) = 0;
So, OnReadComplete is meant to be consumed by DelayedSocketData. That is, it's
"public", but it's supposed to be an implementation detail of the whole Socket
util stuff.

I don't see you actually using the OnWriteComplete interface here - instead,
you're using it to handle ordrering/sequencing outside of this file, and that
gives me a little concern.

This is more of a design issue on the net/ OWNERs side, in that it's not clear
how to 'best'/'correctly' use these interfaces.

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_uti...
net/socket/socket_test_util.h:721: scoped_refptr<IOBuffer> pending_buf_;
While I agree that this is the right change, in order to match the interface,
I'm curious if you ran into a bug when working on this.

[Nicolas Zea, 2013-09-04 00:43:33]

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
google_apis/gcm/base/socket_stream.h:35: : public
google::protobuf::io::ZeroCopyInputStream {
On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> So, as I mentioned over Chat, I think we need to be careful here.
> 
> In order to really leverage Chrome's network stack, you'll want to be doing
the
> IO reads on the actual IO thread. Even Chromoting is designed around having an
> IO thread that doesn't block.
> 
> However, the requirement for the ZCIS is that it *does* block, particularly
when
> Next is called. With StreamSocket, you always have the chance that it will
> enqueue a read operation and return ERR_IO_PENDING (due to the normalizing of
> the Reactor/Proactor patterns of Windows vs Posix I/O that net/ does).
> 
> In an ideal world, we could fix Protobuf so that it could do something more
> intelligent here (such as advising message lengths / expected consumption
side).
> Barring that, if we could change GCM to have explicit message lengths before
the
> protobufs, we could make intelligent decisions about how much data to read
from
> the socket (This is what Chromoting does, FWIW).
> 
> Absent both of those things, we'll need to split the ZCIS to be something that
> can live on the 'blocking' thread (GCM thread?), and communicates with a
> TaskRunner that it can issue Read/Write requests to. The pre-condition for the
> TaskRunner is !RunsTasksOnCurrentThread().
> 
> You basically have three classes at play:
> A ZCIS implementation [that lives on the GCM thread]
> A (inner class) shared ZCIS<->network object that holds the synchronization
> primitives used to coordinate [either WeakPtr or RefCountedThreadSafe]
> A (inner class) network object that handles the reading and writing from the
> network and signalling back to the shared state object
> 
> When ZCIS::Next() is called, the ZCIS would first check if it has any data
> internally buffered (eg: from a prior call to BackUp?). If it does, it can
> return that immediately.
> If not, ZCIS needs to PostTask to the network TaskRunner to schedule a Read.
It
> then waits on a CV for your read timeout duration.
> When the Read completes, it signals the CV, waking up the GCM thread.
> 
> The tricky thing here is going to be maintaining ownership and thread safety.
> Requirements:
> 1) Any call to StreamSocket Read/Write happens on the IO thread
> 2) Ergo, any callbacks to Read/Write need to remain valid until the
StreamSocket
> is destroyed
> 3) The StreamSocket can only be destroyed on the IO thread
> 4) The CV needs to be accessible to both the network task runner and the GCM
> task runner
> 5) [figure out which task runner lives longer. Likely network > GCM]
> 6) When the GCM ZCIS is destroyed, it should clean up the StreamSocket
> 
> You should be able to do this by having an (inner, private) state object that
> lives on the network task runner, that holds onto things like the CV, the
> socket, etc. The CV & a buffer is shared between the two, but all other
> members/methods 'live' on the network task runner. Wait on the CV for the
> timeout. If the CV times out, post to the network task runner a DeleteSoon of
> that object, then return. Otherwise, if the CV is signalled, snag the buffer
and
> use that for the ZCIS.

How does Chromoting make use of the message length? GCM does in fact send the
protobuf length before the actual data, and I use this via a call to SetLimit.
|limit_| is then used to tell the socket how much data is left to read, and once
limit_ bytes have been consumed by Next(..) it will immediately return false.
But in
the meantime the thread will block until those bytes have been received.

https://codereview.chromium.org/23181011/diff/13001/google_apis/gcm/engine/gc...
shows how this is done (see ReadProto(..)).

Although I suppose I could delay attempting to process _any_ data in the
protobuf
until all of it is available, with a timeout of course, and just make as many
Socket::Read
calls as it takes to get all the data. WDYT?

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
File google_apis/gcm/base/socket_stream_test_data_provider.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
google_apis/gcm/base/socket_stream_test_data_provider.h:17: class
SocketStreamTestDataProvider : public net::SocketDataProvider {
On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> I'm not clear the motivation why you needed to do a new SocketDataProvider.
Was
> this purely for the async write bits? I'd like to see if we can fit this into
> the existing SDPs.
> 
> That is, we've tried to fit all of the types of SocketDataProviders into
> socket_test_util - static, dynamic, ordered, delayed, etc - so I'm trying to
> understand what use case this is trying to address.
> 
> As best I can tell, it's closest to the Dynamic SDP, but I may be misreading,
> and this may just be an instance of a Static+Delayed SDP use case.

Ah, I missed most of those implementations. I'll take a closer look to see if I
can
just use one (or modify one slightly)

[Nicolas Zea, 2013-09-07 01:07:52]

PTAL. In particular, I've made SocketStream completely asynchronous (there's a
GetNextMessage method that will read msg_size bytes and only upon completion
invoke the callback), and removed the custom data provider implementations in
favor of DelayedSocketData.

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/gcm_e...
File google_apis/gcm/base/gcm_export.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/gcm_e...
google_apis/gcm/base/gcm_export.h:16: #define GCM_EXPORT_PRIVATE
__declspec(dllimport)
On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> You shouldn't need a _PRIVATE export for new components
> 
> See this thread at
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/XMDgIB2nn-0/j6WUq...
> 

Done.

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
google_apis/gcm/base/socket_stream.h:35: : public
google::protobuf::io::ZeroCopyInputStream {
On 2013/09/04 00:43:33, Nicolas Zea wrote:
> On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> > So, as I mentioned over Chat, I think we need to be careful here.
> > 
> > In order to really leverage Chrome's network stack, you'll want to be doing
> the
> > IO reads on the actual IO thread. Even Chromoting is designed around having
an
> > IO thread that doesn't block.
> > 
> > However, the requirement for the ZCIS is that it *does* block, particularly
> when
> > Next is called. With StreamSocket, you always have the chance that it will
> > enqueue a read operation and return ERR_IO_PENDING (due to the normalizing
of
> > the Reactor/Proactor patterns of Windows vs Posix I/O that net/ does).
> > 
> > In an ideal world, we could fix Protobuf so that it could do something more
> > intelligent here (such as advising message lengths / expected consumption
> side).
> > Barring that, if we could change GCM to have explicit message lengths before
> the
> > protobufs, we could make intelligent decisions about how much data to read
> from
> > the socket (This is what Chromoting does, FWIW).
> > 
> > Absent both of those things, we'll need to split the ZCIS to be something
that
> > can live on the 'blocking' thread (GCM thread?), and communicates with a
> > TaskRunner that it can issue Read/Write requests to. The pre-condition for
the
> > TaskRunner is !RunsTasksOnCurrentThread().
> > 
> > You basically have three classes at play:
> > A ZCIS implementation [that lives on the GCM thread]
> > A (inner class) shared ZCIS<->network object that holds the synchronization
> > primitives used to coordinate [either WeakPtr or RefCountedThreadSafe]
> > A (inner class) network object that handles the reading and writing from the
> > network and signalling back to the shared state object
> > 
> > When ZCIS::Next() is called, the ZCIS would first check if it has any data
> > internally buffered (eg: from a prior call to BackUp?). If it does, it can
> > return that immediately.
> > If not, ZCIS needs to PostTask to the network TaskRunner to schedule a Read.
> It
> > then waits on a CV for your read timeout duration.
> > When the Read completes, it signals the CV, waking up the GCM thread.
> > 
> > The tricky thing here is going to be maintaining ownership and thread
safety.
> > Requirements:
> > 1) Any call to StreamSocket Read/Write happens on the IO thread
> > 2) Ergo, any callbacks to Read/Write need to remain valid until the
> StreamSocket
> > is destroyed
> > 3) The StreamSocket can only be destroyed on the IO thread
> > 4) The CV needs to be accessible to both the network task runner and the GCM
> > task runner
> > 5) [figure out which task runner lives longer. Likely network > GCM]
> > 6) When the GCM ZCIS is destroyed, it should clean up the StreamSocket
> > 
> > You should be able to do this by having an (inner, private) state object
that
> > lives on the network task runner, that holds onto things like the CV, the
> > socket, etc. The CV & a buffer is shared between the two, but all other
> > members/methods 'live' on the network task runner. Wait on the CV for the
> > timeout. If the CV times out, post to the network task runner a DeleteSoon
of
> > that object, then return. Otherwise, if the CV is signalled, snag the buffer
> and
> > use that for the ZCIS.
> 
> How does Chromoting make use of the message length? GCM does in fact send the
> protobuf length before the actual data, and I use this via a call to SetLimit.
> |limit_| is then used to tell the socket how much data is left to read, and
once
> limit_ bytes have been consumed by Next(..) it will immediately return false.
> But in
> the meantime the thread will block until those bytes have been received.
> 
>
https://codereview.chromium.org/23181011/diff/13001/google_apis/gcm/engine/gc...
> shows how this is done (see ReadProto(..)).
> 
> Although I suppose I could delay attempting to process _any_ data in the
> protobuf
> until all of it is available, with a timeout of course, and just make as many
> Socket::Read
> calls as it takes to get all the data. WDYT?

Went ahead and made SocketStream completely asynchronous. SetLimit has been
replaced with GetNextMessage, and Next(..) no longer triggers refreshes.

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
File google_apis/gcm/base/socket_stream_test_data_provider.h (right):

https://codereview.chromium.org/23684017/diff/3001/google_apis/gcm/base/socke...
google_apis/gcm/base/socket_stream_test_data_provider.h:17: class
SocketStreamTestDataProvider : public net::SocketDataProvider {
On 2013/09/04 00:43:33, Nicolas Zea wrote:
> On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> > I'm not clear the motivation why you needed to do a new SocketDataProvider.
> Was
> > this purely for the async write bits? I'd like to see if we can fit this
into
> > the existing SDPs.
> > 
> > That is, we've tried to fit all of the types of SocketDataProviders into
> > socket_test_util - static, dynamic, ordered, delayed, etc - so I'm trying to
> > understand what use case this is trying to address.
> > 
> > As best I can tell, it's closest to the Dynamic SDP, but I may be
misreading,
> > and this may just be an instance of a Static+Delayed SDP use case.
> 
> Ah, I missed most of those implementations. I'll take a closer look to see if
I
> can
> just use one (or modify one slightly)

Switched to Delayed SDP.

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_util.h
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_uti...
net/socket/socket_test_util.h:194: // complete the asynchronous read operation.
On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> Comment is out of date

n/a now

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_uti...
net/socket/socket_test_util.h:204: virtual void OnWriteComplete(const
MockWriteResult& result) = 0;
On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> For review sake, and because socket_test_util itself is a huge file, can we
> split this into a separate CL?

These changes have been reverted now that I don't have a custom data provider.

https://codereview.chromium.org/23684017/diff/3001/net/socket/socket_test_uti...
net/socket/socket_test_util.h:721: scoped_refptr<IOBuffer> pending_buf_;
On 2013/09/03 23:04:08, Ryan Sleevi wrote:
> While I agree that this is the right change, in order to match the interface,
> I'm curious if you ran into a bug when working on this.

Yep, in particular when handling a timeout, as the IOBuffer is discarded in my
socket stream when the stream is closed. If the read then finishes at a later
time it'll try to write into the buffer, but because it hadn't added a reference
it's already been destroyed by then.

[Ryan Sleevi, 2013-09-10 19:35:27]

Ok, I'm still a little concerned about the interaction between the
pseudo-blockiness and how net::SocketStream should be done on the IOThread. I'm
not sure I fully understand how the new design mitigates this, other than
through the 8K buffer limits. Have I missed something?

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:286:
base::MessageLoop::current()->PostTask(FROM_HERE, callback);
FWIW, net/ code synchronously runs callbacks (and designs around this assumption
that callbacks may invalidate the current object). This has the benefit of
avoiding the PostTask overhead. Worth considering here, if trying to match the
net/ API.

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:58: virtual int64 ByteCount() const
OVERRIDE;
STYLE: I thought the policy was NOT to OVERRIDE from third-party libraries (of
which protobuf is). This is same with our Blink no-override policy.

The reasoning being that it makes merges difficult.

Of course, this may be specific to Blink, with its 3 way merges.

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:71: void Reset();
STYLE: Naming: I would expect Reset() to actually clear the buffer, but here it
shifts it along, ring-buffer style. Perhaps theres a better name?

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:111: int buffer_size_;
STYLE: naming: io_buffer_ is an IOBufferWithSize, so naming this buffer_size_ is
confusing as to which size is the right size.

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:119: int backup_bytes_;
STYLE: Pronouns in comments considered harmful. Or at least confusing. Rephrase
sans "we"

[Nicolas Zea, 2013-09-10 21:22:22]

On 2013/09/10 19:35:27, Ryan Sleevi wrote:
> Ok, I'm still a little concerned about the interaction between the
> pseudo-blockiness and how net::SocketStream should be done on the IOThread.
I'm
> not sure I fully understand how the new design mitigates this, other than
> through the 8K buffer limits. Have I missed something?
> 

There are two ways that data can be read from the stream, and they have the
following restrictions:

1. Refresh is called and upon finishing (via callback) new data is received. The
caller then processes a specific number of bytes via a single Next call. This is
the case where a read |limit_| has not been set, and is useful for reading the
single byte message size and message tag packets. Blocking is prevented because
Next will DCHECK if it is called and there is no remaining data or it is busy
(state_ must be READY).

2. GetNextMessage is called (which sets the read |limit_|), and will only call
back when all the data is received or an error occurs. Next calls will provide
data and return true until |limit_| bytes have been processed, at which point it
will return false, indicating the stream is finished (and informing any caller
to stop spinning). A misbehaving caller could theoretically ignore the return
value, so I could add another dcheck here just in case, but the protobuf
implementations know to stop spinning in that case.

In other words, all data processing is only done with the data already in
memory, and with an idle stream. Next calls no longer trigger Socket::Read's,
which only come via Refresh and GetNextMessage, both of which are fully
asynchronous.

[Nicolas Zea, 2013-09-12 22:46:58]

PTAL

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:286:
base::MessageLoop::current()->PostTask(FROM_HERE, callback);
On 2013/09/10 19:35:27, Ryan Sleevi wrote:
> FWIW, net/ code synchronously runs callbacks (and designs around this
assumption
> that callbacks may invalidate the current object). This has the benefit of
> avoiding the PostTask overhead. Worth considering here, if trying to match the
> net/ API.

Done.

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:58: virtual int64 ByteCount() const
OVERRIDE;
On 2013/09/10 19:35:27, Ryan Sleevi wrote:
> STYLE: I thought the policy was NOT to OVERRIDE from third-party libraries (of
> which protobuf is). This is same with our Blink no-override policy.
> 
> The reasoning being that it makes merges difficult.
> 
> Of course, this may be specific to Blink, with its 3 way merges.

Chromium style guide just says to use OVERRIDE for all implementations (and the
clang style plugins complain otherwise).

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:71: void Reset();
On 2013/09/10 19:35:27, Ryan Sleevi wrote:
> STYLE: Naming: I would expect Reset() to actually clear the buffer, but here
it
> shifts it along, ring-buffer style. Perhaps theres a better name?

Renamed to Rebuild

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:111: int buffer_size_;
On 2013/09/10 19:35:27, Ryan Sleevi wrote:
> STYLE: naming: io_buffer_ is an IOBufferWithSize, so naming this buffer_size_
is
> confusing as to which size is the right size.

Updated to buffer_write_pos_ (and changed buffer_pos_ to buffer_read_pos_ to try
and make that a bit clearer too).

https://codereview.chromium.org/23684017/diff/14001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:119: int backup_bytes_;
On 2013/09/10 19:35:27, Ryan Sleevi wrote:
> STYLE: Pronouns in comments considered harmful. Or at least confusing.
Rephrase
> sans "we"

Done.

[Nicolas Zea, 2013-09-17 17:45:11]

ping?

[Ryan Sleevi, 2013-09-17 19:43:30]

Sorry, I'm still having trouble mapping in all the assumptions about Protobuf as
well as trying to see how (and where) this would run in practice in terms of
threading.

I did want to make sure I pointed you to the remoting code, which I think has a
much simpler design and provides clearer separation of the relevant bits, as a
point of consideration.

- You have a remoting::protocol::MessageReader, that handles reading messages
(tagged length / arbitrary bytes)
- It builds a Compound Buffer of these (although a growable buffer would be just
the same concept), since Read() may return < message length, based on data
available
- Once it's read a full message, it signals the MessageReceivedCallback with the
full CompoundBuffer

There is ProtobufMessageReader, which listens for events from a given
MessageReader.
- Once a MessageReader signals a message is received, it attempts to read a full
Protobuf message using a CompoundBufferInputStream.
- If the message parses, it signals its own Callback (MessageReceivedCallback)
that passes along the data.

The MessageReader / ProtobufMessageReader are designed that they also pass along
a |done_task|, which indicates that processing the message is complete. This is
used to throttle MessageReader, to ensure that only one message is
received/processed at a time.

Note the separation of responsibilities here:
- MessageReader handles *all* Socket interaction.
- MessageReader handles reading the (length, message) tuples on the wire
- ProtobufMessageReader provides the high-level interface for signalling when
new *protobuf* messages are arrived
- CompoundBufferInputStream handles providing the ZeroCopyInputStream interface
atop the underlying storage (in this case, a CompoundBuffer, but in yours, an
IOBuffer)

Note that this separation also lends itself very easily to providing thread
separations. The MessageReader can run solely on the IO Thread, and it can use
task marshalling to marshal data over to any other thread that needs it.

This also fits somewhat easier with the normal net/ flow of socket ownership,
which has a socket being bound to some owner and living there 'a long time'.
That is, one discrete object to manage the lifetime of the underlying socket.

As I understand your design (perhaps incorrectly), you would end up creating
multiple SocketInputStream's for the underlying socket - one for each message.
Is that correct?

Note that you can still achieve the ZeroCopy semantics desired here with the
above approach - a GrowableIOBuffer for the input that can grow for {message
size}, then you can pass that buffer to a ZeroCopyInputStream adapter that just
has Next() return the full buffer size.

Have I missed a design consideration that makes this undesirable? Is there some
streaming zero-copy that's going on that I'm missing?

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:158: char* unread_data_ptr =
&(io_buffer_->data()[last_read_pos]);
Why not io_buffer_->data() + last_read_pos?

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:82: // data is received for too long. The
timeout will close the stream and
grammar nit: does have a timeout if data is not received in a reasonable time.
The timeout will close the stream and trigger the callback.

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:156: SocketOutputStream(net::StreamSocket*
socket);
style nit: explicit

[Ryan Sleevi, 2013-09-20 22:14:11]

Moving myself to CC and Fred to owner, as he's graciously agreed to take over
this review on the GCM<->net interaction (particularly useful for his SPDY
knowledge as well)

[Nicolas Zea, 2013-09-25 01:21:27]

Fred, PTAL.

I've gone ahead and pulled all timeout logic out of the SocketInputStream
implementation, and into its own ConnectionHandler component (which I'll send
for review separately, the current implementation can be seen at
https://codereview.chromium.org/23181011/). The ConnectionHandler design is
modeled on the remoting MessageReader, and is completely asynchronous.

Additionally, Socket[Input/Output]Streams lifetimes are tied to their associated
net::SocketStream within the ConnectionHandler. In other words, a single
SocketInputStream will, assuming no connection issues, read all incoming
messages.

Please let me know if you have any questions/concerns on the design.

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:158: char* unread_data_ptr =
&(io_buffer_->data()[last_read_pos]);
On 2013/09/17 19:43:30, Ryan Sleevi wrote:
> Why not io_buffer_->data() + last_read_pos?

Done.

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:82: // data is received for too long. The
timeout will close the stream and
On 2013/09/17 19:43:30, Ryan Sleevi wrote:
> grammar nit: does have a timeout if data is not received in a reasonable time.
> The timeout will close the stream and trigger the callback.

This functionality has been refactored out now

https://codereview.chromium.org/23684017/diff/20001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:156: SocketOutputStream(net::StreamSocket*
socket);
On 2013/09/17 19:43:30, Ryan Sleevi wrote:
> style nit: explicit

Done.

[akalin, 2013-09-25 21:49:27]

Actually, Ryan will take a look for now, since I'm going on sick leave starting
tomorrow. I'll take a look when I get back!

[Ryan Sleevi, 2013-09-25 23:30:24]

A few draft comments I had saved before I saw your larger CL

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/DEPS
File google_apis/gcm/DEPS (right):

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/DEPS#newc...
google_apis/gcm/DEPS:7: "+google",
Can you add a comment that this is for protobuf (aka third_party/protobuf)?

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:30: class GCM_EXPORT SocketInputStream
style nit: Can you provide a brief description of this class here?

In particular, address comments like does it take ownership of the socket,
what's the right way to use it to read a sample message, etc.

I realize this may be asking for a bit much, but it's good to setup a good
pattern here.

[Nicolas Zea, 2013-10-02 22:56:02]

Forgot to publish the changes. I've addressed the comments, as well as made some
slight changes to better support the usage in the ConnectionHandler (the other
CL has also been updated to address your comments there). PTAL

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/DEPS
File google_apis/gcm/DEPS (right):

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/DEPS#newc...
google_apis/gcm/DEPS:7: "+google",
On 2013/09/25 23:30:24, Ryan Sleevi wrote:
> Can you add a comment that this is for protobuf (aka third_party/protobuf)?

Done.

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/28001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:30: class GCM_EXPORT SocketInputStream
On 2013/09/25 23:30:24, Ryan Sleevi wrote:
> style nit: Can you provide a brief description of this class here?
> 
> In particular, address comments like does it take ownership of the socket,
> what's the right way to use it to read a sample message, etc.
> 
> I realize this may be asking for a bit much, but it's good to setup a good
> pattern here.

Added better comments to both input and output stream.

[akalin, 2013-10-04 18:37:04]

first pass, mainly concentrated on SocketInputStream

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:14: // TODO(zea): consider having
dynamically sized buffers if this becomes too
dynamically sized -> dynamically-sized

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:16: const uint32 kDefaultBufferSize =
8*1024;
put this in anon namespace?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:45: if (backup_bytes_ > 0) {
DCHECK backup_bytes_ isn't too big?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:46: *size = backup_bytes_;
put *size assignment after *data assignment

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:49: DCHECK_GT(*size, 0);
i'm all for copious DCHECKs but this seems pretty obvious (since *size is just
set to backup_bytes_)

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:204: if (!callback.is_null())
callback.Run();
newline before callback.Run()

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:232:
SocketOutputStream::SocketOutputStream(net::StreamSocket* socket)
(blanket comment) apply above comments on SocketInputStream to
SocketOutputStream also when applicable

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:24: }
// namespace net

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:33: // input last_error_, and state will be
set to CLOSED.
input last_error_ -> in |last_error_|
state -> the state or state()

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:77: void Refresh(const base::Closure&
callback, int byte_limit);
so |callback| can be invoked directly by Refresh, right? It seems worth calling
this out, esp. since it departs from net:: convention

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:86: int last_error() const;
net::Error?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:102: net::StreamSocket* socket_;
* const socket_?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:103: scoped_refptr<net::IOBufferWithSize>
io_buffer_;
since drainable_io_buffer_ stores the size already, does io_buffer_ need to be
IOBufferWithSize? Can it be IOBuffer instead?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:104: scoped_refptr<net::DrainableIOBuffer>
drainable_io_buffer_;
do we need this to be a member variable? Seems to violate the DRY principle
since drainable_io_buffer_'s offset is basically buffer_read_pos_, but we have
to keep them in sync. Maybe eliminate one or the other?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:110: int buffer_write_pos_;
the name suggests that it's analogous to buffer_read_pos_, but this is more like
a limit to buffer_read_pos_. Perhaps rename to 'buffer_valid_size_' or
something?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:115: int backup_bytes_;
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...
is ambiguous on how to handle a call to BackUp(0). (Should the next call to
Next() return 0 for size?) Perhaps document the assumption you make? (i.e.,
backup_bytes_ == 0 means BackUp() hasn't been called)

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:115: int backup_bytes_;
is it possible to omit backup_bytes_ entirely and just adjust buffer_read_pos_
appropriately? It might be, if we assume that it's called only at most once
after a successful call to Next

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:118: int skipped_bytes_;
i suppose we're assuming it's not true that both BackUp() and Skip() will be
called?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:121: int last_error_;
net::Error?

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:177: int last_error() const;
net::Error

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:184: scoped_refptr<net::IOBufferWithSize>
io_buffer_;
IOBufferWithSize -> IOBuffer

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:192: int last_error_;
net::Error?

[Nicolas Zea, 2013-10-04 20:55:28]

Comments addressed, PTAL

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:14: // TODO(zea): consider having
dynamically sized buffers if this becomes too
On 2013/10/04 18:37:04, akalin wrote:
> dynamically sized -> dynamically-sized

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:16: const uint32 kDefaultBufferSize =
8*1024;
On 2013/10/04 18:37:04, akalin wrote:
> put this in anon namespace?

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:45: if (backup_bytes_ > 0) {
On 2013/10/04 18:37:04, akalin wrote:
> DCHECK backup_bytes_ isn't too big?

n/a now

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:46: *size = backup_bytes_;
On 2013/10/04 18:37:04, akalin wrote:
> put *size assignment after *data assignment

n/a now

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:49: DCHECK_GT(*size, 0);
On 2013/10/04 18:37:04, akalin wrote:
> i'm all for copious DCHECKs but this seems pretty obvious (since *size is just
> set to backup_bytes_)

n/a now

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:204: if (!callback.is_null())
callback.Run();
On 2013/10/04 18:37:04, akalin wrote:
> newline before callback.Run()

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:232:
SocketOutputStream::SocketOutputStream(net::StreamSocket* socket)
On 2013/10/04 18:37:04, akalin wrote:
> (blanket comment) apply above comments on SocketInputStream to
> SocketOutputStream also when applicable

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:24: }
On 2013/10/04 18:37:04, akalin wrote:
> // namespace net

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:33: // input last_error_, and state will be
set to CLOSED.
On 2013/10/04 18:37:04, akalin wrote:
> input last_error_ -> in |last_error_|
> state -> the state or state()

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:77: void Refresh(const base::Closure&
callback, int byte_limit);
On 2013/10/04 18:37:04, akalin wrote:
> so |callback| can be invoked directly by Refresh, right? It seems worth
calling
> this out, esp. since it departs from net:: convention

I've gone ahead and made it so synchronous callbacks don't happen (seems
cleaner/safer), here and in flush.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:86: int last_error() const;
On 2013/10/04 18:37:04, akalin wrote:
> net::Error?

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:102: net::StreamSocket* socket_;
On 2013/10/04 18:37:04, akalin wrote:
> * const socket_?

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:103: scoped_refptr<net::IOBufferWithSize>
io_buffer_;
On 2013/10/04 18:37:04, akalin wrote:
> since drainable_io_buffer_ stores the size already, does io_buffer_ need to be
> IOBufferWithSize? Can it be IOBuffer instead?

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:104: scoped_refptr<net::DrainableIOBuffer>
drainable_io_buffer_;
On 2013/10/04 18:37:04, akalin wrote:
> do we need this to be a member variable? Seems to violate the DRY principle
> since drainable_io_buffer_'s offset is basically buffer_read_pos_, but we have
> to keep them in sync. Maybe eliminate one or the other?

The offset is actually buffer_write_pos_. You're right though that I can
therefore replace buffer_write_pos_ usage with
drainable_io_buffer_->BytesConsumed().

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:110: int buffer_write_pos_;
On 2013/10/04 18:37:04, akalin wrote:
> the name suggests that it's analogous to buffer_read_pos_, but this is more
like
> a limit to buffer_read_pos_. Perhaps rename to 'buffer_valid_size_' or
> something?

Removed in favor of reusing DrainableIOBuffer::BytesConsumed()

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:115: int backup_bytes_;
On 2013/10/04 18:37:04, akalin wrote:
> is it possible to omit backup_bytes_ entirely and just adjust buffer_read_pos_
> appropriately? It might be, if we assume that it's called only at most once
> after a successful call to Next

Good point. It seemed easier at dev time to track them separately, so I
understood what was going on better, but at this point just updating
buffer_read_pos_ is simpler. Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:118: int skipped_bytes_;
On 2013/10/04 18:37:04, akalin wrote:
> i suppose we're assuming it's not true that both BackUp() and Skip() will be
> called?

Fair enough. I had originally written this to support all the CodedStream usage,
but for my purposes Skip isn't used, and it's better to just keep things simple.
Removed and made it NOTIMPLEMENTED().

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:121: int last_error_;
On 2013/10/04 18:37:04, akalin wrote:
> net::Error?

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:177: int last_error() const;
On 2013/10/04 18:37:04, akalin wrote:
> net::Error

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:184: scoped_refptr<net::IOBufferWithSize>
io_buffer_;
On 2013/10/04 18:37:04, akalin wrote:
> IOBufferWithSize -> IOBuffer

Done.

https://codereview.chromium.org/23684017/diff/35001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:192: int last_error_;
On 2013/10/04 18:37:04, akalin wrote:
> net::Error?

Done.

[akalin, 2013-10-07 23:00:07]

few more

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:24: io_buffer_(new
net::IOBufferWithSize(kDefaultBufferSize)),
plain IOBuffer here?

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:87: if
(drainable_io_buffer_->BytesRemaining() < byte_limit) {
can you reverse the order of this comparison? (to make it easier to compare to
the above dcheck)

wait, this is the same check as the DCHECK above, right? Why not:

if (byte_limit > BytesRemaining()) {
  NOTREACHED() << ...;
  ...
}

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:132:
drainable_io_buffer_->SetOffset(unread_buffer_size);
i feel like you want to call SetOffset even when unread_buffer_size == 0, since
the client can still call BackUp, right?

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:193: drainable_io_buffer_ = new
net::DrainableIOBuffer(io_buffer_.get(),
use setoffset here?

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:104: scoped_refptr<net::IOBuffer>
io_buffer_;
const scoped_refptr

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:105: scoped_refptr<net::DrainableIOBuffer>
drainable_io_buffer_;
const scoped_refptr

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:105: scoped_refptr<net::DrainableIOBuffer>
drainable_io_buffer_;
can this be given a better name? read_buffer_ perhaps? and maybe a comment
explaining that it's the suffix of io_buffer_ which hasn't been filled yet

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:108: int buffer_read_pos_;
a bit confusing since this is separate from the offset related to the Read()
calls (i.e., drainable_io_buffer_'s offset)

Perhaps 'next_pos_'? The comment is also outdated since the offset may be moved
back. Perhaps:

// The offset into |io_buffer_| that will be returned by the next call to Next.

[Nicolas Zea, 2013-10-09 00:44:22]

PTAL

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:24: io_buffer_(new
net::IOBufferWithSize(kDefaultBufferSize)),
On 2013/10/07 23:00:08, akalin wrote:
> plain IOBuffer here?

Done.

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:87: if
(drainable_io_buffer_->BytesRemaining() < byte_limit) {
On 2013/10/07 23:00:08, akalin wrote:
> can you reverse the order of this comparison? (to make it easier to compare to
> the above dcheck)
> 
> wait, this is the same check as the DCHECK above, right? Why not:
> 
> if (byte_limit > BytesRemaining()) {
>   NOTREACHED() << ...;
>   ...
> }

Done.

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:132:
drainable_io_buffer_->SetOffset(unread_buffer_size);
On 2013/10/07 23:00:08, akalin wrote:
> i feel like you want to call SetOffset even when unread_buffer_size == 0,
since
> the client can still call BackUp, right?

ResetInternal builds a new drainable_io_buffer_, which has an initial offset of
0 already.

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:193: drainable_io_buffer_ = new
net::DrainableIOBuffer(io_buffer_.get(),
On 2013/10/07 23:00:08, akalin wrote:
> use setoffset here?

SetOffset (and DrainableIOBuffer in general) only allows you to move forward in
offset. We want to go back to 0 here.

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:104: scoped_refptr<net::IOBuffer>
io_buffer_;
On 2013/10/07 23:00:08, akalin wrote:
> const scoped_refptr

Done.

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:105: scoped_refptr<net::DrainableIOBuffer>
drainable_io_buffer_;
On 2013/10/07 23:00:08, akalin wrote:
> const scoped_refptr

Can't due to having to rebuild this periodically (since offset can't move
backwards)

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:105: scoped_refptr<net::DrainableIOBuffer>
drainable_io_buffer_;
On 2013/10/07 23:00:08, akalin wrote:
> can this be given a better name? read_buffer_ perhaps? and maybe a comment
> explaining that it's the suffix of io_buffer_ which hasn't been filled yet

Done (and write_buffer_ in the output stream implementation).

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:108: int buffer_read_pos_;
On 2013/10/07 23:00:08, akalin wrote:
> a bit confusing since this is separate from the offset related to the Read()
> calls (i.e., drainable_io_buffer_'s offset)
> 
> Perhaps 'next_pos_'? The comment is also outdated since the offset may be
moved
> back. Perhaps:
> 
> // The offset into |io_buffer_| that will be returned by the next call to
Next.

Done.

[akalin, 2013-10-09 22:04:27]

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:193: drainable_io_buffer_ = new
net::DrainableIOBuffer(io_buffer_.get(),
On 2013/10/09 00:44:22, Nicolas Zea wrote:
> On 2013/10/07 23:00:08, akalin wrote:
> > use setoffset here?
> 
> SetOffset (and DrainableIOBuffer in general) only allows you to move forward
in
> offset. We want to go back to 0 here.

Huh, are you sure? I'm looking at DrainableIOBuffer::SetOffset:

void DrainableIOBuffer::SetOffset(int bytes) {
  DCHECK_GE(bytes, 0);
  DCHECK_LE(bytes, size_);
  used_ = bytes;
  data_ = base_->data() + used_;
}

and it looks like you can set it to an arbitrary point

[Nicolas Zea, 2013-10-09 23:24:49]

Doh. PTAL

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:193: drainable_io_buffer_ = new
net::DrainableIOBuffer(io_buffer_.get(),
On 2013/10/09 22:04:28, akalin wrote:
> On 2013/10/09 00:44:22, Nicolas Zea wrote:
> > On 2013/10/07 23:00:08, akalin wrote:
> > > use setoffset here?
> > 
> > SetOffset (and DrainableIOBuffer in general) only allows you to move forward
> in
> > offset. We want to go back to 0 here.
> 
> Huh, are you sure? I'm looking at DrainableIOBuffer::SetOffset:
> 
> void DrainableIOBuffer::SetOffset(int bytes) {
>   DCHECK_GE(bytes, 0);
>   DCHECK_LE(bytes, size_);
>   used_ = bytes;
>   data_ = base_->data() + used_;
> }
> 
> and it looks like you can set it to an arbitrary point

Huh, you're right. I completely misread that code somehow. Previous comments
addressed.

[akalin, 2013-10-10 08:47:40]

few more, almost there I think

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:132:
drainable_io_buffer_->SetOffset(unread_buffer_size);
On 2013/10/09 00:44:22, Nicolas Zea wrote:
> On 2013/10/07 23:00:08, akalin wrote:
> > i feel like you want to call SetOffset even when unread_buffer_size == 0,
> since
> > the client can still call BackUp, right?
> 
> ResetInternal builds a new drainable_io_buffer_, which has an initial offset
of
> 0 already.

forgot to address this? (now that we're not building a new one anymore)

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:48:
DCHECK_NE(read_buffer_->BytesConsumed(), next_pos_);
don't you want something stronger, i.e. DCHECK_LT(next_pos_,
read_buffer_->BytesConsumed())?

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:59: DCHECK_GE(count, 0);
perhaps change this to _GT? if not, then READY might not be the correct state
below, right

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:87: NOTREACHED();
you want NOTREACHED() << ... since NOTREACHED() turns into LOG(ERROR) for
release

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:110:
base::MessageLoop::current()->PostTask(
forgot to mention this last time. This does simplify the flow, but it does add
latency since even a synchronous return has to wait for the task to run. Are you
sure you're okay with that?

i'd normally just make this return OK || ERR_IO_PENDING and let the caller
handle the OK case and the callback handle the ERR_IO_PENDING case.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:126: char* unread_data_ptr =
io_buffer_->data() + last_read_pos;
use Next() instead of manually calculating?

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:134: if (last_read_pos != 0) {
you can do:

if (unread_data_ptr != io_buffer_->data())

I think, eliminating the need for last_read_pos

In fact, you can probably replace this entire block with:

if (unread_data_ptr != io_buffer_->data()) {
  std::memmove(...);
}
read_buffer_->Consume(unread_buffer_size);
if (unread_buffer_size > 0)
  state_ = READY;

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:167: DCHECK_GT(result, 0);
Isn't Read() allowed to return a result of 0? Double-check?

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:183:
weak_ptr_factory_.InvalidateWeakPtrs();  // Invalidate any callbacks.
follow declaration order to be parallel to the constructor?

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:70: virtual void BackUp(int count)
OVERRIDE;
just checking, you can't also just make BackUp not be implemented, right?

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:110: // Next(..) call.
i'd also comment that 0 <= next_pos_ < read_buffer_.BytesConsumed().

[Nicolas Zea, 2013-10-11 01:14:30]

PTAL

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/46001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:132:
drainable_io_buffer_->SetOffset(unread_buffer_size);
On 2013/10/10 08:47:40, akalin wrote:
> On 2013/10/09 00:44:22, Nicolas Zea wrote:
> > On 2013/10/07 23:00:08, akalin wrote:
> > > i feel like you want to call SetOffset even when unread_buffer_size == 0,
> > since
> > > the client can still call BackUp, right?
> > 
> > ResetInternal builds a new drainable_io_buffer_, which has an initial offset
> of
> > 0 already.
> 
> forgot to address this? (now that we're not building a new one anymore)

n/a in newest version I believe.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:48:
DCHECK_NE(read_buffer_->BytesConsumed(), next_pos_);
On 2013/10/10 08:47:40, akalin wrote:
> don't you want something stronger, i.e. DCHECK_LT(next_pos_,
> read_buffer_->BytesConsumed())?

Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:59: DCHECK_GE(count, 0);
On 2013/10/10 08:47:40, akalin wrote:
> perhaps change this to _GT? if not, then READY might not be the correct state
> below, right

Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:87: NOTREACHED();
On 2013/10/10 08:47:40, akalin wrote:
> you want NOTREACHED() << ... since NOTREACHED() turns into LOG(ERROR) for
> release

Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:110:
base::MessageLoop::current()->PostTask(
On 2013/10/10 08:47:40, akalin wrote:
> forgot to mention this last time. This does simplify the flow, but it does add
> latency since even a synchronous return has to wait for the task to run. Are
you
> sure you're okay with that?
> 
> i'd normally just make this return OK || ERR_IO_PENDING and let the caller
> handle the OK case and the callback handle the ERR_IO_PENDING case.

Yeah, I guess I may as well keep it as consistent with net:: convention as
possible. I originally figured it wouldn't matter, but I suppose it helps in the
case of receiving multiple messages at once and processing them with little
latency. Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:126: char* unread_data_ptr =
io_buffer_->data() + last_read_pos;
On 2013/10/10 08:47:40, akalin wrote:
> use Next() instead of manually calculating?

Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:134: if (last_read_pos != 0) {
On 2013/10/10 08:47:40, akalin wrote:
> you can do:
> 
> if (unread_data_ptr != io_buffer_->data())
> 
> I think, eliminating the need for last_read_pos
> 
> In fact, you can probably replace this entire block with:
> 
> if (unread_data_ptr != io_buffer_->data()) {
>   std::memmove(...);
> }
> read_buffer_->Consume(unread_buffer_size);
> if (unread_buffer_size > 0)
>   state_ = READY;

Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:167: DCHECK_GT(result, 0);
On 2013/10/10 08:47:40, akalin wrote:
> Isn't Read() allowed to return a result of 0? Double-check?

Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:183:
weak_ptr_factory_.InvalidateWeakPtrs();  // Invalidate any callbacks.
On 2013/10/10 08:47:40, akalin wrote:
> follow declaration order to be parallel to the constructor?

Done.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:70: virtual void BackUp(int count)
OVERRIDE;
On 2013/10/10 08:47:40, akalin wrote:
> just checking, you can't also just make BackUp not be implemented, right?

No, it's necessary. CodedInputStream will consume all the data available (which
will sometimes be more than necessary), and then back up after processing a
chunk.

https://codereview.chromium.org/23684017/diff/56001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:110: // Next(..) call.
On 2013/10/10 08:47:40, akalin wrote:
> i'd also comment that 0 <= next_pos_ < read_buffer_.BytesConsumed().

Technically it's <= BytesConsumed (when there's no unread data remaining). That
implies state_ == EMPTY though, which is handled at the beginning of Next().
Added comment though.

[akalin, 2013-10-11 16:47:21]

couple more

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:8: #include
"base/message_loop/message_loop.h"
if you remove all task posting, you can remove this include

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:159:
CloseStream(static_cast<net::Error>(result), callback);
if result == 0, then you'll call this with 'net::OK', which is not what you
want. Perhaps map EOF to some other error code?

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:259:
base::MessageLoop::current()->PostTask(
avoid task-posting analogous to SocketInputStream?

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:55: EMPTY,
I'm a fan of having as few states in a state machine as possible. I think you
can eliminate a couple:

- You can remove EMPTY and instead interpret READY && next_pos_ ==
read_buffer_.BytesConsumed() as empty (perhaps add a utility function)
- You can remove CLOSED and instead interpret last_error_ != net::OK as CLOSED.

So typical usage would instead be:

1. Check last_error() before using it...
2. If is_empty() is true, ...
3. Check last_error() again ...
...
What do you think?

[Nicolas Zea, 2013-10-11 18:28:32]

PTAL

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:8: #include
"base/message_loop/message_loop.h"
On 2013/10/11 16:47:22, akalin wrote:
> if you remove all task posting, you can remove this include

Done.

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:159:
CloseStream(static_cast<net::Error>(result), callback);
On 2013/10/11 16:47:22, akalin wrote:
> if result == 0, then you'll call this with 'net::OK', which is not what you
> want. Perhaps map EOF to some other error code?

Done.

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:259:
base::MessageLoop::current()->PostTask(
On 2013/10/11 16:47:22, akalin wrote:
> avoid task-posting analogous to SocketInputStream?

Done.

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:55: EMPTY,
On 2013/10/11 16:47:22, akalin wrote:
> I'm a fan of having as few states in a state machine as possible. I think you
> can eliminate a couple:
> 
> - You can remove EMPTY and instead interpret READY && next_pos_ ==
> read_buffer_.BytesConsumed() as empty (perhaps add a utility function)
> - You can remove CLOSED and instead interpret last_error_ != net::OK as
CLOSED.
> 
> So typical usage would instead be:
> 
> 1. Check last_error() before using it...
> 2. If is_empty() is true, ...
> 3. Check last_error() again ...
> ...
> What do you think?

Given that the states are used as a method of exposing internal state rather
than for controlling a state machine, I think I prefer the current approach.
Rather than having to check multiple values in different scenarios, you can
instead check things like state() == READY or state() != READY (which is what
ConnectionHandler does in the larger patch), and then specialize from there.

I think that's more readable/less prone to error.

[akalin, 2013-10-11 18:47:08]

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:149: DCHECK_EQ(state_, READING);
is this DCHECK still valid? you immediately check for CLOSED afterwards.
Perhaps:

if (state_ != READING) {
  NOTREACHED() << ...;
  return;
}

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:55: EMPTY,
On 2013/10/11 18:28:32, Nicolas Zea wrote:
> On 2013/10/11 16:47:22, akalin wrote:
> > I'm a fan of having as few states in a state machine as possible. I think
you
> > can eliminate a couple:
> > 
> > - You can remove EMPTY and instead interpret READY && next_pos_ ==
> > read_buffer_.BytesConsumed() as empty (perhaps add a utility function)
> > - You can remove CLOSED and instead interpret last_error_ != net::OK as
> CLOSED.
> > 
> > So typical usage would instead be:
> > 
> > 1. Check last_error() before using it...
> > 2. If is_empty() is true, ...
> > 3. Check last_error() again ...
> > ...
> > What do you think?
> 
> Given that the states are used as a method of exposing internal state rather
> than for controlling a state machine, I think I prefer the current approach.
> Rather than having to check multiple values in different scenarios, you can
> instead check things like state() == READY or state() != READY (which is what
> ConnectionHandler does in the larger patch), and then specialize from there.
> 
> I think that's more readable/less prone to error.

Okay, I see. My main concern is the possibility of the state_ variable going out
of sync with the 'real' state. How about another proposal: keep the State enum,
but make state() figure out the state instead of keeping a separate state_
variable? (and rename it to GetState())

In fact, if we set last_error_ to ERR_IO_PENDING when there's a read/write
pending, then we don't need the state_ variable at all. Then GetState can be:

State GetState() const {
  if (last_error_ < ERR_IO_PENDING)
    return CLOSED;

  if (error == ERR_IO_PENDING)
    return READING;

  DCHECK_EQ(error, OK);
  if (next_pos_ == read_buffer_.BytesConsumed())
    return EMPTY;

  return READY;
}

and the rest of the code can stay the same. What do you think?

[Nicolas Zea, 2013-10-11 23:41:28]

PTAL

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:149: DCHECK_EQ(state_, READING);
On 2013/10/11 18:47:09, akalin wrote:
> is this DCHECK still valid? you immediately check for CLOSED afterwards.
> Perhaps:
> 
> if (state_ != READING) {
>   NOTREACHED() << ...;
>   return;
> }

Done.

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/65013/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:55: EMPTY,
On 2013/10/11 18:47:09, akalin wrote:
> On 2013/10/11 18:28:32, Nicolas Zea wrote:
> > On 2013/10/11 16:47:22, akalin wrote:
> > > I'm a fan of having as few states in a state machine as possible. I think
> you
> > > can eliminate a couple:
> > > 
> > > - You can remove EMPTY and instead interpret READY && next_pos_ ==
> > > read_buffer_.BytesConsumed() as empty (perhaps add a utility function)
> > > - You can remove CLOSED and instead interpret last_error_ != net::OK as
> > CLOSED.
> > > 
> > > So typical usage would instead be:
> > > 
> > > 1. Check last_error() before using it...
> > > 2. If is_empty() is true, ...
> > > 3. Check last_error() again ...
> > > ...
> > > What do you think?
> > 
> > Given that the states are used as a method of exposing internal state rather
> > than for controlling a state machine, I think I prefer the current approach.
> > Rather than having to check multiple values in different scenarios, you can
> > instead check things like state() == READY or state() != READY (which is
what
> > ConnectionHandler does in the larger patch), and then specialize from there.
> > 
> > I think that's more readable/less prone to error.
> 
> Okay, I see. My main concern is the possibility of the state_ variable going
out
> of sync with the 'real' state. How about another proposal: keep the State
enum,
> but make state() figure out the state instead of keeping a separate state_
> variable? (and rename it to GetState())
> 
> In fact, if we set last_error_ to ERR_IO_PENDING when there's a read/write
> pending, then we don't need the state_ variable at all. Then GetState can be:
> 
> State GetState() const {
>   if (last_error_ < ERR_IO_PENDING)
>     return CLOSED;
> 
>   if (error == ERR_IO_PENDING)
>     return READING;
> 
>   DCHECK_EQ(error, OK);
>   if (next_pos_ == read_buffer_.BytesConsumed())
>     return EMPTY;
> 
>   return READY;
> }
> 
> and the rest of the code can stay the same. What do you think?

Done.

[akalin, 2013-10-12 03:18:36]

Remaining nits for socket_stream.{h,cc}, looking at test file and rest of CL
now.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:36: DCHECK_NE(GetState(), CLOSED);
i'd probably do

if (GetState() != EMPTY && GetState() != READY) {
  NOTREACHED() ...;
  return false;
}

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:158: // Result == 0 implies EOF, which is
treated as an error.
i'd probably structure it as:

if (result == 0)
  result = ERR_UNEXPECTED;

DCHECK_NE(result, ERR_IO_PENDING);

if (result < net::OK) {
  ...
  CloseStream(result, ...);
}

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:288: if (result < net::OK) {
just double checking -- does this do what you want if result == 0?

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:36: // 1. Check the state() of the input
stream before using it. If CLOSED, the
state() -> GetState()

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:39: // 2. If state() is EMPTY, call
Refresh(..), passing the maximum byte size for
GetState()

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:42: // 3. Check state() again to ensure the
Refresh was successful.
GetState()

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:77: // the refresh can complete
synchronously returns net::OK (without invoking
may want to clarify that there might be en error even if net::OK is returned.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:79: // Note: state() (and possibly
last_error()) should be checked upon completion
GetState()

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:117: // If < net::OK, the last net error
received.
< net::ERR_IO_PENDING. May want to comment what happens when == ERR_IO_PENDING

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:130: // 1. Check the state() of the output
stream before using it. If CLOSED, the
GetState() (and rest of file)

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:185: int buffer_used_;
rename to next_pos_ and change comment a la SocketInputStream?

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:187: // If < net::OK, the last net error
received.
< net::ERR_IO_PENDING. May want to comment what happens when == ERR_IO_PENDING

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:85: new net::DelayedSocketData(0,
i actually prefer DeterministicSocketData since DelayedSocketData et al.
introduces artificial delays (!), leading to noticeably slow tests.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:110: if
(!socket_input_stream_->Next((const void **)&buffer, &size))
static_cast (here and everywhere else)

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:113: if (initial_buffer != NULL) 
// Verify the buffer doesn't skip data.
usual style is to omit != NULL

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:113: if (initial_buffer != NULL) 
// Verify the buffer doesn't skip data.
braces since it's an if/else

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:119: *output_buffer = (const
void*)initial_buffer;
static_cast<>

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:139: uint64 bytes_to_write =
((uint64)size < bytes ? size : bytes);
static_cast, etc.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:162: if
(input_stream()->Refresh(base::Bind(&ReadCallback,
why not run_loop.QuitClosure()?

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:200:
WaitForData(strlen(kReadData));
arraysize(kReadData) - 1? Have kReadDataSize?

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:324: std::string full_data =
std::string(kWriteData);
you can avoid creating strings by using the (const char* data, int len)
MockWrite() constructors. I prefer doing it that way, actually, since it makes
it clear when the terminating null is included. Perhaps do this for all the
tests?

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/gcm.gyp#n...
google_apis/gcm/gcm.gyp:21: 'gcm_core.gypi',
do you anticipate gcm_core being big? why not inline?

[Nicolas Zea, 2013-10-14 23:39:15]

Comments addressed, PTAL

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:36: DCHECK_NE(GetState(), CLOSED);
On 2013/10/12 03:18:36, akalin wrote:
> i'd probably do
> 
> if (GetState() != EMPTY && GetState() != READY) {
>   NOTREACHED() ...;
>   return false;
> }

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:158: // Result == 0 implies EOF, which is
treated as an error.
On 2013/10/12 03:18:36, akalin wrote:
> i'd probably structure it as:
> 
> if (result == 0)
>   result = ERR_UNEXPECTED;
> 
> DCHECK_NE(result, ERR_IO_PENDING);
> 
> if (result < net::OK) {
>   ...
>   CloseStream(result, ...);
> }

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.cc:288: if (result < net::OK) {
On 2013/10/12 03:18:36, akalin wrote:
> just double checking -- does this do what you want if result == 0?

The Socket::Write documentation doesn't mention 0 as a valid result, and implies
if data isn't written it will return an error. For now this'll DCHECK if result
== 0.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:36: // 1. Check the state() of the input
stream before using it. If CLOSED, the
On 2013/10/12 03:18:36, akalin wrote:
> state() -> GetState()

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:39: // 2. If state() is EMPTY, call
Refresh(..), passing the maximum byte size for
On 2013/10/12 03:18:36, akalin wrote:
> GetState()

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:42: // 3. Check state() again to ensure the
Refresh was successful.
On 2013/10/12 03:18:36, akalin wrote:
> GetState()

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:77: // the refresh can complete
synchronously returns net::OK (without invoking
On 2013/10/12 03:18:36, akalin wrote:
> may want to clarify that there might be en error even if net::OK is returned.

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:79: // Note: state() (and possibly
last_error()) should be checked upon completion
On 2013/10/12 03:18:36, akalin wrote:
> GetState()

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:117: // If < net::OK, the last net error
received.
On 2013/10/12 03:18:36, akalin wrote:
> < net::ERR_IO_PENDING. May want to comment what happens when == ERR_IO_PENDING

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:130: // 1. Check the state() of the output
stream before using it. If CLOSED, the
On 2013/10/12 03:18:36, akalin wrote:
> GetState() (and rest of file)

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:185: int buffer_used_;
On 2013/10/12 03:18:36, akalin wrote:
> rename to next_pos_ and change comment a la SocketInputStream?

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream.h:187: // If < net::OK, the last net error
received.
On 2013/10/12 03:18:36, akalin wrote:
> < net::ERR_IO_PENDING. May want to comment what happens when == ERR_IO_PENDING

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:85: new net::DelayedSocketData(0,
On 2013/10/12 03:18:36, akalin wrote:
> i actually prefer DeterministicSocketData since DelayedSocketData et al.
> introduces artificial delays (!), leading to noticeably slow tests.

As I understood it, the artificial delays are only used if you explicitly pass a
write_delay > 0. BuildSocket, which I use to create the data provider, always
sets a write delay of 0, so there should never be an artificial delay (beyond
those I introduce by returning IO_PENDING and optionally later calling
ForceNextRead.

Otherwise, when write_delay <= 0, GetNextRead completes synchronously.

For reference, running this entire suite of tests takes 3ms total on my machine.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:110: if
(!socket_input_stream_->Next((const void **)&buffer, &size))
On 2013/10/12 03:18:36, akalin wrote:
> static_cast (here and everywhere else)

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:113: if (initial_buffer != NULL) 
// Verify the buffer doesn't skip data.
On 2013/10/12 03:18:36, akalin wrote:
> usual style is to omit != NULL

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:113: if (initial_buffer != NULL) 
// Verify the buffer doesn't skip data.
On 2013/10/12 03:18:36, akalin wrote:
> braces since it's an if/else

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:119: *output_buffer = (const
void*)initial_buffer;
On 2013/10/12 03:18:36, akalin wrote:
> static_cast<>

Changed declaration to be void* to begin with.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:139: uint64 bytes_to_write =
((uint64)size < bytes ? size : bytes);
On 2013/10/12 03:18:36, akalin wrote:
> static_cast, etc.

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:162: if
(input_stream()->Refresh(base::Bind(&ReadCallback,
On 2013/10/12 03:18:36, akalin wrote:
> why not run_loop.QuitClosure()?

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:200:
WaitForData(strlen(kReadData));
On 2013/10/12 03:18:36, akalin wrote:
> arraysize(kReadData) - 1? Have kReadDataSize?

Done.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/base/sock...
google_apis/gcm/base/socket_stream_unittest.cc:324: std::string full_data =
std::string(kWriteData);
On 2013/10/12 03:18:36, akalin wrote:
> you can avoid creating strings by using the (const char* data, int len)
> MockWrite() constructors. I prefer doing it that way, actually, since it makes
> it clear when the terminating null is included. Perhaps do this for all the
> tests?

Done. Although note that in this particular test, the intermediate strings are
used so that I can split up a single message into two parts, which does involve
intermediate strings. Updated all MockRead/MockWrite creations to explicitly
pass the length though.

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/23684017/diff/99001/google_apis/gcm/gcm.gyp#n...
google_apis/gcm/gcm.gyp:21: 'gcm_core.gypi',
On 2013/10/12 03:18:36, akalin wrote:
> do you anticipate gcm_core being big? why not inline?

Done.

[akalin, 2013-10-15 00:56:06]

few more test comments

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.cc:162: result = net::ERR_UNEXPECTED;
ERR_UNEXPECTED is the error code equivalent to DCHECK/NOTREACHED, so I think you
should map it to ERR_CONNECTION_CLOSED or something similar.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.cc:293: if (result < net::OK) {
from chromium-dev discussion, sounds like a 0 result should be treated like EOF
/ connection closed. Map to ERR_CONNECTION_CLOSED?

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:103: uint64
GCMSocketStreamTest::DoInputStreamRead(uint64 bytes,
perhaps have DoInputStreamRead() return a std::string with the read data so you
can verify that it matches what you expect? That would catch bugs where the size
isn't changed but the contents are corrupted

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:104: bool set_limit,
you don't seem to use set_limit. Remove?

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:117: if (initial_buffer) { //
Verify the buffer doesn't skip data.
two spaces before //

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:135: uint64
GCMSocketStreamTest::DoOutputStreamWrite(uint64 bytes) {
avoid depending on kWriteData explicitly and change this function to take a
StringPiece or something?

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:160: return total_bytes_written;
Do you need to return total_bytes_written? Perhaps EXPECT_EQ(bytes) here?

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:167: msg_size) ==
net::ERR_IO_PENDING) {
don't you mean msg_size - input_stream()->ByteCount()? Test didn't catch this?

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/gcm_test...
File google_apis/gcm/gcm_tests.gypi (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/gcm_test...
google_apis/gcm/gcm_tests.gypi:4: 
mind inlining this into gcm.gyp, too?

https://codereview.chromium.org/23684017/diff/114001/net/socket/socket_test_u...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/23684017/diff/114001/net/socket/socket_test_u...
net/socket/socket_test_util.h:719: scoped_refptr<IOBuffer> pending_buf_;
how'd you end up needing this? Perhaps decomp into separate CL?

[Nicolas Zea, 2013-10-15 20:07:37]

PTAL

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.cc:162: result = net::ERR_UNEXPECTED;
On 2013/10/15 00:56:07, akalin wrote:
> ERR_UNEXPECTED is the error code equivalent to DCHECK/NOTREACHED, so I think
you
> should map it to ERR_CONNECTION_CLOSED or something similar.

Done.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.cc:293: if (result < net::OK) {
On 2013/10/15 00:56:07, akalin wrote:
> from chromium-dev discussion, sounds like a 0 result should be treated like
EOF
> / connection closed. Map to ERR_CONNECTION_CLOSED?

Done.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:103: uint64
GCMSocketStreamTest::DoInputStreamRead(uint64 bytes,
On 2013/10/15 00:56:07, akalin wrote:
> perhaps have DoInputStreamRead() return a std::string with the read data so
you
> can verify that it matches what you expect? That would catch bugs where the
size
> isn't changed but the contents are 

That's what output_buffer is used for. I don't return a string in order to
exersize the zero-copy semantics.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:104: bool set_limit,
On 2013/10/15 00:56:07, akalin wrote:
> you don't seem to use set_limit. Remove?

Done.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:117: if (initial_buffer) { //
Verify the buffer doesn't skip data.
On 2013/10/15 00:56:07, akalin wrote:
> two spaces before //

Done.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:135: uint64
GCMSocketStreamTest::DoOutputStreamWrite(uint64 bytes) {
On 2013/10/15 00:56:07, akalin wrote:
> avoid depending on kWriteData explicitly and change this function to take a
> StringPiece or something?

Done.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:160: return total_bytes_written;
On 2013/10/15 00:56:07, akalin wrote:
> Do you need to return total_bytes_written? Perhaps EXPECT_EQ(bytes) here?

That's not true in the cases where an error is encountered. I think it's cleaner
to let the test itself decide whether to enforce that.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:167: msg_size) ==
net::ERR_IO_PENDING) {
On 2013/10/15 00:56:07, akalin wrote:
> don't you mean msg_size - input_stream()->ByteCount()? Test didn't catch this?

This method is waiting for |msg_size| new bytes, not |msg_size| total bytes
(note that Refresh treats that value as the byte_limit for the Socket::Read
call). That way, I can do WaitForData(kReadDataSize), followed by
WaitForData(kReadData2Size), in order to receive two messages. So I think this
is correct as is.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/gcm_test...
File google_apis/gcm/gcm_tests.gypi (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/gcm_test...
google_apis/gcm/gcm_tests.gypi:4: 
On 2013/10/15 00:56:07, akalin wrote:
> mind inlining this into gcm.gyp, too?

Done.

https://codereview.chromium.org/23684017/diff/114001/net/socket/socket_test_u...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/23684017/diff/114001/net/socket/socket_test_u...
net/socket/socket_test_util.h:719: scoped_refptr<IOBuffer> pending_buf_;
On 2013/10/15 00:56:07, akalin wrote:
> how'd you end up needing this? Perhaps decomp into separate CL?

It's necessary in the case that the SocketStream is destroyed while the
read/write is still pending, in which case the Socket itself should retain
ownership of the IOBuffer (otherwise it crashes in my tests). I suppose I could
pull it into another patch, but it seems pretty minimal and is to fix something
exposed by this patch. WDYT?

[akalin, 2013-10-15 23:24:14]

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:103: uint64
GCMSocketStreamTest::DoInputStreamRead(uint64 bytes,
On 2013/10/15 20:07:38, Nicolas Zea wrote:
> On 2013/10/15 00:56:07, akalin wrote:
> > perhaps have DoInputStreamRead() return a std::string with the read data so
> you
> > can verify that it matches what you expect? That would catch bugs where the
> size
> > isn't changed but the contents are 
> 
> That's what output_buffer is used for. I don't return a string in order to
> exersize the zero-copy semantics.

I see. What about returning a base::StringPiece?

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:155: if
(socket_output_stream_->Flush(run_loop.QuitClosure()) ==
if Next() returns false in the above loop, isn't it incorrect to call Flush()
here?

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:167: msg_size) ==
net::ERR_IO_PENDING) {
On 2013/10/15 20:07:38, Nicolas Zea wrote:
> On 2013/10/15 00:56:07, akalin wrote:
> > don't you mean msg_size - input_stream()->ByteCount()? Test didn't catch
this?
> 
> This method is waiting for |msg_size| new bytes, not |msg_size| total bytes
> (note that Refresh treats that value as the byte_limit for the Socket::Read
> call). That way, I can do WaitForData(kReadDataSize), followed by
> WaitForData(kReadData2Size), in order to receive two messages. So I think this
> is correct as is.

Right, but that's only for the first call to Refresh, right? What I mean is
that, if you're waiting for msg_size new bytes, shouldn't you have something
like:

size_t old_size = input_stream()->ByteCount();
while (input_stream()->ByteCount() < old_size + msg_size) {
  ...->Refresh(..., old_size + msg_size - input_stream()->ByteCount()...)
}

? Perhaps I'm missing something.

[Nicolas Zea, 2013-10-16 01:20:40]

PTAL

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:103: uint64
GCMSocketStreamTest::DoInputStreamRead(uint64 bytes,
On 2013/10/15 23:24:15, akalin wrote:
> On 2013/10/15 20:07:38, Nicolas Zea wrote:
> > On 2013/10/15 00:56:07, akalin wrote:
> > > perhaps have DoInputStreamRead() return a std::string with the read data
so
> > you
> > > can verify that it matches what you expect? That would catch bugs where
the
> > size
> > > isn't changed but the contents are 
> > 
> > That's what output_buffer is used for. I don't return a string in order to
> > exersize the zero-copy semantics.
> 
> I see. What about returning a base::StringPiece?

Done.

https://codereview.chromium.org/23684017/diff/114001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:167: msg_size) ==
net::ERR_IO_PENDING) {
On 2013/10/15 23:24:15, akalin wrote:
> On 2013/10/15 20:07:38, Nicolas Zea wrote:
> > On 2013/10/15 00:56:07, akalin wrote:
> > > don't you mean msg_size - input_stream()->ByteCount()? Test didn't catch
> this?
> > 
> > This method is waiting for |msg_size| new bytes, not |msg_size| total bytes
> > (note that Refresh treats that value as the byte_limit for the Socket::Read
> > call). That way, I can do WaitForData(kReadDataSize), followed by
> > WaitForData(kReadData2Size), in order to receive two messages. So I think
this
> > is correct as is.
> 
> Right, but that's only for the first call to Refresh, right? What I mean is
> that, if you're waiting for msg_size new bytes, shouldn't you have something
> like:
> 
> size_t old_size = input_stream()->ByteCount();
> while (input_stream()->ByteCount() < old_size + msg_size) {
>   ...->Refresh(..., old_size + msg_size - input_stream()->ByteCount()...)
> }
> 
> ? Perhaps I'm missing something.

Ah, the thing is that ByteCount is actually BytesConsumed() - next_pos_. In
other words, it's returning the amount of unread data. That said, you're right
that the refresh call is setting too high a limit, it should be msg_size -
ByteCount(). This doesn't really affect correctness, as the input stream is
built to be able to process correctly when there is more data than expected. 

One thing this does make me realize though is that, according to the
documentation, ByteCount is actually supposed to be the amount read via Next(),
not the bytes remaining. As it happens it's not used by CodedInputStream, so
that wasn't caught. I've gone ahead and made that change for consistency though,
and added an UnreadByteCount method for what I actually wanted.

[akalin, 2013-10-16 07:16:31]

mostly nits at this point

https://codereview.chromium.org/23684017/diff/114001/net/socket/socket_test_u...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/23684017/diff/114001/net/socket/socket_test_u...
net/socket/socket_test_util.h:719: scoped_refptr<IOBuffer> pending_buf_;
On 2013/10/15 20:07:38, Nicolas Zea wrote:
> On 2013/10/15 00:56:07, akalin wrote:
> > how'd you end up needing this? Perhaps decomp into separate CL?
> 
> It's necessary in the case that the SocketStream is destroyed while the
> read/write is still pending, in which case the Socket itself should retain
> ownership of the IOBuffer (otherwise it crashes in my tests). I suppose I
could
> pull it into another patch, but it seems pretty minimal and is to fix
something
> exposed by this patch. WDYT?

Okay, fair enough. Add comment on CL description re. this?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS
File google_apis/gcm/DEPS (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:2: "-chrome",
don't think you need these - lines explicitly?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:5: "+base",
note that the root DEPS already includes this stuff

but if you want, you can emulate sync/DEPS, but also copy comment from it

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:6: "+build",
need build?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:9: "+url",
need url yet?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.cc:6: 
include bind.h

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.h:15: #include "base/memory/scoped_ptr.h"
no need for scoped_ptr anymore

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:89: &(mock_reads_[0]),
mock_reads_.size(),
use vector_as_array from base/stl_util.h

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:319: std::string full_data =
std::string(kWriteData);
why not work with kWriteData directly and adjust sizes?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:338: std::string full_data =
std::string(kWriteData);
here too

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:16: 'include_dirs': [
need include_dirs for gcm_unit_tests too?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:25: '../../url/url.gyp:url_lib',
don't need to depend on url_lib yet, do you?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:38: # Typed-parametrized tests generate exit-time
destructors.
we don't use type-parametrized tests yet, right? enable warning until then?

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:41: 'GCM_TEST',
don't think you need this define

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:46: '../../net/net.gyp:net',
need to depend on net directly? (don't think so)

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:49:
'../../third_party/protobuf/protobuf.gyp:protobuf_lite',
need to depend on protobuf_lite directly? (don't think so)

[Nicolas Zea, 2013-10-16 19:56:17]

PTAL

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS
File google_apis/gcm/DEPS (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:2: "-chrome",
On 2013/10/16 07:16:31, akalin wrote:
> don't think you need these - lines explicitly?

I think they're used when a higher level DEPS allows them, but in this case
google_apis also disallows them. Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:5: "+base",
On 2013/10/16 07:16:31, akalin wrote:
> note that the root DEPS already includes this stuff
> 
> but if you want, you can emulate sync/DEPS, but also copy comment from it

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:6: "+build",
On 2013/10/16 07:16:31, akalin wrote:
> need build?

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/DEPS#new...
google_apis/gcm/DEPS:9: "+url",
On 2013/10/16 07:16:31, akalin wrote:
> need url yet?

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream.cc (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.cc:6: 
On 2013/10/16 07:16:31, akalin wrote:
> include bind.h

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream.h (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream.h:15: #include "base/memory/scoped_ptr.h"
On 2013/10/16 07:16:31, akalin wrote:
> no need for scoped_ptr anymore

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:89: &(mock_reads_[0]),
mock_reads_.size(),
On 2013/10/16 07:16:31, akalin wrote:
> use vector_as_array from base/stl_util.h

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:319: std::string full_data =
std::string(kWriteData);
On 2013/10/16 07:16:31, akalin wrote:
> why not work with kWriteData directly and adjust sizes?

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:338: std::string full_data =
std::string(kWriteData);
On 2013/10/16 07:16:31, akalin wrote:
> here too

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:16: 'include_dirs': [
On 2013/10/16 07:16:31, akalin wrote:
> need include_dirs for gcm_unit_tests too?

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:25: '../../url/url.gyp:url_lib',
On 2013/10/16 07:16:31, akalin wrote:
> don't need to depend on url_lib yet, do you?

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:38: # Typed-parametrized tests generate exit-time
destructors.
On 2013/10/16 07:16:31, akalin wrote:
> we don't use type-parametrized tests yet, right? enable warning until then?

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:41: 'GCM_TEST',
On 2013/10/16 07:16:31, akalin wrote:
> don't think you need this define

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:46: '../../net/net.gyp:net',
On 2013/10/16 07:16:31, akalin wrote:
> need to depend on net directly? (don't think so)

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:49:
'../../third_party/protobuf/protobuf.gyp:protobuf_lite',
On 2013/10/16 07:16:31, akalin wrote:
> need to depend on protobuf_lite directly? (don't think so)

Done.

[akalin, 2013-10-16 23:44:24]

lgtm after nits

hooray!

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:319: std::string full_data =
std::string(kWriteData);
On 2013/10/16 19:56:18, Nicolas Zea wrote:
> On 2013/10/16 07:16:31, akalin wrote:
> > why not work with kWriteData directly and adjust sizes?
> 
> Done.

I meant more like:

write_list.push_back(MockWrite(..., kWriteData, kWriteDataSize / 2),
                     MockWrite(..., kWriteData + kWriteDataSize / 2,
kWriteDataSize / 2)

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:338: std::string full_data =
std::string(kWriteData);
On 2013/10/16 19:56:18, Nicolas Zea wrote:
> On 2013/10/16 07:16:31, akalin wrote:
> > here too
> 
> Done.

here too

https://codereview.chromium.org/23684017/diff/152001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/23684017/diff/152001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:40: '../../third_party/protobuf/src',
this shouldn't be necessary. Remove this line (the protobuf include_dirs line)
and instead put this into the gcm target:

'export_dependent_settings': [
  <contents of 'dependencies' block>
],

then the includes line for protobuf should be propagated all the way from the
protobuf_lite target itself

[Nicolas Zea, 2013-10-17 19:51:52]

Done, yay! Thanks for the patient review.

TBR'ing Daring for adding base/ and google/ the new DEPS file.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
File google_apis/gcm/base/socket_stream_unittest.cc (right):

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:319: std::string full_data =
std::string(kWriteData);
On 2013/10/16 23:44:25, akalin wrote:
> On 2013/10/16 19:56:18, Nicolas Zea wrote:
> > On 2013/10/16 07:16:31, akalin wrote:
> > > why not work with kWriteData directly and adjust sizes?
> > 
> > Done.
> 
> I meant more like:
> 
> write_list.push_back(MockWrite(..., kWriteData, kWriteDataSize / 2),
>                      MockWrite(..., kWriteData + kWriteDataSize / 2,
> kWriteDataSize / 2)

Done.

https://codereview.chromium.org/23684017/diff/140001/google_apis/gcm/base/soc...
google_apis/gcm/base/socket_stream_unittest.cc:338: std::string full_data =
std::string(kWriteData);
On 2013/10/16 23:44:25, akalin wrote:
> On 2013/10/16 19:56:18, Nicolas Zea wrote:
> > On 2013/10/16 07:16:31, akalin wrote:
> > > here too
> > 
> > Done.
> 
> here too

Done.

https://codereview.chromium.org/23684017/diff/152001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/23684017/diff/152001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:40: '../../third_party/protobuf/src',
On 2013/10/16 23:44:25, akalin wrote:
> this shouldn't be necessary. Remove this line (the protobuf include_dirs line)
> and instead put this into the gcm target:
> 
> 'export_dependent_settings': [
>   <contents of 'dependencies' block>
> ],
> 
> then the includes line for protobuf should be propagated all the way from the
> protobuf_lite target itself

Done.

[Nicolas Zea, 2013-10-17 19:58:18]

+Joi for addition to google_apis/ directory. The eng review suggested it as the
destination because it's a new google service (GCM = Google Cloud Messaging). I
can forward you the discussions if you're interested.

[Jói, 2013-10-17 22:05:11]

Addition to google_apis LGTM. This is exactly the place for stuff like this, and
your DEPS file has only the kind of dependencies I would expect.

Cheers,
Jói

[commit-bot: I haz the power, 2013-10-17 23:17:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/23684017/168001

[commit-bot: I haz the power, 2013-10-18 01:17:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/23684017/168001

[commit-bot: I haz the power, 2013-10-18 18:12:51]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-10-18 20:24:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/23684017/168001

[commit-bot: I haz the power, 2013-10-18 20:57:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/23684017/168001

[commit-bot: I haz the power, 2013-10-19 03:59:20]

Retried try job too often on win7_aura for step(s) app_list_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-19 04:59:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/23684017/168001

[commit-bot: I haz the power, 2013-10-19 10:23:22]

Change committed as 229533