Update the Safe Browsing interstitial histograms

Update the Safe Browsing interstitial histograms

This organizes the Safe Browsing interstitial histograms in a way that
should scale better as new warnings or buttons are added.

Plus: all of the recording is done from within the command handling
method, instead of in the secondary methods like OnProceed.

BUG=

Committed: https://crrev.com/e6e17b9dc4640dc192a8fb92a2098325cb8f7054
Cr-Commit-Position: refs/heads/master@{#295294}

[felt, 2014-09-16 06:05:32]

felt@chromium.org changed reviewers:
+ mattm@chromium.org

[felt, 2014-09-16 06:05:44]

Matt, hopefully this will be more clear now that it's separated out from the
other CLs.

[mattm, 2014-09-16 07:48:20]

https://codereview.chromium.org/576653002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/576653002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:331:
RecordUserDecision(DONT_PROCEED);
I still don't think this will be recorded if the user hits the back button or
navigates to a different URL.

[mattm, 2014-09-16 07:48:54]

(oh, and thanks for splitting it up!)

[felt, 2014-09-16 15:03:32]

https://codereview.chromium.org/576653002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/576653002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:331:
RecordUserDecision(DONT_PROCEED);
On 2014/09/16 07:48:20, mattm wrote:
> I still don't think this will be recorded if the user hits the back button or
> navigates to a different URL.

Oh, hmm, good point. Fixed.

[felt, 2014-09-16 15:04:11]

felt@chromium.org changed reviewers:
+ asvitkine@google.com

[felt, 2014-09-16 15:04:24]

felt@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[felt, 2014-09-16 15:04:40]

alexei PTAL also?

[Alexei Svitkine (slow), 2014-09-16 17:30:56]

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10452: +<histogram
name="interstitial.malware.repeat_visit_decision"
How about naming this interstitial.malware.decision.repeat_visit to indicate
it's a specialization of the other histogram.

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:49260: +  <int value="0"
label="SHOW_ADVANCED"/>
I'm not sure how useful this enum is without a normalizing value (e.g. number of
times the interstitial was shown). Perhaps add an entry here for it like you
have for decision?

[felt, 2014-09-16 20:34:56]

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10452: +<histogram
name="interstitial.malware.repeat_visit_decision"
On 2014/09/16 17:30:56, Alexei Svitkine wrote:
> How about naming this interstitial.malware.decision.repeat_visit to indicate
> it's a specialization of the other histogram.

Done.

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:49260: +  <int value="0"
label="SHOW_ADVANCED"/>
On 2014/09/16 17:30:56, Alexei Svitkine wrote:
> I'm not sure how useful this enum is without a normalizing value (e.g. number
of
> times the interstitial was shown). Perhaps add an entry here for it like you
> have for decision?

The normalizing value is SB3InterstitialDecision::SHOW. It'll be the same value.
Should I repeat it anyway?

[Alexei Svitkine (slow), 2014-09-16 21:05:01]

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:49260: +  <int value="0"
label="SHOW_ADVANCED"/>
On 2014/09/16 20:34:56, felt wrote:
> On 2014/09/16 17:30:56, Alexei Svitkine wrote:
> > I'm not sure how useful this enum is without a normalizing value (e.g.
number
> of
> > times the interstitial was shown). Perhaps add an entry here for it like you
> > have for decision?
> 
> The normalizing value is SB3InterstitialDecision::SHOW. It'll be the same
value.
> Should I repeat it anyway?

Right. But in the current set up, you have to look at another histogram to get
that normalizing value. So it's up to you - if you feel that inconvenience can
be lived with, then you can leave as is (but maybe mention it in the histogram
description). Otherwise you could also include it in this one, to make the
histogram stand on its own.

[felt, 2014-09-17 01:01:27]

On 2014/09/16 21:05:01, Alexei Svitkine wrote:
>
https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/576653002/diff/40001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:49260: +  <int value="0"
> label="SHOW_ADVANCED"/>
> On 2014/09/16 20:34:56, felt wrote:
> > On 2014/09/16 17:30:56, Alexei Svitkine wrote:
> > > I'm not sure how useful this enum is without a normalizing value (e.g.
> number
> > of
> > > times the interstitial was shown). Perhaps add an entry here for it like
you
> > > have for decision?
> > 
> > The normalizing value is SB3InterstitialDecision::SHOW. It'll be the same
> value.
> > Should I repeat it anyway?
> 
> Right. But in the current set up, you have to look at another histogram to get
> that normalizing value. So it's up to you - if you feel that inconvenience can
> be lived with, then you can leave as is (but maybe mention it in the histogram
> description). Otherwise you could also include it in this one, to make the
> histogram stand on its own.

OK, done. Added TOTAL_VISITS

[mattm, 2014-09-17 01:22:21]

lgtm

https://codereview.chromium.org/576653002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/576653002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:564: switch
(interaction) {
Do you need TOTAL_VISITS in this switch?

[felt, 2014-09-17 01:27:12]

https://codereview.chromium.org/576653002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/576653002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:564: switch
(interaction) {
On 2014/09/17 01:22:21, mattm wrote:
> Do you need TOTAL_VISITS in this switch?

Done.

[Alexei Svitkine (slow), 2014-09-17 15:18:32]

lgtm

[felt, 2014-09-17 15:19:00]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-09-17 15:20:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/576653002/100001

[commit-bot: I haz the power, 2014-09-17 15:23:51]

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

[commit-bot: I haz the power, 2014-09-17 15:23:52]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/57181)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[felt, 2014-09-17 16:39:56]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-09-17 16:40:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/576653002/120001

[commit-bot: I haz the power, 2014-09-17 17:43:59]

Committed patchset #7 (id:120001) as 06c2100edd4d7d9c77f0ab934a8ea2b8777ca80e

[commit-bot: I haz the power, 2014-09-17 17:44:29]

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