Add unittests for BufferedSocketWriter and fix some bugs in that code.

Add unittests for BufferedSocketWriter and fix some bugs in that code.

One issue that is being fixed is that previously that class didn't
properly handle the case when write is called from a callback or
the object is destroyed from a callback.


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

[Sergey Ulanov, 2012-07-31 18:12:33]

This fixes issues I discovered when debugging
http://codereview.chromium.org/10830046/ . It shouldn't affects any existing
code.

[simonmorris, 2012-07-31 20:19:50]

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
File remoting/protocol/buffered_socket_writer.cc (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.cc:17: class
BufferedSocketWriterBase::PendingPacket {
Maybe just make this a struct?

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.cc:95: bool* write_again) {
Can't HandleWriteResult just return write_again?

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.cc:113: destroyed_flag_ = NULL;
Isn't this use-after-free if the BufferedSocketWriterBase has been destroyed?

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
File remoting/protocol/buffered_socket_writer.h (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.h:71: 
Maybe document what the caller may/should/must do with the returned
base::Closure?

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.h:74: // Returns next packet that needs
to be written to the socket. |buffer| must
"|buffer| must" -> "*buffer will"? "Must" sounds like a constraint on the
caller. Or if the comment is addressed to the implementer of a derived class,
not a client of the base class, say "An implementation must...".

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
File remoting/protocol/buffered_socket_writer_unittest.cc (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer_unittest.cc:27: 
Remove blank line.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/fake_session.cc
File remoting/protocol/fake_session.cc (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/fake_session...
remoting/protocol/fake_session.cc:21: : async_write_(false),
Initialize write_pending_.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/fake_session...
remoting/protocol/fake_session.cc:93: }
Is next_write_error_ used anywhere else?

[Sergey Ulanov, 2012-07-31 21:37:52]

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
File remoting/protocol/buffered_socket_writer.cc (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.cc:17: class
BufferedSocketWriterBase::PendingPacket {
On 2012/07/31 20:19:50, simonmorris wrote:
> Maybe just make this a struct?

yes, that was my intent

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.cc:95: bool* write_again) {
On 2012/07/31 20:19:50, simonmorris wrote:
> Can't HandleWriteResult just return write_again?

It can by, it's more readable this way because the result is named. Otherwise I
would have to add comment in the header to explain the meaning of that result
and that comment would have to be read every time somebody reads this code.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.cc:113: destroyed_flag_ = NULL;
On 2012/07/31 20:19:50, simonmorris wrote:
> Isn't this use-after-free if the BufferedSocketWriterBase has been destroyed?

Good catch. Fixed.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
File remoting/protocol/buffered_socket_writer.h (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.h:71: 
On 2012/07/31 20:19:50, simonmorris wrote:
> Maybe document what the caller may/should/must do with the returned
> base::Closure?

Done.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.h:74: // Returns next packet that needs
to be written to the socket. |buffer| must
On 2012/07/31 20:19:50, simonmorris wrote:
> "|buffer| must" -> "*buffer will"? "Must" sounds like a constraint on the
> caller. Or if the comment is addressed to the implementer of a derived class,
> not a client of the base class, say "An implementation must...".

Done.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
File remoting/protocol/buffered_socket_writer_unittest.cc (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer_unittest.cc:27: 
On 2012/07/31 20:19:50, simonmorris wrote:
> Remove blank line.

Done.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/fake_session.cc
File remoting/protocol/fake_session.cc (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/fake_session...
remoting/protocol/fake_session.cc:21: : async_write_(false),
On 2012/07/31 20:19:50, simonmorris wrote:
> Initialize write_pending_.

Done.

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/fake_session...
remoting/protocol/fake_session.cc:93: }
On 2012/07/31 20:19:50, simonmorris wrote:
> Is next_write_error_ used anywhere else?

It's set in set_next_write_error() and used only here.
Also added support for asynch write errors and copied this code to
DoAsyncWrite()

[simonmorris, 2012-07-31 22:20:12]

lgtm with optional suggestion

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
File remoting/protocol/buffered_socket_writer.cc (right):

http://codereview.chromium.org/10836030/diff/1/remoting/protocol/buffered_soc...
remoting/protocol/buffered_socket_writer.cc:95: bool* write_again) {
On 2012/07/31 21:37:53, sergeyu wrote:
> On 2012/07/31 20:19:50, simonmorris wrote:
> > Can't HandleWriteResult just return write_again?
> 
> It can by, it's more readable this way because the result is named. Otherwise
I
> would have to add comment in the header to explain the meaning of that result
> and that comment would have to be read every time somebody reads this code.

I think this way makes the call sites unnecessarily hard to read, but I don't
mind if you still prefer it.

[commit-bot: I haz the power, 2012-08-01 00:28:02]

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

[commit-bot: I haz the power, 2012-08-01 01:48:05]

Change committed as 149350