Fix recording of DataReductionProxy.ConfigService.HTTPRequests

Fix recording of DataReductionProxy.ConfigService.HTTPRequests

Do not record if:
(1) Chrome is in data reduction proxy holdback experiment.
(2) URL is a local host URL.

Also, record the sample as false only when the config is missing.

BUG=705167
Review-Url: https://codereview.chromium.org/2776583002
Cr-Commit-Position: refs/heads/master@{#460217}
Committed: https://chromium.googlesource.com/chromium/src/+/13390fea415c0b7ac32a5c8de7e370ba57a15440

[tbansal1, 2017-03-23 22:02:29]

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

[commit-bot: I haz the power, 2017-03-23 22:03:24]

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, 2017-03-23 22:52:50]

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

[commit-bot: I haz the power, 2017-03-23 22:52:51]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-25 00:24:17]

Description was changed from

==========
Fix recording of UMA histogram

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

to

==========
Fix recording of DataReductionProxy.ConfigService.HTTPRequests

Do not record if:
(1) Chrome is in data reduction proxy holdback experiment.
(2) URL is a local host URL.

Also, record the sample as false only when the config is missing.

BUG=705167
==========

[tbansal1, 2017-03-25 00:39:54]

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

[tbansal1, 2017-03-25 00:39:55]

sclittle: ptal. Thanks.

[sclittle, 2017-03-25 00:51:04]

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:309:
!params::IsIncludedInHoldbackFieldTrial() && url.SchemeIsHTTPOrHTTPS() &&
nit: this field trial check is more expensive than the later conditions, could
you put it at the end of the conditions?

Also, (url.SchemeIsHTTPOrHTTPS() && !url.SchemeIsCryptographic()) ===
(url.SchemeIs("http")). I'd recommend simplifying this, unless you think keeping
the separate conditions improves the readability significantly.

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:310:
!url.SchemeIsCryptographic() && !net::IsLocalhost(url.host()) &&
!net::IsLocalhost(url.host()) is already in the if statement, you don't need to
add a new one.

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:311:
!net::IsLocalhost(url.host())) {
nit: change this from |url.host()| to |url.host_piece()| to save a string
allocation.

[tbansal1, 2017-03-25 01:22:24]

ptal. Thanks.

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:309:
!params::IsIncludedInHoldbackFieldTrial() && url.SchemeIsHTTPOrHTTPS() &&
On 2017/03/25 00:51:04, sclittle wrote:
> nit: this field trial check is more expensive than the later conditions, could
> you put it at the end of the conditions?
> 
> Also, (url.SchemeIsHTTPOrHTTPS() && !url.SchemeIsCryptographic()) ===
> (url.SchemeIs("http")). I'd recommend simplifying this, unless you think
keeping
> the separate conditions improves the readability significantly.

Done.

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:310:
!url.SchemeIsCryptographic() && !net::IsLocalhost(url.host()) &&
On 2017/03/25 00:51:04, sclittle wrote:
> !net::IsLocalhost(url.host()) is already in the if statement, you don't need
to
> add a new one.

Done.

https://codereview.chromium.org/2776583002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:311:
!net::IsLocalhost(url.host())) {
On 2017/03/25 00:51:03, sclittle wrote:
> nit: change this from |url.host()| to |url.host_piece()| to save a string
> allocation.

Done.

[tbansal1, 2017-03-25 01:22:29]

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

[commit-bot: I haz the power, 2017-03-25 01:22:53]

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, 2017-03-25 02:09:02]

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

[commit-bot: I haz the power, 2017-03-25 02:09:03]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-28 18:23:05]

ping! Thanks.

[sclittle, 2017-03-28 18:32:58]

lgtm

[tbansal1, 2017-03-28 20:04:43]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2017-03-28 20:05:09]

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, 2017-03-28 21:10:04]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490731483901880,
"parent_rev": "4cbf5c1bc96f24e9e660e538cdce3d00562626f4", "commit_rev":
"13390fea415c0b7ac32a5c8de7e370ba57a15440"}

[commit-bot: I haz the power, 2017-03-28 21:11:34]

Description was changed from

==========
Fix recording of DataReductionProxy.ConfigService.HTTPRequests

Do not record if:
(1) Chrome is in data reduction proxy holdback experiment.
(2) URL is a local host URL.

Also, record the sample as false only when the config is missing.

BUG=705167
==========

to

==========
Fix recording of DataReductionProxy.ConfigService.HTTPRequests

Do not record if:
(1) Chrome is in data reduction proxy holdback experiment.
(2) URL is a local host URL.

Also, record the sample as false only when the config is missing.

BUG=705167

Review-Url: https://codereview.chromium.org/2776583002
Cr-Commit-Position: refs/heads/master@{#460217}
Committed:
https://chromium.googlesource.com/chromium/src/+/13390fea415c0b7ac32a5c8de7e3...
==========

[commit-bot: I haz the power, 2017-03-28 21:11:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/13390fea415c0b7ac32a5c8de7e3...