Remove the confusing request_url argument from the constructors

Remove the confusing request_url argument from the constructors
of HttpProxyClientSocket and SpdyProxyClientSocket. Instead,
contruct a "URL" from the endpoint used in the CONNECT request.

Committed: https://crrev.com/ecd3c554712ec96b5cfb3e43c714620b2bb74814
Cr-Commit-Position: refs/heads/master@{#324113}

[Ryan Hamilton, 2015-04-06 03:33:17]

rch@chromium.org changed reviewers:
+ rtenneti@chromium.org

[ramant (doing other things), 2015-04-06 05:27:53]

lgtm

https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.cc (right):

https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:62: request_.url = GURL("https://" +
endpoint.ToString());
nit: should we support http and do (is_https_proxy ? "https://" : "http://") ala
auth_?

I am not an expert was GET used because of http? Just curious thanks.

[ramant (doing other things), 2015-04-06 05:34:13]

https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.cc (right):

https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:62: request_.url = GURL("https://" +
endpoint.ToString());
On 2015/04/06 05:27:53, ramant wrote:
> nit: should we support http and do (is_https_proxy ? "https://" : "http://")
ala
> auth_?
> 
> I am not an expert was GET used because of http? Just curious thanks.

it looks like CONNECT could be used for port 80.

https://www.ietf.org/rfc/rfc2817.txt

[Ryan Hamilton, 2015-04-06 14:06:23]

https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.cc (right):

https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:62: request_.url = GURL("https://" +
endpoint.ToString());
On 2015/04/06 05:34:13, ramant wrote:
> On 2015/04/06 05:27:53, ramant wrote:
> > nit: should we support http and do (is_https_proxy ? "https://" : "http://")
> ala
> > auth_?
> > 
> > I am not an expert was GET used because of http? Just curious thanks.
> 
> it looks like CONNECT could be used for port 80.
> 
> https://www.ietf.org/rfc/rfc2817.txt

So this class in only used when making CONNECT tunnels through a proxy. This
happens when an https:// URL is requested over a proxy. First the browser says:

  CONNECT host:port HTTP/1.1
  ...

to the proxy. The proxy makes a TCP connection to host:port and returns:

  HTTP/1.1 200 OK
  ...

Then it simply passes bytes in both directions. At this point, the browser can
do a TLS handshake (or whatever).

The only purpose that request_.url is serving here is to be used to populate the
"host:port" field in the request. You're right that someone may request:
https://host:80/ explicitly which will result in a CONNECT request to port 80.
But I don't think that changes what we want this code to do. You'll notice that
as a result of this change, there are some place in unit tests where we
explicitly set :80 in the Host: header, but as you see from the RFC, this
matches the spec.

What do you think?

[ramant (doing other things), 2015-04-06 16:06:36]

On 2015/04/06 14:06:23, Ryan Hamilton wrote:
>
https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
> File net/http/http_proxy_client_socket.cc (right):
> 
>
https://codereview.chromium.org/1060883002/diff/20001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket.cc:62: request_.url = GURL("https://" +
> endpoint.ToString());
> On 2015/04/06 05:34:13, ramant wrote:
> > On 2015/04/06 05:27:53, ramant wrote:
> > > nit: should we support http and do (is_https_proxy ? "https://" :
"http://")
> > ala
> > > auth_?
> > > 
> > > I am not an expert was GET used because of http? Just curious thanks.
> > 
> > it looks like CONNECT could be used for port 80.
> > 
> > https://www.ietf.org/rfc/rfc2817.txt
> 
> So this class in only used when making CONNECT tunnels through a proxy. This
> happens when an https:// URL is requested over a proxy. First the browser
says:
> 
>   CONNECT host:port HTTP/1.1
>   ...
> 
> to the proxy. The proxy makes a TCP connection to host:port and returns:
> 
>   HTTP/1.1 200 OK
>   ...
> 
> Then it simply passes bytes in both directions. At this point, the browser can
> do a TLS handshake (or whatever).
> 
> The only purpose that request_.url is serving here is to be used to populate
the
> "host:port" field in the request. You're right that someone may request:
> https://host:80/ explicitly which will result in a CONNECT request to port 80.
> But I don't think that changes what we want this code to do. You'll notice
that
> as a result of this change, there are some place in unit tests where we
> explicitly set :80 in the Host: header, but as you see from the RFC, this
> matches the spec.
> 
> What do you think?

Still LGTM.

Thanks very much Ryan for the above explanation. The above sounds great.
-raman

[Ryan Hamilton, 2015-04-07 18:46:19]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2015-04-07 18:47:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060883002/20001

[commit-bot: I haz the power, 2015-04-07 20:54:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-07 20:55:02]

Patchset 2 (id:??) landed as
https://crrev.com/ecd3c554712ec96b5cfb3e43c714620b2bb74814
Cr-Commit-Position: refs/heads/master@{#324113}