Issue 2101014: Eliminate the establishing_tunnel_ internal state and move to explicit...

Issue 2101014: Eliminate the establishing_tunnel_ internal state and move to explicit... (Closed)

Created:
10 years, 7 months ago by Mike Belshe

Modified:
9 years, 5 months ago

Reviewers:
vandebo (ex-Chrome), eroman

CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

Eliminate the establishing_tunnel_ internal state and move to explicit states in the state machine for proxy tunnel establishment. Original work from svandebo. BUG=none TEST=existing Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48173

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Total comments: 4

Patch Set 7 : '' #

Total comments: 5

Created: 10 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+227 lines, -133 lines)			Patch
M	chrome/app/generated_resources.grd	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/tab_contents/tab_contents.cc	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	net/base/load_states.h	View	1 2 3 4	1 chunk	+4 lines, -0 lines	0 comments	Download
M	net/base/net_log_event_type_list.h	View	3 4	2 chunks	+20 lines, -14 lines	0 comments	Download
M	net/http/http_network_transaction.h	View	1 2 3 4	5 chunks	+12 lines, -8 lines	0 comments	Download
M	net/http/http_network_transaction.cc	View	1 2 3 4 5 6	17 chunks	+186 lines, -111 lines	5 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

vandebo (ex-Chrome)

LGTM. The changes are just mostly just rebasing changes - you could have just asked ...

10 years, 7 months ago (2010-05-24 23:46:54 UTC) #2

vandebo (ex-Chrome)

http://codereview.chromium.org/2101014/diff/29001/30005 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/2101014/diff/29001/30005#newcode965 net/http/http_network_transaction.cc:965: // ------------------------------------------------------------------------- Don't need these delimiters in the committed ...

10 years, 7 months ago (2010-05-24 23:48:38 UTC) #3

Mike Belshe

10 years, 7 months ago (2010-05-24 23:54:15 UTC) #4

eroman

10 years, 7 months ago (2010-05-26 03:21:26 UTC) #5

http://codereview.chromium.org/2101014/diff/38001/15006
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2101014/diff/38001/15006#newcode483
net/http/http_network_transaction.cc:483: ssl_connect_start_time_.is_null())
Using the time variable feels a bit hockey.

http://codereview.chromium.org/2101014/diff/38001/15006#newcode521
net/http/http_network_transaction.cc:521: if (headers->response_code() == 407) {
Before it was only failing in the case where the URL was using SSL (i.e.
establishing tunnel), whereas now it always fails to read on 407.

At the very least the error code is wrong now (since this may not apply to the
tunnel case). Even if the error code were correct, I don't think we want this
new behavior, but should continue to show the error page for failed 407 when on
non-https.

http://codereview.chromium.org/2101014/diff/38001/15006#newcode632
net/http/http_network_transaction.cc:632: NULL);
nit: line these up with the paren of the previous line.

http://codereview.chromium.org/2101014/diff/38001/15006#newcode1012
net/http/http_network_transaction.cc:1012: return result;
No call to ClearTunnelState() in this case?

http://codereview.chromium.org/2101014/diff/38001/15006#newcode1028
net/http/http_network_transaction.cc:1028: ClearTunnelState();
The repeated calls to ClearTunnelState() seems pretty error prone. Especially if
someone else modifies this function in the future.

Expand Messages | Collapse Messages