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

Issue 3259006: Add support for speaking SPDY to an HTTPS proxy.... (Closed)

Created:
10 years, 3 months ago by Ryan Hamilton
Modified:
9 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Add support for speaking SPDY to an HTTPS proxy. Currently only http urls are supported. BUG=29625 TEST=HttpNetworkTransactionTest.HttpsProxySpdyGet Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58236

Patch Set 1 : '' #

Total comments: 12

Patch Set 2 : '' #

Total comments: 14

Patch Set 3 : '' #

Patch Set 4 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -20 lines) Patch
M net/http/http_network_transaction_unittest.cc View 1 2 3 1 chunk +59 lines, -0 lines 0 comments Download
M net/http/http_proxy_client_socket.h View 1 2 3 chunks +8 lines, -1 line 0 comments Download
M net/http/http_proxy_client_socket.cc View 1 2 3 chunks +9 lines, -2 lines 0 comments Download
M net/http/http_proxy_client_socket_pool.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_proxy_client_socket_pool.cc View 1 2 4 chunks +9 lines, -2 lines 0 comments Download
M net/http/http_stream_request.cc View 1 2 4 chunks +38 lines, -5 lines 0 comments Download
M net/spdy/spdy_http_stream.h View 1 2 2 chunks +4 lines, -1 line 0 comments Download
M net/spdy/spdy_http_stream.cc View 1 2 5 chunks +10 lines, -5 lines 0 comments Download
M net/spdy/spdy_http_stream_unittest.cc View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M net/spdy/spdy_test_util.h View 2 3 1 chunk +12 lines, -0 lines 0 comments Download
M net/spdy/spdy_test_util.cc View 2 3 1 chunk +16 lines, -2 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
Ryan Hamilton
Hi Guys, Here's a patch which adds support for speaking SPDY to an HTTPS proxy, ...
10 years, 3 months ago (2010-08-30 20:47:41 UTC) #1
vandebo (ex-Chrome)
http://codereview.chromium.org/3259006/diff/11001/12006 File net/http/http_stream_request.cc (right): http://codereview.chromium.org/3259006/diff/11001/12006#newcode483 net/http/http_stream_request.cc:483: // Check first if we have a spdy session ...
10 years, 3 months ago (2010-08-30 22:09:30 UTC) #2
Ryan Hamilton
http://codereview.chromium.org/3259006/diff/11001/12006 File net/http/http_stream_request.cc (right): http://codereview.chromium.org/3259006/diff/11001/12006#newcode483 net/http/http_stream_request.cc:483: // Check first if we have a spdy session ...
10 years, 3 months ago (2010-08-30 22:43:21 UTC) #3
Mike Belshe
http://codereview.chromium.org/3259006/diff/11001/12002 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3259006/diff/11001/12002#newcode88 net/http/http_proxy_client_socket.cc:88: // TODO(rch): figure out the right way to set ...
10 years, 3 months ago (2010-08-30 23:41:08 UTC) #4
Ryan Hamilton
http://codereview.chromium.org/3259006/diff/11001/12002 File net/http/http_proxy_client_socket.cc (right): http://codereview.chromium.org/3259006/diff/11001/12002#newcode88 net/http/http_proxy_client_socket.cc:88: // TODO(rch): figure out the right way to set ...
10 years, 3 months ago (2010-08-30 23:50:36 UTC) #5
Mike Belshe
http://codereview.chromium.org/3259006/diff/37001/4008 File net/spdy/spdy_http_stream.cc (right): http://codereview.chromium.org/3259006/diff/37001/4008#newcode136 net/spdy/spdy_http_stream.cc:136: (*headers)["url"] = net::HttpUtil::PathForRequest(info.url); On 2010/08/30 23:50:37, rch wrote: > ...
10 years, 3 months ago (2010-08-31 19:10:15 UTC) #6
Ryan Hamilton
Hi Guys... can I go ahead and commit this? http://codereview.chromium.org/3259006/diff/37001/4008 File net/spdy/spdy_http_stream.cc (right): http://codereview.chromium.org/3259006/diff/37001/4008#newcode136 net/spdy/spdy_http_stream.cc:136: ...
10 years, 3 months ago (2010-09-01 15:47:15 UTC) #7
mbelshe
lgtm On Wed, Sep 1, 2010 at 8:47 AM, <rch@chromium.org> wrote: > Hi Guys... can ...
10 years, 3 months ago (2010-09-01 15:52:58 UTC) #8
mbelshe
lgtm On Wed, Sep 1, 2010 at 8:47 AM, <rch@chromium.org> wrote: > Hi Guys... can ...
10 years, 3 months ago (2010-09-01 15:55:46 UTC) #9
vandebo (ex-Chrome)
http://codereview.chromium.org/3259006/diff/37001/4007 File net/http/http_stream_request.cc (right): http://codereview.chromium.org/3259006/diff/37001/4007#newcode433 net/http/http_stream_request.cc:433: if (proxy_info()->is_https()) { Is is true that only one ...
10 years, 3 months ago (2010-09-01 16:50:08 UTC) #10
Ryan Hamilton
Just FYI, I've rebased this CL, so there are a number of diffs in the ...
10 years, 3 months ago (2010-09-01 17:15:16 UTC) #11
vandebo (ex-Chrome)
LGTM On 2010/09/01 17:15:16, rch wrote: > On 2010/09/01 16:50:08, vandebo wrote: > http://codereview.chromium.org/3259006/diff/37001/4012 > ...
10 years, 3 months ago (2010-09-01 17:36:05 UTC) #12
Ryan Hamilton
10 years, 3 months ago (2010-09-01 17:47:26 UTC) #13
On 2010/09/01 17:36:05, vandebo wrote:
> LGTM

Awesome, thanks!
 
> On 2010/09/01 17:15:16, rch wrote:
> > On 2010/09/01 16:50:08, vandebo wrote:
> > http://codereview.chromium.org/3259006/diff/37001/4012
> > File net/spdy/spdy_test_util.h (right):
> > 
> > http://codereview.chromium.org/3259006/diff/37001/4012#newcode197
> > net/spdy/spdy_test_util.h:197: spdy::SpdyFrame* ConstructSpdyGet(const char*
> > const extra_headers[],
> > On 2010/09/01 16:50:08, vandebo wrote:
> > > There are only a few calls to this function, it'd probably be better to
just
> > > amend all the callers instead of overloading this function. The style
guide
> > > isn't too strong about overloading, but I think the spirit of it is
against
> > this
> > > one.
> > 
> > No ternary, and no overloading?  Google C++ is clearly not Java :>
> > 
> > Actually, looking at other methods i spdy_test_util.h, I see overloading all
> > over the place.  The following methods are overloaded:
> > ChopWriteFrame
> > ChopReadFrame
> 
> At least in the these first couple cases, it's the same data in different
forms,
> which is allowed.

Oh, I see.  Good to know, thanks.

> > AppendToBuffer
> > ConstructSpdyControlFrame
> > ConstructSpdyGet (thought I think I may have created the overloaded method
in
> > the first place :>)
> > ConstructSpdyPush
> > ConstructSpdyBodyFrame
> > CreateMockWrite (3 versions)
> > CreateMockRead (3 versions)
> 
> Ick... We tend to be more lax for testing code.

*nod*

> > That being said, I'll change this if you'd like me to.  However, it looks
like
> > this method is called 56 times.  I'd rather not fix up all the call sites. 
> > Instead, I would be more inclined to pick a new name for the new method
(with
> an
> > extra arg).
> 
> I did a code search and only saw a few... I missed the *more than* 8 results
for
> one file :-).

Heh, yeah I searched in eclipse and only saw a few files.  But then I noticed 50
in spdy_network_transaction_unittests.cc.  Whoops! :>

>  It might be less bad to put the extra argument at the end, so
> it's easier to see?

Done!

Powered by Google App Engine
This is Rietveld 408576698