Store net::ProxyServer in HttpResponseInfo object

Store net::ProxyServer in HttpResponseInfo object.

Store net::ProxyServer in HttpResponseInfo instead of
net::HostPortPair. This makes it possible for the net
consumers to determine the proxy scheme used for a given
request (e.g., QUIC vs. HTTPS)

BUG=646197
Committed: https://crrev.com/2ecbbc7334d7debb8ae7c0b4504920c3f222d2b6
Cr-Commit-Position: refs/heads/master@{#423567}

[tbansal1, 2016-09-12 19:13:21]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 19:13:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 19:28:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 19:28:27]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tbansal1, 2016-09-12 20:38:51]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 20:39:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-09-12 20:49:52]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-09-12 21:07:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 21:07:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tbansal1, 2016-09-12 21:28:06]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-09-12 21:28:18]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-12 21:28:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 21:31:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 21:31:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2016-09-12 22:18:51]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-09-12 22:19:25]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-12 22:19:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 23:00:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 23:00:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tbansal1, 2016-09-12 23:31:14]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2016-09-12 23:32:28]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 23:33:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-09-12 23:33:23]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2016-09-12 23:44:41]

Patchset #1 (id:100001) has been deleted

[tbansal1, 2016-09-12 23:45:09]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 23:45:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-09-13 00:11:38]

Description was changed from

==========
PS


BUG=
==========

to

==========
PS


BUG=646197
==========

[commit-bot: I haz the power, 2016-09-13 01:07:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 01:07:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tbansal1, 2016-09-13 01:10:35]

Description was changed from

==========
PS


BUG=646197
==========

to

==========
Store net::ProxyServer in HttpResponseInfo object.

Store net::ProxyServer in HttpResponseInfo instead of
net::HostPortPair. This makes it possible for the net 
consumers to determine the proxy scheme used for a given 
request (e.g., QUIC vs. HTTPS)

BUG=646197
==========

[tbansal1, 2016-09-13 01:12:52]

tbansal@chromium.org changed reviewers:
+ kundaji@chromium.org, ryansturm@google.com

[tbansal1, 2016-09-13 01:12:53]

ryansturm, kundaji: ptal at *. Thanks!!

[tbansal1, 2016-09-13 01:17:14]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org
- ryansturm@google.com

[RyanSturm, 2016-09-13 23:46:33]

lgtm

[tbansal1, 2016-09-14 15:37:47]

tbansal@chromium.org changed reviewers:
+ eroman@chromium.org

[tbansal1, 2016-09-14 15:37:48]

kundaji: ptal at d_r_p or everything.
eroman:ptat at //net.
Thanks.

[Not at Google. Contact bengr, 2016-09-14 21:54:09]

Looks good overall. 

Only concern is the use of the defensive checks for is_valid and is_direct.
Would suggest using them only if they are really required. Change them to
DCHECKs if possible and correctness of the method depends on it. We should avoid
having the same if checks repeatedly in the same code path.

https://codereview.chromium.org/2334623003/diff/120001/components/cronet/andr...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/2334623003/diff/120001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:207: :
request->response_info().proxy_server.host_port_pair();
Is this test required? If proxy server is invalid or direct, isnt its
HostPortPair the default "net::HostPortPair()" anyways?

Same for other occurances in this file.

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc
(right):

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:98:
request->proxy_server().host_port_pair().IsEmpty()) {
Why does this test need to change? Shouldn't the last part be sufficient?

Same for other files.

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:347:
if (!proxy_server.is_valid() || proxy_server.is_direct())
Why is this test required now, when it wasn't before? Should they be DCHECKs
instead?

[tbansal1, 2016-09-14 22:04:17]

https://codereview.chromium.org/2334623003/diff/120001/components/cronet/andr...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/2334623003/diff/120001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:207: :
request->response_info().proxy_server.host_port_pair();
On 2016/09/14 21:54:09, kundaji wrote:
> Is this test required? If proxy server is invalid or direct, isnt its
> HostPortPair the default "net::HostPortPair()" anyways?
No. In fact, there is a DCHECK in ProxyServer::host_port_pair() method:
https://cs.chromium.org/chromium/src/net/proxy/proxy_server.cc?rcl=1473866283...
Adding the check here avoids tripping on those DCHECKs.
> 
> Same for other occurances in this file.
I can remove the checks here but then we are making an assumption that
ResponseInfo will contain a ProxyServer only when it is a valid non-direct Proxy
Server. I am not sure if we should be making that assumption (it holds
currently, but may not in future).

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc
(right):

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:98:
request->proxy_server().host_port_pair().IsEmpty()) {
On 2016/09/14 21:54:09, kundaji wrote:
> Why does this test need to change? Shouldn't the last part be sufficient?
> 
> Same for other files.

I did not want to make an assumption that request->proxy_server() is set only
when it is a valid non-direct proxy.

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/2334623003/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:347:
if (!proxy_server.is_valid() || proxy_server.is_direct())
On 2016/09/14 21:54:09, kundaji wrote:
> Why is this test required now, when it wasn't before? Should they be DCHECKs
> instead?

There are DCHECKS in net::PrxoyServer::host_port_pair() method that we want to
avoid tripping.
Conceptually, IsDataReductionProxy() takes any ProxyServer and returns
true/false based on if it is a DRP proxy or not. I do not think we should add
DCHECK to check if the passed proxy server is a valid non-direct proxy server.

[Not at Google. Contact bengr, 2016-09-14 22:25:41]

lgtm

[tbansal1, 2016-09-14 23:47:43]

Patchset #3 (id:160001) has been deleted

[tbansal1, 2016-09-14 23:47:52]

Patchset #2 (id:140001) has been deleted

[tbansal1, 2016-09-15 00:06:15]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-09-15 00:06:29]

Patchset #2 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-09-15 00:07:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-09-15 00:19:40]

eroman: ptal.  thanks.

[commit-bot: I haz the power, 2016-09-15 01:12:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 01:12:08]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-16 16:46:48]

tbansal@chromium.org changed reviewers:
+ mmenke@chromium.org

[tbansal1, 2016-09-16 16:46:49]

mmenke: ptal at components/cronet/* and content/* (//net optional).
Thanks.

[tbansal1, 2016-09-16 16:46:51]

mmenke: ptal at components/cronet/* and content/* (//net optional).
Thanks.

[eroman, 2016-09-16 22:52:43]

Some high-level comments before going into a detailed review:

  * Changing HttpResponseInfo::proxy_server to be of type ProxyServer sounds
good

  * Is HttpResponseInfo::was_fetched_via_proxy redundant with
HttpResponseInfo::proxy_server now (which also encodes DIRECT vs proxy server) ?

  * There is a lot of duplicated code for dealing with the fact that
ProxyServer::host_port_pair() requires is_valid() and !is_direct(). I will give
some more concrete guidance around this, when I respond in detail. But it is
certainly too complicated right now to have to check that everywhere. Maybe we
change this to return empty HostPortPair, or maybe we can get rid of the
is_invalid() state. Will check out what makes sense from other consumers...

Thanks!

[tbansal1, 2016-09-16 23:11:20]

On 2016/09/16 22:52:43, eroman wrote:
> Some high-level comments before going into a detailed review:
> 
>   * Changing HttpResponseInfo::proxy_server to be of type ProxyServer sounds
> good
> 
>   * Is HttpResponseInfo::was_fetched_via_proxy redundant with
> HttpResponseInfo::proxy_server now (which also encodes DIRECT vs proxy server)
?
I think so. I can remove it in this CL, or the next one (preferred, since this
is already too long).
> 
>   * There is a lot of duplicated code for dealing with the fact that
> ProxyServer::host_port_pair() requires is_valid() and !is_direct(). I will
give
> some more concrete guidance around this, when I respond in detail. But it is
> certainly too complicated right now to have to check that everywhere. Maybe we
> change this to return empty HostPortPair, or maybe we can get rid of the
> is_invalid() state. Will check out what makes sense from other consumers...
It seems the simplest way is to return empty host port pair in that case.
Is there a possibility that just removing those DCHECKs from
host_port_pair(), and instead returning an empty host port pair actually breaks
any
of the consumers?
> 
> Thanks!

[eroman, 2016-09-16 23:27:38]

On Fri, Sep 16, 2016 at 4:11 PM, <tbansal@chromium.org> wrote:

> On 2016/09/16 22:52:43, eroman wrote:
> > Some high-level comments before going into a detailed review:
> >
> > * Changing HttpResponseInfo::proxy_server to be of type ProxyServer
> sounds
> > good
> >
> > * Is HttpResponseInfo::was_fetched_via_proxy redundant with
> > HttpResponseInfo::proxy_server now (which also encodes DIRECT vs proxy
> server)
> ?
> I think so. I can remove it in this CL, or the next one (preferred, since
> this
> is already too long).
> >
> > * There is a lot of duplicated code for dealing with the fact that
> > ProxyServer::host_port_pair() requires is_valid() and !is_direct(). I
> will
> give
> > some more concrete guidance around this, when I respond in detail. But
> it is
> > certainly too complicated right now to have to check that everywhere.
> Maybe we
> > change this to return empty HostPortPair, or maybe we can get rid of the
> > is_invalid() state. Will check out what makes sense from other
> consumers...
> It seems the simplest way is to return empty host port pair in that case.
> Is there a possibility that just removing those DCHECKs from
> host_port_pair(), and instead returning an empty host port pair actually
> breaks
> any
> of the consumers?
>

It shouldn't be possible to break existing consumers, since that codepath
is currently a DCHECK failure. But I would like to think about this before
we do that, will get back to you.

>
> > Thanks!
>
>
>
> https://codereview.chromium.org/2334623003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tbansal1, 2016-09-21 18:12:56]

On 2016/09/16 23:27:38, eroman wrote:
> On Fri, Sep 16, 2016 at 4:11 PM, <mailto:tbansal@chromium.org> wrote:
> 
> > On 2016/09/16 22:52:43, eroman wrote:
> > > Some high-level comments before going into a detailed review:
> > >
> > > * Changing HttpResponseInfo::proxy_server to be of type ProxyServer
> > sounds
> > > good
> > >
> > > * Is HttpResponseInfo::was_fetched_via_proxy redundant with
> > > HttpResponseInfo::proxy_server now (which also encodes DIRECT vs proxy
> > server)
> > ?
> > I think so. I can remove it in this CL, or the next one (preferred, since
> > this
> > is already too long).
> > >
> > > * There is a lot of duplicated code for dealing with the fact that
> > > ProxyServer::host_port_pair() requires is_valid() and !is_direct(). I
> > will
> > give
> > > some more concrete guidance around this, when I respond in detail. But
> > it is
> > > certainly too complicated right now to have to check that everywhere.
> > Maybe we
> > > change this to return empty HostPortPair, or maybe we can get rid of the
> > > is_invalid() state. Will check out what makes sense from other
> > consumers...
> > It seems the simplest way is to return empty host port pair in that case.
> > Is there a possibility that just removing those DCHECKs from
> > host_port_pair(), and instead returning an empty host port pair actually
> > breaks
> > any
> > of the consumers?
> >
> 
> It shouldn't be possible to break existing consumers, since that codepath
> is currently a DCHECK failure. But I would like to think about this before
> we do that, will get back to you.
So, currently in this CL there are more than 10 places where we check
if calling ProxyServer::host_port_pair() is safe or not. However, majority
of those calls are in components/data_reduction_proxy. Once this CL goes
through,
then it would be possible for d_r_p to use net::ProxyServer everywhere, and
majority of those
repeated checks will go away.

After, d_r_p is updated to use net::proxyserver everywhere, there would be 3
calls left:
2 in cronet, 1 in resourceloader. These 3 are slightly more difficult to
transition since
they interface with Java, and renderer (through IPC), respectively.

Calling same code 3 times is probably bad, but much better once d_r_p code is
fixed
(probably in next CL).
> 
> >
> > > Thanks!
> >
> >
> >
> > https://codereview.chromium.org/2334623003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tbansal1, 2016-09-23 17:37:11]

ping.

[tbansal1, 2016-10-03 05:44:48]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 05:44:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 06:39:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 06:39:10]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-03 16:26:05]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-10-03 16:26:21]

Patchset #3 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-10-03 16:26:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-10-03 16:29:33]

eroman: ping. Can you ptal. I need to land this CL so I can add UMA for when
Chrome uses QUIC vs. HTTP2 protocol to fetch resources from the data reduction
proxy. That UMA will help me analyze the correctness of the data reduction proxy
+ QUIC experiment.

If you are busy, can you please suggest somebody else from //net OWNERS. Thanks!

[commit-bot: I haz the power, 2016-10-03 17:17:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 17:17:07]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-10-05 01:45:12]

LGTM

(sorry, wasn't doing code reviews last week)

There are some followups I would like to explore but it shouldn't block you
landing this CL, thanks!

https://codereview.chromium.org/2334623003/diff/240001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/2334623003/diff/240001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:356:
auto proxy_it = std::find_if(
Can you just use std::find() now? 
ProxyServer has an operator== which should be appropriate here.

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_network_...
net/http/http_network_transaction.cc:462: response_.proxy_server =
ProxyServer::Direct();
Do we need an else that sets an invalid ProxyServer?
i.e.    ProxyServer()

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_response...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_response...
net/http/http_response_info.h:130: bool was_fetched_via_proxy;
Can you add a TODO() to remove this?

[tbansal1, 2016-10-06 01:28:41]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 01:29:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-10-06 01:42:46]

Patchset #4 (id:260001) has been deleted

[tbansal1, 2016-10-06 01:43:12]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 01:43:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-10-06 02:34:16]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 02:34:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-10-06 02:34:59]

Patchset #4 (id:280001) has been deleted

[commit-bot: I haz the power, 2016-10-06 03:27:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 03:27:11]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-06 14:35:09]

mmenke: ptal at components/cronet/.
Thanks.

https://codereview.chromium.org/2334623003/diff/240001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/2334623003/diff/240001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:356:
auto proxy_it = std::find_if(
On 2016/10/05 01:45:12, eroman wrote:
> Can you just use std::find() now? 
> ProxyServer has an operator== which should be appropriate here.

Done.

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_network_...
net/http/http_network_transaction.cc:462: response_.proxy_server =
ProxyServer::Direct();
On 2016/10/05 01:45:12, eroman wrote:
> Do we need an else that sets an invalid ProxyServer?
> i.e.    ProxyServer()

Done.

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_response...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/2334623003/diff/240001/net/http/http_response...
net/http/http_response_info.h:130: bool was_fetched_via_proxy;
On 2016/10/05 01:45:12, eroman wrote:
> Can you add a TODO() to remove this?

Done.

[mmenke, 2016-10-06 14:58:04]

Was waiting for eroman to sign off before reviewing, but then missed it when he
did.  :)

https://codereview.chromium.org/2334623003/diff/300001/components/cronet/andr...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/2334623003/diff/300001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:221: const
net::HostPortPair host_port_pair =
nit:  Prevailing style is not to use const except for class members and
compile-time constants.

https://codereview.chromium.org/2334623003/diff/300001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:221: const
net::HostPortPair host_port_pair =
"host_port_pair" is too generic.  proxy_host_port_pair?  Or even just proxy?

https://codereview.chromium.org/2334623003/diff/300001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:225: :
request->response_info().proxy_server.host_port_pair();
I think this would be a lot easier to read as:

net::HostPortPair ;;;/
if (/* using proxy checks here */) {
  // Set HostPortPair to something here.
}

https://codereview.chromium.org/2334623003/diff/300001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:279: :
request->response_info().proxy_server.host_port_pair();
Same comments as above...Or better, can we make a helper method, rather than
duplicating code?

// Could make this return a variety of things - a string, a
// JavaString, a HostPortPair.  Could also make it take
// response_info() or response_info().proxy_server - I'm fine
// with any of those options.
GetProxy(ResponseInfo) {
  // "Failure" case is generally first, which corresponds to no proxy here
  if (...)
    return HostPortPair();
  return response_info().proxy_server.host_port_pair();
}

[tbansal1, 2016-10-06 15:30:52]

Patchset #5 (id:320001) has been deleted

[tbansal1, 2016-10-06 15:32:59]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 15:33:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-10-06 15:36:26]

mmenke: ptal. thanks.

[tbansal1, 2016-10-06 15:38:03]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 15:38:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-10-06 15:38:45]

LGTM!

[tbansal1, 2016-10-06 15:50:13]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-10-06 15:50:22]

Patchset #6 (id:360001) has been deleted

[commit-bot: I haz the power, 2016-10-06 15:50:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-10-06 16:02:54]

Patchset #6 (id:370001) has been deleted

[tbansal1, 2016-10-06 16:10:06]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 16:10:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-10-06 16:57:54]

The CQ bit was unchecked by tbansal@chromium.org

[tbansal1, 2016-10-06 16:58:01]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-10-06 16:58:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, kundaji@chromium.org, eroman@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2334623003/#ps390001
(title: "Rebased, fix compile error")

[commit-bot: I haz the power, 2016-10-06 16:58:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 17:16:07]

Description was changed from

==========
Store net::ProxyServer in HttpResponseInfo object.

Store net::ProxyServer in HttpResponseInfo instead of
net::HostPortPair. This makes it possible for the net 
consumers to determine the proxy scheme used for a given 
request (e.g., QUIC vs. HTTPS)

BUG=646197
==========

to

==========
Store net::ProxyServer in HttpResponseInfo object.

Store net::ProxyServer in HttpResponseInfo instead of
net::HostPortPair. This makes it possible for the net 
consumers to determine the proxy scheme used for a given 
request (e.g., QUIC vs. HTTPS)

BUG=646197
==========

[commit-bot: I haz the power, 2016-10-06 17:16:09]

Committed patchset #6 (id:390001)

[commit-bot: I haz the power, 2016-10-06 17:18:33]

Description was changed from

==========
Store net::ProxyServer in HttpResponseInfo object.

Store net::ProxyServer in HttpResponseInfo instead of
net::HostPortPair. This makes it possible for the net 
consumers to determine the proxy scheme used for a given 
request (e.g., QUIC vs. HTTPS)

BUG=646197
==========

to

==========
Store net::ProxyServer in HttpResponseInfo object.

Store net::ProxyServer in HttpResponseInfo instead of
net::HostPortPair. This makes it possible for the net
consumers to determine the proxy scheme used for a given
request (e.g., QUIC vs. HTTPS)

BUG=646197

Committed: https://crrev.com/2ecbbc7334d7debb8ae7c0b4504920c3f222d2b6
Cr-Commit-Position: refs/heads/master@{#423567}
==========

[commit-bot: I haz the power, 2016-10-06 17:18:34]

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