Rename SocketReaderBase to SocketReader and move it to remoting/base

Rename SocketReaderBase to SocketReader and move it to remoting/base

SocketReaderBase is not used anywhere. Renamed it to SocketReader 
and fixed it so that it can be encapsulated instead of inheriting 
from it. Also moved it to remoting/base directory.

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

[Wez, 2012-11-09 22:55:02]

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.cc
File remoting/base/socket_reader.cc (right):

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.cc...
remoting/base/socket_reader.cc:31: DCHECK(socket);
nit: I'd prefer blank lines after DCHECK and before DoRead.

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.cc...
remoting/base/socket_reader.cc:42: base::Unretained(this)));
This class doesn't own |socket_|, so it's possible for it to have been deleted
by the time Read() completes, so this should use WeakPtr as well.

However, that means we'll potentially read and lose data from |socket_| after
this object is torn down, which may not be what the caller expects.  Should this
class instead own |socket_|?

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h
File remoting/base/socket_reader.h (right):

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h#...
remoting/base/socket_reader.h:24: SocketReader();
Would it make sense to merge this with Init() - the caller would e.g. create one
and assign it to a scoped_ptr<> and then simply reset the scoped_ptr<> to stop
getting callbacks.

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h#...
remoting/base/socket_reader.h:27: void Init(net::Socket* socket,
ReadResultCallback read_result_callback);
nit: Add a comment describing whcih Init() does and e.g. can it be called more
than once to change the Socket, does the caller retain ownership of Socket, etc.

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h#...
remoting/base/socket_reader.h:35: base::WeakPtrFactory<SocketReader>
weak_factory_;
Strictly this should be the final member of the class, so that it will be the
first member to be torn down.

[Sergey Ulanov, 2012-11-10 00:22:11]

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.cc
File remoting/base/socket_reader.cc (right):

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.cc...
remoting/base/socket_reader.cc:31: DCHECK(socket);
On 2012/11/09 22:55:03, Wez wrote:
> nit: I'd prefer blank lines after DCHECK and before DoRead.

Done.

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.cc...
remoting/base/socket_reader.cc:42: base::Unretained(this)));
On 2012/11/09 22:55:03, Wez wrote:
> This class doesn't own |socket_|, so it's possible for it to have been deleted
> by the time Read() completes, so this should use WeakPtr as well.
> 
> However, that means we'll potentially read and lose data from |socket_| after
> this object is torn down, which may not be what the caller expects.  Should
this
> class instead own |socket_|?

I don't think this class should own the socket because normally the calling code
will also need socket_ to write to it. Changed this call to use weak pointer and
added comments about this corner case in .h .

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h
File remoting/base/socket_reader.h (right):

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h#...
remoting/base/socket_reader.h:24: SocketReader();
On 2012/11/09 22:55:03, Wez wrote:
> Would it make sense to merge this with Init() - the caller would e.g. create
one
> and assign it to a scoped_ptr<> and then simply reset the scoped_ptr<> to stop
> getting callbacks.

IMO it's better to keep Init() separate. It allows it to be a member of the
owning class instead of being allocated on the heap, e.g.
http://codereview.chromium.org/11358190/diff/5001/remoting/host/websocket_con...
. This approach saves one malloc(), and also consistent with
BufferedSocketWriter.

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h#...
remoting/base/socket_reader.h:27: void Init(net::Socket* socket,
ReadResultCallback read_result_callback);
On 2012/11/09 22:55:03, Wez wrote:
> nit: Add a comment describing whcih Init() does and e.g. can it be called more
> than once to change the Socket, does the caller retain ownership of Socket,
etc.

Done.

http://codereview.chromium.org/11361197/diff/1/remoting/base/socket_reader.h#...
remoting/base/socket_reader.h:35: base::WeakPtrFactory<SocketReader>
weak_factory_;
On 2012/11/09 22:55:03, Wez wrote:
> Strictly this should be the final member of the class, so that it will be the
> first member to be torn down.

Done, but I don't think it matters - there should be no code that calls
callbacks in destructors (and it would be wrong if we had such behavior
anywhere).

[Wez, 2012-11-10 00:57:05]

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader.h
File remoting/base/socket_reader.h (right):

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:22: // code should never be creating more than one
reader for the same socket.
nit: The caller may also miss data if it tries to read from the socket after
deleting this object.

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:27: int result)> ReadResultCallback;
nit: ReadCallback?

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:32: // Starts reading from |socket|.
|read_result_callback| for each completed
nit: |read_result_callback| is called for each ...

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:33: // read. Reading stops on the first error.
Must not be called more than once.
How is the caller notified in case of error?  A ReadResultCallback with NULL
buffer, perhaps?

[Sergey Ulanov, 2012-11-10 01:13:04]

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader.h
File remoting/base/socket_reader.h (right):

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:22: // code should never be creating more than one
reader for the same socket.
On 2012/11/10 00:57:05, Wez wrote:
> nit: The caller may also miss data if it tries to read from the socket after
> deleting this object.

Right, but I don't think it's worth mentioning here as the underlying socket
will DCHECK() anyway if the caller tries to abuse this class.

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:27: int result)> ReadResultCallback;
On 2012/11/10 00:57:05, Wez wrote:
> nit: ReadCallback?

"ReadCallback" would hide the fact that it is also called in case when read
fails.

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:32: // Starts reading from |socket|.
|read_result_callback| for each completed
On 2012/11/10 00:57:05, Wez wrote:
> nit: |read_result_callback| is called for each ...

Done.

http://codereview.chromium.org/11361197/diff/1006/remoting/base/socket_reader...
remoting/base/socket_reader.h:33: // read. Reading stops on the first error.
Must not be called more than once.
On 2012/11/10 00:57:05, Wez wrote:
> How is the caller notified in case of error?  A ReadResultCallback with NULL
> buffer, perhaps?

Yes, mentioned it in the comment for ReadResultCallback.

[Wez, 2012-11-10 01:16:05]

LGTM once the comment on behaviour when the class is deleted is addressed, as
per our discussion offline. :)

[Sergey Ulanov, 2012-11-10 01:18:44]

http://codereview.chromium.org/11361197/diff/13001/remoting/base/socket_reader.h
File remoting/base/socket_reader.h (right):

http://codereview.chromium.org/11361197/diff/13001/remoting/base/socket_reade...
remoting/base/socket_reader.h:22: // code should never be creating more than one
reader for the same socket.
Updated this comment to make it clearer.

[commit-bot: I haz the power, 2012-11-10 01:19:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11361197/10007

[commit-bot: I haz the power, 2012-11-10 03:27:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-10 07:40:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11361197/10007

[commit-bot: I haz the power, 2012-11-10 09:34:45]

Change committed as 167078