Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8457002:
Integration tests for HTTP pipelining. (Closed)
|
Created:
9 years, 1 month ago by James Simonsen Modified:
9 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntegration tests for HTTP pipelining.
Added tests that exercise the code from HttpNetworkTransaction down to
HttpStreamParser with pipelining enabled.
BUG=None
TEST=net_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109354
Patch Set 1 #Patch Set 2 : Added additional tests #
Total comments: 13
Patch Set 3 : Fix leaks #
Total comments: 19
Patch Set 4 : Test pipelining #
Total comments: 4
Patch Set 5 : Restore second read #
Messages
Total messages: 15 (0 generated)
Let me know if there are more integration tests you want now. I think this test suite will be more valuable once we are dynamically disabling pipelining and have more error correction. It'll ensure that's all invisible to the user.
On 2011/11/04 00:23:41, James Simonsen wrote: > Let me know if there are more integration tests you want now. I think this test > suite will be more valuable once we are dynamically disabling pipelining and > have more error correction. It'll ensure that's all invisible to the user. Quick suggestions for other tests, before I do a full review: I'd like to see an evict to new pipeline on session close, and when another transaction errors out. I'd also like to see an eviction while a send is still pending.
On 2011/11/04 15:18:21, Matt Menke wrote: > On 2011/11/04 00:23:41, James Simonsen wrote: > > Let me know if there are more integration tests you want now. I think this > test > > suite will be more valuable once we are dynamically disabling pipelining and > > have more error correction. It'll ensure that's all invisible to the user. > > Quick suggestions for other tests, before I do a full review: I'd like to see > an evict to new pipeline on session close, and when another transaction errors > out. I'd also like to see an eviction while a send is still pending. Good ideas. Done.
Sorry I didn't get back to you on Friday. The individual tests look good, though want to take a second look at a couple of them. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:44: class DummyHttpServerProperties : public HttpServerProperties { I don't think this class is necessary. We should just be able to use HttpServerPropertiesImpl. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:71: class DummyHostResolverProc : public HostResolverProc { I don't think this class is needed, either. We should be able to just use MockHostResolver. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:96: void SetUp() { virtual/OVERRIDE for this, and the next one, too. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:104: void Initialize() { Any reason not to do this in SetUp? It's called once per run test (SetUp -> TestBody - > TearDown). Could even do it in the constructor, since each test is actually its own subclass of this test class. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:108: session_params.host_resolver = new HostResolverImpl( None of these will be owned by the HttpNetworkSession, so they're all currently leaked. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:148: void PipelineTwoRequests() { nit: CompleteTwoRequests, maybe? They're not exactly both pipelined in some of the tests that use this.
Don't worry. I wasn't expecting a response after TGIF on Friday. :) I just wanted to get it done before I left. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:44: class DummyHttpServerProperties : public HttpServerProperties { On 2011/11/07 16:53:46, Matt Menke wrote: > I don't think this class is necessary. We should just be able to use > HttpServerPropertiesImpl. Done. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:71: class DummyHostResolverProc : public HostResolverProc { On 2011/11/07 16:53:46, Matt Menke wrote: > I don't think this class is needed, either. We should be able to just use > MockHostResolver. Done. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:96: void SetUp() { On 2011/11/07 16:53:46, Matt Menke wrote: > virtual/OVERRIDE for this, and the next one, too. Done. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:104: void Initialize() { On 2011/11/07 16:53:46, Matt Menke wrote: > Any reason not to do this in SetUp? It's called once per run test (SetUp -> > TestBody - > TearDown). Could even do it in the constructor, since each test is > actually its own subclass of this test class. It's the ReusesOnSpaceAvailable test that screws it up. It needs to set the max_sockets_per_group global before the session is created in Initialize(). I didn't want to mock 18 requests in order to fill all the pipelines, so I clamped the number of sockets so I'd only have to mock 3. Any other ideas? http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:108: session_params.host_resolver = new HostResolverImpl( On 2011/11/07 16:53:46, Matt Menke wrote: > None of these will be owned by the HttpNetworkSession, so they're all currently > leaked. Bah. I even ran valgrind ahead of time, but leak checking doesn't error by default, so I missed them. :( Fixed. http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:148: void PipelineTwoRequests() { On 2011/11/07 16:53:46, Matt Menke wrote: > nit: CompleteTwoRequests, maybe? They're not exactly both pipelined in some of > the tests that use this. Done.
As usual, I'm falling behind on reviews. I'll check back later when I catch up, but don't wait for me. I trust Matt on this. On 2011/11/07 20:30:29, James Simonsen wrote: > Don't worry. I wasn't expecting a response after TGIF on Friday. :) I just > wanted to get it done before I left. > > http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... > File net/http/http_pipelined_network_transaction_unittest.cc (right): > > http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... > net/http/http_pipelined_network_transaction_unittest.cc:44: class > DummyHttpServerProperties : public HttpServerProperties { > On 2011/11/07 16:53:46, Matt Menke wrote: > > I don't think this class is necessary. We should just be able to use > > HttpServerPropertiesImpl. > > Done. > > http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... > net/http/http_pipelined_network_transaction_unittest.cc:71: class > DummyHostResolverProc : public HostResolverProc { > On 2011/11/07 16:53:46, Matt Menke wrote: > > I don't think this class is needed, either. We should be able to just use > > MockHostResolver. > > Done. > > http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... > net/http/http_pipelined_network_transaction_unittest.cc:96: void SetUp() { > On 2011/11/07 16:53:46, Matt Menke wrote: > > virtual/OVERRIDE for this, and the next one, too. > > Done. > > http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... > net/http/http_pipelined_network_transaction_unittest.cc:104: void Initialize() { > On 2011/11/07 16:53:46, Matt Menke wrote: > > Any reason not to do this in SetUp? It's called once per run test (SetUp -> > > TestBody - > TearDown). Could even do it in the constructor, since each test > is > > actually its own subclass of this test class. > > It's the ReusesOnSpaceAvailable test that screws it up. It needs to set the > max_sockets_per_group global before the session is created in Initialize(). > > I didn't want to mock 18 requests in order to fill all the pipelines, so I > clamped the number of sockets so I'd only have to mock 3. > > Any other ideas? > > http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... > net/http/http_pipelined_network_transaction_unittest.cc:108: > session_params.host_resolver = new HostResolverImpl( > On 2011/11/07 16:53:46, Matt Menke wrote: > > None of these will be owned by the HttpNetworkSession, so they're all > currently > > leaked. > > Bah. I even ran valgrind ahead of time, but leak checking doesn't error by > default, so I missed them. :( > > Fixed. > > http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... > net/http/http_pipelined_network_transaction_unittest.cc:148: void > PipelineTwoRequests() { > On 2011/11/07 16:53:46, Matt Menke wrote: > > nit: CompleteTwoRequests, maybe? They're not exactly both pipelined in some > of > > the tests that use this. > > Done.
http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_netw... net/http/http_pipelined_network_transaction_unittest.cc:104: void Initialize() { On 2011/11/07 20:30:29, James Simonsen wrote: > On 2011/11/07 16:53:46, Matt Menke wrote: > > Any reason not to do this in SetUp? It's called once per run test (SetUp -> > > TestBody - > TearDown). Could even do it in the constructor, since each test > is > > actually its own subclass of this test class. > > It's the ReusesOnSpaceAvailable test that screws it up. It needs to set the > max_sockets_per_group global before the session is created in Initialize(). > > I didn't want to mock 18 requests in order to fill all the pipelines, so I > clamped the number of sockets so I'd only have to mock 3. > > Any other ideas? You could just have a function to create the session, and create it again, deleting the old one, for those tests. Or set it to 3 for all tests. I'm fine with it as-is, though, since there's a good reason for it. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:35: class DummySocketParams : public base::RefCounted<DummySocketParams> { Come to think of it, this isn't needed, either. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:51: HttpStreamFactory::set_http_pipelining_enabled(true); This should be set to false again (Or restored to its original value) in TearDown() to avoid affecting other tests, since all tests run in the same process. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:60: mock_resolver_.reset(new MockHostResolver); nit: You don't need to use a scoped_ptr for either this or HttpServerPropertiesImpl. Not a huge simplification, I suppose. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:61: mock_resolver_->rules()->AddRule("localhost", "127.0.0.1"); nit: Not needed. By default, the MockHostResolver maps everything to 127.0.0.1. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:69: session_params.ssl_config_service = new SSLConfigServiceDefaults; |ssl_config_| should be a scoped_refptr, and you should have it refer to your shiny new SSLConfigServiceDefaults. While HttpNetworkSession will actually maintain a reference to it, that behavior isn't documented, so I think it's best to do what the other tests do, and not to rely on it. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:136: SSLConfig ssl_config_; Once you use this (As per comment above), you should put this above |session_|, so it gets destroyed after the |session_|. With refptrs, not strictly necessary, but on the off chance that ever changes, just seems safer. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:137: ProxyInfo proxy_info_; This is not used. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:172: MockWrite(false, 4, "GET /two.html HTTP/1.1\r\n" I think it's a little odd that we always send the second "pipelined" request after reading the results from earlier requests. Not sure if there's an easy fix for this.
I just added some more comments. Didn't get an automatic email from them yet, and it's normally pretty much instantaneous, so sending this, just in case. On Mon, Nov 7, 2011 at 3:39 PM, <willchan@chromium.org> wrote: > As usual, I'm falling behind on reviews. I'll check back later when I > catch up, > but don't wait for me. I trust Matt on this. > > > On 2011/11/07 20:30:29, James Simonsen wrote: > >> Don't worry. I wasn't expecting a response after TGIF on Friday. :) I just >> wanted to get it done before I left. >> > > > http://codereview.chromium.**org/8457002/diff/4001/net/** > http/http_pipelined_network_**transaction_unittest.cc<http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_network_transaction_unittest.cc> > >> File net/http/http_pipelined_**network_transaction_unittest.**cc (right): >> > > > http://codereview.chromium.**org/8457002/diff/4001/net/** > http/http_pipelined_network_**transaction_unittest.cc#**newcode44<http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_network_transaction_unittest.cc#newcode44> > >> net/http/http_pipelined_**network_transaction_unittest.**cc:44: class >> DummyHttpServerProperties : public HttpServerProperties { >> On 2011/11/07 16:53:46, Matt Menke wrote: >> > I don't think this class is necessary. We should just be able to use >> > HttpServerPropertiesImpl. >> > > Done. >> > > > http://codereview.chromium.**org/8457002/diff/4001/net/** > http/http_pipelined_network_**transaction_unittest.cc#**newcode71<http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_network_transaction_unittest.cc#newcode71> > >> net/http/http_pipelined_**network_transaction_unittest.**cc:71: class >> DummyHostResolverProc : public HostResolverProc { >> On 2011/11/07 16:53:46, Matt Menke wrote: >> > I don't think this class is needed, either. We should be able to just >> use >> > MockHostResolver. >> > > Done. >> > > > http://codereview.chromium.**org/8457002/diff/4001/net/** > http/http_pipelined_network_**transaction_unittest.cc#**newcode96<http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_network_transaction_unittest.cc#newcode96> > >> net/http/http_pipelined_**network_transaction_unittest.**cc:96: void >> SetUp() { >> On 2011/11/07 16:53:46, Matt Menke wrote: >> > virtual/OVERRIDE for this, and the next one, too. >> > > Done. >> > > > http://codereview.chromium.**org/8457002/diff/4001/net/** > http/http_pipelined_network_**transaction_unittest.cc#**newcode104<http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_network_transaction_unittest.cc#newcode104> > >> net/http/http_pipelined_**network_transaction_unittest.**cc:104: void >> Initialize() >> > { > >> On 2011/11/07 16:53:46, Matt Menke wrote: >> > Any reason not to do this in SetUp? It's called once per run test >> (SetUp -> >> > TestBody - > TearDown). Could even do it in the constructor, since each >> > test > >> is >> > actually its own subclass of this test class. >> > > It's the ReusesOnSpaceAvailable test that screws it up. It needs to set >> the >> max_sockets_per_group global before the session is created in >> Initialize(). >> > > I didn't want to mock 18 requests in order to fill all the pipelines, so I >> clamped the number of sockets so I'd only have to mock 3. >> > > Any other ideas? >> > > > http://codereview.chromium.**org/8457002/diff/4001/net/** > http/http_pipelined_network_**transaction_unittest.cc#**newcode108<http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_network_transaction_unittest.cc#newcode108> > >> net/http/http_pipelined_**network_transaction_unittest.**cc:108: >> session_params.host_resolver = new HostResolverImpl( >> On 2011/11/07 16:53:46, Matt Menke wrote: >> > None of these will be owned by the HttpNetworkSession, so they're all >> currently >> > leaked. >> > > Bah. I even ran valgrind ahead of time, but leak checking doesn't error by >> default, so I missed them. :( >> > > Fixed. >> > > > http://codereview.chromium.**org/8457002/diff/4001/net/** > http/http_pipelined_network_**transaction_unittest.cc#**newcode148<http://codereview.chromium.org/8457002/diff/4001/net/http/http_pipelined_network_transaction_unittest.cc#newcode148> > >> net/http/http_pipelined_**network_transaction_unittest.**cc:148: void >> PipelineTwoRequests() { >> On 2011/11/07 16:53:46, Matt Menke wrote: >> > nit: CompleteTwoRequests, maybe? They're not exactly both pipelined in >> > some > >> of >> > the tests that use this. >> > > Done. >> > > > > http://codereview.chromium.**org/8457002/<http://codereview.chromium.org/8457... >
Matt seems to be doing a great job, and I don't have anything detailed to say. I'm going to let Matt finish the detailed review. At a high level, I'm curious if you plan to add tests (or maybe I missed them) for breadth and depth of pipelines. I know you aren't doing anything smart now, but I'd like to get the dumb behavior enforced by tests, so when you make tweaks to it, we can verify how the behavior changes in the tests. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:34: If nothing needs to be friended or otherwise externally referenced, can you put all these tests within the net::(anonymous) namespace?
ReusesOnSpaceAvailable tests depth. Breadth is ugly to test because it requires mocking so many transactions. I tried to hack it up before leaving tonight, but it was a mess. I'll try to come up with a better way, but it's still not a very exciting test. I think we might be better off just directly unit testing whatever clever logic we end up with, rather than an 18 transaction behemoth of an integration test. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:34: On 2011/11/08 22:44:54, willchan wrote: > If nothing needs to be friended or otherwise externally referenced, can you put > all these tests within the net::(anonymous) namespace? Done. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:35: class DummySocketParams : public base::RefCounted<DummySocketParams> { On 2011/11/08 18:12:41, Matt Menke wrote: > Come to think of it, this isn't needed, either. Done. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:51: HttpStreamFactory::set_http_pipelining_enabled(true); On 2011/11/08 18:12:41, Matt Menke wrote: > This should be set to false again (Or restored to its original value) in > TearDown() to avoid affecting other tests, since all tests run in the same > process. Done. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:60: mock_resolver_.reset(new MockHostResolver); On 2011/11/08 18:12:41, Matt Menke wrote: > nit: You don't need to use a scoped_ptr for either this or > HttpServerPropertiesImpl. Not a huge simplification, I suppose. I may be misunderstanding you... If I remove the scoped_ptrs and just pass the raw pointer into session_params and forget about it, then valgrind complains. I'd prefer to use the scoped_ptr than manually delete. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:61: mock_resolver_->rules()->AddRule("localhost", "127.0.0.1"); On 2011/11/08 18:12:41, Matt Menke wrote: > nit: Not needed. By default, the MockHostResolver maps everything to > 127.0.0.1. Done. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:69: session_params.ssl_config_service = new SSLConfigServiceDefaults; On 2011/11/08 18:12:41, Matt Menke wrote: > |ssl_config_| should be a scoped_refptr, and you should have it refer to your > shiny new SSLConfigServiceDefaults. While HttpNetworkSession will actually > maintain a reference to it, that behavior isn't documented, so I think it's best > to do what the other tests do, and not to rely on it. Done. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:136: SSLConfig ssl_config_; On 2011/11/08 18:12:41, Matt Menke wrote: > Once you use this (As per comment above), you should put this above |session_|, > so it gets destroyed after the |session_|. With refptrs, not strictly > necessary, but on the off chance that ever changes, just seems safer. Done. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:137: ProxyInfo proxy_info_; On 2011/11/08 18:12:41, Matt Menke wrote: > This is not used. Done. http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:172: MockWrite(false, 4, "GET /two.html HTTP/1.1\r\n" On 2011/11/08 18:12:41, Matt Menke wrote: > I think it's a little odd that we always send the second "pipelined" request > after reading the results from earlier requests. Not sure if there's an easy > fix for this. Yeah, it makes the code uglier. But you're right, we really need to test the pipeline aspect of it, so I changed the tests that use CompleteTwoRequests().
http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/10001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:60: mock_resolver_.reset(new MockHostResolver); On 2011/11/09 05:24:41, James Simonsen wrote: > On 2011/11/08 18:12:41, Matt Menke wrote: > > nit: You don't need to use a scoped_ptr for either this or > > HttpServerPropertiesImpl. Not a huge simplification, I suppose. > > I may be misunderstanding you... If I remove the scoped_ptrs and just pass the > raw pointer into session_params and forget about it, then valgrind complains. > I'd prefer to use the scoped_ptr than manually delete. HttpPipelinedNetworkTransactionTest doesn't need pointers for them at all. You can make them non-pointer class members, since they don't need to be created by funky static constructors. Doesn't really matter. http://codereview.chromium.org/8457002/diff/15001/net/http/http_pipelined_net... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/15001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:33: namespace net { nit: It's more common in Chrome code to have a linebreak here, and between their close braces as well. No idea if this is official style, or just common convention. http://codereview.chromium.org/8457002/diff/15001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:125: one_transaction.reset(); Why is this needed now, but wasn't before?
Oh, duh. Yeah, I should've understood what you meant about just using pointers to members. http://codereview.chromium.org/8457002/diff/15001/net/http/http_pipelined_net... File net/http/http_pipelined_network_transaction_unittest.cc (right): http://codereview.chromium.org/8457002/diff/15001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:33: namespace net { On 2011/11/09 16:10:58, Matt Menke wrote: > nit: It's more common in Chrome code to have a linebreak here, and between > their close braces as well. No idea if this is official style, or just common > convention. Done. http://codereview.chromium.org/8457002/diff/15001/net/http/http_pipelined_net... net/http/http_pipelined_network_transaction_unittest.cc:125: one_transaction.reset(); On 2011/11/09 16:10:58, Matt Menke wrote: > Why is this needed now, but wasn't before? Good point. I forgot to copy the second Read() from ExpectResponse(), which closes the stream. This was just another way of closing the stream. I've switched it back to using 2 Read() calls to close the stream.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/8457002/17003
Try job failure for 8457002-17003 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
