Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1053343002:
Use TCPServerSocket instead of TCPListenSocket in transport_client_socket_unittest.cc (Closed)
DescriptionUse TCPServerSocket instead of TCPListenSocket in transport_client_socket_unittest.cc
TCPListenSocket will be removed, use TCPServerSocket instead.
BUG=472766
Committed: https://crrev.com/5937d1a03090cc6a52a3322c34ef3c9f99f05de9
Cr-Commit-Position: refs/heads/master@{#326879}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Address comments #
Total comments: 23
Patch Set 3 : Address comments #
Total comments: 14
Patch Set 4 : Moved tests to tcp_client_socket_unittests.cc #
Total comments: 4
Patch Set 5 : Revert and remove checks #Messages
Total messages: 25 (6 generated)
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
Hi Matt, not urgent, I can wait after the weekend. Just a first attempt before I touch more files. Thanks! https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:102: base::Bind(&TransportClientSocketTest::AcceptCallback, Using a dummy callback here, not sure if there's a better way. When I use "TestCompletionCallback", the object is already deallocated when the callback is called. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:399: rv = callback.WaitForResult(); I wonder what the best way to deal with these two disabled tests is. I tried simply removing "PauseServerReads()", and doing listen_sock_->Read in "ResumeServerReads()". But the read callback seems to be blocked on line 399 and on line 453.
https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:96: for (port = kMinPort; port < kMaxPort; port++) { While we're here, can we get rid of this loop? I believe we can just use: ASSERT_EQ(OK, listen_sock_->Listen(IPEndPoint(address, 0), 1)); And then grab the port we actually got using listen_sock_->GetLocalAddress. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:102: base::Bind(&TransportClientSocketTest::AcceptCallback, On 2015/04/03 20:19:23, xunjieli wrote: > Using a dummy callback here, not sure if there's a better way. When I use > "TestCompletionCallback", the object is already deallocated when the callback is > called. How about having AcceptCallback quite a base::RunLoop, which you can wait on whenever you call Connect()? Makes sure both sides know they have been connected before we continue with the test. Should also be checking the result passed to the callback. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:144: int request_len = arraysize(request_text) - 1; Much cleaner to have this in a variable. Maybe just use strlen, though (And include the appropriate header)? https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:155: EXPECT_EQ(rv, request_len); nit: Flip order, expected should go first (Know this was wrong before) https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:159: new IOBufferWithSize(request_len + 1)); Is the +1 needed? https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:162: read_buffer.get(), read_buffer->size(), read_callback.callback()); I don't believe can actually be completely sure we read the entire thing in one go (Actually, the same is true for the Write call above, too). While in practice, the text is short enough that it should generally be taken care in one call in either direct, I'd be more comfortable with loops on both sides of things. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:178: write_result = write_callback.GetResult(write_result); Should handle ERR_IO_PENDING, like above, and I also suggest a for loop, for partial writes. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:399: rv = callback.WaitForResult(); On 2015/04/03 20:19:23, xunjieli wrote: > I wonder what the best way to deal with these two disabled tests is. I tried > simply removing "PauseServerReads()", and doing listen_sock_->Read in > "ResumeServerReads()". But the read callback seems to be blocked on line 399 and > on line 453. In this test, the problem is the buffer is full. You should read everything from the connected_sock_, and then this should pass (Could also count total bytes written from the last n - 1 calls to write, and make sure you can read them all) You'll also need to write something to connected_sock_ before waiting on the read callback, of course. Next test presumably has similar issues. Haven't looked at it.
https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:93: #endif Also, we can remove this #if block - creating the TCPServerSocket creates a TCPSocketWin, which calls this for us.
https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:399: rv = callback.WaitForResult(); On 2015/04/06 18:30:03, mmenke wrote: > On 2015/04/03 20:19:23, xunjieli wrote: > > I wonder what the best way to deal with these two disabled tests is. I tried > > simply removing "PauseServerReads()", and doing listen_sock_->Read in > > "ResumeServerReads()". But the read callback seems to be blocked on line 399 > and > > on line 453. > > In this test, the problem is the buffer is full. You should read everything > from the connected_sock_, and then this should pass (Could also count total > bytes written from the last n - 1 calls to write, and make sure you can read > them all) > > You'll also need to write something to connected_sock_ before waiting on the > read callback, of course. > > Next test presumably has similar issues. Haven't looked at it. One other option: You could make the connected_socket_ behave as the old listen socket did, though instead of pause/resume methods, you'd just need a StartReading method, which calls Read. Once any data is read, it writes it all (Setting a write callback, and writing again as needed), and once all data is written, calling read again. Up to you which approach you prefer.
Patchset #2 (id:40001) has been deleted
Thanks for the review! https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:93: #endif On 2015/04/06 18:37:33, mmenke wrote: > Also, we can remove this #if block - creating the TCPServerSocket creates a > TCPSocketWin, which calls this for us. Done. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:96: for (port = kMinPort; port < kMaxPort; port++) { On 2015/04/06 18:30:03, mmenke wrote: > While we're here, can we get rid of this loop? > > I believe we can just use: > > ASSERT_EQ(OK, listen_sock_->Listen(IPEndPoint(address, 0), 1)); > > And then grab the port we actually got using listen_sock_->GetLocalAddress. Done. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:102: base::Bind(&TransportClientSocketTest::AcceptCallback, On 2015/04/06 18:30:03, mmenke wrote: > On 2015/04/03 20:19:23, xunjieli wrote: > > Using a dummy callback here, not sure if there's a better way. When I use > > "TestCompletionCallback", the object is already deallocated when the callback > is > > called. > > How about having AcceptCallback quite a base::RunLoop, which you can wait on > whenever you call Connect()? Makes sure both sides know they have been > connected before we continue with the test. > > Should also be checking the result passed to the callback. Done. Good idea! thanks https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:144: int request_len = arraysize(request_text) - 1; On 2015/04/06 18:30:03, mmenke wrote: > Much cleaner to have this in a variable. Maybe just use strlen, though (And > include the appropriate header)? Partially done. Sorry, I am not sure what you meant by "include the appropriate header." Should I make |request_test| contain some request headers? https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:155: EXPECT_EQ(rv, request_len); On 2015/04/06 18:30:03, mmenke wrote: > nit: Flip order, expected should go first (Know this was wrong before) Done. Right! https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:159: new IOBufferWithSize(request_len + 1)); On 2015/04/06 18:30:03, mmenke wrote: > Is the +1 needed? I am not sure. I thought we need to have 1 byte extra for the EOF. I guess not. Done. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:162: read_buffer.get(), read_buffer->size(), read_callback.callback()); On 2015/04/06 18:30:03, mmenke wrote: > I don't believe can actually be completely sure we read the entire thing in one > go (Actually, the same is true for the Write call above, too). > > While in practice, the text is short enough that it should generally be taken > care in one call in either direct, I'd be more comfortable with loops on both > sides of things. Done. https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:178: write_result = write_callback.GetResult(write_result); On 2015/04/06 18:30:03, mmenke wrote: > Should handle ERR_IO_PENDING, like above, and I also suggest a for loop, for > partial writes. But GetResult will wait if it is ERR_IO_PENDING right? https://codereview.chromium.org/1053343002/diff/20001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:399: rv = callback.WaitForResult(); On 2015/04/06 18:44:38, mmenke wrote: > On 2015/04/06 18:30:03, mmenke wrote: > > On 2015/04/03 20:19:23, xunjieli wrote: > > > I wonder what the best way to deal with these two disabled tests is. I tried > > > simply removing "PauseServerReads()", and doing listen_sock_->Read in > > > "ResumeServerReads()". But the read callback seems to be blocked on line 399 > > and > > > on line 453. > > > > In this test, the problem is the buffer is full. You should read everything > > from the connected_sock_, and then this should pass (Could also count total > > bytes written from the last n - 1 calls to write, and make sure you can read > > them all) > > > > You'll also need to write something to connected_sock_ before waiting on the > > read callback, of course. > > > > Next test presumably has similar issues. Haven't looked at it. > > One other option: You could make the connected_socket_ behave as the old listen > socket did, though instead of pause/resume methods, you'd just need a > StartReading method, which calls Read. Once any data is read, it writes it all > (Setting a write callback, and writing again as needed), and once all data is > written, calling read again. > > Up to you which approach you prefer. Done. Right, I should have write something to connected_sock_, otherwise the read callback will block forever. The original listen socket does not seem to do writes, I wonder why read callback does not block (line 422 of the new patch).
https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:59: ASSERT_TRUE(loop_.running()); I don't think we need this check - this basically requires we always call loop_.Run before callback.GetResult, which seems like an unintuitive requirement. RunLoops handle Quit before Run fine (That is, Run will do nothing if the loop has already been quit). https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:69: void ProcessRequest(); Think this is worth a comment (Maybe something like "Send request from client to server socket, have the server read it, and send the response"?) https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:69: void ProcessRequest(); Also should probably rename it. SendRequestAndResponse? https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:101: ASSERT_EQ(OK, listen_sock_->GetLocalAddress(&local_address)); nit: Think this is worth a comment or two, to make it clear why we need to get the address we just set after listening. Just something like: Just above the "listen_sock_.reset" line (Or the listen line). // Open a server socket on an ephemeral port. And just above this line: // Get the server's address (including actual port number). https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:133: EXPECT_GE(rv, 0); While you're here, this can actually be ASSERT_GT https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:143: int request_len = strlen(request_text); On 2015/04/08 14:47:19, xunjieli wrote: > On 2015/04/06 18:30:03, mmenke wrote: > > Much cleaner to have this in a variable. Maybe just use strlen, though (And > > include the appropriate header)? > > Partially done. Sorry, I am not sure what you meant by "include the appropriate > header." Should I make |request_test| contain some request headers? Sorry, I meant the header for strlen (Which is just <string>) https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:155: ASSERT_GE(write_result, 0); nit: This can actually be ASSERT_GT https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:161: // Confirms that the server receives what client sent. nit: Confirm, to be consistent with other comments. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:180: ASSERT_EQ(message, received_message); nit: Think you can just inline request_text - ASSERT_EQ is smart enough to compare a string and a char*. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:208: void TransportClientSocketTest::StartServerReads(int expected_bytes_read) { Suggestion: Modify StartServerReads to just read expected_bytes_read bytes, and return it as a string (Can then use it in TransportClientSocketTest::ProcessRequest). Also don't have if call SendServerResponse() - just have tests call it manually when needed. Most tests should probably also check that connected_sock_ IsConnnectedAndIdle after calling StartServerReads. Also suggest renaming StartServerReads to ReadServerData or somesuch. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:237: EXPECT_EQ(OK, callback.GetResult(rv)); Suggest leaving this where it was. Serves largely to make sure the right netlog events are emitted before and after connection. Not exactly critical, but moving it does somewhat change what the test is testing.
Just realized that that it took me two ways to get back to this. Sorry abt that. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:59: ASSERT_TRUE(loop_.running()); On 2015/04/09 15:16:15, mmenke wrote: > I don't think we need this check - this basically requires we always call > loop_.Run before callback.GetResult, which seems like an unintuitive > requirement. > > RunLoops handle Quit before Run fine (That is, Run will do nothing if the loop > has already been quit). Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:69: void ProcessRequest(); On 2015/04/09 15:16:15, mmenke wrote: > Also should probably rename it. SendRequestAndResponse? Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:69: void ProcessRequest(); On 2015/04/09 15:16:15, mmenke wrote: > Think this is worth a comment (Maybe something like "Send request from client to > server socket, have the server read it, and send the response"?) Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:101: ASSERT_EQ(OK, listen_sock_->GetLocalAddress(&local_address)); On 2015/04/09 15:16:15, mmenke wrote: > nit: Think this is worth a comment or two, to make it clear why we need to get > the address we just set after listening. Just something like: > > Just above the "listen_sock_.reset" line (Or the listen line). > // Open a server socket on an ephemeral port. > > And just above this line: > > // Get the server's address (including actual port number). Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:133: EXPECT_GE(rv, 0); On 2015/04/09 15:16:15, mmenke wrote: > While you're here, this can actually be ASSERT_GT Hmm, tried to use ASSERT_GT, but it complains that "cannot initialize return object of type 'int' with an rvalue of type 'void". https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:143: int request_len = strlen(request_text); On 2015/04/09 15:16:15, mmenke wrote: > On 2015/04/08 14:47:19, xunjieli wrote: > > On 2015/04/06 18:30:03, mmenke wrote: > > > Much cleaner to have this in a variable. Maybe just use strlen, though (And > > > include the appropriate header)? > > > > Partially done. Sorry, I am not sure what you meant by "include the > appropriate > > header." Should I make |request_test| contain some request headers? > > Sorry, I meant the header for strlen (Which is just <string>) Done. Ah, I see! thanks! https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:155: ASSERT_GE(write_result, 0); On 2015/04/09 15:16:15, mmenke wrote: > nit: This can actually be ASSERT_GT Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:161: // Confirms that the server receives what client sent. On 2015/04/09 15:16:15, mmenke wrote: > nit: Confirm, to be consistent with other comments. Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:180: ASSERT_EQ(message, received_message); On 2015/04/09 15:16:15, mmenke wrote: > nit: Think you can just inline request_text - ASSERT_EQ is smart enough to > compare a string and a char*. Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:208: void TransportClientSocketTest::StartServerReads(int expected_bytes_read) { On 2015/04/09 15:16:15, mmenke wrote: > Suggestion: Modify StartServerReads to just read expected_bytes_read bytes, and > return it as a string (Can then use it in > TransportClientSocketTest::ProcessRequest). Also don't have if call > SendServerResponse() - just have tests call it manually when needed. > > Most tests should probably also check that connected_sock_ IsConnnectedAndIdle > after calling StartServerReads. > > Also suggest renaming StartServerReads to ReadServerData or somesuch. Done. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:237: EXPECT_EQ(OK, callback.GetResult(rv)); On 2015/04/09 15:16:15, mmenke wrote: > Suggest leaving this where it was. Serves largely to make sure the right netlog > events are emitted before and after connection. Not exactly critical, but > moving it does somewhat change what the test is testing. Done. I see. Makes sense. Thanks! https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:21: #include "net/socket/tcp_client_socket.h" Presubmit complains about the import order unless I move net/socket/tcp_client_socket.h down. Not sure why.
I'll try and get to this today, but was just assigned a new top 10 browser crasher on stable, so may not get to it until tomorrow.
On 2015/04/22 15:01:45, mmenke wrote: > I'll try and get to this today, but was just assigned a new top 10 browser > crasher on stable, so may not get to it until tomorrow. Oh, and it's fine to just work on this cleanup stuff as time permits. The code's been waiting 4+ years to be removed. A little longer won't hurt.
On 2015/04/22 15:02:58, mmenke wrote: > On 2015/04/22 15:01:45, mmenke wrote: > > I'll try and get to this today, but was just assigned a new top 10 browser > > crasher on stable, so may not get to it until tomorrow. > > Oh, and it's fine to just work on this cleanup stuff as time permits. The > code's been waiting 4+ years to be removed. A little longer won't hurt. Right :) I have other stuff to work in the meanwhile. Don't worry about it. Thanks!
LGTM, thanks for doing this! Just minor comments. https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/60001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:133: EXPECT_GE(rv, 0); On 2015/04/21 19:50:24, xunjieli wrote: > On 2015/04/09 15:16:15, mmenke wrote: > > While you're here, this can actually be ASSERT_GT > > Hmm, tried to use ASSERT_GT, but it complains that "cannot initialize return > object of type 'int' with an rvalue of type 'void". Sorry, meant EXPECT_GT https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:21: #include "net/socket/tcp_client_socket.h" On 2015/04/21 19:50:24, xunjieli wrote: > Presubmit complains about the import order unless I move > net/socket/tcp_client_socket.h down. Not sure why. Because this file is transport_client_socket_unittest, and the file is tcp_client_socket. Could optionally rename this file and the test fixture, and move the include back up top. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:85: base::RunLoop loop_; Suggest calling this connect_loop_ or somesuch. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:171: ASSERT_EQ(request_len, bytes_written); nit: This should probably be right after the for loop. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:228: loop_.Run(); Suggest a comment. Maybe "Wait for |listen_sock_| to accept a connection." https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:229: EXPECT_EQ(OK, callback.GetResult(rv)); Suggest a comment "Now wait for the client socket to accept the connection." https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:229: EXPECT_EQ(OK, callback.GetResult(rv)); Just because of the comments, may want to make these 3 lines into a method in the test fixture. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:426: // PauseServerReads(); Remove unused code.
Thanks for the review! https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... File net/socket/transport_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:21: #include "net/socket/tcp_client_socket.h" On 2015/04/23 15:43:07, mmenke (Out 4-27 to 5-1) wrote: > On 2015/04/21 19:50:24, xunjieli wrote: > > Presubmit complains about the import order unless I move > > net/socket/tcp_client_socket.h down. Not sure why. > > Because this file is transport_client_socket_unittest, and the file is > tcp_client_socket. Could optionally rename this file and the test fixture, and > move the include back up top. Done. Ah, I see! There's already a tcp_client_socket_unittests.cc so I combined the two. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:85: base::RunLoop loop_; On 2015/04/23 15:43:07, mmenke (Out 4-27 to 5-1) wrote: > Suggest calling this connect_loop_ or somesuch. Done. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:171: ASSERT_EQ(request_len, bytes_written); On 2015/04/23 15:43:07, mmenke (Out 4-27 to 5-1) wrote: > nit: This should probably be right after the for loop. Done. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:228: loop_.Run(); On 2015/04/23 15:43:07, mmenke (Out 4-27 to 5-1) wrote: > Suggest a comment. Maybe "Wait for |listen_sock_| to accept a connection." Done. https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:229: EXPECT_EQ(OK, callback.GetResult(rv)); On 2015/04/23 15:43:07, mmenke (Out 4-27 to 5-1) wrote: > Just because of the comments, may want to make these 3 lines into a method in > the test fixture. Done. Changed for everywhere else. You suggested leaving this as before, since this one tests netlog before and after. I thought I did revert it, but somehow I didn't :). https://codereview.chromium.org/1053343002/diff/80001/net/socket/transport_cl... net/socket/transport_client_socket_unittest.cc:426: // PauseServerReads(); On 2015/04/23 15:43:07, mmenke (Out 4-27 to 5-1) wrote: > Remove unused code. Done.
https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... File net/socket/tcp_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... net/socket/tcp_client_socket_unittest.cc:232: ::testing::Values(TCP)); This TODO seems to indicate this is intended to test another protocol as well. Given that it was added in 2011, we're probably safe getting rid of it, but I think we're way too far afield of the original purpose of this CL. Suggest just moving it back into another file for now.
https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... File net/socket/tcp_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... net/socket/tcp_client_socket_unittest.cc:232: ::testing::Values(TCP)); On 2015/04/24 20:45:49, mmenke (Out 4-27 to 5-1) wrote: > This TODO seems to indicate this is intended to test another protocol as well. > Given that it was added in 2011, we're probably safe getting rid of it, but I > think we're way too far afield of the original purpose of this CL. Suggest just > moving it back into another file for now. Sounds good! will do. https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... net/socket/tcp_client_socket_unittest.cc:466: ASSERT_TRUE(connected_sock_->IsConnectedAndIdle()); Matt, I should remove line 466 and 412's ASSERT_TRUE(connected_sock_->IsConnectedAndIdle()) (which I added), right? The two checks fail on two win bots. I think we should remove these two checks because even we read bytes_written server data, we still have data pending, so connected_sock_ should not be idle. Not sure why it is not the case on other platforms.
https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... File net/socket/tcp_client_socket_unittest.cc (right): https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... net/socket/tcp_client_socket_unittest.cc:466: ASSERT_TRUE(connected_sock_->IsConnectedAndIdle()); On 2015/04/24 20:50:15, xunjieli wrote: > Matt, I should remove line 466 and 412's > ASSERT_TRUE(connected_sock_->IsConnectedAndIdle()) (which I added), right? > The two checks fail on two win bots. I think we should remove these two checks > because even we read bytes_written server data, we still have data pending, so > connected_sock_ should not be idle. Not sure why it is not the case on other > platforms. Yes, you should remove just remove them. They may be returning true on other platforms based on the vagaries of when the ERR_IO_PENDING data is actually written to the socket after we drain it, or when it's actually added to the read buffer of the other socket.
Patchset #5 (id:120001) has been deleted
On 2015/04/24 20:55:40, mmenke (Out 4-27 to 5-1) wrote: > https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... > File net/socket/tcp_client_socket_unittest.cc (right): > > https://codereview.chromium.org/1053343002/diff/100001/net/socket/tcp_client_... > net/socket/tcp_client_socket_unittest.cc:466: > ASSERT_TRUE(connected_sock_->IsConnectedAndIdle()); > On 2015/04/24 20:50:15, xunjieli wrote: > > Matt, I should remove line 466 and 412's > > ASSERT_TRUE(connected_sock_->IsConnectedAndIdle()) (which I added), right? > > The two checks fail on two win bots. I think we should remove these two checks > > because even we read bytes_written server data, we still have data pending, so > > connected_sock_ should not be idle. Not sure why it is not the case on other > > platforms. > > Yes, you should remove just remove them. > > They may be returning true on other platforms based on the vagaries of when the > ERR_IO_PENDING data is actually written to the socket after we drain it, or when > it's actually added to the read buffer of the other socket. Done. Thanks!
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1053343002/#ps140001 (title: "Revert and remove checks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1053343002/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5937d1a03090cc6a52a3322c34ef3c9f99f05de9 Cr-Commit-Position: refs/heads/master@{#326879} |
