Treat QUIC proxy as a valid HTTP proxy.

Treat QUIC proxy as a valid HTTP proxy.

QUIC Proxy servers are valid HTTP proxies and should be treated
as such. This is required for using Chrome with a QUIC-based data
reduction proxy.

BUG=343579
Committed: https://crrev.com/7cec381bf3e65920fcb34a3d733dd1cdc0f99061
Cr-Commit-Position: refs/heads/master@{#314879}

[tbansal1, 2015-02-03 21:06:14]

tbansal@chromium.org changed reviewers:
+ bengr@chromium.org, rvargas@chromium.org

[rvargas (doing something else), 2015-02-03 22:06:01]

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

[rvargas (doing something else), 2015-02-03 22:06:02]

+rch

[Ryan Hamilton, 2015-02-03 22:31:35]

looks good. Can you add a test somewhere? We have tests in
QuicNetworkTransactionTest where we use a QUIC proxy, so perhaps you can hook
into those?

[bengr, 2015-02-03 23:03:08]

https://codereview.chromium.org/880483004/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/880483004/diff/1/net/http/http_network_transa...
net/http/http_network_transaction.cc:575: proxy_info_.is_quic()) &&
@rch: would proxy_info_.is_quic() return false if tunneled?

[Ryan Hamilton, 2015-02-03 23:06:50]

https://codereview.chromium.org/880483004/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/880483004/diff/1/net/http/http_network_transa...
net/http/http_network_transaction.cc:575: proxy_info_.is_quic()) &&
On 2015/02/03 23:03:08, bengr wrote:
> @rch: would proxy_info_.is_quic() return false if tunneled?

ProxyInfo doesn't know about any tunnel which might be used. It simply describes
the nature of the proxy (if any) that is being connected to. This method does
the "using a tunnel" detection.

(As far as I know)

[bengr, 2015-02-05 00:34:07]

https://codereview.chromium.org/880483004/diff/20001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/880483004/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:287: bool expect_headers_callback
= false) {
Don't use default values

https://codereview.chromium.org/880483004/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:1066: 
Remove the newline

[tbansal1, 2015-02-05 01:06:45]

https://codereview.chromium.org/880483004/diff/20001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/880483004/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:287: bool expect_headers_callback
= false) {
On 2015/02/05 00:34:07, bengr wrote:
> Don't use default values

Done.

https://codereview.chromium.org/880483004/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:1066: 
On 2015/02/05 00:34:07, bengr wrote:
> Remove the newline

Done.

[bengr, 2015-02-05 01:33:42]

lgtm

I fixed up the description. In the future, make sure the description has line
breaks.

[Ryan Hamilton, 2015-02-05 18:43:12]

Thanks for adding the test! Just a couple nits.

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:109: bool WasCalled() { return
was_called_; }
nit: according to the style guide, this should be was_called().

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:287: bool expect_headers_callback)
{
nit: I think this parameter should be named something like
"expect_proxy_headers" or even better "used_proxy".

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:438:
SendRequestAndExpectQuicResponse("hello!", true);
It's not obvious here what true and false refer to without going back to read
the method description. I recommend one of two approaches:
1) Create two variants of this method, like :
  SendRequestAndExpectQuicResponse("hello!")
  SendRequestAndExpectQuicResponseFromProxy("hello!")
which delegate to a private helper passing in false or true.
2) do something like:
  SendRequestAndExpectQuicResponse("hello!", /*used_proxy=*/true);
  SendRequestAndExpectQuicResponse("hello!", /*used_proxy=*/false);
or
  SendRequestAndExpectQuicResponse("hello!", kUsedProxy);
  SendRequestAndExpectQuicResponse("hello!", !kUsedProxy);

Personally, I'd probably go with #1

[tbansal1, 2015-02-05 20:09:22]

New patchsets have been uploaded after l-g-t-m from bengr@chromium.org

[tbansal1, 2015-02-05 20:11:55]

Thanks for the comments. All done.

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:109: bool WasCalled() { return
was_called_; }
On 2015/02/05 18:43:12, Ryan Hamilton wrote:
> nit: according to the style guide, this should be was_called().

Done.

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:287: bool expect_headers_callback)
{
On 2015/02/05 18:43:12, Ryan Hamilton wrote:
> nit: I think this parameter should be named something like
> "expect_proxy_headers" or even better "used_proxy".

Done.

https://codereview.chromium.org/880483004/diff/40001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:438:
SendRequestAndExpectQuicResponse("hello!", true);
On 2015/02/05 18:43:12, Ryan Hamilton wrote:
> It's not obvious here what true and false refer to without going back to read
> the method description. I recommend one of two approaches:
> 1) Create two variants of this method, like :
>   SendRequestAndExpectQuicResponse("hello!")
>   SendRequestAndExpectQuicResponseFromProxy("hello!")
> which delegate to a private helper passing in false or true.
> 2) do something like:
>   SendRequestAndExpectQuicResponse("hello!", /*used_proxy=*/true);
>   SendRequestAndExpectQuicResponse("hello!", /*used_proxy=*/false);
> or
>   SendRequestAndExpectQuicResponse("hello!", kUsedProxy);
>   SendRequestAndExpectQuicResponse("hello!", !kUsedProxy);
> 
> Personally, I'd probably go with #1

Done.

[Ryan Hamilton, 2015-02-05 20:15:30]

lgtm

[tbansal1, 2015-02-05 20:21:35]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2015-02-05 20:21:42]

The CQ bit was unchecked by tbansal@chromium.org

[tbansal1, 2015-02-05 20:21:51]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:22:21]

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

[commit-bot: I haz the power, 2015-02-05 21:25:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-05 21:26:29]

Patchset 4 (id:??) landed as
https://crrev.com/7cec381bf3e65920fcb34a3d733dd1cdc0f99061
Cr-Commit-Position: refs/heads/master@{#314879}