Record Proxy Server scheme accurately

Record Proxy Server scheme accurately

The scheme of the proxy server is current read from
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading
the scheme from the proxy server set in the request (instead
of from the proxy_info).

BUG=654793
Committed: https://crrev.com/10939b0d35b8e8ac58010b67c860f2a5a53da5d3
Cr-Commit-Position: refs/heads/master@{#426346}

[tbansal1, 2016-10-18 21:09:34]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-10-18 21:09:43]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-10-18 21:11:33]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-10-18 21:13:08]

Description was changed from

==========
Record Proxy Server scheme accurately

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

to

==========
Record Proxy Server scheme accurately

The scheme of the proxy server is current read from 
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading 
the scheme from the proxy server set in the request.

BUG=654793
==========

[tbansal1, 2016-10-18 21:13:23]

Description was changed from

==========
Record Proxy Server scheme accurately

The scheme of the proxy server is current read from 
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading 
the scheme from the proxy server set in the request.

BUG=654793
==========

to

==========
Record Proxy Server scheme accurately

The scheme of the proxy server is current read from 
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading 
the scheme from the proxy server set in the request (instead 
of from the proxy_info).

BUG=654793
==========

[tbansal1, 2016-10-18 21:14:05]

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

[tbansal1, 2016-10-18 21:14:06]

ryansturm: ptal. thanks

[RyanSturm, 2016-10-18 21:44:57]

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc
(right):

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:153:
if (data_reduction_proxy_config_->WasDataReductionProxyUsed(request,
Can you flip the if and early return here instead of scoping the rest of the
method?

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:167:
// |proxy_info.proxy_servers|. However, currently for some of the unittests
Is it a lot of work to fix those unittests? It would be more straightforward
just to DCHECK this without the if and DCHECK request->proxy_server() isn't
direct or invalid. Then below it would just be request->proxy_server() instead
of proxy_server_used.

The ternary operator below seems really strange and it makes this code pretty
hard to understand.

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc
(right):

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:1100:
TEST_F(DataReductionProxyBypassStatsEndToEndTest, HttpProxyScheme) {
Is there a QUIC version of this test?

[tbansal1, 2016-10-19 05:22:47]

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

[commit-bot: I haz the power, 2016-10-19 05:23: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-19 08:32:39]

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

[commit-bot: I haz the power, 2016-10-19 09:31:00]

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-19 12:08:58]

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

[commit-bot: I haz the power, 2016-10-19 12:08:59]

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

[tbansal1, 2016-10-19 16:11:54]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2016-10-19 16:12:20]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2016-10-19 16:12:24]

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

[commit-bot: I haz the power, 2016-10-19 16:12:43]

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-19 16:53:25]

Patchset #2 (id:120001) has been deleted

[tbansal1, 2016-10-19 16:54:08]

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

[commit-bot: I haz the power, 2016-10-19 16:54:37]

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-19 17:50:26]

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

[commit-bot: I haz the power, 2016-10-19 17:50:28]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-19 18:15:26]

Patchset #3 (id:120002) has been deleted

[tbansal1, 2016-10-19 18:15:57]

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

[commit-bot: I haz the power, 2016-10-19 18:16:29]

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-19 18:17:39]

ryansturm: ptal. I have changed the tests in
data_reduction_proxy_bypass_stats_unittest.cc, so now they do not use mocking
test framework.

There is still one test left in data_reduction_proxy_bypass_stats_unittest.cc
that uses mocking which I think can be removed because the relevant code is no
longer active (field trial was turned down).

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc
(right):

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:153:
if (data_reduction_proxy_config_->WasDataReductionProxyUsed(request,
On 2016/10/18 21:44:57, Ryan Sturm wrote:
> Can you flip the if and early return here instead of scoping the rest of the
> method?

Done.

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:167:
// |proxy_info.proxy_servers|. However, currently for some of the unittests
On 2016/10/18 21:44:57, Ryan Sturm wrote:
> Is it a lot of work to fix those unittests? It would be more straightforward
> just to DCHECK this without the if and DCHECK request->proxy_server() isn't
> direct or invalid. Then below it would just be request->proxy_server() instead
> of proxy_server_used.
Right, I did try to update tests but it seems like it is non trivial amount of
work.
> 
> The ternary operator below seems really strange and it makes this code pretty
> hard to understand.
Rewritten.

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc
(right):

https://codereview.chromium.org/2426903003/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:1100:
TEST_F(DataReductionProxyBypassStatsEndToEndTest, HttpProxyScheme) {
On 2016/10/18 21:44:57, Ryan Sturm wrote:
> Is there a QUIC version of this test?

No. I do not see a easy way to write the QUIC test which is meaningful enough.
However, I added the HTTPS version of the test. Since the code only impacts
recording of histogram data, it may not be worth the effort.

[tbansal1, 2016-10-19 18:17:59]

Patchset #2 (id:140001) has been deleted

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

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

[commit-bot: I haz the power, 2016-10-19 19:18:07]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-10-19 23:47:24]

lgtm.

[tbansal1, 2016-10-19 23:52:14]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-10-19 23:52:44]

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-20 00:12:08]

Description was changed from

==========
Record Proxy Server scheme accurately

The scheme of the proxy server is current read from 
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading 
the scheme from the proxy server set in the request (instead 
of from the proxy_info).

BUG=654793
==========

to

==========
Record Proxy Server scheme accurately

The scheme of the proxy server is current read from 
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading 
the scheme from the proxy server set in the request (instead 
of from the proxy_info).

BUG=654793
==========

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

Committed patchset #2 (id:170001)

[commit-bot: I haz the power, 2016-10-21 13:13:36]

Description was changed from

==========
Record Proxy Server scheme accurately

The scheme of the proxy server is current read from 
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading 
the scheme from the proxy server set in the request (instead 
of from the proxy_info).

BUG=654793
==========

to

==========
Record Proxy Server scheme accurately

The scheme of the proxy server is current read from
proxy_info which may not match the scheme of the proxy
server actually used (e.g., when there is a race between
HTTP2 and QUIC connection). This CL fixes the bug by reading
the scheme from the proxy server set in the request (instead
of from the proxy_info).

BUG=654793

Committed: https://crrev.com/10939b0d35b8e8ac58010b67c860f2a5a53da5d3
Cr-Commit-Position: refs/heads/master@{#426346}
==========

[commit-bot: I haz the power, 2016-10-21 13:13:38]

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