Add HTTPS specific UMA for data reduction proxy requests

Add HTTPS specific UMA for data reduction proxy requests

BUG=374264
Committed: https://crrev.com/360767605cd4fe197f054bbd066dfefd33959807
Cr-Commit-Position: refs/heads/master@{#302701}

[jeremyim, 2014-10-30 21:59:33]

jeremyim@chromium.org changed reviewers:
+ tonyg@chromium.org

[tonyg, 2014-10-31 01:28:49]

tonyg@chromium.org changed reviewers:
+ ppi@chromium.org

[tonyg, 2014-10-31 01:29:03]

ppi@, would you mind taking this one?

[ppi, 2014-11-03 10:49:17]

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
chrome/renderer/page_load_histograms.cc:127: if ((scheme_type &
URLPattern::SCHEME_HTTPS) == 0) {              \
Afaiu |drp_counter| is a static variable, so we'll have only for multiple
subsequent pageloads in the renderer. What if scheme_type changes between loads?
Wouldn't we record the samples in the wrong histogram in this case?

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
chrome/renderer/page_load_histograms.cc:746:
data_reduction_proxy_params_.IsDataReductionProxy(
I might be missing something - where |data_reduction_proxy_params_| is set?

[jeremyim, 2014-11-03 18:40:40]

Patchset #2 (id:20001) has been deleted

[jeremyim, 2014-11-03 18:46:17]

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
chrome/renderer/page_load_histograms.cc:127: if ((scheme_type &
URLPattern::SCHEME_HTTPS) == 0) {              \
On 2014/11/03 10:49:17, ppi wrote:
> Afaiu |drp_counter| is a static variable, so we'll have only for multiple
> subsequent pageloads in the renderer. What if scheme_type changes between
loads?
> Wouldn't we record the samples in the wrong histogram in this case?

Thanks for catching this! Updated to add a new static https_drp_counter.

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
chrome/renderer/page_load_histograms.cc:746:
data_reduction_proxy_params_.IsDataReductionProxy(
On 2014/11/03 10:49:17, ppi wrote:
> I might be missing something - where |data_reduction_proxy_params_| is set?

This is set in the PLH constructor (line 723/728).

[ppi, 2014-11-04 16:12:43]

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
chrome/renderer/page_load_histograms.cc:746:
data_reduction_proxy_params_.IsDataReductionProxy(
On 2014/11/03 18:46:16, jeremyim wrote:
> On 2014/11/03 10:49:17, ppi wrote:
> > I might be missing something - where |data_reduction_proxy_params_| is set?
> 
> This is set in the PLH constructor (line 723/728).

Yup, but the flags are set to 0 there. In io_thread.cc I see that there's a few
flags we can set that will affect whether we use the proxy or not.

Will it work correctly with flags set to 0 here?

[jeremyim, 2014-11-04 18:12:22]

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/688403002/diff/1/chrome/renderer/page_load_hi...
chrome/renderer/page_load_histograms.cc:746:
data_reduction_proxy_params_.IsDataReductionProxy(
On 2014/11/04 16:12:42, ppi wrote:
> On 2014/11/03 18:46:16, jeremyim wrote:
> > On 2014/11/03 10:49:17, ppi wrote:
> > > I might be missing something - where |data_reduction_proxy_params_| is
set?
> > 
> > This is set in the PLH constructor (line 723/728).
> 
> Yup, but the flags are set to 0 there. In io_thread.cc I see that there's a
few
> flags we can set that will affect whether we use the proxy or not.
> 
> Will it work correctly with flags set to 0 here?

Updated to use the same base flags as io_thread; it wouldn't affect whether we
use the proxy or not (since the renderer doesn't initiate the request) but in a
fallback case, would not detect as having used the DRP. There is a TODO in
io_thread to move a lot of the initialization into the params constructor, so
I'll be working on that, but in the meantime, this should give us the
functionality we desire.

[ppi, 2014-11-04 18:18:51]

Thanks for the explanation. If you are confident that the
|data_reduction_proxy_was_used| part will work as intended, this lgtm.

[jeremyim, 2014-11-04 19:20:26]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2014-11-04 19:22:17]

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

[commit-bot: I haz the power, 2014-11-04 20:44:49]

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

[commit-bot: I haz the power, 2014-11-04 20:44:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jeremyim, 2014-11-04 21:52:43]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2014-11-04 21:54:22]

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

[commit-bot: I haz the power, 2014-11-04 23:13:03]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-11-04 23:13:35]

Patchset 4 (id:??) landed as
https://crrev.com/360767605cd4fe197f054bbd066dfefd33959807
Cr-Commit-Position: refs/heads/master@{#302701}