Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(60)

Issue 25312002: net: don't preserve 1xx responses in parser buffer. (Closed)

Created:
5 years, 8 months ago by agl
Modified:
5 years, 8 months ago
Reviewers:
SkyLined, wtc, inferno
CC:
chromium-reviews, cbentzel+watch_chromium.org
Visibility:
Public.

Description

net: don't preserve 1xx responses in parser buffer. It's unclear why 1xx responses were kept in the parser's buffer previously so this change aligns their processing with all other response types. BUG=299892 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226539

Patch Set 1 #

Total comments: 16

Patch Set 2 : ... #

Total comments: 4

Patch Set 3 : g try #

Total comments: 12

Patch Set 4 : Address wtc's comments. #

Total comments: 2

Patch Set 5 : .. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -51 lines) Patch
M net/http/http_proxy_client_socket_pool_unittest.cc View 1 2 3 1 chunk +31 lines, -0 lines 0 comments Download
M net/http/http_stream_parser.h View 1 chunk +2 lines, -1 line 0 comments Download
M net/http/http_stream_parser.cc View 1 2 3 4 10 chunks +59 lines, -50 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
agl
5 years, 8 months ago (2013-09-30 17:35:43 UTC) #1
SkyLined
LGTM, with a few remarks. https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/1/net/http/http_stream_parser.cc#newcode307 net/http/http_stream_parser.cc:307: DCHECK_EQ(0, read_buf_unused_offset_); Should we ...
5 years, 8 months ago (2013-09-30 18:27:47 UTC) #2
inferno
5 years, 8 months ago (2013-09-30 20:14:31 UTC) #3
agl
I've checked that tunnel establishment still fails with a 1xx response and have added a ...
5 years, 8 months ago (2013-09-30 20:59:27 UTC) #4
SkyLined
LGTM, with minor remarks https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parser.cc#newcode619 net/http/http_stream_parser.cc:619: } else { Nit: coding ...
5 years, 8 months ago (2013-09-30 22:02:55 UTC) #5
agl
https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/8001/net/http/http_stream_parser.cc#newcode619 net/http/http_stream_parser.cc:619: } else { On 2013/09/30 22:02:55, SkyLined wrote: > ...
5 years, 8 months ago (2013-10-01 14:07:45 UTC) #6
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
5 years, 8 months ago (2013-10-01 14:09:56 UTC) #7
wtc
Patch set 3 LGTM. I can't read the bug report, so I only read the ...
5 years, 8 months ago (2013-10-01 18:12:57 UTC) #8
wtc
Another nit: the CL's description says "don't preserve 1xx responses in parser buffer". Since 1xx ...
5 years, 8 months ago (2013-10-01 18:16:20 UTC) #9
agl
https://codereview.chromium.org/25312002/diff/13001/net/http/http_proxy_client_socket_pool_unittest.cc File net/http/http_proxy_client_socket_pool_unittest.cc (right): https://codereview.chromium.org/25312002/diff/13001/net/http/http_proxy_client_socket_pool_unittest.cc#newcode579 net/http/http_proxy_client_socket_pool_unittest.cc:579: "Proxy-Authorization: Basic Zm9vOmJhcg==\r\n\r\n"), On 2013/10/01 18:12:57, wtc wrote: > ...
5 years, 8 months ago (2013-10-01 19:53:18 UTC) #10
wtc
Patch set 4 LGTM. https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_parser.cc#newcode607 net/http/http_stream_parser.cc:607: // 1xx response because they ...
5 years, 8 months ago (2013-10-01 21:49:27 UTC) #11
agl
https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/25312002/diff/44001/net/http/http_stream_parser.cc#newcode607 net/http/http_stream_parser.cc:607: // 1xx response because they cannot be returned in ...
5 years, 8 months ago (2013-10-02 16:23:13 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/25312002/64001
5 years, 8 months ago (2013-10-02 16:23:26 UTC) #13
commit-bot: I haz the power
5 years, 8 months ago (2013-10-02 19:59:25 UTC) #14
Message was sent while issue was closed.
Change committed as 226539

Powered by Google App Engine
This is Rietveld 408576698