Adds UMA to measure when the data reduction proxy via header is missing

Adds UMA to measure when the data reduction proxy via header is missing

Keep track of the situations when Chrome expects the data reduction
proxy via header to be present in a response, but the data reduction
proxy via header is missing.

This change is the same as the reverted change
https://codereview.chromium.org/577343002/,
except that it includes a 1-line fix in
data_reduction_proxy_usage_stats_unittests.cc to make the tests pass
when run with Valgrind in release builds.

BUG=412888
Committed: https://crrev.com/f72ee64ee192ff8eab696bf68130f18c2837305b
Cr-Commit-Position: refs/heads/master@{#296496}

[sclittle, 2014-09-24 04:22:57]

sclittle@chromium.org changed reviewers:
+ asvitkine@chromium.org, bengr@chromium.org, mmenke@chromium.org

[sclittle, 2014-09-24 04:22:58]

https://codereview.chromium.org/577343002/ was reverted, this CL is the same
except for a 1-line change of a DCHECK to a CHECK in
data_reduction_proxy_usage_stats_unittest.cc to fix Valgrind in release builds.

bengr, mmenke, asvitkine: rubber stamp

Thanks in advance!

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:66:
CHECK(test_job_factory_.SetProtocolHandler(url::kHttpScheme,
This line (and the comment above it) are the only differences between this CL
and https://codereview.chromium.org/577343002/. Changing this from a DCHECK to a
CHECK fixed Valgrind in release builds.

[Alexei Svitkine (slow), 2014-09-24 14:52:43]

LGTM, but in the future when doing relands, it's best the upload the original CL
as patchset 1 and then the fix as a second patchset, so that it's easier to see
the diff.

[mmenke, 2014-09-24 15:31:12]

chrome/browser/net/chrome_network_delegate.cc still LGTM.

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:67:
test_job_interceptor_));
I think it's still a bad idea to put code with side effects in a CHECK statement
as well.  Maybe just use an ASSERT_TRUE, which would also abort the rest of the
test when called in the constructor?  Anyhow, just a suggestion, I'll defer to
you guys on this.

[sclittle, 2014-09-24 17:41:44]

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:67:
test_job_interceptor_));
On 2014/09/24 15:31:12, mmenke wrote:
> I think it's still a bad idea to put code with side effects in a CHECK
statement
> as well.  Maybe just use an ASSERT_TRUE, which would also abort the rest of
the
> test when called in the constructor?  Anyhow, just a suggestion, I'll defer to
> you guys on this.

Unfortunately, changing this to ASSERT_TRUE doesn't compile, since the macro
expansion of ASSERT_TRUE includes a return statement of a void expression, which
is not allowed in a constructor.

I've changed it to use an intermediate variable to hold the return value of
SetProtocolHandler, and DCHECK that it's true.

[bengr, 2014-09-24 17:46:47]

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:67:
test_job_interceptor_));
On 2014/09/24 17:41:44, sclittle wrote:
> On 2014/09/24 15:31:12, mmenke wrote:
> > I think it's still a bad idea to put code with side effects in a CHECK
> statement
> > as well.  Maybe just use an ASSERT_TRUE, which would also abort the rest of
> the
> > test when called in the constructor?  Anyhow, just a suggestion, I'll defer
to
> > you guys on this.
> 
> Unfortunately, changing this to ASSERT_TRUE doesn't compile, since the macro
> expansion of ASSERT_TRUE includes a return statement of a void expression,
which
> is not allowed in a constructor.
> 
> I've changed it to use an intermediate variable to hold the return value of
> SetProtocolHandler, and DCHECK that it's true.

You shouldn't be using DCHECK in tests. See:
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

[sclittle, 2014-09-24 18:03:43]

Patchset #3 (id:40001) has been deleted

[sclittle, 2014-09-24 18:05:14]

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/602503002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:67:
test_job_interceptor_));
On 2014/09/24 17:46:47, bengr1 wrote:
> On 2014/09/24 17:41:44, sclittle wrote:
> > On 2014/09/24 15:31:12, mmenke wrote:
> > > I think it's still a bad idea to put code with side effects in a CHECK
> > statement
> > > as well.  Maybe just use an ASSERT_TRUE, which would also abort the rest
of
> > the
> > > test when called in the constructor?  Anyhow, just a suggestion, I'll
defer
> to
> > > you guys on this.
> > 
> > Unfortunately, changing this to ASSERT_TRUE doesn't compile, since the macro
> > expansion of ASSERT_TRUE includes a return statement of a void expression,
> which
> > is not allowed in a constructor.
> > 
> > I've changed it to use an intermediate variable to hold the return value of
> > SetProtocolHandler, and DCHECK that it's true.
> 
> You shouldn't be using DCHECK in tests. See:
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

OK, I've changed it to use EXPECT_TRUE.

[bengr, 2014-09-24 19:03:03]

lgtm

[sclittle, 2014-09-24 19:03:27]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2014-09-24 19:04:10]

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

[commit-bot: I haz the power, 2014-09-24 20:04:35]

Committed patchset #3 (id:60001) as 3b2d599ed7386f09cb926ad5d4dff46dc38b4315

[commit-bot: I haz the power, 2014-09-24 20:05:16]

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