Metrics fix for data reduction proxy.

Metrics fix for data reduction proxy.

Refactors histogram increments into virtual methods for mocking.

Adds unit tests that expect the correct metrics calls.

Fixes timing of RecordDataReductionInit() so that it happens when the
proxy is unavailable.


BUG=328579
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243201

[bengr, 2014-01-02 17:30:39]

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:98:
RecordDataReductionInit();
Did you move this here so we record something even if the data reduction proxy
is not supported on the platform? Does this mean that every platform (Windows,
etc) will have UMA for the data reduction proxy?

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:525: state =
IsDataReductionProxyEnabled() ? spdyproxy::PROXY_ENABLED
It's probably cleaner now to write this as:

if (IsDataReductionProxyExabled())
  state = spdyproxy::PROXY_ENABLED;
else
  state = spdyproxy::PROXY_DISABLED;

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:531:
ProbeURLFetchResult result) {
Can this fit on the line above?

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:40: // Values of
the UMA DataReductionProxy.StartupState histogram.
Must this enum stay in sync with something in histograms.xml? If so, add a
comment like the one for ProbeURLFetchResult.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:49: // This enum
must remain synchronized with DataReductionProxyProbeURLFetchResult
> 80 characters

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:51: // These names
are misleading -- for 'DISABLED', read 'RESTRICTED'.
Why aren't we just changing these to 'RESTRICTED' ?

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:70:
FETCH_RESULT_COUNT
Rename to PROBE_URL_FETCH_RESULT_COUNT?

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:88: 
Remove extra blank line.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:209: // Virtualzied
for mocking
spelling.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:59: :
spdyproxy::FAILED_PROXY_DISABLED;
I'd rewrite these with if/else instead of the ternary operator.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:165:
const std::string& test_url,
Can this be moved up a line?

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:212:
request_success && (response == "OK")),
maybe rename as 'request_succeeded'

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:659: //
We don't call |AddProxyToCommandLine()|, so the proxy feature
Don't use 'We'. Maybe: 

// The proxy feature should be unavailable, because |AddProxyToCommandLine()| is
not called.

[marq (ping after 24h), 2014-01-02 20:45:47]

Thanks for the review, PTAL.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:98:
RecordDataReductionInit();
On 2014/01/02 17:30:40, bengr1 wrote:
> Did you move this here so we record something even if the data reduction proxy
> is not supported on the platform? Does this mean that every platform (Windows,
> etc) will have UMA for the data reduction proxy?

No, because this code only exists for the mobile platforms because of how we
have it set up in .gyp, and because there's only code on those platforms to
create and init a DataReductionProxySettings object.

This change is the actual bug fix -- without it, we will never record the
PROXY_NOT_AVAILABLE startup state.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:525: state =
IsDataReductionProxyEnabled() ? spdyproxy::PROXY_ENABLED
On 2014/01/02 17:30:40, bengr1 wrote:
> It's probably cleaner now to write this as:
> 
> if (IsDataReductionProxyExabled())
>   state = spdyproxy::PROXY_ENABLED;
> else
>   state = spdyproxy::PROXY_DISABLED;

Done.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:531:
ProbeURLFetchResult result) {
On 2014/01/02 17:30:40, bengr1 wrote:
> Can this fit on the line above?

Not by nine characters.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:40: // Values of
the UMA DataReductionProxy.StartupState histogram.
On 2014/01/02 17:30:40, bengr1 wrote:
> Must this enum stay in sync with something in histograms.xml? If so, add a
> comment like the one for ProbeURLFetchResult.

Done.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:49: // This enum
must remain synchronized with DataReductionProxyProbeURLFetchResult
On 2014/01/02 17:30:40, bengr1 wrote:
> > 80 characters

Done.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:51: // These names
are misleading -- for 'DISABLED', read 'RESTRICTED'.
On 2014/01/02 17:30:40, bengr1 wrote:
> Why aren't we just changing these to 'RESTRICTED' ?

Mostly because I didn't want to propagate the rename to histograms.xml and break
any metrics queries that used them.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:70:
FETCH_RESULT_COUNT
On 2014/01/02 17:30:40, bengr1 wrote:
> Rename to PROBE_URL_FETCH_RESULT_COUNT?

Done.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:88: 
On 2014/01/02 17:30:40, bengr1 wrote:
> Remove extra blank line.

Done.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:209: // Virtualzied
for mocking
On 2014/01/02 17:30:40, bengr1 wrote:
> spelling.

Done.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:59: :
spdyproxy::FAILED_PROXY_DISABLED;
On 2014/01/02 17:30:40, bengr1 wrote:
> I'd rewrite these with if/else instead of the ternary operator.

Done.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:165:
const std::string& test_url,
On 2014/01/02 17:30:40, bengr1 wrote:
> Can this be moved up a line?

Done.

[bengr, 2014-01-06 16:51:23]

lgtm, with nit.

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:51: // These names
are misleading -- for 'DISABLED', read 'RESTRICTED'.
On 2014/01/02 20:45:47, marq_ wrote:
> On 2014/01/02 17:30:40, bengr1 wrote:
> > Why aren't we just changing these to 'RESTRICTED' ?
> 
> Mostly because I didn't want to propagate the rename to histograms.xml and
break
> any metrics queries that used them.

Please add a TODO to rename.

[marq (ping after 24h), 2014-01-06 17:50:58]

On 2014/01/06 16:51:23, bengr1 wrote:
> lgtm, with nit.
> 
>
https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
> File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):
> 
>
https://codereview.chromium.org/111113006/diff/1/chrome/browser/net/spdyproxy...
> chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:51: // These
names
> are misleading -- for 'DISABLED', read 'RESTRICTED'.
> On 2014/01/02 20:45:47, marq_ wrote:
> > On 2014/01/02 17:30:40, bengr1 wrote:
> > > Why aren't we just changing these to 'RESTRICTED' ?
> > 
> > Mostly because I didn't want to propagate the rename to histograms.xml and
> break
> > any metrics queries that used them.
> 
> Please add a TODO to rename.

Done.

[commit-bot: I haz the power, 2014-01-06 17:51:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marq@chromium.org/111113006/180001

[commit-bot: I haz the power, 2014-01-06 20:46:47]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-06 21:02:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marq@chromium.org/111113006/180001

[commit-bot: I haz the power, 2014-01-06 23:41:32]

Change committed as 243201