Improve socket.write() to make sure all data will be sent out.

Improve socket.write() to make sure all data will be sent out.


BUG=128220
TEST=browser_tests --gtest_filter=SocketTest.*

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

[Peng, 2012-05-16 16:00:15]

Hi Mike & Jeremy, Could you please check this CL. It improves socket.write() to
make sure all data will be sent out. Thanks.

[Peng, 2012-05-16 16:13:27]

-jeremy
+jeremya

[miket_OOO, 2012-05-16 19:29:10]

lgtm

[jeremya, 2012-05-17 01:09:06]

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.cc (right):

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:44: io_buffer_write_ = new
net::WrappedIOBuffer(
Why does io_buffer_write_ need to be stored in a member variable? It seems like
you could just have a bool is_write_pending_ instead, which is true when waiting
for an OnWriteComplete callback.

Another possible architecture would be to have a WriteRequest
pending_write_request_ member variable, which represents the WriteRequest
currently being processed.

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:56: io_buffer_write_ = NULL;
Is this leaking?

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:60: if (result >= 0) {
What does result == 0 mean?

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.h (right):

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:53: // Returns the number of bytes
successfully written, or a negative error
Update comment.

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:83: 
nit: extra blank line

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:103: byte_written(0) {}
bytes_written

[Peng, 2012-05-17 02:42:22]

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.cc (right):

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:44: io_buffer_write_ = new
net::WrappedIOBuffer(
On 2012/05/17 01:09:06, jeremya wrote:
> Why does io_buffer_write_ need to be stored in a member variable? It seems
like
> you could just have a bool is_write_pending_ instead, which is true when
waiting
> for an OnWriteComplete callback.
> 
> Another possible architecture would be to have a WriteRequest
> pending_write_request_ member variable, which represents the WriteRequest
> currently being processed.

Because WriteImpl() & TCPClientSocket::Write() take real IOBuffer pointer as
argument. I through we should not delete the IOBuffer until OnComplete be
called.

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:56: io_buffer_write_ = NULL;
On 2012/05/17 01:09:06, jeremya wrote:
> Is this leaking?
It is a scoped_refptr<net::IOBuffer>. So it is not a leaking.

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:60: if (result >= 0) {
On 2012/05/17 01:09:06, jeremya wrote:
> What does result == 0 mean?
I remember Linux allows call write() with a zero size buffer to test if the
connection is alive.

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.h (right):

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:53: // Returns the number of bytes
successfully written, or a negative error
On 2012/05/17 01:09:06, jeremya wrote:
> Update comment.

Will fix it tomorrow. I am home and can not access my machine.

[Peng, 2012-05-17 15:05:13]

Updated this CL. Please check it. Thanks

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.h (right):

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:53: // Returns the number of bytes
successfully written, or a negative error
On 2012/05/17 01:09:06, jeremya wrote:
> Update comment.

Done.

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:83: 
On 2012/05/17 01:09:06, jeremya wrote:
> nit: extra blank line

Done.

https://chromiumcodereview.appspot.com/10310170/diff/3002/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:103: byte_written(0) {}
On 2012/05/17 01:09:06, jeremya wrote:
> bytes_written

Done.

[jeremya, 2012-05-18 01:32:12]

lgtm

https://chromiumcodereview.appspot.com/10310170/diff/7008/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.h (right):

https://chromiumcodereview.appspot.com/10310170/diff/7008/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:51: // written, or a negative
number if an error occurred.
The number of bytes will always be the same as the byte_count passed in (if
successful), yes? If so, can you mention this in the comment?

[Peng, 2012-05-18 15:12:19]

https://chromiumcodereview.appspot.com/10310170/diff/7008/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.h (right):

https://chromiumcodereview.appspot.com/10310170/diff/7008/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.h:51: // written, or a negative
number if an error occurred.
On 2012/05/18 01:32:12, jeremya wrote:
> The number of bytes will always be the same as the byte_count passed in (if
> successful), yes? If so, can you mention this in the comment?

Done.

[commit-bot: I haz the power, 2012-05-18 15:13:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10310170/13001

[commit-bot: I haz the power, 2012-05-18 15:29:13]

Try job failure for 10310170-13001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-18 17:49:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10310170/18001

[commit-bot: I haz the power, 2012-05-18 19:28:27]

Change committed as 137923