Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Issue 1146693003: Fix the use of SocketDataProviders in HttpNetworkTransactionTest.GenerateAuthToken (Closed)

Created:
5 years, 7 months ago by Ryan Hamilton
Modified:
5 years, 7 months ago
Reviewers:
cbentzel
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix the use of SocketDataProviders in HttpNetworkTransactionTest.GenerateAuthToken BUG=489701 Committed: https://crrev.com/cb68dc606b60d51794dec064a5405fd3e0705310 Cr-Commit-Position: refs/heads/master@{#330898}

Patch Set 1 #

Patch Set 2 : Fix comments #

Total comments: 12

Patch Set 3 : fix comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -15 lines) Patch
M net/http/http_network_transaction_unittest.cc View 1 2 1 chunk +29 lines, -15 lines 0 comments Download

Messages

Total messages: 13 (3 generated)
Ryan Hamilton
5 years, 7 months ago (2015-05-19 19:06:04 UTC) #2
Ryan Hamilton
cbentzel: ping
5 years, 7 months ago (2015-05-20 13:39:45 UTC) #3
cbentzel
On 2015/05/20 13:39:45, Ryan Hamilton wrote: > cbentzel: ping Will take a look now. May ...
5 years, 7 months ago (2015-05-20 21:40:34 UTC) #4
cbentzel
Thanks for working on this. Just trying to increase my understanding a bit, but this ...
5 years, 7 months ago (2015-05-21 00:10:26 UTC) #5
cbentzel
LGTM with request for comment shown below. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_transaction_unittest.cc File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_transaction_unittest.cc#newcode10146 net/http/http_network_transaction_unittest.cc:10146: if (read_write_round.read.data ...
5 years, 7 months ago (2015-05-21 00:22:04 UTC) #6
Ryan Hamilton
https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_transaction_unittest.cc File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_transaction_unittest.cc#oldcode10156 net/http/http_network_transaction_unittest.cc:10156: session_deps_.socket_factory->AddSocketDataProvider(&data_provider); On 2015/05/21 00:10:25, cbentzel wrote: > Wow, I'm ...
5 years, 7 months ago (2015-05-21 02:31:24 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146693003/40001
5 years, 7 months ago (2015-05-21 02:31:45 UTC) #10
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 7 months ago (2015-05-21 04:45:44 UTC) #11
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/cb68dc606b60d51794dec064a5405fd3e0705310 Cr-Commit-Position: refs/heads/master@{#330898}
5 years, 7 months ago (2015-05-21 04:47:15 UTC) #12
cbentzel
5 years, 7 months ago (2015-05-21 10:10:05 UTC) #13
Message was sent while issue was closed.
https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:10146: if
(read_write_round.read.data == kProxyChallenge.data &&
On 2015/05/21 02:31:24, Ryan Hamilton wrote:
> On 2015/05/21 00:10:25, cbentzel wrote:
> > This isn't immediately obvious to me what is going on here. Could you add a
> > comment for why this is being done? In the interim, I'll try to figure out
> why.
> 
> So kProxyChallenge has Proxy-Connection: close which results in the connection
> being closed. However, I don't understand why I need the second clause. Do you
> know? If the write is kConnect and the read is kProxyChallenge the connection
is
> not closed. Is this a bug?

I don't know why off the top of my head. This is worth investigating.

https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:10152: if
(read_write_round.extra_read) {
On 2015/05/21 02:31:24, Ryan Hamilton wrote:
> On 2015/05/21 00:22:04, cbentzel wrote:
> > It doesn't look like there is ever a case where a round includes both
> > kProxyChallenge and extra_read/extra_write, but I was wondering about
whether
> > this should go above the push_back statements (so it would apply to the
> current
> > socket, not the new socket).
> > 
> > Not a huge deal either way since it doesn't look like that combination
> happens,
> > but wondering if ytou had considered it.
> 
> I *think* it's correct as is, because if there was an extra read/write it
would
> happen after the Proxy-Connection: close response was processed which *should*
> mean the old socket was closed, right? Happy to revisit this if you think it
> best.

This seems reasonable.

Powered by Google App Engine
This is Rietveld 408576698